unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Use `jar`, not `maven-jar-plugin`, to create MR-JAR #348

Open sormuras opened 3 years ago

sormuras commented 3 years ago

https://github.com/unitsofmeasurement/indriya/blob/1fa2d727bcb4e602ed07c07a31b165a73e29aea4/pom.xml#L711-L725

This leads from https://maven.apache.org/plugins/maven-jar-plugin/ to https://maven.apache.org/shared/maven-archiver/ to basically a .zip file with a different file name ending, here .jar -- with jar never being executed, iirc.

keilw commented 3 years ago

Thanks, that's an interesting suggestion, but can you point to other projects on GitHub that successfully apply this to create multi-release-JARs? Ideally with links to these projects along the lines of Spring, Apache (you seem to be an Apache committer or similars?

I know some of them use Gradle, at this point, switching to Gradle is not acceptable or easy (of course PRs and contributions would be welcome then we could always consider it) so we need a working solution that works in the CI servers ideally not requiring some low-level BAT or SH scripts that are OS-specific.

Any chance you can provide us with this?

sormuras commented 3 years ago

Switching to Gradle would not help.

https://github.com/junit-team/junit5/blob/d597bb5826ff7c5ccb77e3ed94109ae15dfd5e1d/junit-platform-commons/junit-platform-commons.gradle.kts#L19-L28

keilw commented 3 years ago

Ok so what do other projects do in such a case? Imagine Glassfish or similar implementations may require a MR-JAR at some point, what should they do? Especially with modularity and optionality, also on the TCK level, this JSR has been a bit of a "guinea pig" in some areas trying out things we hope also accomplish in Jakarta EE at some point, see the Jakarta EE Optional Features Proposal Our TCK was derived from CDI but refined and improved, especially CDI currently has no optionality yet, but the whole "CDI Light" discussion aims at it for a future version.

keilw commented 3 years ago

At the moment unit-api has one way to create a "dual-release JAR" (for Java 8+9) using maven-antrun, but for a true "multi-release JAR" (more than 2 Java versions, currently 4) this was not feasible, so I evaluated several other options with the maven-toolchain plugin. Especially for Maven there are not so many that really work, and most plugins were experimental and soon abandoned, like https://github.com/metlos/multi-release-jar-maven-plugin.

desruisseaux commented 3 years ago

At the time I proposed the antrun approach, I was unable to get standard Maven plugins to work with multi-modules or multi-versions features. There is probably better solutions now, but I'm not yet familiar with them. However one thing that I wish to keep is to include in META-INF/versions/ only the classes that are significantly different, not every classes. It is not clear for me if the Maven Compiler Plugin requires to recompile the whole project.

sormuras commented 3 years ago

jar --update --file indriya-2.1.2.jar --release 9 ANY-FILE-WILL-DO

entry: META-INF/versions/12/tech/units/indriya/format/NumberFormatStyle.class, contains a new public class not found in base entries
entry: META-INF/versions/12/tech/units/indriya/format/NumberDelimiterQuantityFormat.class, contains a class with different api from earlier version
module-info.class in a versioned directory contains additional "requires transitive"
module-info.class in a versioned directory contains additional "requires transitive"
entry: META-INF/versions/14/tech/units/indriya/AbstractSystemOfUnits.class, contains a class with different api from earlier version
invalid multi-release jar file indriya-2.1.2.jar deleted
keilw commented 3 years ago

