refactor-nrepl
refactor-nrepl copied to clipboard
clojure-emacs
Bring hotload-dependency back
Previously this op was built on top of alembic, but that lib no longer seems to be maintained at all so we're moving to pomegranate.
tools.deps would've been another alternative by their add-lib
operation has yet to be merged to the main branch. Once it's merged and
has proven itself in the wild nothing prevents us from moving over,
though, if that solution proves to be better in some way.
@benedekfazekas this seems to fail due to the mranderson pass. Is that something you have time to investigate?
Now that the other PR is merged, I guess you can rebase this one on top of master.
I'd try to replicate this test as part of this PR: https://github.com/clojure-emacs/orchard/blob/dbe1bef72697a89843763dd112d151a9f365e5c8/test/orchard/java/classpath_test/third_party_compat_test.clj - else I'd fear that users of this feature could have issues that could manifest themselves in third-party libs.
(in this case the when-not conditional is not needed)
Now that the other PR is merged, I guess you can rebase this one on top of master.
Done
I'd try to replicate this test as part of this PR
Thanks. That's a good idea. I read some of the discussion you linked yesterday, but it's not entirely clear what this test is testing.
(deftest works
(is (seq (clojure.java.classpath/classpath-directories))
"The presence of `clojure.java` does not affect third-party libraries"))
If you get stuff back it works, otherwise you got an exception as the failure mode?
If you get stuff back it works, otherwise you got an exception as the failure mode?
The side-effects of using dynapath were causing the result of (clojure.java.classpath/classpath-directories) to be an empty list
Are you married to *warn-on-reflection* in the :dev profile @bbatsov? Personally I only turn this on when doing performance tuning (as that's the only downside of reflections).
Adding pomegranate makes the REPL quite noisy on startup now:
Reflection warning, cemerick/pomegranate/aether.clj:109:49 - reference to field getMessage on java.lang.Object can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:109:49 - reference to field getMessage on java.lang.Object can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:143:47 - call to method newLocalRepositoryManager on java.lang.Object can't be resolved (no such method).
Reflection warning, cemerick/pomegranate/aether.clj:173:3 - call to method setSnapshotPolicy can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:173:3 - call to method setReleasePolicy can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:181:7 - call to method addPassword on org.eclipse.aether.util.repository.AuthenticationBuilder can't be resolved (argument types: java.lang.Object).
Reflection warning, cemerick/pomegranate/aether.clj:182:7 - call to method addPrivateKey can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:179:3 - reference to field build can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:188:5 - call to method setAuthentication can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:199:34 - reference to field build can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:201:7 - call to method setProxy can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:362:5 - call to method deploy can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:381:5 - call to method install can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:470:36 - reference to field getDependency can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:476:29 - reference to field getChildren can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:614:31 - call to method setProperties can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:621:39 - reference to field getMirrorSelector can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:622:39 - call to method getMirror can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:626:7 - call to method resolveArtifacts can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:630:10 - call to method resolveVersion can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:630:10 - call to method resolveVersion can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:706:33 - reference to field getArtifact can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:706:33 - call to method setProperties can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:703:15 - call to method setArtifact can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:804:65 - reference to field getMirrorSelector can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:805:65 - call to method getMirror can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:800:31 - call to org.eclipse.aether.collection.CollectRequest ctor can't be resolved.
Reflection warning, cemerick/pomegranate/aether.clj:810:7 - call to method resolveDependencies can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:811:7 - call to method collectDependencies can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate/aether.clj:819:3 - reference to field getRoot can't be resolved.
Reflection warning, cemerick/pomegranate.clj:20:13 - call to method getDeclaredMethod can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate.clj:22:5 - call to method setAccessible can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate.clj:23:5 - call to method invoke can't be resolved (target class is unknown).
Reflection warning, cemerick/pomegranate.clj:32:17 - reference to field getParent can't be resolved.
It's been a while since I ran a snapshot locally and I'm not sure why it isn't working.
I did:
(setq cider-jack-in-lein-plugins
'(("refactor-nrepl/refactor-nrepl" "3.0.0-SNAPSHOT" :predicate cljr--inject-middleware-p)
("cider/cider-nrepl" "0.26.0-SNAPSHOT")))
And that gave me:
[nREPL] Starting server via "c:/programdata/chocolatey/bin/lein.bat" update-in :dependencies conj ^"[nrepl/nrepl \^"0.8.3\^"]^" -- update-in :plugins conj ^"[refactor-nrepl/refactor-nrepl \^"3.0.0-SNAPSHOT\^"]^" -- update-in :plugins conj ^"[cider/cider-nrepl \^"0.26.0-SNAPSHOT\^"]^" -- repl :headless :host localhost
The REPL starts, but with a warning:
WARNING: clj-refactor and refactor-nrepl are out of sync.
Their versions are 2.5.1 (package: 20210413.733) and n/a, respectively.
Checking manually, the middleware isn't responding to the version check and when I inspect the classpath no refactor-nrepl is on there. The refactor-nrepl-3.0.0-SNAPSHOT jar did get downloaded though.
I can't see any errors in any of the nrepl-buffers. Does that mean it just crashed silently on startup? Suggestions?
Are you married to warn-on-reflection in the :dev profile @bbatsov? Personally I only turn this on when doing performance tuning (as that's the only downside of reflections).
I don't care about the reflection warnings, but many people do. If we disable this when we develop, then they'll suffer when we ship.
Specifically, it can be a pain when one invokes lein repl in end-user projects with warn-on-reflection and one sees a wall of warns coming from clojure-emacs tools.
i.e. even if performance might mostly be unaffected, it's pretty noisy.
Good points! Hadn't considered that it'll look real bad when someone turns on *warn-on-reflection* in their own project and refactor-nrepl spews a wall of warnings.
Guess it might be prudent to fix these reflection warnings in pomegranate prior to cutting the 3.0.0 release. I'm sure they'll be happy with a PR over at clj-commons fixing this.
edit: I'll hold off on doing that until we get this working, though.
The only reason I know that people care about this is because people are constantly complaining when there are oversights in Orchard and cider-nrepl. 😆
@bbatsov I think something went wrong when you built the latest snapshot. It's almost half the size of the 2.5.1 release.
When I inspect it all the .class files of the inlined deps are missing:
Or maybe that's the new normal with the latest mranderson release?
I did a "make clean deploy", as I always do. Still, it seems the deps weren't properly inlined.
Rebased.
The lint run fails and says this:
Possibly confusing dependencies found:
[clj-commons/pomegranate "1.2.1" :exclusions [org.clojure/clojure org.slf4j/jcl-over-slf4j org.tcrawley/dynapath]] -> [org.apache.httpcomponents/httpclient "4.5.8"]
overrides
[clj-commons/pomegranate "1.2.1" :exclusions [org.clojure/clojure org.slf4j/jcl-over-slf4j org.tcrawley/dynapath]] -> [org.apache.maven.wagon/wagon-http "3.3.4"] -> [org.apache.maven.wagon/wagon-http-shared "3.3.4"] -> [org.apache.httpcomponents/httpclient "4.5.9" :exclusions [commons-logging]]
and
[clj-commons/pomegranate "1.2.1" :exclusions [org.clojure/clojure org.slf4j/jcl-over-slf4j org.tcrawley/dynapath]] -> [org.apache.maven.wagon/wagon-http "3.3.4"] -> [org.apache.httpcomponents/httpclient "4.5.9" :exclusions [commons-logging]]
Consider using these exclusions:
[clj-commons/pomegranate "1.2.1" :exclusions [org.slf4j/jcl-over-slf4j org.clojure/clojure org.apache.httpcomponents/httpclient org.tcrawley/dynapath]]
Aborting due to :pedantic? :abort
Error encountered performing task 'run' with profile(s): 'base,system,user,provided,1.10,clj-kondo'
Suppressed exit
Makefile:21: recipe for target 'kondo' failed
make: *** [kondo] Error 1
Exited with code exit status 2
CircleCI received exit code 2
Yet locally I get no conflicting deps:
$ lein deps :tree
[cider/cider-nrepl "0.25.9" :scope "provided" :exclusions [[org.clojure/clojure]]]
[cider/orchard "0.7.1" :exclusions [[org.clojure/clojure]]]
[org.tcrawley/dynapath "1.1.0" :exclusions [[org.clojure/clojure]]]
[cider/piggieback "0.5.2" :scope "test" :exclusions [[org.clojure/clojure]]]
[clj-commons/fs "1.6.307" :exclusions [[org.clojure/clojure]]]
[org.apache.commons/commons-compress "1.20"]
[org.tukaani/xz "1.8"]
[clj-commons/pomegranate "1.2.1" :exclusions [[org.clojure/clojure] [org.slf4j/jcl-over-slf4j] [org.tcrawley/dynapath]]]
[org.apache.httpcomponents/httpclient "4.5.8"]
[commons-codec "1.11"]
[commons-logging "1.2"]
[org.apache.httpcomponents/httpcore "4.4.11"]
[org.apache.maven.resolver/maven-resolver-api "1.3.3"]
[org.apache.maven.resolver/maven-resolver-connector-basic "1.3.3"]
[org.apache.maven.resolver/maven-resolver-impl "1.3.3"]
[org.apache.maven.resolver/maven-resolver-spi "1.3.3"]
[org.apache.maven.resolver/maven-resolver-transport-file "1.3.3"]
[org.apache.maven.resolver/maven-resolver-transport-http "1.3.3"]
[org.apache.maven.resolver/maven-resolver-transport-wagon "1.3.3"]
[org.apache.maven.resolver/maven-resolver-util "1.3.3"]
[org.apache.maven.wagon/wagon-http "3.3.4"]
[org.apache.maven.wagon/wagon-http-shared "3.3.4"]
[org.jsoup/jsoup "1.12.1"]
[org.apache.maven.wagon/wagon-provider-api "3.3.4"]
[org.apache.maven/maven-resolver-provider "3.6.1"]
[javax.inject "1"]
[org.apache.maven/maven-model-builder "3.6.1"]
[org.apache.maven/maven-artifact "3.6.1"]
[org.apache.commons/commons-lang3 "3.8.1"]
[org.apache.maven/maven-builder-support "3.6.1"]
[org.codehaus.plexus/plexus-component-annotations "1.7.1" :exclusions [[junit]]]
[org.codehaus.plexus/plexus-interpolation "1.25"]
[org.apache.maven/maven-model "3.6.1"]
[org.apache.maven/maven-repository-metadata "3.6.1"]
[org.codehaus.plexus/plexus-utils "3.2.0"]
[org.slf4j/slf4j-api "1.7.25"]
[cljfmt "0.7.0" :exclusions [[org.clojure/clojure]]]
[com.googlecode.java-diff-utils/diffutils "1.3.0"]
[org.clojure/tools.cli "1.0.194"]
[rewrite-cljs "0.4.5"]
[clojure-complete "0.2.5" :exclusions [[org.clojure/clojure]]]
[com.google.code.findbugs/jsr305 "3.0.2" :scope "provided" :exclusions [[org.clojure/clojure]]]
[com.google.errorprone/error_prone_annotations "2.1.3" :scope "provided" :exclusions [[org.clojure/clojure]]]
[commons-io "2.8.0" :scope "test" :exclusions [[org.clojure/clojure]]]
[http-kit "2.5.1" :exclusions [[org.clojure/clojure]]]
[nrepl "0.8.3" :exclusions [[org.clojure/clojure]]]
[org.clojure/clojure "1.9.0" :scope "provided" :exclusions [[org.clojure/clojure]]]
[org.clojure/core.specs.alpha "0.1.24" :scope "provided"]
[org.clojure/spec.alpha "0.1.143" :scope "provided"]
[org.clojure/clojurescript "1.10.520" :scope "test" :exclusions [[org.clojure/clojure]]]
[com.cognitect/transit-clj "0.8.309" :scope "test" :exclusions [[org.clojure/clojure]]]
[com.cognitect/transit-java "0.8.332" :scope "test"]
[com.fasterxml.jackson.core/jackson-core "2.8.7" :scope "test"]
[org.msgpack/msgpack "0.6.12" :scope "test"]
[com.googlecode.json-simple/json-simple "1.1.1" :scope "test" :exclusions [[junit]]]
[org.javassist/javassist "3.18.1-GA" :scope "test"]
[com.google.javascript/closure-compiler-unshaded "v20180805" :scope "test"]
[args4j "2.0.26" :scope "test"]
[com.google.code.gson/gson "2.7" :scope "test"]
[com.google.guava/guava "25.1-jre" :scope "test"]
[com.google.j2objc/j2objc-annotations "1.1" :scope "test"]
[org.checkerframework/checker-qual "2.0.0" :scope "test"]
[org.codehaus.mojo/animal-sniffer-annotations "1.14" :scope "test"]
[com.google.javascript/closure-compiler-externs "v20180805" :scope "test"]
[com.google.jsinterop/jsinterop-annotations "1.0.0" :scope "test"]
[com.google.protobuf/protobuf-java "3.0.2" :scope "test"]
[org.clojure/google-closure-library "0.0-20170809-b9c14c6b" :scope "test"]
[org.clojure/google-closure-library-third-party "0.0-20170809-b9c14c6b" :scope "test"]
[org.mozilla/rhino "1.7R5" :scope "test"]
[org.clojure/data.json "2.3.1" :exclusions [[org.clojure/clojure]]]
[org.clojure/tools.analyzer.jvm "1.1.0" :exclusions [[org.clojure/clojure]]]
[org.clojure/core.memoize "1.0.236"]
[org.clojure/core.cache "1.0.207"]
[org.clojure/data.priority-map "1.0.0"]
[org.clojure/tools.analyzer "1.0.0"]
[org.ow2.asm/asm "5.2"]
[org.clojure/tools.namespace "1.1.0" :exclusions [[org.clojure/clojure] [org.clojure/tools.reader]]]
[org.clojure/java.classpath "1.0.0"]
[org.clojure/tools.reader "1.3.5" :exclusions [[org.clojure/clojure]]]
[pjstadig/humane-test-output "0.8.1" :exclusions [[org.clojure/clojure]]]
[rewrite-clj "0.6.1" :exclusions [[org.clojure/clojure]]]
[version-clj "1.0.0" :exclusions [[org.clojure/clojure]]]
I tried just taking the suggestion of adding org.apache.httpcomponents/httpclient to the list of exclusions for pomegranate, but it then fails with a ClassNotFoundException (which would be expected if there only was one version of this dep).
What's going on here?
Yet locally I get no conflicting deps:
check out how :pedantic is set. You can use CI=true lein deps or maybe edit temporarily project.clj
I tried just taking the suggestion of adding org.apache.httpcomponents/httpclient to the list of exclusions for pomegranate, but it then fails with a ClassNotFoundException (which would be expected if there only was one version of this dep).
Yeah :pedantic doesn't always make an optimal suggestion. In this case, instead of an exclusion I'd add an explicit dependency on org.apache.httpcomponents/httpclient "4.5.9" (normally I pick the latest version when multiple are offered)
p.s. I self-assigned a couple issues from our tracker, I assume no overlap?
p.s. I self-assigned a couple issues from our tracker, I assume no overlap?
Yeah, just trying to figure out why the build is failing :)
@benedekfazekas Best I can tell this is the problem:
Line 180 in aether.clj is this:
(defn- authentication
[{:keys [username password passphrase private-key-file] :as settings}]
(-> (AuthenticationBuilder.)
(.addUsername username) <----------
(.addPassword password)
(.addPrivateKey private-key-file passphrase)
.build))
If we check out AuthenticationBuilder.java it requires javax.net.ssl.HostnameVerifier and that isn't in the dir with renamed deps:
check out how :pedantic is set. You can use CI=true lein deps or maybe edit temporarily project.clj
Thanks for the tip, @vemv! For some reason I thought lein deps :tree would print the same information, but only error if pedantic? was true...
Yeah :pedantic doesn't always make an optimal suggestion. In this case, instead of an exclusion I'd add an explicit dependency on org.apache.httpcomponents/httpclient "4.5.9" (normally I pick the latest version when multiple are offered)
Ended up with this solution to the problem 👍
What remains to be done here? Someone just asked me about the state of hotloading and I was reminded about this PR.
@bbatsov mranderson fails to create a viable jar so the integration testing remains. I did manage to pull a new artifact into the REPL manually, though.
I believe I found the problem mranderson runs into: a javax.* class is renamed in an import clause, but not copied into the jar under the new name. Presumably the fix would be to exclude javax classes from getting processed, much like we probably don't process java.util.*?
I tried taking a look in mranderson but it doesn't run on Windows (different path separator) and I don't have a Linux box readily available just now.
will try to find time to have a look tmrw
Benedek Fazekas
On Tue, Aug 3, 2021 at 10:39 AM Lars Andersen @.***> wrote:
@bbatsov https://github.com/bbatsov mranderson fails to create a viable jar. I believe I found the problem: a javax.* class is renamed in an import clause, but not copied into the jar under the new name. Presumably the fix would be to exclude javax classes from getting processed, much like we probably don't process java.util.*?
I tried taking a look in mranderson but it doesn't run on Windows (different path separator) and I don't have a Linux box readily available just now.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/clojure-emacs/refactor-nrepl/pull/301#issuecomment-891696791, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHU7HR27P6T6YX75PYM2PLT262LVANCNFSM47JM7HBA .
have a quick fix that I don't really like and seems that properly fixing this needs a bit more digging. meanwhile as a workaround try to exclude javax.inject from clj-commons/pomegranate deps, locally that gave me some good results, altho I had one failing test, maybe just flaky... let me know if this workaround helps
pushed the exclusion on my own accord maybe that failing test means exclusion not such a good idea?!
Thanks for taking a look @benedekfazekas!
all seems to be well but needs a manual test
I'll try to get that done this week.
@bbatsov or @benedekfazekas could you deploy a snapshot for testing? 🙏
Should we merge this one? I know there's some outstanding feedback from @vemv, but there hasn't been much movement here in the past month and it seems that the PR is an a decent shape. We can follow-up with improvements after it's merged.
I'll add the suggested deftest, at least we should be certain we're not breaking people not using this feature (which is exactly what happened with Orchard)
Btw something might be learned from https://github.com/lambdaisland/classpath . We could keep an eye on how it evolves. It would be kind of undesirable to have N implementations of the "mutable classpath" goal in the Clojure community - it's a hard problem (particularly across JDKs) so the more knowledge is reused the better.