Skip to content
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

Unable to jack-in in some cases when cider-enrich-classpath is t #3581

Closed
rrudakov opened this issue Nov 12, 2023 · 11 comments · Fixed by #3584
Closed

Unable to jack-in in some cases when cider-enrich-classpath is t #3581

rrudakov opened this issue Nov 12, 2023 · 11 comments · Fixed by #3584

Comments

@rrudakov
Copy link
Contributor

Steps to reproduce the problem

  1. Create a simple project with the following files:

deps.edn

{:paths [:clj-paths]

 :deps {}

 :aliases {:clj-paths ["src"]

           :test {:extra-paths ["test"]
                  :extra-deps  {io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}}
                  :main-opts   ["-m" "cognitect.test-runner"]
                  :exec-fn     cognitect.test-runner.api/test}}}

src/core.clj (content is not relevant)

(ns core)

(defn foo []
  (println "Hello"))

test/core_test.clj (content is not relevant)

(ns core-test
  (:require
   [clojure.test :refer [deftest is]]))

(deftest foo-test
  (is (= 4 (+ 2 2))))
  1. Try to jack-in.

Expected behavior

I can successfully jack-in without :test alias or with :test alas

Actual behavior

  1. If I jack-in with default aliases (using normal cider-jack-in command), the REPL is running successfully, but my namespaces are not included to the classpath. The following command is generated by clojure.sh (src path is missing):
/usr/bin/clojure -Sforce -Srepro -J-XX:-OmitStackTraceInFastThrow -J-Dclojure.main.report=stderr -Scp /home/rrudakov/.m2/repository/org/clojure/clojure/1.11.1/clojure-1.11.1.jar:/home/rrudakov/.m2/repository/org/clojure/core.specs.alpha/0.2.62/core.specs.alpha-0.2.62.jar:/home/rrudakov/.m2/repository/org/clojure/spec.alpha/0.3.218/spec.alpha-0.3.218.jar:/home/rrudakov/.m2/repository/cider/cider-nrepl/0.43.3/cider-nrepl-0.43.3.jar:/home/rrudakov/.m2/repository/nrepl/nrepl/1.0.0/nrepl-1.0.0.jar:/home/rrudakov/.m2/repository/refactor-nrepl/refactor-nrepl/3.9.0/refactor-nrepl-3.9.0.jar:/home/rrudakov/.m2/repository/cider/orchard/0.20.0/orchard-0.20.0.jar:/home/rrudakov/.m2/repository/mx/cider/logjam/0.1.1/logjam-0.1.1.jar:/usr/lib/jvm/java-17-temurin/lib/src.zip:/home/rrudakov/.cache/mx.cider/enrich-classpath/1709/1341589140/1730288118.jar:/home/rrudakov/.cache/mx.cider/unzipped-jdk-sources/1709 -J--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED -J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED -m nrepl.cmdline --middleware [refactor-nrepl.middleware/wrap-refactor,cider.nrepl/cider-middleware]
  1. If I jack-in with custom :test alias (either by modifying cider-clojure-cli-aliases to ":test" or by using prefix argument and edit start server command replacing -M:cider/nrepl with -M:test:cider/nrepl at the end) REPL won't start. Instead I see in the *Messages* buffer the output of running tests:
error in process sentinel: Could not start nREPL server: WARNING: Implicit use of clojure.main with options is deprecated, use -M

Running tests in #{"test"}

Testing core-test

Ran 1 tests containing 1 assertions.
0 failures, 0 errors.
 ("finished")

The following command is generated by clojure.sh (looks like cider/nrepl alias is missing or wrong main-opts are picked up):

/usr/bin/clojure -Sforce -Srepro -J-XX:-OmitStackTraceInFastThrow -J-Dclojure.main.report=stderr -Scp test:/home/rrudakov/.m2/repository/org/clojure/clojure/1.11.1/clojure-1.11.1.jar:/home/rrudakov/.m2/repository/org/clojure/core.specs.alpha/0.2.62/core.specs.alpha-0.2.62.jar:/home/rrudakov/.m2/repository/org/clojure/java.classpath/1.0.0/java.classpath-1.0.0.jar:/home/rrudakov/.m2/repository/org/clojure/spec.alpha/0.3.218/spec.alpha-0.3.218.jar:/home/rrudakov/.m2/repository/org/clojure/tools.cli/1.0.206/tools.cli-1.0.206.jar:/home/rrudakov/.m2/repository/org/clojure/tools.namespace/1.3.0/tools.namespace-1.3.0.jar:/home/rrudakov/.m2/repository/org/clojure/tools.reader/1.3.6/tools.reader-1.3.6.jar:/home/rrudakov/.m2/repository/cider/cider-nrepl/0.43.3/cider-nrepl-0.43.3.jar:/home/rrudakov/.m2/repository/nrepl/nrepl/1.0.0/nrepl-1.0.0.jar:/home/rrudakov/.m2/repository/refactor-nrepl/refactor-nrepl/3.9.0/refactor-nrepl-3.9.0.jar:/home/rrudakov/.m2/repository/cider/orchard/0.20.0/orchard-0.20.0.jar:/home/rrudakov/.gitlibs/libs/io.github.cognitect-labs/test-runner/dfb30dd6605cb6c0efc275e1df1736f6e90d4d73/src:/home/rrudakov/.m2/repository/mx/cider/logjam/0.1.1/logjam-0.1.1.jar:/usr/lib/jvm/java-17-temurin/lib/src.zip:/home/rrudakov/.cache/mx.cider/enrich-classpath/1709/1323891204/1730288118.jar:/home/rrudakov/.cache/mx.cider/unzipped-jdk-sources/1709 -J--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED -J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED -m cognitect.test-runner

Environment & Version information

CIDER version information

;; CIDER 1.12.0-snapshot (package: 20231112.518), nREPL 1.0.0
;; Clojure 1.11.1, Java 17.0.9

Lein / Clojure CLI version

Clojure CLI 1.11.1

Emacs version

GNU Emacs 29.1.90 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars) of 2023-10-23

Operating system

6.6.1-arch1-1 GNU/Linux

JDK distribution

openjdk version "17.0.9" 2023-10-17
OpenJDK Runtime Environment Temurin-17.0.9+9 (build 17.0.9+9)
OpenJDK 64-Bit Server VM Temurin-17.0.9+9 (build 17.0.9+9, mixed mode, sharing)
@vemv
Copy link
Member

vemv commented Nov 12, 2023

I had never seen :paths [:clj-paths] syntax I believe - could you comment on it?

@rrudakov
Copy link
Contributor Author

https://clojure.org/reference/deps_and_cli#_paths it's officially supported in deps.edn

@vemv
Copy link
Member

vemv commented Nov 12, 2023

Thanks!

I'll check it around tomorrow.

Feel free to verify if the problem goes away using the traditional syntax (Enrich certainly honors provided aliases)

@rrudakov
Copy link
Contributor Author

Yes, if replace aliases with normal strings, it fixes the first problem. But second problem with :test alias still exists.

@vemv
Copy link
Member

vemv commented Nov 12, 2023

Problem 2 is not unique to Enrich, IME - it's very common in tools.deps projects to unintentionally run the tests.

Over the years (when Enrich wasn't on the radar) I always suggested to split your deps.edn into :test and :test-runner. That way one can e.g. pick an alternative runner.

In general, the best practice is to keep aliases fine-grained - more so than the average Lein project, for instance.

Otherwise I'd be hesitant about 'fixing' this - Enrich supports custom main programs so here, most likely it's doing what you're telling it to do.

@rrudakov
Copy link
Contributor Author

Hm, but why I can successfully jack-in with :test alias enabled when cider-enrich-classpath is nil? I would expect it to fail as well then?

@vemv
Copy link
Member

vemv commented Nov 12, 2023

Enrich supports a broader set of use cases e.g. running it from the terminal, where it's more common to specify a main program (e.g. for specifying a repl - there are a few possible choices).

We may be able to hack something, but I think that nudging users into best practices also is a good idea.

@rrudakov
Copy link
Contributor Author

OK. I've been using :test alias with main-opts for a while without any issues. What's the best way to split it?

{:test {:extra-paths ["test"]
                  :extra-deps  {io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}}}

           :test-runner {:main-opts ["-m" "cognitect.test-runner"]
                         :exec-fn   cognitect.test-runner.api/test}}

If I split it like this I won't be able to run :test-runner without specirying :test additionaly (because of dependencies).

Or should I duplicate all dependencies in both :test and :test-runner aliases? Maybe you have some good article on the topic?

@vemv
Copy link
Member

vemv commented Nov 12, 2023

Thanks for the willingness!

I'd suggest to

  • specify all test-specific dependencies (except those of the runner) and test-specific paths under :test
  • specify io.github.cognitect-labs/test-runner and "-m" "cognitect.test-runner" under :test-runner
  • Running the tests by using -M:test:test-runner

This has the advantages of:

  • allowing dev usages to add :test without bringing the runner
  • allowing other runners (like Kaocha) to reuse :test without duplication or conflicts.

Maybe you have some good article on the topic?

Not really but probably it's easy enough to see how this is a design that favors composition - often valued in Clojure.

@vemv
Copy link
Member

vemv commented Nov 13, 2023

The latest Enrich supports the paths-as-aliases feature.

The issue related to :main-opts will remain as-is for the time being. I'm reasonably confident that we can foster the described, better pattern.

Cheers - V

@rrudakov
Copy link
Contributor Author

Good enough for me :) Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants