cljdoc / cljdoc

πŸ“š A central documentation hub for the Clojure community
https://cljdoc.org
Eclipse Public License 2.0
538 stars 78 forks source link

Analyzer doesn't fetch dependencies from repositories defined in the pom #144

Closed IamDrowsy closed 5 years ago

IamDrowsy commented 5 years ago

During code analysis, all dependencies are fetched from clojars or maven central. The pom can define other maven repositories to look for dependencies but the analyzer ignores them and might fail. As this is not that uncommon in java land, the anlalyzer should search the repositories defined in a the pom.

martinklepsch commented 5 years ago

Some notes for people who would like to help with this:

There's cljdoc.util.pom that extracts some stuff from POM files using jsoup. You could then require that namespace in cljdoc.analysis.runner and change how the deps.edn is generated here:

https://github.com/cljdoc/cljdoc/blob/67fbc69c0f0d78f32c7914dd2761307f46d0f591/modules/analysis-runner/src/cljdoc/analysis/runner.clj#L88-L89

For background: The cljdoc.analysis.runner is the main entry point for running analysis on libraries. To improve dependency isolation that namespace will shell out to launch additional clojure processes with appropriate configuration via -Sdeps.

I assume it might be enough to customise :mvn/repos at the top level of the deps.edn map. (I assume that map is merged with the default Clojars/Central setup but not 100% sure.)

martinklepsch commented 5 years ago

Oh and for testing it might be helpful to just use either:

./script/cljdoc ingest -p some/project -v 0.1.0

or

# ./script/analyze.sh project version jar_path pom_path
./script/analyze.sh reagent 0.8.1 ~/.m2/repository/reagent/reagent/0.8.1/reagent-0.8.1.jar ~/.m2/repository/reagent/reagent/0.8.1/reagent-0.8.1.pom
IamDrowsy commented 5 years ago

Adding repos from the pom to the deps.edn is pretty straight forward (https://github.com/IamDrowsy/cljdoc/commit/0ccc7a99c25f9adb85c8efe38abce9471038baa7)

Running

./script/cljdoc ingest -p cloudship -v 0.1.0

now is able to fetch the dependencies. But now the analyzer fails with a pretty cryptic error can't make any sense of right now.

martinklepsch commented 5 years ago

I think I found the issue with the changes:

diff --git a/modules/analysis-runner/src/cljdoc/analysis/runner.clj b/modules/analysis-runner/src/cljdoc/analysis/runner.clj
index a11ba2d..8067847 100644
--- a/modules/analysis-runner/src/cljdoc/analysis/runner.clj
+++ b/modules/analysis-runner/src/cljdoc/analysis/runner.clj
@@ -86,6 +86,7 @@
                            ;; (println "Classpath:" (:out (sh/sh "clojure" "-Sdeps" (pr-str {:deps deps})
                            ;;                                    "-Spath" :dir (.getParentFile f))))
                            (let [process (sh/sh "clojure" "-Sdeps" (pr-str {:deps deps
+                                                                            :mvn/repos (deps/extra-repos pom)
                                                                             :paths [(.getAbsolutePath impl-src-dir)]})
                                                 "-m" "cljdoc.analysis.impl"
                                                 (pr-str namespaces) jar-contents-path platf (.getAbsolutePath f)
diff --git a/modules/shared-utils/src/cljdoc/util/deps.clj b/modules/shared-utils/src/cljdoc/util/deps.clj
index d54f0aa..b8852e4 100644
--- a/modules/shared-utils/src/cljdoc/util/deps.clj
+++ b/modules/shared-utils/src/cljdoc/util/deps.clj
@@ -59,7 +59,7 @@
                  [(symbol group-id artifact-id) {:mvn/version version}])))
        (into {})))

-(defn- extra-repos
+(defn extra-repos
   [pom]
   (->> (slurp pom)
        (pom/parse)
@@ -67,19 +67,12 @@
        (map (fn [repo] [(:id repo) (dissoc repo :id)]))
        (into {})))

-(defn- assoc-if-not-empty
-  [m k v]
-  (if (not-empty v)
-    (assoc m k v)
-    m))
-
 (defn deps [pom project version]
   (-> (extra-deps pom)
       (merge (hardcoded-deps project))
       (ensure-required-deps)
       (add-cljdoc-codox)
       (ensure-recent-ish)
-      (assoc-if-not-empty :mvn/repos (extra-repos pom))
       (assoc project {:mvn/version version})))

Basically the :mvn/repos key went into the :deps map. The error arguably wasn't super helpful in discovering that but well... πŸ˜†

Want to open a PR with this then?

IamDrowsy commented 5 years ago

Oh, I'm a bit confused because there are examples on https://clojure.org/reference/deps_and_cli where the :mvn/repos is put under :deps, and at least for the retrieval of the deps it worked :). But it's a lot nicer this way.

martinklepsch commented 5 years ago

Oh really? Could you point out the spot in the docs where this is? Maybe it’s a typo that could be fixed or is actually valid in some situations...

IamDrowsy commented 5 years ago

Oh wow, sorry I just had a third and fourth look and realized I'm bad at counting braces. πŸ˜†