tools.deps

Cyclic dependency locks up clojure command line tool

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    MacOS 10.13.4

Description

Minimal deps.edn to reproduce

{:paths ["src/main/clojure"]
 :deps
 {org.apache.xmlgraphics/batik-transcoder {:mvn/version "1.7"}}}

A dtruss of this process, shows cyclic opening on POM and _remote.repositories files

sudo dtruss -p <PID> 2>&1 | grep open

or on Linux

strace -p <PID> -f 2>&1 | grep open

Activity

Hide
Gavin Mercer added a comment -

Obviously meant

or on Linux

strace -p <PID> -f 2>&1 | grep open

Show
Gavin Mercer added a comment - Obviously meant or on Linux strace -p <PID> -f 2>&1 | grep open
Hide
Reid McKenzie added a comment - - edited

I don't quite understand the resolution algorithm tools.deps implements well enough to opine, but it appears to be doing a simple depth-first traversal of libs, coordinates and their reported dependencies. In dependency trees with circular dependencies, this would produce the reported non-termination. In my dependency tree with lots of repeated dependencies, this produces lots of repeated resolution. Running an instrumented copy of deps ala

diff --git a/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj b/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj
index 4187fe87..0f5d6310 100755
--- a/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj
+++ b/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj
@@ -71,6 +71,7 @@
 
 (defmethod ext/coord-deps :mvn
   [lib coord _manifest {:keys [mvn/repos mvn/local-repo]}]
+  (println ":mvn coord-deps]" (pr-str [lib coord]))
   (let [local-repo (or local-repo maven/default-local-repo)
         system (maven/make-system)
         session (maven/make-session system local-repo)

and piping the result(s) to a logfile, I find

# Number of resolutions performed
$ cat resolv.log | wc -l
2632
# Number of unique [lib coord] pairs
$ cat resolv.log | sort | uniq | wc -l
354
# Number of unique libs
$ cat resolv.log | awk '{print $3}' | sort | uniq | wc -l
300
# Relative frequencies of resolution
$ cat resolv.log | sort | uniq -c | sort -r | head -n 10
    170 :mvn coord-deps] [javax.inject/javax.inject {:mvn/version "1", :deps/manifest :mvn}]
    168 :mvn coord-deps] [org.glassfish.hk2/hk2-utils {:mvn/version "2.5.0-b32", :deps/manifest :mvn}]
    168 :mvn coord-deps] [org.glassfish.hk2.external/aopalliance-repackaged {:mvn/version "2.5.0-b32", :deps/manifest :mvn}]
    105 :mvn coord-deps] [commons-logging/commons-logging {:mvn/version "1.1.3", :deps/manifest :mvn}]
    103 :mvn coord-deps] [software.amazon.ion/ion-java {:mvn/version "1.0.2", :deps/manifest :mvn}]
    103 :mvn coord-deps] [org.apache.httpcomponents/httpclient {:mvn/version "4.5.2", :deps/manifest :mvn}]
    103 :mvn coord-deps] [com.fasterxml.jackson.dataformat/jackson-dataformat-cbor {:mvn/version "2.6.6", :deps/manifest :mvn}]
    103 :mvn coord-deps] [com.amazonaws/aws-java-sdk-core {:mvn/version "1.11.155", :deps/manifest :mvn}]
    101 :mvn coord-deps] [com.amazonaws/jmespath-java {:mvn/version "1.11.155", :deps/manifest :mvn}]
     95 :mvn coord-deps] [org.slf4j/slf4j-api {:mvn/version "1.7.25", :deps/manifest :mvn}]
