Follow CycloneDX 1.5 spec for SPDX license expressions #975
Conversation
Signed-off-by: Valentin Dide <[email protected]>
Signed-off-by: Valentin Dide <[email protected]>
|
@validide can you look into this line and see if we can create a new utils method and reuse this logic? https://github.com/CycloneDX/cdxgen/blob/master/binary.js#L577 I have created the below to fix the test failure in blint. |
utils.js
Outdated
| } | ||
|
|
||
| if ( | ||
| spdxLicenseExpressionOp.some((op) => { |
There was a problem hiding this comment.
Can this be simplified to check for the characters space and open bracket ( using regex?
There was a problem hiding this comment.
Should it check for the existence of both characters or at least one? I have see license expressions without the ( character.
There was a problem hiding this comment.
You are correct. I meant either.
I will check but which logic should be preserved the one from here or the one I created? |
|
@validide your logic is good but shall we try with a regex instead of a for-loop? |
Signed-off-by: Valentin Dide <[email protected]>
Signed-off-by: Valentin Dide <[email protected]>
Signed-off-by: Valentin Dide <[email protected]>
Signed-off-by: Valentin Dide <[email protected]>
utils.js
Outdated
| return true; | ||
| } | ||
|
|
||
| if (licenseLoweCase.endsWith("+")) { |
There was a problem hiding this comment.
This is for the "GPL-2.0+" (or latter) scenario. If this would be part of a more complex case it would be covered by the check for (space) or (.
utils.js
Outdated
| * @see https://spdx.dev/learn/handling-license-info/ | ||
| **/ | ||
| export function isSpdxLicenseExpression(license) { | ||
| const licenseLoweCase = (license || "").toLowerCase(); |
There was a problem hiding this comment.
Can we rename this variable to licenseLowerCase or reuse the same license variable?
utils.js
Outdated
| **/ | ||
| export function isSpdxLicenseExpression(license) { | ||
| const licenseLoweCase = (license || "").toLowerCase(); | ||
| if (!licenseLoweCase) { |
There was a problem hiding this comment.
This condition will always be false since we substitute empty value above.
There was a problem hiding this comment.
If the function receives an undefined/null value we will have an empty string here. The empty string is "falsy". I was thinking it's not worth it to do all the logic on an empty string.
There was a problem hiding this comment.
Oh yeah. For some reason, I forgot this falsy thing and assumed only python behaves this way. Another learning for today!
utils.js
Outdated
| return false; | ||
| } | ||
|
|
||
| if (/[(\s]+/gi.test(licenseLoweCase)) { |
There was a problem hiding this comment.
/g instead of /gi since case insensitivity will not make a difference when looking for space and bracket character.
| * | ||
| * @returns {string} ISO formatted timestamp, without milliseconds. | ||
| */ | ||
| export function getTimestamp() { |
| * @returns {string} ISO formatted timestamp, without milliseconds. | ||
| */ | ||
| export function getTimestamp() { | ||
| return new Date().toISOString().split(".")[0] + "Z"; |
There was a problem hiding this comment.
For the record, I think jsonschema date-time is wrong to leave out millseconds and move away from ISO8601 format.
Signed-off-by: Valentin Dide <[email protected]>
You're welcomed! I saw there were other discussions about moving to TypeScript and I believe that would be a huge amount of work. Since you are more familiar with the project would using JSDoc or cyclonedx-javascript-library be simpler? I think this is a nice project but I am more scared to do a change now than 24h ago. |
|
As much as I would like to, I have very limited time at the moment. |
|
@validide, any thoughts on how we can simplify this project to attract more contributors? |
Somewhat duplicate of pull request #690.
Closes #679.
@prabhu, I am not sure if you want the implementation to check all of those cases also considering there is a list of known licenses. If you think something is wrong let me know.