NEW Check that all dependency licenses are permissive

[emteknetnz]

Issue silverstripe/recipe-kitchen-sink#80

I've simply added the checks on the the existing js + phplinting jobs respectively. This means that our CI builds won't take much longer as we don't need to spin up additional jobs to run the license checks, and both these jobs already have most the prerequisites installed i.e. node_modules and the vendor dir already exist.

The npm checker is better in that it will tell you which dependency has a non allowed license, e.g.
Package "@babel/[email protected]" is licensed under "MIT" which is not permitted by the --onlyAllow flag. Exiting.

I've tested the npm checker again admin, which passed, though not other modules with js deps.

The composer checker will only tell you that a non allowed license was found, though not which dependency that applies to. This is a bit annoying though it should be easy enough to manually track this down.

In terms of how it handles multi licenses composer deps like https://github.com/nette/schema it will look in vendor/composer/installed.json and select the first license it finds. I tested this locally by deleting the "BSD-3-Clause" entry, and then the checker showed that there was a "GPL-2.0-only" license in there. If we run into a situation in the future where this doesn't work as we want it to and it says the module is non-permissive licensed even though there's a permissive license available, we can just deal with it then. For now that I think this is fine as is and both CMS 5 + CMS 6 sink appear to pass.

For both of these we're only checking non-dev dependencies, as those are what are actually used on websites. There are some non-permissive licenses in dev-only requirements, for instance npm itself is listed with an Artistic-2.0 license, however that's obviously not part of our distribution

[GuySartorelli]

I've tested the npm checker again admin, which passed, though not other modules with js deps.

We should resolve and license issues we have before merging this. I don't want our builds going red just for license problems while we're trying to do all these breaking changes. We rely on CI green-ness too much for that right now.

The composer checker will only tell you that a non allowed license was found, though not which dependency that applies to.

I think it's worth spending some extra time on this to see if there's a sensible way to sort that out.

In terms of how it handles multi licenses composer deps like https://github.com/nette/schema it will look in vendor/composer/installed.json and select the first license it finds. I tested this locally by deleting the "BSD-3-Clause" entry, and then the checker showed that there was a "GPL-2.0-only" license in there. If we run into a situation in the future where this doesn't work as we want it to and it says the module is non-permissive licensed even though there's a permissive license available, we can just deal with it then.

I don't think that's really suitable. We should be future proofing this now rather than knowing it is likely to cause all our builds to fail (including PR builds and everything else) in the future.
If this was its own CI run that didn't run on PRs and didn't stop all of CI from passing I'd be okay with that, but that future disruption isn't worth accepting to me.

TL;dr

Actually resolving any license issues we have with dependencies is probably worth doing right now, but I don't think this CI is ready for use and I don't think we should prioritise implementing the CI build over the CMS 6 work we already need to do.

[emteknetnz]

I've updated the pull-request to:

• Moved the list of allowed licenses to a shared text file
• Sort the licenses of composer modules with multiple licenses so that allowed licenses are at the start of the list
• Added a list of manually checked npm packages that failed the check when I ran it on other Silverstripe modules with a package.json

I used the following bash script on installs of 5.3 and 6.0 sink to validate that all modules should now pass in CI

# Assumes that `npm install -g license-checker` has been run
# The following NPM package report as UNKNOWN have been checked they have permissive licenses:
# https://github.com/fitzgen/glob-to-regexp
# https://github.com/codedance/jquery.AreYouSure
EXCLUDE_PACKAGES='@silverstripe/[email protected];[email protected];[email protected];[email protected];[email protected]'
ONLY_ALLOW='MIT;MIT-0;ISC;0BSD;BSD-2-Clause;BSD-3-Clause;Apache-2.0;Python-2.0;CC0-1.0;CC-BY-3.0;CC-BY-4.0;Public Domain;Unlicense'
BASEDIR=$(pwd)
FILES=$(find . | grep package.json | grep -v node_modules)
for FILE in $FILES; do
  # remove trailing "/package.json"
  SUBDIR="${FILE/\/package.json/}"
  DIR="$BASEDIR/$SUBDIR"
  echo "Checking $DIR"
  cd $DIR
  yarn install
  license-checker --production --onlyAllow $ONLY_ALLOW --unknown --out /dev/null --excludePackages $EXCLUDE_PACKAGES
done

[GuySartorelli]

I'm still not convinced this should be part of the regular CI, which we would have discussed in refinement. I don't feel comfortable merging this without having that discussion.

Imagine one of our framework deps adopts a GPLv2 dependency for example. Suddenly all modules CI builds will fail, and we may not be able to do anything about it for a substantial amount of time.

[GuySartorelli]

• glob-to-regexp is archived so we should replace it.
• silverstripe/react-injector isn't maintained - the correct react injector is inside silverstripe/admin directly. Whatever has this dependency should remove it immediately.
• cwp-watea-theme and cwp-starter-theme should not be included as npm/yarn dependencies anywhere, so whatever has listed those as dependencies should also remove them immediately.

jquery.are-you-sure is the only item here that we can't really do anything about.

[emteknetnz]

Yes we should sort those out for CMS 6, though remember this would also run on CMS 5.3 builds, and we don't have plans to update JS deps there

I've added a note to the unmaintained deps card to remove these

[GuySartorelli]

I don't see any problem with resolving these dependency problems in CMS 5, or at the very very least looking at what effort is involved with doing so. I suspect the effort will be very small for these deps.

More to the point though, if we're not going to update deps for CMS 5 then this thing that checks deps shouldn't run on CMS 5.

[emteknetnz]

