-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add toolchain detection for Nix package manager #29214
Conversation
Signed-off-by: Lorenz Leutgeb <[email protected]>
|
📊 Changes by Platform: this PR is 99% new code See details
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware:
- I implemented
javaToolchains
as 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
Sorry, we don't want to add more built-in options. I opened a new issue that describes roughly our approach: #29508 We want a mechanism so 3rd parties can add support for different package manager options and not require changes to Gradle to support them. Eventually, we would remove the existing options. |
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
.