Skip to content

Adds basic license check #258

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adds basic license check #258

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6090502
Adds basic license check
Oct 24, 2021
0fb2866
Adds whitespace
Oct 24, 2021
c489c67
Merge branch 'master' into license2
Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ci/src/Registry/API.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import Effect.Ref as Ref
import Foreign.Dhall as Dhall
import Foreign.GitHub (IssueNumber)
import Foreign.GitHub as GitHub
import Foreign.Licensee as Licensee
import Foreign.Object as Object
import Foreign.SPDX as SPDX
import Foreign.SemVer as SemVer
import Foreign.Tar as Tar
import Foreign.Tmp as Tmp
Expand Down Expand Up @@ -205,7 +207,7 @@ addOrUpdate { ref, fromBower, packageName } metadata = do
Left err -> throwWithComment $ "Could not convert Manifest to JSON: " <> err
Right res -> pure res

runChecks metadata manifest
runChecks { metadata, manifest, absoluteFolderPath }

-- After we pass all the checks it's time to do side effects and register the package
log "Packaging the tarball to upload..."
Expand Down Expand Up @@ -238,11 +240,16 @@ addOrUpdate { ref, fromBower, packageName } metadata = do
-- TODO: handle addToPackageSet: we'll try to add it to the latest set and build (see #156)
-- TODO: upload docs to pursuit (see #154)

runChecks :: Metadata -> Manifest -> RegistryM Unit
runChecks metadata manifest = do
runChecks :: { metadata :: Metadata, manifest :: Manifest, absoluteFolderPath :: String } -> RegistryM Unit
runChecks { metadata, manifest, absoluteFolderPath } = do
-- TODO: collect all errors and return them at once. Note: some of the checks
-- are going to fail while parsing from JSON, so we should move them here if we
-- want to handle everything together
log "Checking that the SPDX license in the manifest corresponds to the one in the package"
licenseFromLicensee <- liftAff $ Licensee.detect absoluteFolderPath
case licenseFromLicensee of
Left err -> throwWithComment $ "Could not find a license in the package: " <> err
Right li -> when (Array.notElem (SPDX.print manifest.license) li) (throwWithComment $ "License from the manifest does not match the license detected by licensee. In manifest: " <> SPDX.print manifest.license <> ". Deteceted in package: " <> show li <> ".")
Comment on lines +248 to +252
Copy link
Member

Choose a reason for hiding this comment

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

I added a comment in the other PR to the same effect, but this isn't quite enough -- licensee will detect from the LICENSE file, but we want to also pull the licenses out of the manifest files present in the repository in general, all of which is included in the toManifestFields function in the LegacyImport module.

We want to check the Bowerfile, Spago file, package.json file, and the LICENSE file and make sure they all match with the purs.json (manifest) file, but here we're only checking the LICENSE file.

The work to check all those files is done in the legacy import tool already so it's just a matter of pulling it out.

Copy link
Author

Choose a reason for hiding this comment

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

I think I understand. Lemme write it up here and you can tell me if it is correct or not.

Currently, the API.purs file is looking for a file called purs.json with various bits of information including a license. In addition to this file, we want to parse other manifests which may also contain a license. Currently, the two additional manifest files we will parse are bower.json and spago.dhall + packages.dhall. So, for example, if we were writing it up in a README, it'd look something like this:

License

Packages added to the registry must have a valid SPDX license, and that license must be indicated in a manifest file. We determine license validity using the following steps:

  1. We look for, and if present parse, the following three manifest files to determine a package's license:
  • purs.json
  • spago.dhall
  • bower.json
  1. We then use licensee to find license files in the package.
  2. We cross-reference the licenses found by licensee with the licenses specified in a package's manifest file(s). If there is a mismatch (ie too many licenses, too few licenses or an incorrect license) we will reject the package.

Copy link
Author

Choose a reason for hiding this comment

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

@thomashoneyman ping regarding this just to make sure I'm on the right track - thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Yes! Two clarifications:

  1. We check the manifest file for the license for the project (the purs.json file). We then cross-reference against the other manifest files. In practice this is basically the same as what you’ve written, but until we fully decide what to do about mismatches I think it’s worth calling out that in our minds the purs.json manifest is the “true” one
  2. Licensee checks the package.json file (if I remember correctly), so we’re checking that too.

Copy link
Author

Choose a reason for hiding this comment

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

Would it be ok if I added the following?

In the future, we will be using purs.json as the single source of truth for licenses for PureScript packages and will be deprecating bower.json, package.json, and spago.dhall support. We thus encourage all package authors to include their licenses in a purs.json file so as not to be dropped from a future edition of the registry.

Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure about that — we may always check the other manifests, but in the future you will have to have a purs.json file to publish to the registry. We’re only auto-importing legacy packages

Copy link
Contributor

Choose a reason for hiding this comment

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

Packages added to the registry must have a valid SPDX license

Scenario: I have a lawyer and I have a new open source license type (not "UNLICENSED"). I have it documented in a LICENSE file so consumers understand how to comply. Would this make my package unregisterable? Or do I need to petition to get it in SPDX to be valid? Is creating my own license a good practice or idea? Probably not, but should it straight up not be able to be handled? I could see an argument where it should be a Either SPDX CustomAndIKnowWhatImDoing.

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that we (the registry trustees) also would need to review the license to be sure that it is usable for open source — for example, open enough that we can host the code in the registry backend. Given that our bandwidth is already stretched it’s not something we’d like to take on if we can defer to a widely used standard (SPDX).

If a package does have this requirement in the future, then I’d encourage them to reach out to the registry trustees to see if this restriction can be changed at that time, or just not register the package. Package managers will still let you use the package even if it isn’t registered.


log "Checking that the Manifest includes the `lib` target"
libTarget <- case Object.lookup "lib" manifest.targets of
Expand Down