Replacing deps is out of scope for this PR. We can look CMS 5 deps seperately on the unmaintained deps card I linked to.

and we don't have plans to update JS deps there

I'm probably wrong there actually, we still haven't released CMS 5.4 which presumably will include updating JS deps. We may also need to update CMS 5 deps while it's still under support post CMS 6 stable

[emteknetnz]

Imagine one of our framework deps adopts a GPLv2 dependency for example. Suddenly all modules CI builds will fail, and we may not be able to do anything about it for a substantial amount of time.

Yeah fair point. That would happen on the composer side, though I don't think it should happen on the NPM side as the 'shared' stuff in admin isn't a part of package.json in something like linkfield.

Thinking about this for a couple of seconds it's kind of obvious that the we don't need to do the composer stuff on every build, we can just do it all centrally in a standalone job in recipe-kitchen-sink :-) I'll sort that out

[GuySartorelli]

Imagine one of our framework deps adopts a GPLv2 dependency for example. Suddenly all modules CI builds will fail, and we may not be able to do anything about it for a substantial amount of time.

Yeah fair point. That would happen on the composer side, though I don't think it should happen on the NPM side as the 'shared' stuff in admin isn't a part of package.json in something like linkfield.

Even if it doesn't cause the builds for all of our modules to fail, PR CI failing consistently on a module because of an upstream dependency changing its license is not a great maintenance experience. I'd be much more comfortable if all license checking was done in a CI run that's separate from our regular linting/testing CI.

[GuySartorelli]

Suggested change

	Note that the `Unlicense` is an actual SPDX identifier and not a placeholder for a missing identifier.
	Note that the `Unlicense` is an SPDX identifier for an actual license and not a placeholder for a missing license.

The point is that it's a license, not that it's an identifier. I had initially thought it was an SPDX identifier that specifically meant "no license"

[emteknetnz]

Updated

[emteknetnz]

... all license checking was done in a CI run that's separate from our regular linting/testing CI.

So create a new licenses.yml file and a new separate page in rhino to monitor it? I'd be pretty concerned that we'll simply forget to look at that page since it's be all green 99.999% of the time. Another downside is that it obviously extra work to implement.

Alternatively we could use the kitchen-sink ci.yml run look everything using this script #44 (comment). I've already got a draft PR that does the composer side of this in recipe-kitchen-sink, so this would be pretty easy to implement. Downside there is that it adds a minute or two of extra time to the sink job, though the job already takes over an hour so it shouldn't be noticeable.

Note that a downside of not having npm + composer in gha-run-tests (i.e. both of the two proposed solutions above) is that any pull-requests that introduce new dependencies that themselves have GPLv2 or sub-dependencies with it, is that they won't block the build and thus we could still tag a patch release with a GPLv2 dependency. To be fair though, this alone wouldn't block any sub-dependencies that change their license without us realizing.

I'll make a little matrix of solutions:

Solution	Visibility to CMS squad	Potential to break builds	Blocks PRs that introduce GPLv2
A. gha-run-tests	Good	Yes	Yes
B. recipe-kitchen-sink	Mid*1	No*2	No
C. licenses.yml	Poor	No	No

*1 Lag before it shows in rhino, and lag before we action it i.e. pickup broken-builds card
*2 Except sink

Let me know your thoughts

[GuySartorelli]

I'd be pretty concerned that we'll simply forget to look at that page since it's be all green 99.999% of the time.

We can very easily add a "check the licenses tab" step to the broken CI and merge-ups card that gets automatically generated.
This would have been discussed if this card went through refinement. It's also the approach that https://github.com/silverstripeltd/product-issues/issues/765 recommends (just saw this a few minutes ago while looking for something else)

Another downside is that it obviously extra work to implement.

I think that's balanced by not causing big disruptions as soon as licenses change for our dependencies.

Alternatively we could use the kitchen-sink ci.yml run look everything using this script #44 (comment). I've already got a silverstripe/recipe-kitchen-sink#82 that does the composer side of this in recipe-kitchen-sink, so this would be pretty easy to implement. Downside there is that it adds a minute or two of extra time to the sink job, though the job already takes over an hour so it shouldn't be noticeable.

That wouldn't be the end of the world - as you say, it already takes a long time so a couple extra minutes isn't so bad.

Note that a downside of not having npm + composer in gha-run-tests (i.e. both of the two proposed solutions above) is that any pull-requests that introduce new dependencies that themselves have GPLv2 or sub-dependencies with it, is that they won't block the build and thus we could still tag a patch release with a GPLv2 dependency.

We don't add new dependencies in patches, so it'd sit in the new minor branch and we'd notice it before doing the release.

I disagree about "Poor" visibility for having a separate CI run with a new tab in rhino, since as I mentioned above we should just make that part of checking broken builds each week.
That would be my preferred solution as it represents the least disruption to our existing workflows while still providing a weekly check against unexpected changes in licenses.

[emteknetnz]

Oh cool to see that original card again :-)

OK so how about this:

• Do both composer + npm on a new standalone licenses.yml workflow on recipe-kitchen-sink which on a weekly cron
• Do not add a new screen to rhino
• Update the automatically created broken-builds issue to include a link to the recipe-kitchen-sink actions filtered on licenses.yml runs?

That should means it won't interfere with the existing ci.yml workflows and will minimise the amount of extra work required to implement. There is some risk of a lag before we notice any copyleft licenses sneaking it, though I'd say there is a very low risk of us actually tagging something before we catch it.

Sound alright?

[GuySartorelli]

Sounds like a good plan, let's do it