-
Notifications
You must be signed in to change notification settings - Fork 127
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
Ensure we have a single JDK module #374
Conversation
.version(HttpClient.Version.HTTP_2) | ||
.version(HttpClient.Version.valueOf(ConfigUtils.getString( | ||
session, | ||
"HTTP_1_1", // v2 is not safe yet in the wild |
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 disagree, am using "jdk" transport for quite some, and also the point of this transport is to support HTTP/2 by default. Also, Maven Central supports http/2 quite some time as well... and my testing shows that even with HTTP/2 set, client nicely downgrades to HTTP/1.1 whenever needed, ie. when using local MRM.
I would rather revert this logic: default to http/2 but allow users to "force" http/1.1 if needed.
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.
LGTM sans the HTTP version change. Will merge this but will apply small change to it. Also, as it happens the javaVersion method is needed for MRESOLVER-438 as well
It was a wrong decision of mine to merge the PR, this PR is merely a "smart hack" and as we know, every hack sooner or later will require more hacks down the road. Problems with this approach:
Am undoing this change. |
Strictly speaking:
However mjar enforces:
If the issue is just about mixing bytecode major version we can have jdk module (release=8) depending on jdk11 one and be it, would probably help with IDE issue too (for idea at least). That said I agree with your original analyzis that the IoC should skip the not loadable component (jdk8 there) and compiling everything with java 11 is the simplest, the 17/21 new API can be handled with reflection quite easily so sounds like the best compromise to me too. |
I disagree with you, as MJAR in this case is transparently handled by JDK classloader, as "Maven as resolver using code" in this case is completely oblivious (and should be oblivious) what is "changed" when running on different JDK versions. Moreover, the superiority of pattern that this PR replaced is visible in Central as well: Basically, each "constituent" JAR (along with proper bytecode) is deployed to Central, along with the (my statement above is actually half-true: it works for |
@cstamas assuming MJAR works and is transparently handled by the JRE is wrong as I already mentionned. You are just lucky in your tests it was (side note: as war classloader was rarely used in (unit) tests, real runtime classloader is not too so we should be extremly cautious there). The JRE implementation sits in side note: we should make enforcer evolve to validate META-INF/versions/ classes against a defined bytecode cause today you are happy with it cause it is not seen but issue exists. |
No description provided.