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 checker script #8808

Merged
merged 7 commits into from
Aug 13, 2018
Merged

Add a license checker script #8808

merged 7 commits into from
Aug 13, 2018

Conversation

pento
Copy link
Member

@pento pento commented Aug 10, 2018

Description

To ensure we're only adding GPL2 compatible packages, we need to check all the licenses.

This PR adds a script with defined safe licenses, and runs it in the postinstall script, to ensure devs are immediately aware if they've added something incompatible.

Due to the myriad of ways that package authors define their licenses, this script allows for license strings that aren't SPDX, and for when licenses aren't defined in the package's package.json but can be found in a LICENSE or README file.

Fixes #6508, #7822.

Testing

npm run check-licenses will test that all packages defined in dependencies are GPL2 compatible, and that all devDependencies an OSS license.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@pento pento added the [Tool] WP Scripts /packages/scripts label Aug 10, 2018
@pento pento requested a review from a team August 10, 2018 06:03
@gziolo
Copy link
Member

gziolo commented Aug 10, 2018

It sort of works:

[1] ERROR Module prompts has an incompatible license 'undefined'.
[1] ERROR Module cyclist has an incompatible license 'undefined'.
[1] ERROR Module cli-table has an incompatible license 'undefined'.

😅

@pento
Copy link
Member Author

pento commented Aug 10, 2018

Good times. I sure do enjoy when things behave differently locally.

return true;
}

if ( licenseType.indexOf( 'OR' ) < 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an inline comment explaining what happens in here?

return false;
}

const subLicenseTypes = formattedlicenseType.replace( /^\(*/g, '' ).replace( /\)*$/, '' ).split( ' or ' ).map( ( e ) => e.trim() );
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, I bet w are extracting sublicenses. However, it is not clear how it works.

}, false );
}

const allowed = licenses.reduce( ( satisfied, allowedLicense ) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think it can be implemented using array.find:

const allowed = licenses.find( ( allowedLicense ) => checkLicense( allowedLicense, licenseType ) );

@gziolo gziolo requested review from ntwb and a team August 10, 2018 07:23
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.

Love this PR ❤️ I'm happy that it integrates into wp-scripts so seamlessly and even takes advantage of hasCliArg util.

I think we could iterate on the usage of reduce functions which could stop processing when value is found like in the example I shared: #8808 (comment)

As a general concept, it is good to go. I'm giving 👍 for the functionality.

@gziolo gziolo added this to the 3.6 milestone Aug 10, 2018
@gziolo gziolo requested a review from a team August 10, 2018 07:29
@gziolo
Copy link
Member

gziolo commented Aug 10, 2018

8bc21e9 - this is super helpful. Your future self will be very grateful for those comments :)

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

That's cool; I have a few little thoughts. Good idea 👍

const dev = hasCliArg( '--dev' ) || hasCliArg( '--development' );
const gpl2 = hasCliArg( '--gpl2' );

const gpl2Licenses = [
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick here, but it'd be nice to call this gpl2CompatibleLicenses or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏻

'(MIT AND Zlib)',
];

const ossLicenses = [
Copy link
Member

Choose a reason for hiding this comment

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

What does this variable mean? Aren't all of these licenses open source software licenses? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

THESE ARE NOT THE FREE-EST LICENSES MATT

RICHARD STALLMAN IS JUDGING US FOR EVEN MENTIONING THEM

😁

Copy link
Member

Choose a reason for hiding this comment

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

I CLEARLY HATE FREEDOM, MY MICROWAVE IS RUNNING APACHE 2 FIRMWARE.

/**
* Check if a license string matches the given license.
*
* The license string can be a single license, or an SPDX-compatible OR license string.
Copy link
Member

Choose a reason for hiding this comment

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

Grammar nitpick here but I think removing the comma (which isn't really needed for just a single "or" statement like this) would improve readability. I thought the next "OR" was another "or" in the sentence, but eventually realised "SPDX-compatible OR License String" was a thing. Maybe capitalising "License String" would make it clear that's a whole concept. I didn't know what "SPDX-compatible OR License String" was 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏻

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

:shipit:

@pento pento merged commit fa3e58a into master Aug 13, 2018
@pento pento deleted the add/7822-license-check branch August 13, 2018 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants