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 in check-license for conjunctive (AND) licenses. #46801

Merged
merged 4 commits into from
Apr 17, 2023

Conversation

bdurette
Copy link
Contributor

@bdurette bdurette commented Dec 27, 2022

What?

Conjuctive licenses were being ignored in both the package.json and within various LICENSE files. In the first case, this could lead to false negatives $(e.g., 'MIT AND BSD' being treated as non-compatible). In the second case, the implementation was such that only one license was returned (whichever detected license occurred later in licenseFileStrings). Based on the ordering of that list, this was likely to cause a false positive, because the non-compatible 'Apache-2.0' license occurs before any of the compatible licenses.

Progress on #38461.

Why?

Conjunctive licenses for dependencies could either result in false negative (license-compatible dependencies being deemed incompatible) or false positive (license-incompatible dependencies being deemed compatible), depending on how they were detected.

How?

  • Add tests for basic license file detection.
  • Treat license files with multiple detected licenses as conjunctive. Include test for conjunctive license.
  • Add (primitive) support for conjunctive ("AND") licenses. This support does not include handling of more complex compound licenses which include multiple levels of AND and OR clauses.

Testing Instructions

The following output is observed on trunk:

brandon.durette@brandon-durette gutenberg % npm run other:check-licenses | grep ERROR
[0]  ERROR  Module eme-encryption-scheme-polyfill has an incompatible license 'Apache-2.0'.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! gutenberg@14.8.0-rc.2 other:check-licenses: `concurrently "wp-scripts check-licenses --prod --gpl2 --ignore=@react-native-community/cli,@react-native-community/cli-platform-ios" "wp-scripts check-licenses --dev"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the gutenberg@14.8.0-rc.2 other:check-licenses script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/brandon.durette/.npm/_logs/2022-12-27T19_44_00_508Z-debug.log

On my check-license branch, the following output is observed:

brandon.durette@brandon-durette gutenberg % npm run other:check-licenses | grep ERROR
[0]  ERROR  Module shaka-player has an incompatible license 'Apache-2.0'.
[0]  ERROR  Module eme-encryption-scheme-polyfill has an incompatible license 'Apache-2.0'.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! gutenberg@14.8.0-rc.2 other:check-licenses: `concurrently "wp-scripts check-licenses --prod --gpl2 --ignore=@react-native-community/cli,@react-native-community/cli-platform-ios" "wp-scripts check-licenses --dev"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the gutenberg@14.8.0-rc.2 other:check-licenses script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/brandon.durette/.npm/_logs/2022-12-27T19_46_36_413Z-debug.log

Note the addition of shaka-player as directly incompatible.

Conjuctive licenses were being ignored in both the `package.json` and within
various LICENSE files. In the first case, this could lead to false negatives
$(e.g., 'MIT AND BSD' being treated as non-compatible). In the second case, the
implementation was such that only one license was returned (whichever detected
license occurred later in `licenseFileStrings`). Based on the ordering of that
list, this was likely to cause a false positive, because the non-compatible
'Apache-2.0' license occurs before any of the compatible licenses.

Progress on WordPress#38461.
@bdurette
Copy link
Contributor Author

See my comment in #38461 about the spdx-satisfies package. If trusted, this could be leveraged to significantly simplify this code and make it handle more cases. I'm open to that approach, but am unsure of the appetite for introducing that as a dev dependency.

@gziolo
Copy link
Member

gziolo commented Feb 13, 2023

The patch looks solid. Thank you for working on it and for including test coverage that is going to make it easier to apply further refactoring with confidence. I'm still catching up with your work, but I hope to land this PR when I confirm as soon as I verify it works as expected.

Do you think it deserves a new entry in the changelog file https://github.com/WordPress/gutenberg/blob/trunk/packages/scripts/CHANGELOG.md under ## Unreleased subsection?

@bdurette
Copy link
Contributor Author

Thank you. Changelog entry added.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I missed it. Let's get it included in the @wordpress/scripts package. We might need a follow-up for the CHANGELOG, but I can handle it. Thank you so much for the contribution and the refactoring with accompanying unit tests 🎉

@gziolo gziolo merged commit 7fa9700 into WordPress:trunk Apr 17, 2023
@github-actions github-actions bot added this to the Gutenberg 15.7 milestone Apr 17, 2023
gziolo added a commit that referenced this pull request Apr 17, 2023
@gziolo
Copy link
Member

gziolo commented Apr 17, 2023

We might need a follow-up for the CHANGELOG, but I can handle it.

Fixed in f34b4ac. I didn't want to hold this PR any longer ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants