Add JDK management with Foojay Disco API integration #337
Conversation
|
Whoa .. thats amazing! |
gnodet
force-pushed
the
origin-feature-jdk-enhancement
branch
2 times, most recently
from
bbe1815 to
0160a9c
Compare
|
Is my understanding correct that the |
That's not completely exact. The JDK download is an opt-in if you add the required properties. The default behaviour is unchanged. There's also a |
gnodet
force-pushed
the
origin-feature-jdk-enhancement
branch
3 times, most recently
from
0160a9c to
4aaf12a
Compare
Ok, thanks! I was concerned that the default behavior was changing and users would have to explicitly opt-out to prevent it. |
gnodet
force-pushed
the
origin-feature-jdk-enhancement
branch
2 times, most recently
from
47fe90a to
a4c27d7
Compare
|
Currently broken by foojayio/discoapi#124 |
gnodet
force-pushed
the
origin-feature-jdk-enhancement
branch
3 times, most recently
from
6b3dd03 to
53aaaa0
Compare
|
To confirm my understanding: if we add the new JDK properties into |
Correct |
Perfect, thanks! This sounds like a great feature which we can use with our dev team to allow easy bootstrapping of the preferred JDK distribution on their workstations, whilst continuing to have our Github Actions CI pipelines use the system provided JDK. |
| boolean warningShown = false; | ||
|
|
||
| // Check for non-LTS versions | ||
| if (isNonLtsVersion(version)) { |
There was a problem hiding this comment.
I assume you also want this for Maven 3? Otherwise maybe use multiline string
There was a problem hiding this comment.
Not sure what you mean here ?
There was a problem hiding this comment.
You log a lot of single line strings to generate something that looks like a multiline string? Since java 13 (?) Java supports texts blocks like. As we set Java 17 for Maven 4, maybe we can make use of this here if the change is only aiming at Maven 4.
There was a problem hiding this comment.
Ah, got it. So this is currently not focused on Maven 4. Also, using multiline strings changes the output, as only the first line will be prepended with [WARN] afaik, so it kinda messes the output which is not correctly aligned anymore.
src/site/markdown/JDK_MANAGEMENT.md
Outdated
|
|
||
| ## Distribution Type Support | ||
|
|
||
| **Important**: JDK management is **only available for the `only-script` distribution type**. This design choice avoids the chicken-and-egg problem where Java is needed to download Java. |
There was a problem hiding this comment.
I would move this to line 20, where for the first time it is mentioned that only-script is the only supported way
src/site/markdown/JDK_MANAGEMENT.md
Outdated
| ```properties | ||
| # JDK Management | ||
| jdkVersion=17 # Resolves to latest 17.x (e.g., 17.0.14) | ||
| jdkDistribution=temurin # Disco API distribution name |
There was a problem hiding this comment.
Disco API was never mentioned before, not suddenly pops up in a comment. Should be mentioned before so people can look up whats available
src/site/markdown/JDK_MANAGEMENT.md
Outdated
|
|
||
| ## Supported JDK Distributions | ||
|
|
||
| The Maven Wrapper supports 34+ JDK distributions through the Foojay Disco API using native distribution names: |
There was a problem hiding this comment.
I would remove the number and only mentioned Disco API. Also add link to there
|
Exciting feature! @gnodet |
@gnodet how about to support selecting dynamicly JDK version via an environment, then the mvnw can pin almost the jdk matrix by project. |
I've added env vars to override the JDK properties. |
should we make all the env vars all has the same prefix |
|
I'm not on the committers list, but this looks like fantastic new feature! :) |
They do AFAIK, did I miss something ? |
No. That only verifies that the file specified in the properties file is the file that you actually downloaded. E.g. This can of course be said for regular dependencies as well. The difference here is that users are used to dependencies they're not used to a random JDK being pulled in without their knowledge. And there tends to be infrastructure (at least from an Enterprise perspective) to scan dependencies for CVEs etc which Just to be clear, I'm not against the jdkDistributionUrl - it might be useful. But, when it is used the user should be made aware (read: warned about security risks) and then actively have to approve it (which can be explicitly pre-approved using a command line option or similar). |
I sort of surprised that an enterprise that is that particular about security would use maven-wrapper in the first place, but to each their own. 🤷♂️ |
Not really because the default behaviour DOES NOT change. You have to first opt-in and specify the JDK version. Once you have opted-in, you can then opt-out with
Not sure. That was really my first attempt. Unfortunately, sdkman does not keep track of old releases, which is a problem from a reproducibility pov.
I agree with Jeremy here. A simple to secure things is to use an exact url + sha256. If you want to keep some versatility, another way would be to be able to provide an alternative REST endpoint that could be implemented within an entreprise. At runtime, only |
It's not a matter of Enterprise or not. When I develop on my spare time I don't necessarily want to have a bunch of JDKs downloaded to my computer without my knowledge. |
Maybe I'm missing something here. Who is that has to opt-in? If I download an open source project that uses Maven Wrapper and then run it with
Agreed. Yes, it is.
See my use-case above. I don't see how an exact url and an sha256 in an open source project's
Yes, I saw that that was added. But, I don't think that it's relevant to the security use-case I wrote above - unless I miss understood something. |
|
@gnodet After giving this some more though, the feature is great but I'm actually leaning towards that this feature is in the wrong place. The Maven Wrapper project is just that "it wraps Maven" (and the name implies just that) - not JDKs. I don't think it obvious from a user perspective that a Maven Wrapper also wraps the JDK (and it never will be with that name) and I'm not sure that the two should be mixed. It's convenient but is it the right place? Perhaps, there needs to be a JDK Wrapper? |
You could just use your own Maven installation instead of using the projects embedded |
Yes, I can, but you are missing the point completely. Maven Wrapper was created so that I would have to think of what Maven version to use with the project. Your "suggestion"/"workaround" is because the original functionality of Maven Wrapper is "broken". |
I do wonder: how would the checksum pining work for multi-arch / multi-OS? |
Then you can easily opt-out by default by adding |
Oops, I had not thought about that... that clearly looks broken ;-) |
Yes, I know. I think it's here that we disagree. You are adding new functionality that fundamentally changes Maven Wrapper (wrapping Maven and adding Maven locally - not JDKs) but you want users to opt-out when they might even be aware of the new functionality. I'm not convinced that default being opt-in is the right way to go. |
|
I would like to test it on AIX (bash and maybe the not so old ksh). The default ksh is buggy... However, most AIX systems don't have a direct internet connection. Does anyone have access to such a system? |
|
At least for specific environments (eg CI) off by default? Corporate environments... |
|
I posted a summary of my comments and concerns in the dev mailing list. Reposting here. Feedback on JDK Auto-Download Feature First of all, I think the JDK auto-download feature is a valuable enhancement, but I do have a few concerns I'd like to highlight: 🔧 Scope Creep & Naming Maven Wrapper is intended to wrap Maven—not a JDK. Adding JDK management expands its scope significantly. It might be more appropriate as a standalone "JDK Wrapper" or part of a broader environment-management tool. 🧭 User Experience (UX) Even though JDK configuration is technically opt-in, in practice, a user cloning a repository and running ./mvnw may unknowingly trigger a JDK download. This behavior may surprise developers and potentially even break their environments. Prompting or clearly notifying users before downloading could help. While the feature is opt-in, this is a substantial behavioral and major change. I recommend releasing it under a new major version to avoid surprises for users upgrading Maven Wrapper. 🔐 Security & License Transparency Using jdkDistributionUrl with jdkSha256Sum confirms file integrity, but it doesn’t guarantee security or trustworthiness of the downloaded JDK. A user could unknowingly fetch a malicious JDK. Additionally, users might inadvertently download a JDK under a license incompatible with their project's license or internal policies—leading to unintentional licensing obligations. |
|
interesting it can be made to openjdk maybe and then maven could only use it if jdkw exists? can be better for the community as a whole |
yes. I'm on your side - not allowed and/or able to just download what I want. |
That would be opting-out based on an env var. That's already doable. Just add |
At my security has an Nexus IQ Server with whitelist/blacklist activated. |
My point unless you have a DNS redirect on your computer, it means you have a proxy or a maven mirror defined to point to your Nexus instead of Maven Central. You'll have to do something similar for whitelisting/blacklisting JDK. Granted, there's currently no tool to do that, but Maven does not provide a repository manager, an external tool should be able to easily do that, it's just a few rest calls to handle. |
|
TBH, this feature without auto download of the JDK is probably useless :). |
I was thinking of reading out the CI variable. It's often used like CI=true or even CI=jenkins. For Maven wrapper itself, I have set the download URL to an internal repo in the maven-wrapper.properties. As the default repo is changeable via environment variable, running wrapper:wrapper is no problem. It's a bit different for JDKs, because there's no standardized URL/GAV coordinates. But of course doable. But yes, I also keep JDKs in my internal Nexus, specifying a GAV and repo instead would be a nice alternative, but out of the scope of this PR. But we could check it's not impossible in the future. Also, it might be a significant increase in data transfer for Foojay and hosters, which might or might not be a problem. Just thinking about "tragedy of the commons" from Brian Fox. But don't get me wrong, I really love this PR. I think we should merge it. |
| if [ -z "${MVNW_USERNAME-}" ] && command -v wget >/dev/null; then | ||
| verbose "Found wget ... using wget" | ||
| wget ${__MVNW_QUIET_WGET:+"$__MVNW_QUIET_WGET"} "$distributionUrl" -O "$TMP_DOWNLOAD_DIR/$distributionUrlName" || die "wget: Failed to fetch $distributionUrl" | ||
| if download_file_with_retry "$distributionUrl" "$TMP_DOWNLOAD_DIR/$distributionUrlName" 3 2; then |
There was a problem hiding this comment.
s we have a detecting tools like wget, curl in download_file_with_retry we not need here check for the same
|
|
||
| ```bash | ||
| # Generate wrapper with JDK management support | ||
| mvn wrapper:wrapper -Dtype=only-script -Djdk=17 -DjdkDistribution=temurin |
There was a problem hiding this comment.
only-script is default
| mvn wrapper:wrapper -Dtype=only-script -Djdk=17 -DjdkDistribution=temurin | ||
| ``` | ||
|
|
||
| The `only-script` distribution uses shell scripts (Unix) and PowerShell (Windows) to handle JDK download and installation directly, without requiring Java to be pre-installed. |
There was a problem hiding this comment.
should we check in code that only-script type is used with jdk properties?
There was a problem hiding this comment.
should be added in site menu somewhere
|
As we add a many code that will be not used is standard configuration I would like to split it to separate files and add only when is needed. We can include code by Similar as we do with debug with option |
🚀 JDK Management for Maven Wrapper
This PR adds comprehensive JDK management capabilities to Maven Wrapper using the Foojay Disco API, enabling automatic JDK download and installation for the
only-scriptdistribution type.✨ Key Features
🎯 Automatic JDK Management
only-scriptdistribution17) resolve to latest patch versionstoolchains.xmlgeneration for multi-JDK buildsMVNW_SKIP_JDKenvironment variable to use system JDK🔧 Configuration Options
🆔 Environment Variables
📦 Supported Distributions
Popular Distributions:
temurin- Eclipse Adoptium (default)corretto- Amazon Correttozulu- Azul Zululiberica- BellSoft Libericaoracle_open_jdk- Oracle OpenJDKmicrosoft- Microsoft OpenJDKsemeru- IBM SemeruSpecialized Distributions:
graalvm_ce11,graalvm_ce17- GraalVM Community Editionsap_machine- SAP Machinedragonwell- Alibaba Dragonwelljetbrains- JetBrains Runtimebisheng- Huawei BiShengkona- Tencent Konamandrel- Red Hat MandrelComplete list: Foojay Disco API
🛠️ Usage Examples
Maven Plugin
Properties Configuration
Bypassing JDK Management
Multi-JDK Toolchain Setup
🏗️ Implementation Details
Architecture
only-scriptdistribution typeDynamic LTS Detection
/major_versionsendpointDistribution Validation
Bypass Mechanisms
Caching & Performance
Security Features
🎯 Benefits
For Developers
For CI/CD
For Projects
🔄 Migration Path
Existing projects can easily adopt JDK management:
only-scriptdistribution typemaven-wrapper.properties# Simple migration command mvn wrapper:wrapper -Dtype=only-script -Djdk=17 -DjdkDistribution=temurin🧪 Testing
📚 Documentation
Comprehensive documentation added:
🎉 Summary
This PR transforms Maven Wrapper into a complete JDK management solution that:
The implementation is production-ready, well-tested, and thoroughly documented, making it easy for projects to adopt automatic JDK management while maintaining flexibility for edge cases.