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

add support for licenses not found on list #1540

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Feb 5, 2023

Fixes #1529

As discussed with @kzantow , if a license identifier is not found on the official list, rather than returning "" and false, it creates LicenseRef-<id> per the other licenses section of the spdx spec.

Also adds handling an empty identifier, and adds tests.

The approach may need some tweaking, but it seems a simple enough solution.

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Hey @deitch -- thanks for submitting this PR! I think this is half of what needs to happen -- the LicenseRef-<id> getting put in the appropriate license field. The other half is that we need to add these licenses with a matching LicenseRef-<id> to the Other Licenses section so there is something to refer to -- this would need to be added to this struct in the OtherLicenses field

@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2023

I was kind of wondering about that. Thanks for the pointer to the right struct.

I saw that ID() is used by both spdx and cyclonedx, even though it is part of spdx. Do I need to find a parallel struct for CycloneDX?

@kzantow
Copy link
Contributor

kzantow commented Feb 6, 2023

@deitch CycloneDX is easier -- we just need to include the name instead of ID. See the spec here, optionally including text if we have it

@deitch
Copy link
Contributor Author

deitch commented Feb 7, 2023

Ok, I think I got it, both for spdx and cyclonedx, but have a look. I updated tests so they handle the cases and pass, and I also ran some tests locally.

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Thanks @deitch -- I think this is pretty close, just a few items I've noted

syft/formats/common/spdxhelpers/license.go Outdated Show resolved Hide resolved
syft/formats/common/spdxhelpers/license.go Outdated Show resolved Hide resolved
syft/formats/common/spdxhelpers/to_format_model.go Outdated Show resolved Hide resolved
@kzantow
Copy link
Contributor

kzantow commented Feb 7, 2023

I think this may fix #933 as well.

@deitch it also looks like the TestEncodeDecodeEncodeCycleComparison is failing for CycloneDX, which means we need to add decoding for the licenses with name probably around here.

@kzantow kzantow linked an issue Feb 7, 2023 that may be closed by this pull request
@kzantow
Copy link
Contributor

kzantow commented Feb 7, 2023

@deitch it looks like the last hold-up is the CycloneDX decoding noted above ☝️ -- I could help with this, if you like

Signed-off-by: Avi Deitcher <avi@deitcher.net>
@deitch
Copy link
Contributor Author

deitch commented Feb 7, 2023

I assume it has something to do with this assuming that it should be l.License.ID, when it could be .ID or .Name.

Yeah, fixing that gets the tests passing locally. I will push it out and see if CI is happy.

@deitch
Copy link
Contributor Author

deitch commented Feb 7, 2023

That did it.

@kzantow
Copy link
Contributor

kzantow commented Feb 7, 2023

Thanks much @deitch! This is a great addition!

@kzantow kzantow merged commit 38a090c into anchore:main Feb 7, 2023
@deitch deitch deleted the external-license branch February 7, 2023 16:48
@deitch
Copy link
Contributor Author

deitch commented Feb 7, 2023

Excellent. Thanks for helping get it in. I am going to try and tackle a different issue soon with gopkg licenses.

@deitch
Copy link
Contributor Author

deitch commented Feb 7, 2023

I will flag you on that issue, so I can try and submit for that as well.

@deitch
Copy link
Contributor Author

deitch commented Feb 8, 2023

This did help @kzantow but then exposed the inability to handle "OR", which appears in a number of places, as well as compound expressions with (a OR b) AND c. Submitting a PR soon to handle it.

@kzantow
Copy link
Contributor

kzantow commented Feb 8, 2023

Right -- properly parsing complex license expressions is not something Syft is doing today, I think it's okay to just include these as LicenseRefs for now (it's better than excluding them)

@deitch
Copy link
Contributor Author

deitch commented Feb 8, 2023

But you wouldn't object to it doing so properly, would you? I am close to having a PR ready for it.

GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
Signed-off-by: Avi Deitcher <avi@deitcher.net>
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.

apk packages with simplified license show NOASSERTION Licenses missing in most report format
2 participants