Skip to content

CIDER repl doesn't work with clojure.tools.namespace.repl/refresh classpath #2686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jumarko opened this issue Aug 9, 2019 · 13 comments
Closed

Comments

@jumarko
Copy link

jumarko commented Aug 9, 2019

This is the problem with the classpath as returned by (clojure.java.classpath/classpath) which is used by tools.namespace in combination with JDK 9+.
See also here for additional info: https://ask.clojure.org/index.php/8288/java-11-and-tools-namespace?show=8379#c8379

When I run pure lein repl everything works!

Expected behavior

Running clojure.tools.namespace.repl/refresh after the REPL is started should reload all the project namespaces like this:

(require '[clojure.tools.namespace.repl :as nr :refer [refresh]])
(refresh)
:reloading (lein-sample.core lein-sample.core-test)
:ok

Actual behavior

refresh won't find any directories on the classpath and won't reload anything:

(require '[clojure.tools.namespace.repl :as nr :refer [refresh]])
(refresh)
nil:reloading ()
:ok

The problem is that the following function returns only JDK's src.zip directory nothing else.
(Normally, it's expected to return everything on the classpath):

(clojure.java.classpath/classpath)
;;=> (#object[java.io.File 0x72b58d8c "/Library/Java/JavaVirtualMachines/jdk-11.0.2+9/Contents/Home/lib/src.zip"])

This happens because the clojure/java.classpath contains a special case to support JDK 9+ and doesn't count with anything to be returned via (classpath (clojure.lang.RT/baseLoader)); rather the classpath is expected to be retrieved via (system-classpath) (see the source).

 (or (seq (classpath (clojure.lang.RT/baseLoader)))
       (system-classpath))

The src.zip artifact seems to be added by orchard.

Steps to reproduce the problem

  • git clone [email protected]:jumarko/lein-sample.git
  • Use JDK 9+, preferably JDK 11.
  • cider-jack-in
  • (do (require '[clojure.tools.namespace.repl :as nr :refer [refresh]]) (refresh))

Environment & Version information

CIDER version information

CIDER 0.22.0snapshot (package: 20190808.1659), nREPL 0.6.0
Clojure 1.10.1, Java 11.0.2

Lein/Boot version

lein -v
Leiningen 2.9.1 on Java 11.0.2 OpenJDK 64-Bit Server VM

Emacs version

25.3.1

Using Spacemacs

image

Operating system

Mac OS X 10.14.5

@bbatsov
Copy link
Member

bbatsov commented Aug 9, 2019

The problem is that the following function returns only JDK's src.zip directory nothing else.

Hmm, seems we've messed something up. @jeffvalk Can you take a look in this?

@jeffvalk
Copy link
Contributor

@bbatsov I can't dig too deeply into this right now, but from a quick look, it seems @jumarko is correct that clojure.java.classpath relies on pre-JDK9 behavior. Hence, using this lib on JDK9+ may be its own problem. From the docstring for that lib's classpath function:

If no URLClassloader can be found, as on Java 9, falls back to the 'java.class'path' system property.

Under JDK 9+, this function won't return the actual classpath, only the value of the system property. Obviously, that's not very helpful.

The best solution to this situation would be a version of clojure.java.classpath that's compatible with JDK9+ behavior. Lacking this, there are three options for Cider:

  1. Strip dynamic classpath features from Cider.
  2. Make it clear that Cider provides dynamic classpath features, and such features are not supported by clojure.java.classpath on JDK9+.
  3. Resort to dirty hacks.

@emaphis
Copy link

emaphis commented Aug 15, 2019

This seems to be fixed in version 0.3.1

https://github.com/clojure/tools.namespace/blob/master/CHANGES.md

@jumarko
Copy link
Author

jumarko commented Aug 15, 2019

@emaphis

This seems to be fixed in version 0.3.1

I don't think so.
I do have tools.namespace 0.3.1 on classpath - see the link I referenced: https://ask.clojure.org/index.php/8288/java-11-and-tools-namespace?show=8379#c8379

@stuartsierra
Copy link

stuartsierra commented Sep 13, 2019

I can reproduce this issue, although I have not identified the root cause.

tools.namespace needs to know what directories are on the classpath. It uses java.classpath to get the classpath.

The problem is the return value of (clojure.java.classpath/classpath).
On Java 9+ when launched from CIDER, it returns something like this:

(#object[java.io.File 0x6291e05f "/Library/Java/JavaVirtualMachines/openjdk-12.0.1.jdk/Contents/Home/lib/src.zip"])

This is only when launching Java from CIDER, as with cider-jack-in. If Java is launched from the command line (by lein repl or clj) then (clojure.java.classpath/classpath) works as expected.

In this test, I am using org.clojure/java.classpath 0.3.0.

Background:

There are so many different ways of running Java it's hard to find a universal way to "get the classpath." The current behavior was implemented in CLASSPATH-8 as a workaround for Java 9+. The old behavior in CLASSPATH-1 and CLASSPATH-2 was adapted for application containers.

@bbatsov
Copy link
Member

bbatsov commented Sep 14, 2019

Thanks for looking into this! I'll link here to the bit of code that adds the JDK source to the classpath as I guess that's where things go sideways https://github.com/clojure-emacs/orchard/blob/106cfcd4720bfbd19d04a735316a63084f5a65d3/src/orchard/java.clj#L50

We can always remove the dynamic classpath manipulation, but this will mean that people won't be able to jump into JDK sources.

@jeffvalk
Copy link
Contributor

@stuartsierra

This is only when launching Java from CIDER, as with cider-jack-in. If Java is launched from the command line (by lein repl or clj) then (clojure.java.classpath/classpath) works as expected.

CIDER dynamically adds src.zip to the classpath, if found, which a bare REPL does not.

The issue seems to be that under Java 9+, clojure.java.classpath returns either the dynamically added classpath entries if any, or those on the java.class.path system property, but not both.

The union of these sets gives the expected behavior. This is what CIDER does for namepace/classpath search (see here).

@stuartsierra
Copy link

The current clojure.java.classpath behavior is the result of many compromises and attempted workarounds. At times in the past, it has behaved more like orchard.classpath.

I've opened this up as a new question on ask.clojure.org to solicit feedback: Should java.classpath combine classpath sources?

@jeffvalk
Copy link
Contributor

@stuartsierra Thanks. Classpaths can be messy. It would be great to get consensus around what expected behavior is.

@bbatsov In the meanwhile, I would not call this a bug in CIDER. The functionality in question works as designed.

@bbatsov
Copy link
Member

bbatsov commented Sep 15, 2019

@jeffvalk Yeah, I get this. My point was mostly that we might need to come up with some workaround for the code reloading, while we figure out how this problem can be properly tackled in ctn/java.classpath.

Anyways, I'm grateful that the two of you are looking into this!

@stuartsierra
Copy link

stuartsierra commented Sep 17, 2019

As a workaround for projects using tools.namespace, set-refresh-dirs overrides the set of directories tools.namespace scans for source files.

For example, in the Reloaded template it sets the refresh directories to "dev" "src" and "test". This is deliberate, to prevent scanning non-source directories, such as resources, which may contain .cljc files under some build configurations.

For this to work on Java 9+, one still has to manually load the dev.clj file first. Alternatively, set-refresh-dirs could be called in user.clj, where it would be loaded automatically.

@stale
Copy link

stale bot commented Dec 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale label Dec 16, 2019
@stale
Copy link

stale bot commented Jan 15, 2020

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

@stale stale bot closed this as completed Jan 15, 2020
titonbarua pushed a commit to titonbarua/mount-lite that referenced this issue Dec 16, 2020
….namespace.

- Works with tools.namespace >= 0.3.
- The function now uses `scan-dirs` instead of deprecated `scan`, whenever it can.
- Properly accounts for reloading if `refresh-dirs` is set with `clojure.tools.namespace.repl/set-refresh-dirs` function.
- Note that with newer versions of tools.namespace, setting `refresh-dirs` is mandatory in some cases like this: clojure-emacs/cider#2686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants