-
Notifications
You must be signed in to change notification settings - Fork 57
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
Enhance NPM dependency resolver to resolve all the dependent packages #51
Conversation
What happens if |
devDeps and transitive devDeps are now excluded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much better, just some nits. Can you please run this tool on http://github.com/apache/skywalking-client-js and http://github.com/apache/skywalking-nodejs to see the results and paste here?
func (resolver *NpmResolver) ListPkgPaths() (io.Reader, error) { | ||
buffer := &bytes.Buffer{} | ||
cmd := exec.Command("npm", "ls", "--all", "--production", "--parseable") | ||
cmd.Stderr = os.Stderr | ||
cmd.Stdout = buffer | ||
// Error occurs all the time in npm commands, so no return statement here | ||
err := cmd.Run() | ||
return buffer, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can remove the return value error
because
- the error is mostly expected
- we don't deal with the error when invoking this function
- the stderr is printed and we don't need to get the error when invoking this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the error returned from cmd.Run()
is exist error, like exit status 1
it differs from Stderr
, which records runtime error
so the same error will not be printed twice
same as the problem below
if err := cmd.Run(); err != nil { | ||
return err | ||
logger.Log.Errorln(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to log it again because the stderr has already printed this right?
It seems in some cases, the
It can not be parsed right now. Plan to fix this in next patch |
For skywalking-client-js, the total output is shown as below:root@Samaritan:~/skywalking-eyes# ./bin/linux/license-eye -c ~/testPlace/skywalking-client-js/.licenserc.yaml d r added 652 packages, and audited 653 packages in 29s 50 packages are looking for funding 5 vulnerabilities (4 moderate, 1 high) To address issues that do not require attention, run: To address all issues (including breaking changes), run: Run
And for skywalking-nodejs:( root@Samaritan:~/skywalking-eyes# ./bin/linux/license-eye -c ~/testPlace/skywalking-nodejs/.licenserc.yaml d r
Could not make proto path relative: **/*.proto: No such file or directory Error: Command failed: /root/testPlace/skywalking-nodejs/node_modules/grpc-tools/bin/protoc --plugin=protoc-gen-grpc=/root/testPlace/skywalking-nodejs/node_modules/grpc-tools/bin/grpc_node_plugin --js_out=import_style=commonjs,binary:/root/testPlace/skywalking-nodejs/src/proto/ --grpc_out=/root/testPlace/skywalking-nodejs/src/proto/ --plugin=protoc-gen-grpc=/root/testPlace/skywalking-nodejs/node_modules/.bin/grpc_tools_node_protoc_plugin **/.proto
killed: false, Error: Command failed: /root/testPlace/skywalking-nodejs/node_modules/grpc-tools/bin/protoc --plugin=protoc-gen-grpc=/root/testPlace/skywalking-nodejs/node_modules/grpc-tools/bin/grpc_node_plugin --plugin=protoc-gen-ts=/root/testPlace/skywalking-nodejs/node_modules/.bin/protoc-gen-ts --ts_out=/root/testPlace/skywalking-nodejs/src/proto/ **/.proto
killed: false, npm ERR! A complete log of this run can be found in:
|
Good catch! Projects are able to be dual-licensed or licensed under multiple licenses, let’s tackle this case in next PR |
This patch enhances the NPM dependency resolver to resolve all the dependent packages.
First, it runs the npm command
npm ls --all --parseable
to list all the packages' absolute paths.Then, each package's name is inferred from its relative path from the node_modules dir.
Finally, walk through each package's root path to resolve licenses from the package.json file or the license file.