Show
Reid McKenzie added a comment - - edited I don't quite understand the resolution algorithm tools.deps implements well enough to opine, but it appears to be doing a simple depth-first traversal of libs, coordinates and their reported dependencies. In dependency trees with circular dependencies, this would produce the reported non-termination. In my dependency tree with lots of repeated dependencies, this produces lots of repeated resolution. Running an instrumented copy of deps ala
diff --git a/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj b/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj
index 4187fe87..0f5d6310 100755
--- a/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj
+++ b/clojure-tools/src/clojure/tools/deps/alpha/extensions/maven.clj
@@ -71,6 +71,7 @@
 
 (defmethod ext/coord-deps :mvn
   [lib coord _manifest {:keys [mvn/repos mvn/local-repo]}]
+  (println ":mvn coord-deps]" (pr-str [lib coord]))
   (let [local-repo (or local-repo maven/default-local-repo)
         system (maven/make-system)
         session (maven/make-session system local-repo)
and piping the result(s) to a logfile, I find
# Number of resolutions performed
$ cat resolv.log | wc -l
2632
# Number of unique [lib coord] pairs
$ cat resolv.log | sort | uniq | wc -l
354
# Number of unique libs
$ cat resolv.log | awk '{print $3}' | sort | uniq | wc -l
300
# Relative frequencies of resolution
$ cat resolv.log | sort | uniq -c | sort -r | head -n 10
    170 :mvn coord-deps] [javax.inject/javax.inject {:mvn/version "1", :deps/manifest :mvn}]
    168 :mvn coord-deps] [org.glassfish.hk2/hk2-utils {:mvn/version "2.5.0-b32", :deps/manifest :mvn}]
    168 :mvn coord-deps] [org.glassfish.hk2.external/aopalliance-repackaged {:mvn/version "2.5.0-b32", :deps/manifest :mvn}]
    105 :mvn coord-deps] [commons-logging/commons-logging {:mvn/version "1.1.3", :deps/manifest :mvn}]
    103 :mvn coord-deps] [software.amazon.ion/ion-java {:mvn/version "1.0.2", :deps/manifest :mvn}]
    103 :mvn coord-deps] [org.apache.httpcomponents/httpclient {:mvn/version "4.5.2", :deps/manifest :mvn}]
    103 :mvn coord-deps] [com.fasterxml.jackson.dataformat/jackson-dataformat-cbor {:mvn/version "2.6.6", :deps/manifest :mvn}]
    103 :mvn coord-deps] [com.amazonaws/aws-java-sdk-core {:mvn/version "1.11.155", :deps/manifest :mvn}]
    101 :mvn coord-deps] [com.amazonaws/jmespath-java {:mvn/version "1.11.155", :deps/manifest :mvn}]
     95 :mvn coord-deps] [org.slf4j/slf4j-api {:mvn/version "1.7.25", :deps/manifest :mvn}]
Hide
Reid McKenzie added a comment -

After some digging, the root of the problem is indeed that expand-deps always enqueues all children of every examined dep. The trick is to ensure that we examine every unique dep (tuple of lib and coordinate) exactly once. The goal is to collect all coordinates for a given lib so constraints can be solved consistently, without re-visiting the same coordinates over and over again as showed in my logs. Visiting any given coordinate exactly once also breaks the infinite loop reported by @gavin.

The attached patch 000-canonicalized-deps-once.patch reworks expand-deps to do exactly this. It re-orders the determination of the coordinate to be used for any given dep, so that deps are only ever enqueued with their use-coords. This enables expansion to deduplicate deps.

With the attached patch applied, my use case sees libraries resolved only once, and @gavin's repro expands rather than locking up.

Show
Reid McKenzie added a comment - After some digging, the root of the problem is indeed that expand-deps always enqueues all children of every examined dep. The trick is to ensure that we examine every unique dep (tuple of lib and coordinate) exactly once. The goal is to collect all coordinates for a given lib so constraints can be solved consistently, without re-visiting the same coordinates over and over again as showed in my logs. Visiting any given coordinate exactly once also breaks the infinite loop reported by @gavin. The attached patch 000-canonicalized-deps-once.patch reworks expand-deps to do exactly this. It re-orders the determination of the coordinate to be used for any given dep, so that deps are only ever enqueued with their use-coords. This enables expansion to deduplicate deps. With the attached patch applied, my use case sees libraries resolved only once, and @gavin's repro expands rather than locking up.
Hide
Alex Miller added a comment -

The test patch seems wrong (picture in comments doesn’t match the setup, and it succeeds without the patch). While the general idea of the cause is correct, I think there is a better way to solve this in keeping with the original intent and I am working on it. I also think this may cover TDEPS-58.

Show
Alex Miller added a comment - The test patch seems wrong (picture in comments doesn’t match the setup, and it succeeds without the patch). While the general idea of the cause is correct, I think there is a better way to solve this in keeping with the original intent and I am working on it. I also think this may cover TDEPS-58.
Hide
Paul Dumais added a comment -

My call to clj hangs with the following deps.edn:

{:paths ["resources" "src"]
 :deps {org.clojure/clojure {:mvn/version "RELEASE"}
        org.openjfx/javafx-graphics {:mvn/version "11"}}
 :aliases
 {:test {:extra-paths ["test"]
         :extra-deps {org.clojure/test.check {:mvn/version "RELEASE"}}}
  :runner
  {:extra-deps {com.cognitect/test-runner
                {:git/url "https://github.com/cognitect-labs/test-runner"
                 :sha "76568540e7f40268ad2b646110f237a60295fa3c"}}
   :main-opts ["-m" "cognitect.test-runner"
               "-d" "test"]}}}

I'm using ubunu-linux 18.04 LTS, java 10, clj version 1.9.0.397.

Show
Paul Dumais added a comment - My call to clj hangs with the following deps.edn:
{:paths ["resources" "src"]
 :deps {org.clojure/clojure {:mvn/version "RELEASE"}
        org.openjfx/javafx-graphics {:mvn/version "11"}}
 :aliases
 {:test {:extra-paths ["test"]
         :extra-deps {org.clojure/test.check {:mvn/version "RELEASE"}}}
  :runner
  {:extra-deps {com.cognitect/test-runner
                {:git/url "https://github.com/cognitect-labs/test-runner"
                 :sha "76568540e7f40268ad2b646110f237a60295fa3c"}}
   :main-opts ["-m" "cognitect.test-runner"
               "-d" "test"]}}}
I'm using ubunu-linux 18.04 LTS, java 10, clj version 1.9.0.397.
Hide
Peter Monks added a comment -

Regarding org.openjfx/javafx-graphics, I wonder if that's because tools.deps is somehow ignoring classifiers?

That library doesn't have a circular dependency, at least once classifiers are taken into account:

$ lein deps :tree
 [clojure-complete "0.2.4" :exclusions [[org.clojure/clojure]]]
 [com.github.javaparser/javaparser-core "3.7.0" :scope "test"]
 [org.clojure/clojure "1.9.0"]
   [org.clojure/core.specs.alpha "0.1.24"]
   [org.clojure/spec.alpha "0.1.143"]
 [org.clojure/tools.nrepl "0.2.12" :exclusions [[org.clojure/clojure]]]
 [org.openjfx/javafx-controls "11"]
   [org.openjfx/javafx-controls "11" :classifier "mac"]
 [org.openjfx/javafx-fxml "11"]
   [org.openjfx/javafx-fxml "11" :classifier "mac"]
 [org.openjfx/javafx-graphics "11"]
   [org.openjfx/javafx-base "11"]
     [org.openjfx/javafx-base "11" :classifier "mac"]
   [org.openjfx/javafx-graphics "11" :classifier "mac"]
 [org.openjfx/javafx-media "11"]
   [org.openjfx/javafx-media "11" :classifier "mac"]
 [org.openjfx/javafx-swing "11"]
   [org.openjfx/javafx-swing "11" :classifier "mac"]
 [org.openjfx/javafx-web "11"]
   [org.openjfx/javafx-web "11" :classifier "mac"]
 [org.reflections/reflections "0.9.11"]
   [com.google.guava/guava "20.0"]
   [org.javassist/javassist "3.21.0-GA"]

(from fn-fx)

Show
Peter Monks added a comment - Regarding org.openjfx/javafx-graphics, I wonder if that's because tools.deps is somehow ignoring classifiers? That library doesn't have a circular dependency, at least once classifiers are taken into account:
$ lein deps :tree
 [clojure-complete "0.2.4" :exclusions [[org.clojure/clojure]]]
 [com.github.javaparser/javaparser-core "3.7.0" :scope "test"]
 [org.clojure/clojure "1.9.0"]
   [org.clojure/core.specs.alpha "0.1.24"]
   [org.clojure/spec.alpha "0.1.143"]
 [org.clojure/tools.nrepl "0.2.12" :exclusions [[org.clojure/clojure]]]
 [org.openjfx/javafx-controls "11"]
   [org.openjfx/javafx-controls "11" :classifier "mac"]
 [org.openjfx/javafx-fxml "11"]
   [org.openjfx/javafx-fxml "11" :classifier "mac"]
 [org.openjfx/javafx-graphics "11"]
   [org.openjfx/javafx-base "11"]
     [org.openjfx/javafx-base "11" :classifier "mac"]
   [org.openjfx/javafx-graphics "11" :classifier "mac"]
 [org.openjfx/javafx-media "11"]
   [org.openjfx/javafx-media "11" :classifier "mac"]
 [org.openjfx/javafx-swing "11"]
   [org.openjfx/javafx-swing "11" :classifier "mac"]
 [org.openjfx/javafx-web "11"]
   [org.openjfx/javafx-web "11" :classifier "mac"]
 [org.reflections/reflections "0.9.11"]
   [com.google.guava/guava "20.0"]
   [org.javassist/javassist "3.21.0-GA"]
(from fn-fx)
Hide
Alex Miller added a comment -

