-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Follow CycloneDX 1.4 spec for SPDX license expressions for npm. #690
base: master
Are you sure you want to change the base?
Follow CycloneDX 1.4 spec for SPDX license expressions for npm. #690
Conversation
Signed-off-by: ansonallard <ansononlineinfo@gmail.com>
@ansonallard any ideas about the test failures? |
Signed-off-by: ansonallard <ansononlineinfo@gmail.com>
@prabhu Is there a way to see what the inputs were to the test that failed? I just see that the schema validation failed, but I don't have the data to determine why. |
@ansonallard I could never get the jsonschema to return that information. Usually I would run the same test locally and use jq :( |
@@ -234,7 +236,9 @@ export function getLicenses(pkg, format = "xml") { | |||
} | |||
return licenseContent; | |||
}) | |||
.map((l) => ({ license: l })); | |||
.map((l) => | |||
l.expression ? { expression: l.expression } : { license: l } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there can be only one expression, so possible we are returning multiple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The licenses
list supports multiple schemas in the array. I'm not certain if the spec requires all data to be the same schema in the list. Spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah. Looks like that is new 1.5 restriction. I remember the rationale for enforcing a single expression. We may have to try to fix this bug in index.js or elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few situations that I can think of:
- A component has multiple licenses and a SPDX expression
- A component has multiple SPDX expressions and multiple licenses
- A component has multiple licenses, no SPDX expressions
- A component has one SPDX expression
For 1, it may make sense to ignore the license list and serve the single SPDX expression (the expression takes precedence).
For 2, one would have to choose which expression to persist. It could be simple, but arbitrary, like choosing the first entry, or more complex, like choosing the most permissive or open.
For 3 and 4, those behaviors are known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If there are legitimately multiple expressions (with a space or bracket), then we can use the first one and show a warning.
@ansonallard, could you kindly rebase since we have refactored things a bit. Also, could you take a look at the new known-licenses.json to see if this bug could be resolved with an entry there? |
Is there any progress on this? If not I would like move the changes to a new branch and try to fix it. |
@validide New branches are better. Also why are people still using 1.4? |
Support spdx expressions for CycloneDX 1.4 Spec