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

Always prepend -m with -M to avoid deprecation warning #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rrudakov
Copy link

Hopefully close #37

@vemv
Copy link
Member

vemv commented Feb 21, 2024

Thanks much! Looks nice to me, will take another look asap

@vemv
Copy link
Member

vemv commented Mar 3, 2024

@rrudakov my analysis would be that this namespace should generally forward what's given as an argument - it shouldn't fix stuff for the user.

Our official clojure.sh script already uses -M -m.

I suspect that what happens is that Elisp side, defun cider-clojure-cli-jack-in-dependencies does not emit a correct -M -m call?

Analysis / PR welcome for cider.

Cheers - V

@rrudakov
Copy link
Author

rrudakov commented Mar 4, 2024

Hi @vemv. This PR is not related to elisp or emacs. I want to start nrepl outside of emacs and connect to it using cider-connect (I followed this manual). The problem is that generated command in .enrich-classpath-deps-repl doesn't have -M before -m, so I'm constantly getting a deprecation warning when I'm trying to connect to nREPL.

@rrudakov
Copy link
Author

rrudakov commented Mar 4, 2024

A bit more details.

I have the following alias in ~/.clojure/deps.edn:

:nREPL
  {:extra-deps {com.kohlschutter.junixsocket/junixsocket-common        {:mvn/version "2.9.0"}
                com.kohlschutter.junixsocket/junixsocket-native-common {:mvn/version "2.9.0"}
                nrepl/nrepl                                            {:mvn/version "1.1.1"}
                cider/cider-nrepl                                      {:mvn/version "0.45.0"}
                refactor-nrepl/refactor-nrepl                          {:mvn/version "3.9.1"}}
   :main-opts  ["-m" "nrepl.cmdline"
                "--middleware" "[refactor-nrepl.middleware/wrap-refactor,cider.nrepl/cider-middleware]"
                "--socket" "nrepl.sock"]}

If I copy the Makefile from the README and add :nREPL alias to DEPS_MAIN_OPTS the generated command in the .enrich-classpath-deps-repl will be a huge string which ends with J-Djdk.attach.allowAttachSelf -m nrepl.cmdline --middleware [refactor-nrepl.middleware/wrap-refactor,cider.nrepl/cider-middleware] --socket nrepl.sock.

As you can see -m is not prepended with -M, but it should be.

clojure.sh adds main-opts in a different way as far as I can see, if I jack-in from emacs the command is the following:

