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

analyzer: Fix SPDX metadata to point to a source artifact #3787

Closed
wants to merge 2 commits into from

Conversation

sschuberth
Copy link
Member

The "curl-7.70.0.tar.gz" archive contains the source code, so it should
be treated as a source artifact instead of a binary artifact, which is
signalled by setting "filesAnalyzed" to "true".

Also see 15512d3.

Signed-off-by: Sebastian Schuberth sebastian.schuberth@bosch.io

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.

We can't set filesAnalyzed to true as then Package Verification Code becomes mandatory per the spec which is something we don't want as makes creating SPDX package manifest more complicated.

The current definition of "Files Analyzed" has been problematic since I started working on SPDX, hope to drop it from the spec in 3.0.

@sschuberth
Copy link
Member Author

We can't set filesAnalyzed to true as then Package Verification Code becomes mandatory per the spec

Argh. Fortunately, our SPDX reader does not check for that 😁 So maybe we could slightly violate the spec here? Or, maybe we could simply set packageVerificationCode to NONE?

hope to drop it from the spec in 3.0.

How would v3 then distinguish between download URLs of binary vs sources? Ideally, these would just be separate fields (like in ORT 😉 ) so that you can also capture both at the same time.

@sschuberth sschuberth force-pushed the spdx-source-artifacts branch 2 times, most recently from 96fb6ac to 7ff589e Compare March 25, 2021 15:33
@sschuberth sschuberth requested a review from tsteenbe March 25, 2021 15:34
@sschuberth
Copy link
Member Author

Ping @tsteenbe, mind having another look?

@tsteenbe
Copy link
Member

tsteenbe commented Apr 6, 2021

@sschuberth Setting filesAnalyzed to true and not providing Package Verification Code will cause an exception in SPDX reader whilst keeping it false and misusing downloadLocation as source code location will not cause any issues.

@sschuberth
Copy link
Member Author

Setting filesAnalyzed to true and not providing Package Verification Code will cause an exception in SPDX reader

Which reader exactly?

misusing downloadLocation as source code location will not cause any issues.

But we'll also have no man to tell that it is a source location. It will be treated as a binary location instead.

@sschuberth sschuberth force-pushed the spdx-source-artifacts branch from 7ff589e to 991dcc7 Compare April 6, 2021 20:36
@sschuberth sschuberth requested a review from a team as a code owner April 6, 2021 20:36
@sschuberth
Copy link
Member Author

Anyway, I've added the packageVerificationCode fields now.

Copy link
Contributor

@neubs-bsi neubs-bsi left a comment

Choose a reason for hiding this comment

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

would it not make sense to have the changes in the same commit, as otherwise there will be a commit which "cause[s] an exception in [the] SPDX reader"

@sschuberth
Copy link
Member Author

would it not make sense to have the changes in the same commit, as otherwise there will be a commit which "cause[s] an exception in [the] SPDX reader"

I didn't want to "mix-in" an otherwise unrelated change to project.spdx.yml, that's why I separated the commits. I'll simply swap the order of the commits to make it impossible to run such an SPDX reader on an invalid file in our Git history.

@sschuberth sschuberth force-pushed the spdx-source-artifacts branch from 991dcc7 to 2f792d8 Compare April 7, 2021 10:47
@sschuberth sschuberth requested a review from neubs-bsi April 7, 2021 10:47
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.

@sschuberth Adding just packageVerificationCode with filesAnalyzed set to true you also for instance need to add PackageLicenseInfoFromFiles see https://spdx.github.io/spdx-spec/3-package-information/#314-all-licenses-information-from-files

It's a bad idea to set filesAnalyzed to true

@sschuberth
Copy link
Member Author

It's a bad idea to set filesAnalyzed to true

But how can we then distinguish between downloadLocations to source artifacts vs, downloadLocations to binary artifacts?

@tsteenbe
Copy link
Member

tsteenbe commented Apr 8, 2021

But how can we then distinguish between downloadLocations to source artifacts vs, downloadLocations to binary artifacts?

A rather simple solution for that would be to require that source artifacts have to use VCS identifiers in the URLs e.g. git+https://git.myproject.org/MyProject.git. I know this does not work for maven source artifacts but for the few exceptions we could build in regexes to detected the edge cases.

@sschuberth
Copy link
Member Author

I know this does not work for maven source artifacts but for the few exceptions we could build in regexes to detected the edge cases.

Unfortunately, it's not only a "few exceptions". We heavily rely on custom SPDX files that refer to source artifacts in ZIP files in arbitrary locations. There's no sane way to deal with these with regexes.

neubs-bsi
neubs-bsi previously approved these changes Apr 9, 2021
@sschuberth
Copy link
Member Author

@tsteenbe I just checked on this PR again. I'm reading the spec so that for filesAnalyzed == true

So as both allow a cardinality of 0, these fields should be optional even with filesAnalyzed == true. Do we agree here?

When making local changes to that file, test would always fail as the
file is not merged upstream yet.

A better approach to test the resolution of remote SPDX files will be
added later.

Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
The "curl-7.70.0.tar.gz" archive contains the source code, so it should
be treated as a source artifact instead of a binary artifact, which is
signalled by setting "filesAnalyzed" to "true".

Also see 15512d3.

Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
@sschuberth sschuberth force-pushed the spdx-source-artifacts branch from aca412a to b74d04b Compare November 23, 2021 16:07
@sschuberth
Copy link
Member Author

Closing this in favor of the different approach in #4816.

@sschuberth sschuberth closed this Dec 7, 2021
@sschuberth sschuberth deleted the spdx-source-artifacts branch December 7, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants