Add ability to skip dynapath-based functionality #114

vemv · 2021-04-11T12:34:18Z

Fixes #113
Closes #112

The disabling is opt-in, i.e. nothing is changed (relative to current behavior) by default.

In order to exercise the proposed changes properly (and also, to prove that a plugin-based approach can work), it is proposed that the build matrix exercises both the dynapath and no-dynapath choices.

⚠️ Between and a tenative addition of JDK16 to the matrix, the matrix would become quite extensive - perhaps a bit over the top?

Green build in my account:

⚠️ for the no-dynapath choice and JDK 8, a CI job will take 3m instead of 30-60s. Seems a legit issue, can investigate.

The commits are consistent with our contribution guidelines
You've added tests to cover your change(s)
All tests are passing
The new code is not generating reflection warnings
You've updated the changelog (if adding/changing user-visible functionality)

vemv · 2021-04-11T12:39:18Z

project.clj

+               (clojure.java.io/file parent "lib" f)]]
+    (->> paths (filter #(.canRead ^java.io.File %)) first str)))
+
+(def jdk-sources


These top-level defs can be considered an "inline Lein plugin" for fetching the JDK sources and adding them to the classpath without altering classloaders.

They are not essential to this PR, but they allow the tests depending on the JDK sources being reachable to still pass.

...One could opt instead for disabling the related tests when dynapath is disabled, but disabling tests seems delicate (less coverage, less confidence).

It also seems a useful point to show that consumers can add JDK sources now, with little code.

Admittedly it would be more tidy to extract this to a Lein plugin, but first I'd like to know what you think of the approach to begin with.

It seems reasonable overall. The code itself is faily simple.

vemv · 2021-04-11T12:41:03Z

project.clj

+                   (java-path->zip-path "java/lang/Object.java") ; JDK8-
+        (jdk-find "src.zip"))))
+
+(defn uncompress [path target]


For some reason simply adding src.zip to the classpath didn't work. I tried different variations (prepending file:, appending !/), no luck.

One has to uncompress src.zip instead, and add the resulting dir to the classpath.

(this adds ~5s to the build, doesn't seem worth caching... I'd be afraid of a given JDK creating a cache for a different JDK)

I think that's fine.

vemv · 2021-04-11T12:41:34Z

project.clj

+             :no-dynapath {:jvm-opts ["-Dorchard.fetch-java-sources-via-dynapath=false"]
+                           :resource-paths [~(unzipped-jdk-source)]
+                           :plugins ~(if jdk8?
+                                       '[[lein-jdk-tools "0.1.1"]]


Ref: https://github.com/pallet/lein-jdk-tools

Turned out to be necessary.

Why so?

It compensates, when needed, for the loss of this functionality:

orchard/src/orchard/java.clj

Lines 73 to 81 in f763175

(def jdk-tools

"The `tools.jar` path, for JDK8 and earlier. If found on the existing

classpath, this is the corresponding classpath entry. Otherwise, if available,

this is added to the classpath."

(when (<= misc/java-api-version 8)

(or (some-> (io/resource "com/sun/javadoc/Doc.class")

^JarURLConnection (. openConnection)

(. getJarFileURL))

(some-> (jdk-find "tools.jar") cp/add-classpath!))))

bbatsov · 2021-04-13T05:00:04Z

README.md

@@ -91,8 +91,13 @@ functionality that's provided.
 So far, Orchard follows these options, which can be specified as Java system properties
 (which means that end users can choose to set them globally without fiddling with tooling internals):

+* `"-Dorchard.fetch-java-sources-via-dynapath=false"` (default: true)
+  * if `false`, all features that currently depend on dynapath (and therefore alter the classpath) will be disabled.
+  * This is a way to avoid a number of known issues.


I guess you can link to #103 and friends here, so this is more specific.

bbatsov · 2021-04-13T05:01:05Z

project.clj

@@ -1,3 +1,58 @@
+(require 'clojure.java.io)


I'd add some section comments around this, explaining the need for it, so casual readers of the project's code are not surprised.

bbatsov · 2021-04-13T05:08:50Z

src/orchard/java.clj

@@ -58,17 +58,24 @@
               (io/file parent "lib" f)]]
    (->> paths (filter #(.canRead ^File %)) first io/as-url)))

+(def fetch-java-sources-via-dynapath?


I think "fetch" is not the right word here, as we're just adding them to the classpath if they are already present locally. Probably "add/load" would be better.

bbatsov · 2021-04-13T05:13:57Z

⚠️ Between and a tenative addition of JDK16 to the matrix, the matrix would become quite extensive - perhaps a bit over the top?

Yeah, it'ss nice you simplified it a bit. In general only 8, 11 and the last JDK are needed there. See also https://docs.cider.mx/cider/about/compatibility.html

Overall the PR seems reasonable to me. I guess we should add some instructions about your plugin to CIDER's docs as that's where it's most likelyt for the users to find those.

vemv

In general only 8, 11 and the last JDK are needed there. See also https://docs.cider.mx/cider/about/compatibility.html

👍 will remove jdk15 (for jdk16) then

I guess we should add some instructions about your plugin to CIDER's docs as that's where it's most likelyt for the users to find those.

If you don't mind, I'd like to defer that a couple weeks: that gives me time to:

extract the plugin to its own repo
expand the plugin to also fetch JDK sources when needed
expand the plugin to also fetch tools.jar when needed.

This way, end users will ever be pointed once to a plugin that solves all problems at the same time.

Else it could be confusing to e.g. eventually require (and communicate the need for) N separate plugins, or tell people "you need to upgrade the plugin".

https://github.com/threatgrid/clj-experiments/tree/master/resolve-java-sources-and-javadocs is in a really mature state anyway so I expect the additions to be ready (+ tested/CId) before a couple weeks.

vemv · 2021-04-13T06:25:18Z

project.clj

+             :no-dynapath {:jvm-opts ["-Dorchard.fetch-java-sources-via-dynapath=false"]
+                           :resource-paths [~(unzipped-jdk-source)]
+                           :plugins ~(if jdk8?
+                                       '[[lein-jdk-tools "0.1.1"]]


Why so?

It compensates, when needed, for the loss of this functionality:

orchard/src/orchard/java.clj

Lines 73 to 81 in f763175

(def jdk-tools

"The `tools.jar` path, for JDK8 and earlier. If found on the existing

classpath, this is the corresponding classpath entry. Otherwise, if available,

this is added to the classpath."

(when (<= misc/java-api-version 8)

(or (some-> (io/resource "com/sun/javadoc/Doc.class")

^JarURLConnection (. openConnection)

(. getJarFileURL))

(some-> (jdk-find "tools.jar") cp/add-classpath!))))

bbatsov · 2021-04-13T06:50:31Z

If you don't mind, I'd like to defer that a couple weeks: that gives me time to:

Yeah, totally. I meant this is "we should add them there eventually". :-) I'll cut a new Orchard release once this PR is merged and I'll do another one once we remove the fallback and your plugin is ready for the limelight.

Rationale: clojure-emacs/cider-nrepl#693

vemv · 2021-04-13T08:02:18Z

Rebased against master, and amended the second commit to take into account PR feedback.

JDK15 has been removed from the CI matrix (for 16)

bbatsov · 2021-04-13T08:13:59Z

CHANGELOG.md

@@ -8,6 +8,8 @@

 ### Changes

+* [#113](https://github.com/clojure-emacs/orchard/issues/113) Add ability to skip functionality that works by altering the classpath
+  * You an opt in to this choice by setting `"-Dorchard.fetch-java-sources-via-dynapath=false"`.


Btw, why don't we just shorted this to orchard.use-dynapath? After all the sources were its only use anyways.

Btw, why don't we just shorted this to orchard.use-dynapath? After all the sources were its only use anyways.

Good call! Done

Closes clojure-emacs#112 Related clojure-emacs#103 Related clojure-emacs#105

bbatsov · 2021-04-13T08:31:43Z

I've cut orchard 0.7, and I'm in the process of cutting cider-nrepl 0.26-snapshot

vemv commented Apr 11, 2021

View reviewed changes

vemv force-pushed the opt-in branch from b54fa87 to 97ec7e6 Compare April 11, 2021 12:48

bbatsov reviewed Apr 13, 2021

View reviewed changes

vemv commented Apr 13, 2021

View reviewed changes

Run clj-kondo by a more stable means

76793ff

Rationale: clojure-emacs/cider-nrepl#693

vemv force-pushed the opt-in branch 2 times, most recently from 53090e8 to 58635bc Compare April 13, 2021 08:01

bbatsov reviewed Apr 13, 2021

View reviewed changes

Add ability to skip dynapath-based functionality

67a223d

Closes clojure-emacs#112 Related clojure-emacs#103 Related clojure-emacs#105

vemv force-pushed the opt-in branch from 58635bc to 67a223d Compare April 13, 2021 08:18

bbatsov merged commit d4fa4b8 into clojure-emacs:master Apr 13, 2021

bbatsov mentioned this pull request Apr 13, 2021

Orchard error on thrown error: java.lang.ClassNotFoundException: com.sun.tools.javac.util.List #105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to skip dynapath-based functionality #114

Add ability to skip dynapath-based functionality #114

vemv commented Apr 11, 2021

vemv Apr 11, 2021 •

edited

Loading

bbatsov Apr 13, 2021

vemv Apr 11, 2021

bbatsov Apr 13, 2021

vemv Apr 11, 2021

bbatsov Apr 13, 2021

vemv Apr 13, 2021

bbatsov Apr 13, 2021

bbatsov Apr 13, 2021

bbatsov Apr 13, 2021

bbatsov Apr 13, 2021

bbatsov commented Apr 13, 2021

vemv left a comment

vemv Apr 13, 2021

bbatsov commented Apr 13, 2021

vemv commented Apr 13, 2021

bbatsov Apr 13, 2021

vemv Apr 13, 2021

bbatsov commented Apr 13, 2021

	(def jdk-tools
	"The `tools.jar` path, for JDK8 and earlier. If found on the existing
	classpath, this is the corresponding classpath entry. Otherwise, if available,
	this is added to the classpath."
	(when (<= misc/java-api-version 8)
	(or (some-> (io/resource "com/sun/javadoc/Doc.class")
	^JarURLConnection (. openConnection)
	(. getJarFileURL))
	(some-> (jdk-find "tools.jar") cp/add-classpath!))))

Add ability to skip dynapath-based functionality #114

Add ability to skip dynapath-based functionality #114

Conversation

vemv commented Apr 11, 2021

vemv Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Apr 13, 2021

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Apr 13, 2021

vemv commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Apr 13, 2021

vemv Apr 11, 2021 •

edited

Loading