Resolved issue with not adding children of version when it's not selected in commit 6068e0b. Included test similar to the one here.

Show
Alex Miller added a comment - Resolved issue with not adding children of version when it's not selected in commit 6068e0b. Included test similar to the one here.
Hide
Alex Miller added a comment -

Latest updates in dep resolution and classifier support now address this case.

Show
Alex Miller added a comment - Latest updates in dep resolution and classifier support now address this case.
Hide
Alex Miller added a comment -

Released in tools.deps.alpha 0.6.474

Show
Alex Miller added a comment - Released in tools.deps.alpha 0.6.474
Hide
Chris Nuernberger added a comment -

Hey, sorry I didn't check this out sooner but the chosen fix breaks any dependency viewer based on tools.deps; users cannot see why a particular dependency is chosen or where all conflicts have occurred.

I am reading through the code now to figure out what a middle ground is with the issue or if there is a middle ground at all.

Show
Chris Nuernberger added a comment - Hey, sorry I didn't check this out sooner but the chosen fix breaks any dependency viewer based on tools.deps; users cannot see why a particular dependency is chosen or where all conflicts have occurred. I am reading through the code now to figure out what a middle ground is with the issue or if there is a middle ground at all.
Hide
Alex Miller added a comment -

tools.deps does not yet have any public feature to describe the dependency choices, so I'm not sure what you're using for this. If it's the :verbose flag - that is not considered public api and is subject to change, removal, and change without warning.

I would like to have features that could expose more of this information to users, but I expect that feature would be something completely new.

Show
Alex Miller added a comment - tools.deps does not yet have any public feature to describe the dependency choices, so I'm not sure what you're using for this. If it's the :verbose flag - that is not considered public api and is subject to change, removal, and change without warning. I would like to have features that could expose more of this information to users, but I expect that feature would be something completely new.
Hide
Chris Nuernberger added a comment -

I currently use (-> canonicalize expand-deps):

https://github.com/cnuernber/depsviz/blob/master/src/cnuernber/depsviz/tools_deps.clj#L18

I hacked around in the library for a while and have a patch that passes all the tests but also allows the expand-deps pathway to find dependency conflicts:

https://github.com/cnuernber/tools.deps.alpha/commit/3607b26fbe683fa63c701a957dc7c01c61a56227

I think it is not too much trouble for me to maintain this fork. My use case is special and probably not ideal for the average users; I want to explicitly traverse as much of the potential graph including conflicts as possible and highlight as many conflicts as possible. Most users want to traverse as little as possible as long as the resolution mechanism is consistent but transitive dependencies change and thus their expected behavior of their programs changes.

The great thing about deps.edn is that it was trivial for me to include an unpublished branch that I can ensure always works for my use case. So it is overall a win; I am not sure how much build tooling really does want to know all the possible dependencies. It increases your test manifold significantly for you to include such functionality as a reference.

Show
Chris Nuernberger added a comment - I currently use (-> canonicalize expand-deps): https://github.com/cnuernber/depsviz/blob/master/src/cnuernber/depsviz/tools_deps.clj#L18 I hacked around in the library for a while and have a patch that passes all the tests but also allows the expand-deps pathway to find dependency conflicts: https://github.com/cnuernber/tools.deps.alpha/commit/3607b26fbe683fa63c701a957dc7c01c61a56227 I think it is not too much trouble for me to maintain this fork. My use case is special and probably not ideal for the average users; I want to explicitly traverse as much of the potential graph including conflicts as possible and highlight as many conflicts as possible. Most users want to traverse as little as possible as long as the resolution mechanism is consistent but transitive dependencies change and thus their expected behavior of their programs changes. The great thing about deps.edn is that it was trivial for me to include an unpublished branch that I can ensure always works for my use case. So it is overall a win; I am not sure how much build tooling really does want to know all the possible dependencies. It increases your test manifold significantly for you to include such functionality as a reference.
Hide
Alex Miller added a comment -

I think it would be best to file a new feature issue in TDEPS rather than continuing here. I don't think I would want to take the change you're suggesting in the fork, but would be good to have a new ticket to discuss what a thing might look like.

Without doing too much thinking about it, I would probably suggest some kind of event based api as the most flexible thing.

Show
Alex Miller added a comment - I think it would be best to file a new feature issue in TDEPS rather than continuing here. I don't think I would want to take the change you're suggesting in the fork, but would be good to have a new ticket to discuss what a thing might look like. Without doing too much thinking about it, I would probably suggest some kind of event based api as the most flexible thing.

People

Vote (3)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: