Add licenses command to pnpm #5567

weyert · 2022-10-30T22:57:34Z

Introduces a new command named licenses which generates a license compliance report

weyert · 2022-10-30T22:59:34Z

If you run the command pd licenses --dir ~/temporary/license-proto/pnpm-licenses --json you will get a output similar to:

{
  "MIT": [
    {
      "name": "next",
      "path": "/Users/User/temporary/license-proto/pnpm-licenses/node_modules/next",
      "license": "MIT",
      "vendorUrl": "https://nextjs.org"
    }
  ],
  "Apache-2.0": [
    {
      "name": "typescript",
      "path": "/Users/User/temporary/license-proto/pnpm-licenses/node_modules/typescript",
      "license": "Apache-2.0",
      "vendorName": "Microsoft Corp.",
      "vendorUrl": "https://www.typescriptlang.org/"
    }
  ]
}

If you leave out the --json and the command is pd licenses --dir ~/temporary/license-proto/pnpm-licenses --long you will get:

┌──────────────────┬────────────┬─────────────────────────────────┐
│ Package          │ License    │ Details                         │
├──────────────────┼────────────┼─────────────────────────────────┤
│ typescript (dev) │ Apache-2.0 │ Microsoft Corp.                 │
│                  │            │ https://www.typescriptlang.org/ │
├──────────────────┼────────────┼─────────────────────────────────┤
│ next             │ MIT        │ https://nextjs.org              │
└──────────────────┴────────────┴─────────────────────────────────┘

If you exclude the --long you will get an output similar to:

┌──────────────────┬────────────┐
│ Package          │ License    │
├──────────────────┼────────────┤
│ typescript (dev) │ Apache-2.0 │
├──────────────────┼────────────┤
│ next             │ MIT        │
└──────────────────┴────────────┘

weyert · 2022-10-30T23:01:37Z

It's currently a draft because of two issues:

Need to sort the unit tests
Unable to get transitive dependencies (e.g. the dependencies of the next and typescript packages)

Would appreciate some helper on the second issue how I could make that work

packages/plugin-commands-patching/package.json

packages/licenses/package.json

zkochan · 2022-10-31T00:09:20Z

Unable to get transitive dependencies (e.g. the dependencies of the next and typescript packages)

There is a package for walking the lockfile: https://github.com/pnpm/pnpm/tree/main/packages/lockfile-walker

weyert · 2022-10-31T01:34:47Z

Unable to get transitive dependencies (e.g. the dependencies of the next and typescript packages)

There is a package for walking the lockfile: https://github.com/pnpm/pnpm/tree/main/packages/lockfile-walker

Thank you, I will have a look and see how I can use it :)

weyert · 2022-11-01T01:13:30Z

I have reimplemented to use the lockfile-walker package and added tests to the new packages this PR is introducing

weyert · 2022-11-01T01:20:58Z

Only question I am having whether peerDependencies of a package that user hasn't installed should be included?

zkochan · 2022-11-01T23:35:25Z

The manifests should be updated with pnpm update-manifests

packages/licenses/src/getPkgInfo.ts

weyert · 2022-11-01T23:49:54Z

The manifests should be updated with pnpm update-manifests

When I try to run this I am getting this error:

 WARN  Failed to create bin at /Users/weyert/Development/Projects/Opensource/pnpm/packages/exe/node_modules/.bin/pkg. The source file at /Users/weyert/Development/Projects/Opensource/pnpm/node_modules/.pnpm/github.com+vercel+pkg@7e4fef9f0828b11c7a9655b95425f32094f2daa2_pp6fkuhwkrqq7cjcj7uqpf37e4/node_modules/pkg/lib-es5/bin.js does not exist.
 ```

Not sure what's wrong, re-running `pnpm install` doesn't fix it

packages/licenses/src/getPkgInfo.ts

packages/licenses/src/licenses.ts

packages/licenses/src/lockfileToLicenseNodeTree.ts

zkochan · 2022-11-01T23:59:30Z

packages/licenses/src/lockfileToLicenseNodeTree.ts

+        version,
+        prefix: modules,
+      });
+    } catch (err: unknown) {}


why should this error be ignored?

I have the problem that sometimes an optional dependency is listed in the lock file but not stored in modules directory and then the fetching fails. In that case I am skipping/ignoring this package so it won't get listed.

No, it is not OK to skip such issues. It would mean that your license checker doesn't check all the dependencies, so a package with a bad license might pass under your radar. Besides, now that you read from the CAFS, the optional deps will always be found. So this try/catch is not needed.

I think this problem is resolved now. I have removed it, while making this change I did struggle to make it work with PNPM repo and the pkg-package as it's a tarball and the workspace projects but I think I cracked it. Only definitely not 100% sure it's the right approach

packages/licenses/src/lockfileToLicenseNodeTree.ts

packages/plugin-commands-licenses/src/licenses.ts

weyert · 2022-11-02T00:34:16Z

@zkochan Thank you for taking the time to review my PR. I have tried to apply your suggestions to my branch. I can totally imagine that are better ways to do some things :)

matzeeable · 2022-11-02T07:30:00Z

Hey @weyert !

Nice to see such a command finding a place in PNPM and thank you for your work! 😀

Does this PR also include disclaimer-file generation, like yarn generate-disclaimer?

weyert · 2022-11-02T10:08:38Z

Hey @weyert !

Nice to see such a command finding a place in PNPM and thank you for your work! 😀

Does this PR also include disclaimer-file generation, like yarn generate-disclaimer?

Not yet as I didn't have a need for it yet . I am happy to work on it in a follow-up pull request

zkochan · 2022-11-03T00:43:18Z

The approach that is used here has some disadvantages.

It won't work with a hoisted node_modules (when node-linked is set to hoisted) as you are reading the package.json files from node_modules/.pnpm, which will not exist in a hoisted node_modules
It won't print the licenses for skipped optional packages
It won't work on a project that has no node_modules. So, you'll have to run pnpm install first. This one might not be critical.

A better solution would be to read the package.json file and the license files from the content-addressable store. It would solve issues the 1st and 2nd issues. The 3rd issue will still exist unless we will dynamically fetch missing packages to the store (which may be done in a separate PR).

You can see examples how to read info from the store here: https://github.com/pnpm/pnpm/blob/main/packages/package-requester/src/packageRequester.ts

For instance, see how the readManifestFromStore function is used to read the manifest.

To find the license file in the content-addressable store, you should read the index file of the package. It contains the list of files. The path to the index file is calculated from the package's integrity checksum:

pnpm/packages/package-requester/src/packageRequester.ts

Lines 331 to 333 in e19356c

    
           const filesIndexFile = opts.pkg.resolution['integrity'] 
        
             ? ctx.getFilePathInCafs(opts.pkg.resolution['integrity'], 'index') 
        
             : path.join(target, 'integrity.json')

weyert · 2022-11-03T20:55:48Z

The approach that is used here has some disadvantages.

It won't work with a hoisted node_modules (when node-linked is set to hoisted) as you are reading the package.json files from node_modules/.pnpm, which will not exist in a hoisted node_modules

It won't print the licenses for skipped optional packages

It won't work on a project that has no node_modules. So, you'll have to run pnpm install first. This one might not be critical.

I have tried to accommodate your change requests. I hope what I changed is what you intended. Hopefully I didn't misunderstood you

packages/licenses/src/getPkgInfo.ts

zkochan · 2022-11-05T01:01:59Z

packages/licenses/src/lockfileToLicenseNodeTree.ts

+        version,
+        prefix: modules,
+      });
+    } catch (err: unknown) {}


No, it is not OK to skip such issues. It would mean that your license checker doesn't check all the dependencies, so a package with a bad license might pass under your radar. Besides, now that you read from the CAFS, the optional deps will always be found. So this try/catch is not needed.

packages/plugin-commands-licenses/src/licenses.ts

weyert · 2022-11-05T21:09:26Z

I have updated the PR to resolve issues and accommodated the request changes

I have attached an example of the command when running against this repo:
pnpm-licenses.txt

packages/licenses/src/getPkgInfo.ts

packages/licenses/src/lockfileToLicenseNodeTree.ts

zkochan · 2022-11-06T01:55:37Z

packages/licenses/src/lockfileToLicenseNodeTree.ts

+    }
+  }
+
+  const licenseNodeTree: LicenseNodeTree = {


Why do you create a tree of license nodes if in the output you just list all the packages in a flat list?

I will have a look at changing this

Would this be a blocker for the PR to be merged?

Isn't that simpler code?

packages/licenses/package.json

packages/licenses/src/getPkgInfo.ts

zkochan · 2022-11-08T01:21:11Z

packages/licenses/src/getPkgInfo.ts

+  const isLocalPkg = packageResolution.type === 'directory'
+  if (isLocalPkg) {
+    const localInfo = await fetchFromDir(
+      path.join(opts.lockfileDir, packageResolution.directory),
+      {}
+    )
+    return {
+      local: true,
+      files: localInfo.filesIndex,
+    }


This will scan the whole package with all subdirectories even though the license files are only searched for in the root directory of the package.

I couldn't find a way to get the package file index of a pnpm workspace project, so generating one. Is there a way to get it? It's not available in the CAFS

packages/licenses/src/getPkgInfo.ts

packages/licenses/src/index.ts

packages/license-scanner/src/getPkgInfo.ts

zkochan · 2022-11-08T21:11:30Z

packages/license-scanner/src/getPkgInfo.ts

+
+      return {
+        name: 'Unknown',
+        licenseFile: licenseContents?.toString('utf-8'),
+      }


What if the package has multiple license files?

Hah good question, I haven't discovered any of such package yet. Are you aware of one?

I don't think other licenses commands of other packages handle this use case, though

I have seen such packages in the past.

Packages that don't use license-property but have multiple license files? Happy to improve the code after this PR has been merged sounds like a pretty edge case to me

I work in a company that archives npm data and I can tell you without question that there are more packages that do that than you'd think. Roughly 10% of packages in the registry. People do nutty stuff in package.json and npm has no validation.

If you happen to archive npm data via work, could younbe so kind and suggest some npm packages that does this so I can make it part of a test case?

give me a bit of time to get to that, but I'll take a look for you. if I don't get to this in a week, please ping me

Sure, of course, I am already happy you want to take the time help me :)

packages/license-scanner/src/licenses.ts

zkochan · 2022-11-11T01:02:15Z

I see that in yarn v1 there is a yarn licenses list command that does something similar.

In yarn v2 they don't have this command but there is a plugin that may be used. With this plugin, the command is: yarn licenses audit.

I wonder if we need to make this command pnpm licenses list. In the future we might add a pnpm licenses audit command as well.

cc @pnpm/collaborators

Updated the code so that the `getPkgInfo` function attempts to fetch the `package.json` and potential license files from the cafs directory by integrity id

…encies

Improved handling of reading package manifest for packages resolving to workspace projects,and tarballs Improved the handlign of parsing the license from the `package.json`-file

…gument

Renamed the `@pnpm/licenses`-package to `@pnpm/license-scanner` Updated the keywords for the packages Renamed the main function of `@pnpm/license-scanner` to be `findDependencyLicenses` Renamed to the fields `vendorName`, `vendorUrl` to the original fields used in the packages: `author` and `homepage`

welcome · 2022-11-17T10:51:42Z

Congrats on merging your first pull request! 🎉🎉🎉

weyert commented Oct 30, 2022

View reviewed changes

packages/plugin-commands-patching/package.json Outdated Show resolved Hide resolved

zkochan reviewed Oct 31, 2022

View reviewed changes

packages/licenses/package.json Outdated Show resolved Hide resolved

weyert marked this pull request as ready for review November 1, 2022 01:13

zkochan reviewed Nov 1, 2022

View reviewed changes

packages/licenses/src/getPkgInfo.ts Outdated Show resolved Hide resolved

zkochan reviewed Nov 1, 2022

View reviewed changes

packages/licenses/src/getPkgInfo.ts Outdated Show resolved Hide resolved

packages/licenses/src/getPkgInfo.ts Outdated Show resolved Hide resolved

packages/licenses/src/licenses.ts Outdated Show resolved Hide resolved

zkochan reviewed Nov 2, 2022

View reviewed changes

zkochan changed the title ~~Add licenses command to PNPM~~ Add licenses command to pnpm Nov 4, 2022

zkochan requested changes Nov 5, 2022

View reviewed changes

zkochan reviewed Nov 5, 2022

View reviewed changes

packages/licenses/src/getPkgInfo.ts Outdated Show resolved Hide resolved

zkochan reviewed Nov 5, 2022

View reviewed changes

packages/licenses/src/getPkgInfo.ts Outdated Show resolved Hide resolved

packages/licenses/src/getPkgInfo.ts Outdated Show resolved Hide resolved

zkochan reviewed Nov 6, 2022

View reviewed changes

packages/licenses/src/lockfileToLicenseNodeTree.ts Outdated Show resolved Hide resolved

zkochan reviewed Nov 6, 2022

View reviewed changes

zkochan requested changes Nov 8, 2022

View reviewed changes

marcomontalbano mentioned this pull request Nov 8, 2022

Doesn't work with pnpm. Package "x" was not found. tmorell/license-compliance#175

Open

zkochan reviewed Nov 8, 2022

View reviewed changes

Weyert de Boer and others added 22 commits November 17, 2022 03:09

fix: fetch files from cafs diretory

edeca6c

Updated the code so that the `getPkgInfo` function attempts to fetch the `package.json` and potential license files from the cafs directory by integrity id

chore: remove unnecessary dint-package

c6b898d

fix: update manifests

2946604

fix: update manifests

783d037

style: fix styling issues in the code

71d8d39

test: update the test case to avoid platform specific optional depend…

2066ff5

…encies

fix: update code based on feedback

f3ac59b

Improved handling of reading package manifest for packages resolving to workspace projects,and tarballs Improved the handlign of parsing the license from the `package.json`-file

fix: package path so its correctly pointed to the virtual store

567ebdc

fix: missing vendor information when running command with --json ar…

3c76414

…gument

style: rename function parameter's name

33c2a6b

style: changed wording of changeset description

7a9cabd

fix: drop the use of fileExists in getPkgInfo-function

2156c6f

fix: update the package manifests and tsconfig files

56da30c

docs: improve inline documentation

4dfcb94

fix: improve error message when package manifest can not be loaded

9fa13f5

chore: add TODO item for getPkgInfo's package path lookup

c50bdfc

fix: improve the way we find license files

aa74701

chore: remove unwanted console logging statement

2c1b57d

chore: update references to license-scanner-package

c1e0e0b

fix:; remove the ability to pass getPkgInfo

4e795f8

refactor: make a subcommand

2176e74

zkochan force-pushed the add-licenses-command-v2 branch from 99f6499 to 2176e74 Compare November 17, 2022 01:23

zkochan added 3 commits November 17, 2022 03:25

fix: remove not needed dep

84ab63f

refactor: render table

4404057

test: fix

8cfc0d2

zkochan merged commit d84a30a into pnpm:main Nov 17, 2022

zkochan added this to the v7.17 milestone Nov 20, 2022

weyert deleted the add-licenses-command-v2 branch November 22, 2022 00:54

Add licenses command to pnpm #5567

Add licenses command to pnpm #5567

Conversation

weyert commented Oct 30, 2022

weyert commented Oct 30, 2022 • edited

weyert commented Oct 30, 2022

zkochan commented Oct 31, 2022

weyert commented Oct 31, 2022

weyert commented Nov 1, 2022

weyert commented Nov 1, 2022

zkochan commented Nov 1, 2022

weyert commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weyert commented Nov 2, 2022

matzeeable commented Nov 2, 2022

weyert commented Nov 2, 2022

zkochan commented Nov 3, 2022

weyert commented Nov 3, 2022

Choose a reason for hiding this comment

weyert commented Nov 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan commented Nov 11, 2022

welcome bot commented Nov 17, 2022

weyert commented Oct 30, 2022 •

edited