search

rm-hull / nvd-clojure

National Vulnerability Database dependency checker for Clojure projects
MIT License
273 stars 35 forks source link

Calculate classpath using tools.deps and provide nvd-clojure as a regular -X function? #166

Open ivarref opened 10 months ago

ivarref commented 10 months ago

Hi @vemv (and others)

And thanks for a great project!

I'm wondering if it is worthwhile to support calculating the classpath using tools.deps, thus avoiding the need for an extra script. With the changes in this pull request, adding the following alias to an existing project:

:nvd-check      {:replace-deps  {nvd-clojure/nvd-clojure {:local/root "/home/ire/code/nvd-clojure"}} ; real version / change the path to a proper folder on your machine
                              :replace-paths []
                              :jvm-opts      ["-Dclojure.main.report=stderr" "-Dorg.slf4j.simpleLogger.log.org.apache.commons=error"]
                              :exec-fn       nvd.task/check}

enables to execute the nvd check using simply clojure -X:nvd-check.

I've verified that the new code in nvd.task produces identical classpath to the output of clojure -Spath.

(There could of course be something else I've missed, I'm not a classpath and/or tools.deps expert or anything like that.)

What do you think?

Thanks and kind regards.

ivarref commented 10 months ago

Thanks for the thoughtful response @vemv !

For instance, how do you guarantee that users will specify :replace-deps instead of :extra-deps? Same for :replace-paths.

The deps.edn file can be read and verified that no [:aliases :extra-deps] map contains rm-hull/nvd-clojure. This can be done using the code already written, i.e. with tools.deps. I believe that should handle the case of multi module projects (which I've personally never used) as well.

And how do you guarantee that having tools.deps as a dependency does not alter nvd-clojure's transitive dependency tree?

It's possible to inspect this. I pushed a namespace for this:

(in-ns 'nvd.tools-deps-conflict-test)
Loading test/nvd/tools_deps_conflict_test.clj... done
(show-diff 'org.clojure/tools.deps)
com.google.errorprone/error_prone_annotations deps.edn: 2.18.0 vs org.clojure/tools.deps: 2.11.0
com.google.guava/guava deps.edn: 32.1.2-jre vs org.clojure/tools.deps: 31.1-android
com.google.j2objc/j2objc-annotations deps.edn: 2.8 vs org.clojure/tools.deps: 1.3
commons-io/commons-io deps.edn: 2.13.0 vs org.clojure/tools.deps: 2.11.0
org.apache.commons/commons-lang3 deps.edn: 3.13.0 vs org.clojure/tools.deps: 3.12.0
org.checkerframework/checker-qual deps.edn: 3.33.0 vs org.clojure/tools.deps: 3.12.0
org.slf4j/jcl-over-slf4j deps.edn: 1.7.28 vs org.clojure/tools.deps: 1.7.36
org.slf4j/slf4j-api deps.edn: 2.0.7 vs org.clojure/tools.deps: 1.7.36

We can see that introducing tools.deps does alter the tree, bumping org.slf4j/jcl-over-slf4j from 1.7.28 to 1.7.36. Otherwise the classpath is unchanged (deps.edn of nvd-clojure contains newer libs than the ones referred by tools.deps). I don't think this particular bumping is anything to worry about.

Code duplication

I suppose it also would be possible to call make_classpath2 directly.

All that said, I get the impression that you've made up your mind about this, and that is perfectly fine. Personally though I fell it's unnatural to install a tool, write shell scripts, etc. in a CI setting, which is the context I want to use -X in.

Did any of the above change your mind? If not I'm happy to discard the pull request. If you are now more convinced than before that this is a good idea, I could try:

Let me know what you think. Thank and kind regards.

ivarref commented 10 months ago

I'll make a separate post as I've learned more about tools.deps today (thanks to a colleague).

First it seems that -T also supports :exec-fn in an alias. I have not really seen that documented anywhere in writing, but it does work. Which is great! Thus my need/desire for -X disappears.

My project's deps.edn can then be like this:

             :nvd-check      {:deps     {nvd-clojure/nvd-clojure {:local/root "/home/ire/code/nvd-clojure"}}
                              :jvm-opts ["-Dclojure.main.report=stderr" "-Dorg.slf4j.simpleLogger.log.org.apache.commons=error"]
                              :exec-fn  nvd.task/check}}

Clojure CLI does the right thing regardless if you use :extra-deps, :deps or :replace-deps as long as you use -T.

ivarref commented 10 months ago

PS: If it's decided that it's OK to include tools.deps, it is also possible to print a dependency tree with highlighted problematic dependencies. I've done some work in that direction at the following repo: https://github.com/ivarref/finddep

Best regards

vemv commented 10 months ago

Thanks for the ping!

I'll get back to this soon.

I'm not sure I'll be convinced but I'll try to give it a read open-mindedly.

Cheers - V