Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade SPDX output to specification version 2.3 #7839

Closed
wants to merge 1 commit into from

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (40630f4) 66.96% compared to head (8bb326d) 67.02%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7839      +/-   ##
============================================
+ Coverage     66.96%   67.02%   +0.05%     
- Complexity     2041     2045       +4     
============================================
  Files           356      356              
  Lines         17084    17114      +30     
  Branches       2443     2443              
============================================
+ Hits          11440    11470      +30     
  Misses         4623     4623              
  Partials       1021     1021              
Flag Coverage Δ
funTest-docker 66.91% <ø> (ø)
funTest-non-docker 34.46% <ø> (ø)
test 36.28% <100.00%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fviernau fviernau requested a review from tsteenbe November 10, 2023 07:38
@sschuberth sschuberth marked this pull request as ready for review November 10, 2023 08:18
@sschuberth sschuberth requested review from a team as code owners November 10, 2023 08:18
Copy link
Member

@tsteenbe tsteenbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As SPDX 2.2 is the ISO version it has been adopted by many organizations as their standard (know several automotive companies that are 2.2-only ). By solely supporting SPDX 2.3 we likely reduce our potential user pool. Also we upcoming SPDX 3.0 and multiple CycloneDX versions

ORT TSC should imo decide on a supported versions scheme for open standards e.g.
A) We only support latest version
B) We support last 2 major versions
C) We support versions are are standard or popular with the community

Depending on TSC decision this pull request may need to be changed.

@sschuberth
Copy link
Member Author

sschuberth commented Nov 10, 2023

By solely supporting SPDX 2.3 we likely reduce our potential user pool.

Realistically, I believe that's not the case. As you can see from the diff of the spec, the differences between 2.2 and 2.3 are mostly of clarifying nature, and cleaning up with inconsistencies. We don't write out any 2.3-only fields, so unless SPDX consumers really look at the version in the header (which is very rarely the case, in my experience), they can just read ORT's SPDX documents as if they were 2.2.

This PR actually fixes a source of confusion about PACKAGE-MANAGER vs. PACKAGE-MANAGER that the guys from Mercedes-Benz at the FOSS Convention told me about, so it's addressing direct feedback from our user base.

I don't think we need a TSC decision to move forward.

@fviernau
Copy link
Member

I can also share that I have seen the requirement from real world customers to produce SPDX documents in version 2.2.

@sschuberth
Copy link
Member Author

produce SPDX documents in version 2.2.

But what does that actually mean? That spdxVersion: "SPDX-2.2" is literally written in the header, or that the actual document can syntactically be understood by a version 2.2 parser?

@fviernau
Copy link
Member

But what does that actually mean? That spdxVersion: "SPDX-2.2" is literally written in the header, or that the actual document can syntactically be understood by a version 2.2 parser?

That it is valid WRT version 2.2. And that used SPDX tooling acccepts it as a valid 2.2. input.

@sschuberth
Copy link
Member Author

That it is valid WRT version 2.2. And that used SPDX tooling acccepts it as a valid 2.2. input.

Ok, so the version in written in the header does not actually matter as long as the document is property-wise SPDX 2.2-compliant. Which it is, even after this PR.

@tsteenbe
Copy link
Member

Ok, so the version in written in the header does not actually matter as long as the document is property-wise SPDX 2.2-compliant

No, several consumption tools we have seen used actively check version header for both CycloneDX and SPDX to determine the right way to parse and validate the SBOM. Several enterprise standardized on SPDX 2.2 as it's ISO including in their procurement docs.

For me this matter of transparency & trust to our ORT users, we as TSC should set clear expectation - what SBOM version we support, when it support versions will change and why

@sschuberth
Copy link
Member Author

What about the first three commits in this PR, these also apply to SPDX 2.2. Can we merge these if I split them out?

@fviernau
Copy link
Member

What about the first three commits in this PR, these also apply to SPDX 2.2. Can we merge these if I split them out?

I haven't reviewed these in detail yet, but if it's true that these are for version 2.2, then I'd have no objections moving these forward.

@sschuberth sschuberth added the on hold Pull requests that cannot currently be merged label Nov 17, 2023
Note that this changes serialization of reference categories to use
dashes instead of underscores [1]. Continue to accept underscores when
deserializing for backward-compatibility, also see the discussion at
[2]. Generally, deserialization of SPDX 2.2 is still supported.

The diff of `spdx-schema.json` nicely resembles the code changes.

Resolves #5445.

[1]: https://github.com/spdx/spdx-spec/blob/v2.3/schemas/spdx-schema.json#L325
[2]: CycloneDX/cyclonedx-dotnet-library#267

Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
@sschuberth
Copy link
Member Author

Pinging @schr3gl3j, just curious, would you be more interested in SPDX 2.2 or SPDX 2.3 output?

@goneall
Copy link

goneall commented Nov 24, 2023

For reference - here's a comment with a list of changes from SPDX 2.2 to SPDX 2.3 that may impact tooling: spdx/spdx-spec#691 (comment)

If you haven't already, you may want to use the above comment as a checklist to make sure the tooling is updated.

Also note that if you use the SPDX Java Library, it supports multiple versions of the spec and is in use in several other projects.

@goneall
Copy link

goneall commented Nov 24, 2023

W.R.T. the above comments on supporting version 2.2, I have also ran into organizations that will not use SPDX 2.3 since it has not gone through the ISO certification.

@sschuberth
Copy link
Member Author

Also note that if you use the SPDX Java Library, it supports multiple versions of the spec

Yeah, that would be my main incentive to switching (and probably contributing) to it instead of maintaining our own model...

@tsteenbe
Copy link
Member

Yeah, that would be my main incentive to switching (and probably contributing) to it instead of maintaining our own model...

Objections to using SPDX Java Library are still valid, nothing has changed - with all respect to Gary's continuous work maintaining it.

@sschuberth
Copy link
Member Author

Objections to using SPDX Java Library are still valid, nothing has changed

I remember there were objections, but I cannot not find the discussion anymore. So could you please link to it / repeat the objections?

@goneall
Copy link

goneall commented Nov 29, 2023

Objections to using SPDX Java Library are still valid, nothing has changed

I remember there were objections, but I cannot not find the discussion anymore. So could you please link to it / repeat the objections?

If any of the objections are issues that can be reasonably fixed in the library, please let me know. I'm not aware of the issues / objections other than the decision not to use the library.

@sschuberth
Copy link
Member Author

If any of the objections are issues that can be reasonably fixed in the library, please let me know. I'm not aware of the issues / objections other than the decision not to use the library.

I've found my original ask for a justification not to use the upstream SPDX Java library in the PR from 2020 that introduces the SPDX classes. In reply to my remark @fviernau added the following paragraph to the commit message:

Note that there is already a SPDX java library [4] which is
intentionally not used mainly for keeping the development speed and
decisions concerning the SPDX model completely under our control.

Given that we didn't do a lot of development work in the SPDX area for quite some time, I tend to disagree with @tsteenbe's statement that "Objections to using SPDX Java Library are still valid", and we should probably revisit that decision.

@heliocastro
Copy link
Contributor

Not help much that the older Java library is still haunting us, because people are confused.

@goneall
Copy link

goneall commented Nov 30, 2023

Not help much that the older Java library is still haunting us, because people are confused.

Good point - the old library definitely had issues (that's why we re-wrote it).

The old library README has been updated to point to the new library - let me know if you have any ideas on how to better communicate that there is a new and improved library available.

@sschuberth
Copy link
Member Author

Not pursuing this further.

@sschuberth sschuberth closed this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Pull requests that cannot currently be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants