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 a license check for npm modules #6552

Closed
wants to merge 7 commits into from
Closed

Conversation

pento
Copy link
Member

@pento pento commented May 3, 2018

Description

As discussed in #6508, any modules we include must have a GPL2 compatible license. This PR adds a script and Travis check, to ensure that all modules are licensed correctly.

This PR can't be merged until the remaining issues associated with #6508 are resolved.

How has this been tested?

  • Running npm run check-licenses should exit with an error.
  • To simulate a passing check, modify the ALLOWED_LICENSES array to add the failing licenses.

@pento pento added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label May 3, 2018
@pento pento added this to the WordPress 5.0 milestone May 3, 2018
@pento pento self-assigned this May 3, 2018
@pento pento added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label May 3, 2018
package.json Outdated
@@ -129,6 +129,7 @@
"test": "npm run lint && npm run test-unit",
"test-php": "npm run lint-php && npm run test-unit-php",
"ci": "concurrently \"npm run lint && npm run build\" \"npm run test-unit:coverage-ci\"",
"license-check": "! npm ls --production --parseable | xargs -I {} jq --raw-output '.name + \" \" + ( .license // .licenses[0].type )' '{}/package.json' | grep -v -E '^.* .*(MIT|GPL-2|ISC|BSD|CC0).*$'",
Copy link
Member

Choose a reason for hiding this comment

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

npm scripts are usually meant to be portable. My gut feeling is that this command is way too 🔥 for Windows.

Since we only care about Travis CI catching these errors, maybe move it over into e.g. ./bin/check-license.sh?

Copy link
Member

Choose a reason for hiding this comment

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

Having said that, it looks like most of our npm scripts aren't portable... 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty much every part of this command will fail on Windows. 🙃

I think it's a good idea to put it in a script: makes it easier to format the output, so there's extra information about why it failed.

Copy link
Member

Choose a reason for hiding this comment

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

Since we only care about Travis CI catching these errors, maybe move it over into e.g. ./bin/check-license.sh?

Or alternatively ./bin/check-license.js where we could use cross-platform Node modules.

There's a few: https://www.npmjs.com/search?q=license%20check

I also swear I've seen some build integrations which support this as a feature, though I'm coming up short on links.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, here it is: https://fossa.io/

Copy link
Member Author

Choose a reason for hiding this comment

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

"DETECTS HIDDEN GPL" is probably my favourite marketing line on that site. 😂

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should use more Node for those scripts to make them cross-platform.

@pento
Copy link
Member Author

pento commented May 6, 2018

Rebased, mousetrap is the only package that we need to fix, now.

@pento
Copy link
Member Author

pento commented May 18, 2018

Rebased on master. All of the new failures are caused by fsevents and lerna being dependencies, rather than dev deps.

@ntwb, @gziolo: Is it possible for us to shuffle these around?

@gziolo
Copy link
Member

gziolo commented May 18, 2018

Related discussion on Slack: https://wordpress.slack.com/archives/C02QB2JS7/p1526628916000386.

Short answer: yes and yes.

See: #6792.
Lerna was moved to dependencies temporary to make it work with plugins that set Gutenberg GitHub repository as a dependency. It shouldn't be necessary as soon as we publish individual modules as packages to npm.

@ntwb
Copy link
Member

ntwb commented May 18, 2018

#6752 was the Lerna change

#6772 was the fsevents change, this has just now been reverted in #6792

@gziolo gziolo mentioned this pull request May 18, 2018
4 tasks
@nerrad
Copy link
Contributor

nerrad commented May 18, 2018

I'd love to see this as a part of https://github.com/WordPress/packages/tree/master/packages/scripts it'd be a useful tool for WP plugin devs to implement (especially those that have their plugin in the wp repo).

@gziolo
Copy link
Member

gziolo commented May 18, 2018

I'd love to see this as a part of https://github.com/WordPress/packages/tree/master/packages/scripts it'd be a useful tool for WP plugin devs to implement (especially those that have their plugin in the wp repo).

It would have to be ported to Node, but in general big yes to the idea 💯

"predev": "check-node-version --package", - is another thing I would see in scripts package just for consistency and having one dependency less + defaults provided.

@nerrad
Copy link
Contributor

nerrad commented May 18, 2018

Maybe something like this could be used instead? https://www.npmjs.com/package/license-checker It has an onlyAllow argument which could be fed the allowed licenses? You can also indicate what you want to check licenses on (i.e. production/development)

@pento
Copy link
Member Author

pento commented May 22, 2018

license-checker has some weirdness about it, if it doesn't understand the license field, it tries to guess the license, and it frequently gets it wrong.

I agree that this would be useful a script.

@nerrad
Copy link
Contributor

nerrad commented May 22, 2018

Ya I noticed the same thing with it, plus it didn't have a way of whitelisting verified scripts. One we've started giving a go at using in our packages is https://www.npmjs.com/package/js-green-licenses. While it only automatically handles SPDX format licenses, it does allow for exclusions/whitelisting and also utilizes a config file (which is a bit easier to manage). I configured one of our travis jobs to use it.

@gziolo
Copy link
Member

gziolo commented May 22, 2018

@pento, can you double check if js-green-licenses would work for us? I would be happy to expose it from WordPress/packages as a reusable script - similar to what I did today for package.json linter - WordPress/packages#128.

It would be great to have all those tools working with both Gutenberg and packages exactly the same.

@ntwb
Copy link
Member

ntwb commented May 22, 2018

If I'm not mistaken Apache-2.0 license modules are out as they are not compatible with the GPL2 licenses:

https://github.com/google/js-green-licenses/blob/master/package.json#L35
"license": "Apache-2.0",

@nerrad
Copy link
Contributor

nerrad commented May 22, 2018

ya they definitely aren't compatible. I only mentioned it because I wasn't sure whether non-compatible licenses were still okay for dev tools only.

@pento
Copy link
Member Author

pento commented Aug 10, 2018

Closing in favour of #8808.

@pento pento closed this Aug 10, 2018
@pento pento deleted the add/npm-license-check branch August 10, 2018 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants