nodeshift-archived / license-reporter

license-reporter is a tool that gathers licenses for project's dependencies and produces a output in XML, JSON, YAML and HTML format.
Apache License 2.0
13 stars 10 forks source link

No longer contains info on transitive dependencies #237

Closed grdryn closed 6 years ago

grdryn commented 6 years ago

We've been using the 0.3.0 tag for quite a while now, so I just checked the latest master to see if things worked for all of our projects.

When I compare the xml or html files, I see that they used to contain info on all transitive dependencies in the older version, but no longer do on the latest master.

Does this sound correct? Is it supposed to show transitive dependencies, or just direct dependencies?

helio-frota commented 6 years ago

@grdryn yes, the current code is getting only the direct/declared dependencies

Does this sound correct?

AFAIK yes : )

Extra info: Here is the part of code that does it: https://github.com/bucharest-gold/license-reporter/blob/master/lib/modules/console.mdl.js#L70 The tool license-checker produces a lot of content after scan the particular project. This content is on this variable data, which has already had a small modification (only the structure), which can be seen here: https://github.com/bucharest-gold/license-reporter/blob/master/lib/modules/checker.js#L11-L17

In case of semver range:

If a product will use fixed versions (without https://docs.npmjs.com/misc/semver#advanced-range-syntax) then we can avoid the item 1, and only display a message to inform the developer about the dependencies using version range 2

https://github.com/bucharest-gold/license-reporter/issues/207#issuecomment-343167335

Well, at least I think it is a bit more easy to change, in case needed due the refactor : )

grdryn commented 6 years ago

@helio-frota it's possible that we're not talking about the same thing, I'm not sure.

The problem that I'm talking about is that when I run:

license-reporter save --ignore-version-range --all --silent --xml licenses.xml && \
license-reporter report --ignore-version-range --all --silent

I get xml and html files that only contain information about the dependencies that I've declared in my package.json, and no info on all the other dependencies that are pulled in indirectly by those dependencies.

If that's intentional, then I guess I'll stick to the v0.3.0 tag where it gives me license info on all of the dependencies that I've got in my node_modules after running npm install --production, whether they're directly referenced in my package.json or indirectly depended on because one or more of my dependencies references them in their package.json.

lgriffin commented 6 years ago

Hey guys, we had a clarification from Andrea:

So wrt the 1 level...given that now you don't have to deal with the merging of several files, which would have been a nightmare, and especially for Mobile, I would opt for the complete resolution irrespective of dependency level, all down the hierarchy. This is what is being done in Maven repositories, that contain all the runtime dependencies of Maven products. Anything that is inside the Maven repo, will be listed in the html reports. So this is much similar to the content of the nested node_modules, which contain all the runtime dependencies required for your Node components.

It looks like we had this functionality at one point (going on @grdryn comments). @helio-frota do you know how much effort it will take to get a full dependency tree by recursively hitting the package.json? We have an option of launching 1.0 without this -- as technically this is a new requirement. If the work is extensive we can certainly get more Mobile developers involved on it.

helio-frota commented 6 years ago

... do you know how much effort it will take to get a full dependency tree...

--full-dependency-tree nice option, with alias --fdt

@lgriffin @grdryn I'll look on it and give feedback , thanks for the clarification!

helio-frota commented 6 years ago

@lgriffin @grdryn PR sent https://github.com/bucharest-gold/license-reporter/pull/240

helio-frota commented 6 years ago

merged. was done the same as the past code.

past code here: ( if --all then add the full data from license-checker tool ) https://github.com/bucharest-gold/license-reporter/commit/97d9333f33952f1d92531f75787081a9c2960d77#diff-22d5d2b57c69d2e1ac054c4d0f744ff1L123

( else will look only for the declared dependencies ) https://github.com/bucharest-gold/license-reporter/commit/97d9333f33952f1d92531f75787081a9c2960d77#diff-22d5d2b57c69d2e1ac054c4d0f744ff1L130

lgriffin commented 6 years ago

That is a great turn around! @grdryn can you give this a whirl and provide any feedback.

grdryn commented 6 years ago

@lgriffin tried here: https://github.com/feedhenry/fh-mbaas/pull/83, and seems to work great. Thanks @helio-frota!

One thing to note is that this will also include license info on devDependencies, which may not be necessarily needed, depending on your use case, but probably isn't a huge blocker either.

For the PR that I linked above, we're running it at shrinkwrap time, so I've set some hooks that will ensure that only production dependencies are present when it's run. This reduces the amount of licensing files that we'll check-in greatly.

helio-frota commented 6 years ago

One thing to note is that this will also include license info on devDependencies, which may not be necessarily needed, depending on your use case, but probably isn't a huge blocker either.

We can try to apply a filter like we did for declared dependencies.

grdryn commented 6 years ago

We can try to apply a filter like we did for declared dependencies.

If you think that would be straightforward, then it would definitely be useful for our projects in FeedHenry. If it's a complex thing, then it may not be worth it. Let me know if you want me to try anything out!

helio-frota commented 6 years ago

@grdryn thanks ! Issue created #241 I think it is possible to use the same approach for declared dependencies but in this case to remove the devDependencies. And yah, would be great if you want to try it !

helio-frota commented 6 years ago

closing thanks @lgriffin @grdryn