-
Notifications
You must be signed in to change notification settings - Fork 29
[MTOOLCHAINS-49] Automatic discovery of JDK toolchains #14
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
Conversation
src/main/java/org/apache/maven/plugins/toolchain/DisplayDiscoveredToolchainsMojo.java
Outdated
Show resolved
Hide resolved
74e3bba to
93918e2
Compare
93918e2 to
101cfec
Compare
src/main/java/org/apache/maven/plugins/toolchain/jdk/DisplayDiscoveredJdkToolchainsMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/toolchain/jdk/DisplayDiscoveredJdkToolchainsMojo.java
Outdated
Show resolved
Hide resolved
elharo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. It probably should have a JIRA issue for release notes and discussion. I'm wondering if we add this functionality but eliminate a lot of the configuration by establishing some default rules along these lines.
|
Cant we make toolchain.xml physically optional and replaced by some env var naming convention instead? |
Wouldn't it be great if |
|
@norrisjeremy sure but not on this pr I guess ;) |
Settings are already interpolated so we can use env vars already, I'm using it locally with the following: so that I can point to a mirror on my LAN when available. Toolchains are also interpolated and env vars seems to be supported too fwiw. So it's not really settings being optional, but it's easy to setup a project's settings.xml or toolchains.xml that just use env variables, wouldn't that work ? |
Yes this is what makes it complicated compared to not using it. |
The point is to have a standard method by which Maven can detect specific env vars for this without having to explicitly write a |
elharo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs docs, including site docs
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
Let's put the settings.xml aside for this PR. This PR provides a |
|
Personally i use JAVA${major}_HOME and wire it in pom in executable or jvm properties today. But what I dont like in this pr is that it adds complexity to the complexity of toolchain whereas if you totally drop toolchain everything is simple. Dont take it as a -1 but more as not something to promote nor recommend to end users IMHO. |
I'm not sure how it really adds complexity. I agree toolchains can be seen as overly complicated when you can use the Note that with this PR, you don't have to generate the toolchains xml, they are automatically discovered. So with no toolchains preconfigured, if you have an env var If you want to compile targeting JDK 1.7 for example, you can select a JDK 1.8 compile by using |
src/main/java/org/apache/maven/plugins/toolchain/jdk/GenerateJdkToolchainsXmlMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/toolchain/jdk/GenerateJdkToolchainsXmlMojo.java
Show resolved
Hide resolved
| flags.computeIfAbsent(currentJdkHome, p -> new HashMap<>()).put(CURRENT, "true"); | ||
| // check environment variables for JAVA{xx}_HOME | ||
| System.getenv().entrySet().stream() | ||
| .filter(e -> e.getKey().startsWith("JAVA") && e.getKey().endsWith("_HOME")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a regex including the digits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why, this test also captures JAVA_HOME which is fine. I'm not sure we need to restrict to using numbers, even if that's what people usually do. The only place it's used is when matching against a user supplied value in the env filter during selection, but it could be anything, especially something like: JAVA_GRAAL_17_HOME or whatever.
| PersistedToolchains ps = new PersistedToolchains(); | ||
| ps.setToolchains(tcs); | ||
| return ps; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a more specific exception?
| ps.setToolchains(tcs); | ||
| return ps; | ||
| } catch (Exception e) { | ||
| if (log.isDebugEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if block doesn't seem to do much.
More significantly I'm not sure this is the right error handling. Is it expected that there's an error discovering toolchains? if so, no message at all. If it's not expected, maybe always a message. What does it mean if the toolchains fail here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal is the following: the toolchain discovery is optional, so any exception happening should be logged to the user but not forbid the use of the feature, in particular a later selection of the toolchain. Any exception caught is logged at WARNING level (with stack trace if debug is enabled).
This can be moved to the callers, though...
| cache = new ConcurrentHashMap<>(); | ||
| Path cacheFile = getCacheFile(); | ||
| if (Files.isRegularFile(cacheFile)) { | ||
| try (Reader r = Files.newBufferedReader(cacheFile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
charset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The charset used is deterministic, it's UTF-8 as indicated in the javadoc. I'm fine with this charset, so not sure why I would need to make it explicit.
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } catch (IOException | XmlPullParserException e) { | ||
| log.warn("Error reading toolchains cache: " + e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the cache isn't read, what's the effect? if we just read directly without caching, then no warning should be logged. debug level at most.
How important is a cache here anyway? How long does it take to just recompute every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache is imho quite important. The length operation is the following:
https://github.com/gnodet/maven-toolchains-plugin/blob/1cd938cfa39b262e669378135b7df89559408300/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java#L142-L154
The java process is spawned for each JDK, so on my computer, it takes 950 ms (using a the parallel fork join pool). I'd rather avoid it each time the mojo is invoked.
But the cache is just a cache, if an error occurs while reading/writing, the process will still work, that's why errors are caught and logged at WARNING level. I'm not sure to understand the "if we just read directly without caching" sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll buy the cache is important, but if this works when the cache fails then a warning is too high a log level since the user doesn't need to pay attention to it. This should be debug level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the cache does not cache discovered toolchains (i.e. paths which contain a JDK), but rather the information related to each found JDK.
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
|
I've tested this on a Macbook, where I have all my JDKs installed through Homebrew. All of them have been discovered, so that's good! But I've also tested this on a Windows machine, where I've used scoop to install two JDKs. The |
@mthmulders Do you know which paths they are installed to ? |
But what I have on disk (reported on Git Bash, hence /c/, but you get the idea): So, generally speaking, the path is They're all installed from https://github.com/ScoopInstaller/Java, by the way. |
# Conflicts: # src/site/apt/index.apt.vm # src/site/site.xml
Would you mind trying again ? I added support for discovery JDK inside |
Sure! I don't have access to that Windows machine on a daily basis, so it took a while. Here are the results from commit f3b0b29: So it seems not to work yet, because in the meantime, to ensure proper testing, I have installed the following Java runtimes:
The code omits to navigate into the package directory; I've pushed a commit that does just that. Not sure if we only want the "current" version of each package, or in fact "all" versions of a package? |
| stream.forEach(dirsToTest::add); | ||
| // Scoop can install multiple versions of a Java distribution, we only take the one that is | ||
| // currently selected. | ||
| stream.map(path -> Paths.get(path.toString(), "current")).forEach(dirsToTest::add); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to find all JDK installed, not only the current one.
My undersanding is that they are installed by scoop in C:\Users\M67B014\scoop\apps\[jdk-name]\[version].
The installedDirs should then point to C:\Users\M67B014\scoop\apps\[jdk-name] because it's later scanned for JDK in its child directories, see
Lines 432 to 436 in 0e85805
| try (Stream<Path> stream = Files.list(dest)) { | |
| stream.forEach(dir -> { | |
| dirsToTest.add(dir); | |
| dirsToTest.add(dir.resolve("Contents/Home")); | |
| }); |
So by adding all direct children of
scoop\apps, we should later have the children of those directories.
@mthmulders Are you sure the fix in line 414 is not sufficient ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double-check again one of these days, but I'm pretty sure that <userHome>\scoop\apps is not enough. Scoop-installed Java runtimes reside under <userHome>\scoop\apps\<packageName>\<version>, with <userHome>\scoop\apps\<packageName>\current as a symlink to the currently active version of that package.
Regardless of that, the question remains: given that openjdk21 has three installed versions (21.0.0, 21.0.1 and 21.0.2), do we want all of them to be detected or only the current one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double-check again one of these days, but I'm pretty sure that
<userHome>\scoop\appsis not enough. Scoop-installed Java runtimes reside under<userHome>\scoop\apps\<packageName>\<version>
I think the code was already checking those. installedDirs should contain all <userHome>\scoop\apps\<packageName> while dirsToTest should later contain all <userHome>\scoop\apps\<packageName>\<version>.
, with
<userHome>\scoop\apps\<packageName>\currentas a symlink to the currently active version of that package.
That one will be ignored, as symlinks are always transformed to a canonical path. That's the one stored in the toolchain.
Regardless of that, the question remains: given that
openjdk21has three installed versions (21.0.0, 21.0.1 and 21.0.2), do we want all of them to be detected or only the current one?
The code is currently written to get all JDK. The notion of current is specific to a given installer and may not be the latest one. We have heuristic to choose amongst multiple versions and the goal in the discovery phase is to escape from "just the one configured in the shell". So I'd rather have them all listed here and choose during toolchain matching / selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see this gist. To create it, I've added some debug logging to print which directories indeed contain bin/javac or bin/javac.exe. I did that after the filter in line 444 to not print all packages I have installed that aren't a Java runtime.
If we only look at Paths.get(userHome, "scoop", "apps");, we will miss all installed runtimes - only the "current" one (which JAVA_HOME points to) will be detected. This is file-inspect-scoop-apps-packagename-txt.
Looking at the current directory under each installed package will make the code detect the current version of each installed Java runtime. So that is a lot more, but still not all. This is inspect-scoop-apps-packagename-current-txt.
To find all of them (as you wrote), we'd need to look at each subdirectory of an installed package. It will incur a lot of disk scanning, because it will inspect each installed version of each Scoop package, but it will even detect multiple versions of the same Java runtime (see openjdk21, microsoft17-jdk). This is inspect-scoop-apps-packagename-all-subdirs-txt. It will also report the current version of that package (so that's a duplicate), but we could filter those out I believe.
# Conflicts: # src/site/apt/index.apt.vm # src/site/site.xml
|
With your latest additions, the |
Yes, that was my initial idea. The goal is to discover all available JDK and then use some rules to select the one the user wants. There won't be exact doublons because symlinks are resolved to real path, so the "current" one won't be provided as a different JDK. To select the matching JDK, we have multiple variables that can be used to match and sort JDKs. If that's not sufficient, the user can use the |
|
@mthmulders @elharo if there are no more concerns, I'll merge this PR |
I didn't review the code thoroughly, I mainly chimed in to help get Scoop supported. Please go ahead, if other package managers should be added later we could always do that. I think those would make for great "up-for-grabs" issues. |
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java
Outdated
Show resolved
Hide resolved
|
Resolve #107 |
JIRA issue: MTOOLCHAINS-49
Add mojos to:
The generation of the
toolchains.xmlfile is not necessary to use discovered toolchains. Theselect-jdk-toolchainwill select a toolchain amongst explicitely configured toolchains and discovered toolchains. The information for discovered toolchains are cached in~/.m2/discovered-toolchains-cache.xmlfile by default.The
select-jdk-toolchainconfig below allows using the current JDK, or any other discovered JDK >= 17. The benefit is that the current JDK can be kept for speed, but ensuring the usage of any JDK 17 or higher if the current jdk is below the requirements.The
display-discovered-jdk-toolchainsshows human readable discovered JDKs:while the
generate-jdk-toolchains-xmlwill write the output in xml format: