clojure-emacs / enrich-classpath

Enriches Lein/deps.edn dependency trees with Java sources, JDK sources, javadocs, etc
Eclipse Public License 2.0
32 stars 9 forks source link

clojure.sh script breaks for some jvm options #64

Closed jpmonettas closed 6 months ago

jpmonettas commented 6 months ago

If we have a deps.edn file like this :

{:deps {}
 :aliases
 {:dev
  {:jvm-opts ["-Dflowstorm.jarEditorCommand=emacsclient <<FILE>>"]}}}

then running on that folder the enrich initialization command:

/home/jmonetta/.emacs.d/elpa/cider-20240423.1541/clojure.sh /usr/local/bin/clojure -Sdeps \{\:deps\ \{nrepl/nrepl\ \{\:mvn/version\ \"1.1.1\"\}\ cider/cider-nrepl\ \{\:mvn/version\ \"0.47.0\"\}\ refactor-nrepl/refactor-nrepl\ \{\:mvn/version\ \"3.10.0\"\}\}\ \:aliases\ \{\:cider/nrepl\ \{\:main-opts\ \[\"-m\"\ \"nrepl.cmdline\"\ \"--middleware\"\ \"\[refactor-nrepl.middleware/wrap-refactor\,cider.nrepl/cider-middleware\]\"\]\}\}\} -M:cider/nrepl:dev

breaks with error :

/home/jmonetta/.emacs.d/elpa/cider-20240423.1541/clojure.sh: line 48: warning: here-document at line 48 delimited by end-of-file (wanted `FILE')

It looks like the eval "$cmd" breaks when a substring like <<FILE>> gets into it.

vemv commented 6 months ago

It looks like you've chosen a funky format :) as it has a mixture of whitespace and characters with Bash significance (<)

I had certainly never seen whitespace in a Java sys prop.

I won't lie, I'm not super enthusiastic about fixing this. From memory, Java system properties don't like being quoted (I had that bug in earlier iterations of Enrich).

But of course, if you find a fix that works, it would be much welcome.

vemv commented 6 months ago

Maybe you can make Flowstorm understand that x means x <<FILE>>?

Seems a pattern that will work with about 99% of all CLI commands out there.

jpmonettas commented 6 months ago

So I was confused, it doesn't have anything to do with <<FILE>>, it has to do with any jvm property that has whitespaces, which should be common for things like folders with spaces.

jpmonettas commented 6 months ago

So cider.enrich-classpath.clojure is emitting properties like -J-Dflowstorm.jarEditorCommand=emacsclient something in the case of whitespaces which doesn't work, but it works if you change it by -J-Dflowstorm.jarEditorCommand="emacsclient something"

vemv commented 6 months ago

Nice!

-J-Dflowstorm.jarEditorCommand="emacsclient something"

If you can generously confirm that this is what the Clojure CLI emits (without Enrich, that is), I'd be happy to mimic that approach.

jpmonettas commented 6 months ago

The clojure tools does it like this :

"${jvm_opts[@]}"

here is the full command

exec "$JAVA_CMD" -XX:-OmitStackTraceInFastThrow $JAVA_OPTS "${jvm_cache_opts[@]}" "${jvm_opts[@]}" "-Dclojure.basis=$basis_file" -classpath "$cp:$install_dir/libexec/exec.jar" clojure.main -m clojure.run.exec "$@"

but I'm not sure, shell scripting is supper confusing to me. Looks like it is quoting all the options so the shell doesn't interpret the spaces?

vemv commented 6 months ago

Interesting, might end stealing some trick 😄

But I'd be happy to simply see what's the result is at runtime (ps aux | grep java) for a prop with spaces, as created by t.deps

jpmonettas commented 6 months ago

Yeah, that was the first thing I tried, but it doesn't show the original command. It shows like this :

/usr/bin/java -XX:-OmitStackTraceInFastThrow -Dflowstorm.jarEditorCommand=emacsclient something -Dclojure.basis=.cpcache/3515095175.basis -classpath src:/home/jmonetta/.m2/repository/org/clojure/clojure/1.11.1/clojure-1.11.1.jar:/home/jmonetta/.m2/repository/org/clojure/core.specs.alpha/0.2.62/core.specs.alpha-0.2.62.jar:/home/jmonetta/.m2/repository/org/clojure/spec.alpha/0.3.218/spec.alpha-0.3.218.jar clojure.main

but if you copy and paste that in the terminal it doesn't work, you need to add the quotes.

vemv commented 6 months ago

Ok, thanks for the attempt!

Anyway, the proposal sounds reasonable. clojure.clj already specifically deals with jvm opts. You could add the quoting after = if present.

Then make install at the root of the repo and kindly try it out for about a week before PR.

Cheers - V

jpmonettas commented 6 months ago

FWIW this change makes it work :

index 8519012..6ccab1c 100644
--- a/tools.deps/src/cider/enrich_classpath/clojure.clj
+++ b/tools.deps/src/cider/enrich_classpath/clojure.clj
@@ -237,7 +237,9 @@
                 [(str "-J" jdk/javac-code-opens)]))
         (into (if-not main
                 []
-                (mapv (partial str "-J")
+                (mapv (fn [opt-str]
+                        (let [[opt-name opt-val] (string/split opt-str #"=")]
+                          (format "-J%s=\"%s\"" opt-name opt-val)))
                       calculated-jvm-opts)))
         (into @eval-option)
         (into (if (seq main-opts)

but I'm not sure if every JVM prop will have that shape

vemv commented 6 months ago

Yeah = isn't always present

jpmonettas commented 6 months ago

Looking at the tests we also have properties like this :

"--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED"

which I don't even know how to read.

vemv commented 6 months ago

True 😄

Looks like you have to hunt for both -D and =