@sormuras Does jar really delete files rather than flagging differences?!!! I know the TCKs of Jakarta EE also includes calls to SigTest but I was not aware that would delete any JAR file with wrong or mismatching signatures either?`

I'm a little puzzled because the title was about "creating JARs" and not deleting them, is that the case with the JDK jar command?

sormuras commented 3 years ago

No.

invalid multi-release jar file indriya-2.1.2.jar deleted is referring to the temporary JAR file that would receive the update.

keilw commented 3 years ago

Ok that sounds better because invalid multi-release jar file indriya-2.1.2.jar deleted suggested otherwise.

Along the lines of above suggestion I think it would be a better addition to the TCK if not by also using something like SigTest (although being last released May 12, 2017 sounds a bit old and may not even be aware of JPMS or multi-release JARs) or separate shell script calls in the TCK testbed. The above command is a bit unclear about multiple releases, why would --release 9 bother about releases like 12 or 14 or was the output not related to --release 9 anymore?

What I see in junit-platform-commons it is also a dual-release JAR only so far with a class or two different for Java 9, but nowhere near the 4 versions Indriya supports until now. Having to manually create some toolchain instead of the well-established Maven plugins feels a little risky and cumbersome. For a handful of well-known classes only or for a demo what blogs like https://www.baeldung.com/java-multi-release-jar suggest (also mostly hand-crafted calls of jar)

25 was the original ticket long closed to establish an MP-JAR.

@desruisseaux What do you think about it? As for the question regarding the Compile Plugin, well it is called via the Toolchain Plugin and you see:

Compiling 89 source files to /home/circleci/repo/target/classes
...
Compiling 3 source files to /home/circleci/repo/target/classes/META-INF/versions/9
...
Compiling 2 source files to /home/circleci/repo/target/classes/META-INF/versions/12
...
Compiling 2 source files to /home/circleci/repo/target/classes/META-INF/versions/14

(see the entire Circle-CI build log: https://circleci.com/api/v1.1/project/github/unitsofmeasurement/indriya/2020/output/103/0?file=true&allocation-id=6098897e807a794d31bec5d5-0-build%2FF831A5E) each compile step executed separately for the 4 different releases. On the CI server it only uses one JDK beause it would be a great burden to ramp up 4 separate JDKs but if I run it locally each of these compile steps uses one of up to 4 unique JDKs. In theory if somebody wishes to spend enough time, those could also be pulled via Docker and a toolchain.xml file generated accordingly, then each release would even be built with separate JDKs according to the individual version. Since the steps include the release definition it also does not matter that much the JDK must be >= the highest release which is 14 right now. I would not throw that away without a proper and reliable replacement available.

keilw commented 3 years ago

@sormuras I created https://github.com/unitsofmeasurement/unit-tck/issues/42 for possible consideration of any JDK tools like SigTest or others (jar seems less capable but let us know any option to call it for analysing an archive that might work, then it may also be among the candidates) but completely throwing over the true multi-release JAR tool chain is just too risky and we don't have resources for that either. The JSR is in maintenance mode so there is no major development going on and you can check who contributes to see we are a not-for-profit project driven entirely by individuals, no big company and while your GH profile shows a few sponsors that may help all the various developments [bach](like https://github.com/sormuras/bach) or others, which some have more than one contributor, other's dont, but the stability still claiming it's "experimental" since 2017 does not sound safe enough to use with Indriya, at least not in maintenance.

Maybe @desruisseaux could consider it if he finds time to implement JSR 385 in Seshat. As for the JSR there are extensions that need more attention and unless the JDK ever woke-up Valhalla from the dead instead of other features like yet another Garbage Collector (or throwing JIT out like https://openjdk.java.net/jeps/410) as well as more pruning and removal including Applets or the SecurityManager I'm sorry but Java 17 just does not contain any convincing feature that would ask for migrating to it, 16 seemed better including records or fixing that long overdue problem with modular multi-release JARs. Maybe you know something inside the OpenJDK team or mabe others including @AlanBateman know more about Valhalla and its fate?

Without that I'm afraid there is no need for a whole new Unit JSR either and without such a brand new JSR there is neither demand nor justification to completely overhaul and replace the way the RI was built. Beside the things you mentioned above look like only known signature differences all but one (and that serves forensic purposes only) are already resolved to allow even those who have not even reported any actual problem to avoid having them also before Java 16. I won't close it yet but marked it as deferred meaning it is not something for maintenance.

sormuras commented 3 years ago

FYI: https://github.com/openjdk/jdk/pull/3971

keilw commented 3 years ago

What's the target Java version? We won't replace a proper toolchain with some manual puzzle, but if this is available in a future version either for the TCK, see unitsofmeasurement/unit-tck#42 or in something like a post-build step of the CI build pipeline, it could be used there if this new option works on the final JAR file. Thanks for mentioning this new or upcoming feature.

bmarwell commented 3 years ago

We won't replace a proper toolchain with some manual puzzle, but if this is available in a future version either for the TCK,

Hi Werner,

these assumptions puzzles me. There is so much wrong in it. You could, for instance, just add an IT using the invoker plugin which only runs on JDK17+. This is modular and will not affect other builds. You could also add automatic profiles and the exec plugin. There are probably even more solutions, which are not "manual" and will definitely not intervene with existing toolchains. This was, normal users would be able to use the existing verify phase and don't need to run another manual command.

keilw commented 3 years ago

Hi @bmarwell Like everyone you are welcome to help by proposing a PR, ideally as a JCP Associate member, but since these are only parts of the build tool chain should you have a problem to join the JCP even as an Associate, the non Java code pieces that are not used at runtime are usually less critical and we could merge such PRs f they work.

Please propose something. Note as of now, the other plugins won't work with Java 15 or 16 yet (I can't say which one, but at the moment until Circle-CI forces to migrate all pipelines to Java 17 or higher that is not a problem because we are not using Java 17 features and the RI is compatible all the way down to 8 with specific multi-release folders up to 14) so using 17 for the actual tool chain is not an option (unless all the Maven plugins involved have been fixed accordingly) but of course in a post build step as long as the CI server allows to draw in multiple JDKs there feel free to use Java 17 once the CI tool will support it of course, it isn' final yet.

Except a bit of help mostly on the arithmetic side mostly by @andi-huber this JSR in Maintenance mode has no EG anymore hence it is just us 3 Maintenance Leads and in reality it's me alone who does those things. Neither this JSR nor 354 which is also in Maintenance phase now have a large corporate Maintenance Lead company paying us full time day in for working on it, so we have to focus on things the users really need like #355, #356 or #353 (which is likely resolved with a new generation of JSON libraries) and nice to have gimmicks in he toolchain are something with a lower priority, so are other non-functional requirements like #299. I created that myself but I cannot clone or multiply myself so I have to be realistic and pragmatic about some of these tickets like everyone else.