Skip to content
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

Merged
merged 1 commit into from Nov 20, 2023

Conversation

rmannibucau
Copy link
Contributor

No description provided.

.version(HttpClient.Version.HTTP_2)
.version(HttpClient.Version.valueOf(ConfigUtils.getString(
session,
"HTTP_1_1", // v2 is not safe yet in the wild
Copy link
Member

@cstamas cstamas Nov 20, 2023

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.

Copy link
Member

@cstamas cstamas left a 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

@cstamas cstamas merged commit 107f0db into apache:master Nov 20, 2023
4 checks passed
@cstamas
Copy link
Member

cstamas commented Nov 27, 2023

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:

  • IDE is unusable
  • running UTs from IDE is imposible (due that above)
  • reusing the library is impossible in any downstream project that uses enforcer to enforce byte-code level
  • it produces "broken artifact", a JAR that mixes byte-code levels in non standard way (those classes are not in their "proper place" like META-INF/version but in root) -- this also have implications when enumerating classes on classpath with this jar on classpath...

Am undoing this change.

@rmannibucau
Copy link
Contributor Author

Strictly speaking:

  • IDE was unusable before (was lost between java version) so guess it is kind of a limitation of such setups
  • I did it when working on the PR so I assume you just didnt tune the compiler release version of the module?
  • enforcer has several pitfalls can be one but not sure why it would be worse than the mjar flavor, it stays java (actually what we do since > 10 years)
  • This is not broken, any fatjar is in this case and it is java friendly, actually MJAR is this kind of jar

However mjar enforces:

  • more work on the jvm (classloading)
  • more work (or blocker) on any consumer that uses an IoC and scanner
  • more work (or blocker) on any consumer not using a flat classpath or classloader compatible with mjars (most of them actually, keep in mind EE does not know about modules and mjar yet, or several spring boot packaging flavors ignore it for ex)

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.
You mentionned sisu can break that but I guess we should just ensure it stays a feature either with a global or specific flag (ignorable classes or alike).

@cstamas
Copy link
Member

cstamas commented Nov 27, 2023

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:
https://repo.maven.apache.org/maven2/org/apache/maven/resolver/

Basically, each "constituent" JAR (along with proper bytecode) is deployed to Central, along with the -jdk MR one. This means, that consumer has a choice: to consume MR JAR or go directly for Java 11 without any fuss. IMHO, for libraries meant for downstream consumption, this is the cleanest and most "maven way" solution (and works in IDEs as well).

(my statement above is actually half-true: it works for -jdk-11 artifact, but does not for -jdk-19 or -jdk-21 but from alpha-3 (current master, to be released as alpha-3) downstream consumer will have a choice to consume -jdk MR JAR, or go directly for -jdk-11 that is "plain JAR", as 19 and 20 are gone thanks to this very PR 😄 )

@rmannibucau
Copy link
Contributor Author

@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 JarFile (java.util.jar.JarFile#getEntry) so as soon as you don't rely on that to locate .class binaries you are doomed. A simple case where it will not work whereas you can expect it to work is you explode your jars in a directory (recall early javaee 6 glassfish times? ;)) but basically any classloader not locating the classes with a jar file (think zipfile also handled it but no more sure) or using the exact ^^ API (like nested jars which preload in mem bytecode for ex) will not work. OSGi which pre-explode the zip in another location can have issue depending the impl and even EE can have issues depending the classloader itself. So overall I don't think it is sane.
Lastly, MJAR conserves half of the issues you have right now so not sure it solves anything.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants