Odd behavior with kaocha.classpath/add-classpath + clojure.java.classpath/classpath

[robhanlon22]

The implementation of clojure.java.classpath/classpath returns a vector of classpath entries. If Clojure's base classloader has any entries in it, values from the system classpath are not returned. This causes some surprising interactions—for instance, running kaocha.repl/run causes clojure.tools.namespace.repl/refresh to stop functioning correctly in its default configuration, as by default tools.namespace falls back to classpath-directories if the directories it scans for dependencies aren't explicitly set. After running kaocha.repl/run, that vector only contains the directories in which tests reside, so tools.namespace no longer tracks and reloads dependencies correctly.

I am able to work around this with the skip-add-classpath? configuration value added in #144, but I think that this behavior is surprising enough that it should be revisited—it took some spelunking to figure out what was going on.

[plexus]

The kaocha.classpath code is the result of a lot of trial and error and reading through Clojure compiler code to come up with something that works. It's not great, but I'm not sure there's a better way. See also my cry for help on ClojureVerse two years back.

If someone actually knows what they're doing and can show a way to do this that works with lein/clojure cli/boot, and does not aversely interact with nrepl, tools.namespace or other tools then I would be eternally grateful. Until then I don't see what we can really do here.

What is worth thinking about is what the default behavior of Kaocha should be. The idea of adding these entries to the classpath is that you don't have to duplicate that information between deps.edn and tests.edn. In practice most people put it in both places anyway because Kaocha is not the only one running those tests (e.g. driving them from the REPL).

Right now we have the #kaocha/v1 reader tag, which normalizes your configuration, and sets various defaults. I think it's worth thinking about introducing a #kaocha/v2, which has different defaults, in particular disabling the things that surprise people

• automatic classpath handling
• output capturing
• missing (is)

and instead show in the docs how to enable these explicitly (and still make that the recommendation, at least for the last two).

[robhanlon22]

I ended up getting this to work with tools.namespace by using the following config hook:

(defn add-system-classpath-entries
  "Kaocha dynamically modifies the classpath as it discovers tests. However, it
  doesn't include the existing entries from the system classpath before adding
  its own discovered classpath entries, which causes odd interactions with
  tools.namespace."
  [config]
  (run! kaocha.classpath/add-classpath
        (clojure.java.classpath/system-classpath))
  config)

This ended up being more robust, as I had a few situations where :skip-add-classpath? caused issues. It might be worth making this the default behavior.

[plexus]

My guess is that what you are essentially doing here is adding all entries from the whole hierarchy of classloaders to the top level classloader. If that fixes your issue then that probably means that tools.namespace is not accounting for parent class loaders. Maybe we should open an issue there or look at the code they use to collect class path entries.

[robhanlon22]

It’s falling back to org.clojure/java.classpath, which doesn’t account for the whole hierarchy of classloaders: https://github.com/clojure/java.classpath/blob/master/src/main/clojure/clojure/java/classpath.clj#L86

[plexus]

Hmm right, so I guess our options are limited

• propose fixing/changing clojure.java.classpath -> 99% sure that is not going to happen (which makes sense, there will be people relying on the current behavior)
• propose changing how tools.namespace looks up the classpath -> 95% sure that is also not going to happen

So I guess what's left is us dealing with it by essentially doing what you're doing, copy all entries from all URLClassloader instances in the hierarchy to the top-level DynamicClassloader. I think it's ok to have this available but opt-in. I guess the main thing to watch out for there is that we don't change the lookup order, so if a resource appears multiple times in the classpath we don't get back the wrong one.

Ultimately I think this is a contract issue, classloaders are supposed to abstract away resource lookup. We think of "the classpath" as being something like the $PATH, a series of directories where lookup happens, but ultimately the classloader abstraction hides that away, and could be doing something very different to find resources. So code that looks up and deals with "the classpath" is breaking that abstraction. Which I think is ultimately why tools that deal with and manipulate "the classpath" end up breaking each other in interesting ways :)

Which is also why I think this "fix" will break someones use case, given enough users. But having it opt-in won't hurt.

[plexus]

Having spent much more time looking into classloader issues I'm now convinced the clojure.java.classpath behavior is a bug, I've reported it here: https://clojure.atlassian.net/browse/CLASSPATH-9

I believe this was introduced in the process of adjusting c.j.cp to java 9, where the app classloader is no longer an instance of URLClassLoader

[alysbrooks]

There hasn't been any response to CLASSPATH-9 since Arne reported it in August 2021. :(