/Users/rrudakov/.emacs.d/elpa/cider-20240220.720/clojure.sh /opt/homebrew/bin/clojure -Sdeps \{\:deps\ \{nrepl/nrepl\ \{\:mvn/version\ \"1.1.1\"\}\ cider/cider-nrepl\ \{\:mvn/version\ \"0.45.0\"\}\ refactor-nrepl/refactor-nrepl\ \{\:mvn/version\ \"3.9.1\"\}\}\ \:aliases\ \{\:cider/nrepl\ \{\:main-opts\ \[\"-m\"\ \"nrepl.cmdline\"\ \"--middleware\"\ \"\[refactor-nrepl.middleware/wrap-refactor\,cider.nrepl/cider-middleware\]\"\]\}\}\} -M:cider/nrepl

It doesn't pass main-opts to clojure, it just uses alias, not sure if the same effect could be achieved with Makefile.

@vemv
Copy link
Member

vemv commented Mar 4, 2024

Would it be valid if deps.edn specified the following?

:main-opts  ["-M" "-m" "nrepl.cmdline"

(edit: probably this is a bad idea if not invalid - better focus on my next post)

@vemv
Copy link
Member

vemv commented Mar 4, 2024

Also, the Makefile says:

# Expected format: "-M:alias1:alias2"
DEPS_MAIN_OPTS ?= "-M:dev:test"

So -M is expected - are you passing it? Does the code strip it later?

@rrudakov
Copy link
Author

rrudakov commented Mar 4, 2024 via email

@rrudakov
Copy link
Author

rrudakov commented Mar 4, 2024

So -M is expected - are you passing it? Does the code strip it later?

Yes.

My Makefile:

.PHONY: enrich-nrepl nrepl
HOME=$(shell echo $$HOME)
HERE=$(shell echo $$PWD)

.DEFAULT_GOAL := nrepl

SHELL = /bin/bash -Eeu

DEPS_MAIN_OPTS ?= "-M:local/backend:test/clj:nREPL"

ENRICH_CLASSPATH_VERSION="1.19.0"

.enrich-classpath-deps-repl: Makefile deps.edn $(wildcard $(HOME)/.clojure/deps.edn) $(wildcard $(XDG_CONFIG_HOME)/.clojure/deps.edn)
	cd $$(mktemp -d -t enrich-classpath.XXXXXX); clojure -Sforce -Srepro -J-XX:-OmitStackTraceInFastThrow -J-Dclojure.main.report=stderr -Sdeps '{:deps {mx.cider/tools.deps.enrich-classpath {:mvn/version $(ENRICH_CLASSPATH_VERSION)}}}' -M -m cider.enrich-classpath.clojure "clojure" "$(HERE)" "true" "-M" $(DEPS_MAIN_OPTS) | grep "^clojure" > $(HERE)/$@

enrich-nrepl: .enrich-classpath-deps-repl
	@if grep --silent "^clojure" .enrich-classpath-deps-repl; then \
		eval $$(cat .enrich-classpath-deps-repl); \
	else \
		echo "Falling back to Clojure repl... (you can avoid further falling back by removing .enrich-classpath-deps-repl)"; \
		clojure $(DEPS_MAIN_OPTS); \
	fi

nrepl:
	clojure -M:local/backend:test/clj:nREPL

the full generated command in .enrich-classpath-deps-repl is:

clojure "-M:local/backend:test/clj:nREPL" \
-Sforce \
-Srepro \
-J-XX:-OmitStackTraceInFastThrow \
-J-Dclojure.main.report=stderr \
-Scp src/clj:test/clj:test/resources:env/dev/clj:resources:/Users/rrudakov/.m2/repository/org/clojure/clojure/1.11.1/clojure-1.11.1.jar:/Users/rrudakov/.m2/repository/org/clojure/core.async/1.6.673/core.async-1.6.673.jar:................huge-list-here...........................:/Users/rrudakov/.m2/repository/io/netty/incubator/netty-incubator-transport-native-io_uring/0.0.18.Final/netty-incubator-transport-native-io_uring-0.0.18.Final-linux-x86_64.jar:/Users/rrudakov/.cache/mx.cider/enrich-classpath/17010/3847723509/582121401.jar:/Users/rrudakov/.cache/mx.cider/unzipped-jdk-sources/17010 \
-J--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED \
-J-XX:-OmitStackTraceInFastThrow \
-J-client \
-J-XX:+TieredCompilation \
-J-XX:TieredStopAtLevel=1 \
-J-Xmx4g \
-J-Dclojure.tools.logging.factory=clojure.tools.logging.impl/slf4j-factory \
-J-Djdk.attach.allowAttachSelf \
-m nrepl.cmdline \
--middleware [refactor-nrepl.middleware/wrap-refactor,cider.nrepl/cider-middleware] \
--socket nrepl.sock

I see my aliases are specified right after clojure. Actually I'm not sure that we need to pass :main-opts at the end at all, won't they be picked from the :nREPL alias?

@vemv
Copy link
Member

vemv commented Mar 4, 2024

Actually I'm not sure that we need to pass :main-opts at the end at all, won't they be picked from the :nREPL alias?

Probably it came to be like that for a good reason (cannot immediately check).

Thanks much for helping out with the analysis.

I'll give it another review during the week.

Perhaps you can contribute another unit test for the -A behavior? If the user specifies -A, should a -M be prepended? Maybe the right behavior is different for -A relative to -M?

@rrudakov
Copy link
Author

rrudakov commented Mar 4, 2024 via email

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 this pull request may close these issues.

clojure: use -M -m
2 participants