Add toolchain detection for Nix package manager #29214
Conversation
Signed-off-by: Lorenz Leutgeb <lorenz@leutgeb.xyz>
|
|
|
📊 Changes by Platform: this PR is 99% new code See details
|
There was a problem hiding this comment.
I appreciate the initiative and efforts.
However, I'm don't agree, that implementation should try to collect every JDK it can find in the Nix store, which is a shared "cache", and this doesn't represent what user wants to have in the system or environment. Explained more in a code comment bellow.
| // and the length of a hyphen (1), and only look at the suffix. | ||
| private static final int PREFIX_LENGTH = 33; | ||
|
|
||
| private static final Predicate<String> PATTERN = Pattern.compile("jdk", Pattern.CASE_INSENSITIVE).asPredicate(); |
There was a problem hiding this comment.
This isn't a reliable pattern, as some JDKs don't have jdk in the package name (path):
GRAALVM_HOME="/nix/store/hi519j33pfya2fdxpkg67qcf4pnqg2s1-graalvm-ce-22.0.0"
TEMURIN_17_HOME="/nix/store/sf0qsq60dz95mzz82sf32v6wrzp3h32l-temurin-bin-17.0.9"
TEMURIN_21_HOME="/nix/store/43wmv55gdv75i9jx8d8s2xi8mrnbcilb-temurin-bin-21.0.1"
There was a problem hiding this comment.
Well, it's just a heuristic. Graal and Temurin can be added to the pattern.
| final File[] list = store.listFiles(FILTER); | ||
| if (list == null) { | ||
| LOGGER.debug("Listing files within store path '{}' failed."); | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
||
| final HashSet<InstallationLocation> locations = new HashSet<InstallationLocation>(list.length); | ||
| for (final File file : list) { | ||
| locations.add(InstallationLocation.autoDetected(file, getSourceName())); | ||
| } | ||
| return locations; |
There was a problem hiding this comment.
I am not sure, that collecting JDKs from the Nix store is a correct approach.
IMO, the detection should be following what user wants to have in the system, which is:
- currently running configuration (
/run/current-system), - current
nix-shell(or direnv'suse nix), which could also provide JDK packages.
The Nix store contains all derivations, that were installed at some point whether they still in use or not (and not garbage collected, yet), which includes also derivations, that are not wanted for JDK detection:
- temporary installs from experimental shells,
- previous generations of a system with (possibly buggy and insecure) old JDK versions.
There was a problem hiding this comment.
I am aware:
- I implemented
javaToolchainsas an argument to Gradle in Nixpkgs: gradle: Decouple from JDK 8 and support Java Toolchains NixOS/nixpkgs#119444 - I linked Add more Java installations detection options #16645 (comment) in this PR.
But still there's requests for even easier integration. What I am proposing here is the most trivial integration.
Using Nix profiles is a better idea, I'll look into that.
There was a problem hiding this comment.
But still there's requests for even easier integration. What I am proposing here is the most trivial integration.
Well, in this case, the trivial solution could cause harm/confusion to the user. As Nix store is a "cache" and very shared thing. This approach would make Gradle to auto-detect JVM(s), that user doesn't expect to be present ("installed") in the running system or the current nix shell.
|
Marking this as draft until I have feedback on NixOS/nixpkgs#312887 |
|
Thanks for the contribution! Note that we have open conversations on what to do with additional detection mechanism in Gradle: |
big-guy
added
the
👋 team-triage
Related to #16645.
Context
This change adds detection for JDKs installed via the Nix package manager (the Nix package repository contains numerous, see stable and unstable channels).
I've been thinking about implementing this at least since 2021-03-25, and recently there was another request for more liberal detection of JDKs in the Nix forums by @kravemir. Some downsides of this addition were mentioned by @raboof in #16645 (comment).
Status of this PR
I haven't implemented any tests and also have not changed any documentation yet. This is because I would like to get some feedback by Gradle maintainers because I spend more. In case I get positive feedback on the core changes (which are there already) I will add tests and documentation. I might need help to set up integration tests for this change.
Contributor Checklist
<subproject>/src/integTest) to verify changes from a user perspective.<subproject>/src/test) to verify logic../gradlew sanityCheck../gradlew <changed-subproject>:quickTest.