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

Support Multi Release Jars #209

Merged
merged 4 commits into from
Nov 21, 2023
Merged

Conversation

nielsbasjes
Copy link
Contributor

Not finished yet.

@nielsbasjes
Copy link
Contributor Author

Thinking out loud:

Currently the Clazz has 1 or more ClazzpathUnits.
So

  • A class can be in a single jar in multiple files (one for each java version).
  • A class can be in multiple jars.
  • For each class in a jar there can be other jars that hold the same class for a different java version.

If the java 17 variant of a class references an other class, then it may be true that the java 11 variant does not make that reference, simply because that dependency does not run on java 11.

Tricky part:

  • What do we store as references/dependencies? Is that "the class" or "the class for a specific java version"?

Right now this feels like a complete overhaul of the data structures is needed.

@tcurdt I could use some input from you.

@tcurdt
Copy link
Owner

tcurdt commented Nov 6, 2023

Thanks for the PR and you looking into this.

You raise a point I was concerned about when reading it.
This will make a minification implementatoin quite a bit more complex.

Right now a Classpath Unit (or jar) can hold exactly one version of the class. (But may appear in different units)
That used to resemble classpath loading reality.

I am thinking the key might no longer be just the class name but rather a query.

A simplistic approach could be to say the tuple <class name, java version> could be replace the current key .
But the big problem is that while we could nicely lookup:

<some.package.classname, 8>
<some.package.classname, 12>

We cannot easily lookup classes for

<some.package.classname, 12+>
<some.package.classname, any>

like that.

I have never been exposed to this yet (as I am not doing that much java anymore).
So far I found only this as a reference spec https://openjdk.org/jeps/238

Reading

Multi-release jars and the boot loader
Multi-release JARs are not supported by the boot loader (for example, when a multi-release JAR file is declared with the -Xbootclasspath/a option). Such support would complicate the boot loader implementation for what is considered a rare use-case.

I am wondering what real life use cases are without bootloader support.

@nielsbasjes
Copy link
Contributor Author

@tcurdt Please check what you think of this.
Essentially I have only added the filenames in which this class can be found.
Which is partially incorrect: Some filenames only occur in some jar files, and this distinction is lost this way.
The upside is that this is probably enough for something like maven-shade-plugin and it does not change the API for everyone who wants to ignore all of this.

I'm going to try this in combination with maven-shade first, to see if I missed anything.

P.S. I have included the sources of the two test jar files. Both are perfectly reproducible. So if you run mvn clean install in the resource-src then two jar files in resources will be recreated.

@tcurdt
Copy link
Owner

tcurdt commented Nov 6, 2023

Essentially I have only added the filenames in which this class can be found.
Which is partially incorrect: Some filenames only occur in some jar files, and this distinction is lost this way.

I guess adding the tuple <classpath unit, filename> instead of just the filename would make it correct and still easy enough to add.

WDYT?

@nielsbasjes nielsbasjes marked this pull request as ready for review November 12, 2023 09:39
@nielsbasjes
Copy link
Contributor Author

@tcurdt I would like your feedback on this. At this point this works with my changes in apache/maven-shade-plugin#202

@tcurdt
Copy link
Owner

tcurdt commented Nov 12, 2023

I will try to have a closer look tomorrow. Thanks for all the work!

@tcurdt
Copy link
Owner

tcurdt commented Nov 13, 2023

Very nice. Thanks for the work!
Quick summary: That's a great cleanup.
A few more tiny comments inline.

@tcurdt
Copy link
Owner

tcurdt commented Nov 19, 2023

I'll give it another look the next days and looking forward to merge this. Thank you!

@tcurdt tcurdt merged commit 17a8370 into tcurdt:master Nov 21, 2023
4 of 5 checks passed
@tcurdt
Copy link
Owner

tcurdt commented Nov 21, 2023

Thanks for the great contribution!

@tcurdt
Copy link
Owner

tcurdt commented Nov 21, 2023

We still have a problem though:

https://ci.appveyor.com/project/tcurdt/jdependency/builds/48559371

It's not passing tests on windows:

ERROR] org.vafer.jdependency.ClazzpathUnitTestCase.testShouldHaveUnitId -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.vafer.jdependency.ClazzpathUnitTestCase.testShouldHaveUnitId(ClazzpathUnitTestCase.java:170)
...

@nielsbasjes nielsbasjes deleted the MultiReleaseJar branch November 24, 2023 21:38
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.

2 participants