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

[MRESOLVER-442] Build JDK transport MR JAR the proper way #381

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Nov 27, 2023

The current master builds JAR that it is not reusable in any project that uses enforcer (like Maven is).

Make JAR "java 8" and let "java 11" byte classes get into their proper place.


https://issues.apache.org/jira/browse/MRESOLVER-442

The current master builds JAR that it is not reusable in any
project that uses enforcer (like Maven is).

Make JAR "java 8" and let other (higher) byte classes get
into their proper place.

---

https://issues.apache.org/jira/browse/MRESOLVER-442
@cstamas cstamas self-assigned this Nov 27, 2023
@rmannibucau
Copy link
Contributor

As mentionned in #374 I think this is the worse of the 3 options we have (mjar, all in one -> #374, 2 jars - 1 java8 and 1 java 11 and be it)

@cstamas
Copy link
Member Author

cstamas commented Nov 28, 2023

This is the ONLY option that produces "clean" Java8 artifact usable in downstream projects that use enforcer (like Maven is). Moreover, Maven for example sets the bar low, is Java8, so even if I "hack" something here like you propose, hack will need to be carried over (as hack usually pulls more hacks) downstream to exclude Java11 JAR from enforcement etc...

So, IMO this is the best solution as:

  • it works
  • does not impose any "workarounds" is really just a drop-in (no hacks needed)
  • even offers downstream clients choice what to consume (it is not only Maven that consumes this library)

@cstamas cstamas merged commit e255be2 into apache:master Nov 28, 2023
4 checks passed
@cstamas cstamas deleted the MRESOLVER-442 branch November 28, 2023 10:34
@rmannibucau
Copy link
Contributor

This is the ONLY option that produces "clean" Java8 artifact usable in downstream projects that use enforcer (like Maven is).

Once again, this is due to an enforcer bug so a not really a point.
Also note it is not the only version, I explained you how on slack and github (with 2 artifacts).

it works

I explained you it does if you limit the downstream consumers to maven, you said it was not the case (agree on that) so it does NOT work.

does not impose any "workarounds" is really just a drop-in (no hacks needed)

No hack needed with all in one and mjar-free option too, only enforcer can throw a false positive and it is trivial to solve so.

even offers downstream clients choice what to consume (it is not only Maven that consumes this library)

Literally the opposite

I'm quite lost cause you explain the best solution is exactly what you don't reach with mjar..

@cstamas
Copy link
Member Author

cstamas commented Nov 28, 2023

Unsure which "enforcer bug" you refer to... all it does when you ask enforcer "I don't want any higher bytecode on classpath than Java8" is to fail when it encounters Java11 bytecode with your solution, as it really does contain Java11 classes on Java 8 classpath. So, unsure where the bug is?

It does work, as PR shows. Can you show me example where it does not work?

As I said above, if JAR is Java11 and you want to use it downstream in project that declares itself java 8, it is "enforcer bug" according to you, that enforcer fails the build in this case?

For this no hacks (as those are like static electricity, the more you have of those, the more you need downstream) and "bugs" needed, it just works. Clear, simple.

@rmannibucau
Copy link
Contributor

Unsure which "enforcer bug" you refer to...

Enforcer ignores mjar as of today and its validation can be wrong in some cases (here you want to enforce java 8 bytecode at build time but you enables java 11 bytecode, so this shouldn't pass with this configuration, should only pass saying "i want java 8 bytecode and I tolerate higher bytecode in META-INF/versions).

It does work, as PR shows. Can you show me example where it does not work?

Gave you severals, just use a classloader loading classes from anywhere else than a zip/jar - including resolver, it is the most trivial case. Run on java 11 and see the unexpected exception.
Ultimately I can ask you to show me where previous state was not working, it always runs well, rest is build tuning when using specific build plugins, as everytime, no big deal. Only con you have is enforcer plugin which is just a config thing and ultimately the 2 jars option works better than the mjar one - I'll not fight against the 2 jars options, just the mjar one.

@cstamas
Copy link
Member Author

cstamas commented Nov 28, 2023

Enforcer ignores mjar as of today and its validation can be wrong in some cases (here you want to enforce java 8 bytecode at build time but you enables java 11 bytecode, so this shouldn't pass with this configuration, should only pass saying "i want java 8 bytecode and I tolerate higher bytecode in META-INF/versions).

That's not what happens, please check out the code.

Gave you severals, just use a classloader loading classes from anywhere else than a zip/jar - including resolver, it is the most trivial case. Run on java 11 and see the unexpected exception. Ultimately I can ask you to show me where previous state was not working, it always runs well, rest is build tuning when using specific build plugins, as everytime, no big deal. Only con you have is enforcer plugin which is just a config thing and ultimately the 2 jars option works better than the mjar one - I'll not fight against the 2 jars options, just the mjar one.

Just enable sisu debug and use Java8 and watch your screen become full of errors as Sisu scans the classpath. With this solution when running on Java8 there is 0 errors logged.

@struberg
Copy link
Member

struberg commented Nov 28, 2023

My POV:
If a project has say a java8 min requirement via m-enforcer-p then every runtime+compile dependency (minus optional deps) must adhere to this rule as well. We can imo skip provided, test and optional deps.

BUT: if some dependency has a multi-release-jar with additional support for newer java versions, then this is also perfectly fine as long as it can execute on the requested jvm version.

@rmannibucau
Copy link
Contributor

@cstamas these are two ok things:

  • enforcer config, not a big deal IMHO and anyway enforcer is doomed by mjar - and once again 2 jars solve it without issues
  • IoC/scanners must ignore classes they can't run so it is fine IMHO - and once again 2 jars solve it without issues since you will not scan the java 11 one

@struberg literally means that if some dependency has a *NOT* a multi-release-jar but has additional support for newer java versions, then this is also perfectly fine as long as it can execute on the requested jvm version., no idea why it wouldn't, this is how most relocaed packages had been handled since 10+ years without issues.

so overall multi jar release is always pointless and it is better to stay away from it due to the downsides mentionned earlier.

I have really a hard time to see why jumping on a feature which breaks the library nature of the project is good since technically there is nothing which can explain it.

@struberg
Copy link
Member

you mean you have additional classes which will get picked up dynamically by the app only if you run on a jvm which is suitable? I've seen this done, yes, but that cries for an exclude in m-enforcer-plugin or some other trick.

@rmannibucau
Copy link
Contributor

@struberg yep. Agree it needs some tuning of m-enforcer-plugin but isnt it the case anyway?
Also is it that much used (have to admit I abandonned it years ago cause it had too much false positives for me and the cost to tune it was way too high for what it was bringing - validating the bytecode version is almost useless if you have tests with acceptable coverage for ex)?

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

Successfully merging this pull request may close these issues.

3 participants