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

Multi-release JAR with module-info.java #804

Merged
merged 11 commits into from
Nov 11, 2022

Conversation

hadrabap
Copy link
Contributor

@hadrabap hadrabap commented Nov 5, 2022

Hello @xerial, @gotson and others!

I tried to implement module-info.java (#790). I decided to go the multi-release way.

What I had to do is:

  1. Upgrade maven-compiler-plugin to support multirelease and the new <release /> configuration option
  2. Upgrade maven-bundle-plugin to understand multirelease JARs

Next, I don't know if you are using Maven Toolchains, so I left the version selection commented out. Tell me if I should remove them.

Finally, I also registered other JDBC entrypoints via service loader like DataSource to enable automatic configuration in Jakarta EE Containers (Glassfish, OpenLiberty) or others. Once again, it is not necessary. If you don't like it, I'll remove it.

Please tell me your opinion about this.

With best regards,

PETR

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 5, 2022

Now I see a few builds are failing. It looks like the JDK8 build cancels the rest of the test builds.

There are three ways fixing it:

  1. Upgrade the JDK8 build to e.g. JDK11
  2. Leave the JDK8 build intact and make the multi-release build optional (probably with Maven Profile)
  3. Use Maven Toolchains so JDK8 will build everything and JDK9+ will do the multi-release stuff.

What is your opinion?

@jasonli0616
Copy link

Now I see a few builds are failing. It looks like the JDK8 build cancels the rest of the test builds.

There are three ways fixing it:

1. Upgrade the JDK8 build to e.g. JDK11

2. Leave the JDK8 build intact and make the multi-release build optional (probably with Maven Profile)

3. Use Maven Toolchains so JDK8 will build everything and JDK9+ will do the multi-release stuff.

What is your opinion?

@hadrabap I believe number 3 is the best option.
Have JDK8 remain to build, and use JDK9+ for the new changes in this PR. Seems like the most straightforward way to do it (although probably easier said than done?).
As I mentioned in #790 though, Java is not my main language and I may be mistaken for some stuff :)

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

Next, I don't know if you are using Maven Toolchains, so I left the version selection commented out. Tell me if I should remove them.

Honestly i don't know. I personally hate Maven, and would much prefer to use Gradle, with which i have experience with multi-release jars and toolchains. I've never used Maven toolchains.

Finally, I also registered other JDBC entrypoints via service loader like DataSource to enable automatic configuration in Jakarta EE Containers (Glassfish, OpenLiberty) or others. Once again, it is not necessary. If you don't like it, I'll remove it.

Probably best to have this in another PR, and explain the rationale and provide context also.

  • Upgrade the JDK8 build to e.g. JDK11

We already required JDK11 to run Spotless. I think we could make JDK11 mandatory for building, and use the -release flag to build with target of 8.

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

I was checking about Maven Toolchains, the problem i see is that the path of each JDK needs to be specified manually. That's a problem because everyone has a different path on their machine, and the plugin cannot find the JDK by using a discovery mechanism. That's a big drawback compared to the Gradle Java toolchains.

Github Actions setup-java now supports Maven Toolchains, but that doesn't solve the problem for individual developers.

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 7, 2022

I was checking about Maven Toolchains, the problem i see is that the path of each JDK needs to be specified manually. That's a problem because everyone has a different path on their machine, and the plugin cannot find the JDK by using a discovery mechanism. That's a big drawback compared to the Gradle Java toolchains.

Github Actions setup-java now supports Maven Toolchains, but that doesn't solve the problem for individual developers.

There are no such problems. The Toolchain plug-in is used just by the developer of the project. Consumers can see the references to it in the POM, but Maven does not take care. When depending on another project Maven reads <dependencies/> only, nothing else.

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

I was checking about Maven Toolchains, the problem i see is that the path of each JDK needs to be specified manually. That's a problem because everyone has a different path on their machine, and the plugin cannot find the JDK by using a discovery mechanism. That's a big drawback compared to the Gradle Java toolchains.
Github Actions setup-java now supports Maven Toolchains, but that doesn't solve the problem for individual developers.

There are no such problems. The Toolchain plug-in is used just by the developer of the project. Consumers can see the references to it in the POM, but Maven does not take care. When depending on another project Maven reads <dependencies/> only, nothing else.

I don't understand, if you installed your JDK in a different folder than my JDK, the toolchains.xml still has to reference the absolute path. Then it can't work.

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 7, 2022

I don't understand, if you installed your JDK in a different folder than my JDK, the toolchains.xml still has to reference the absolute path. Then it can't work.

The toolchain.xml is used locally by the developer and is not shared. The project POM references the appropriate JDK by some sort of criteria e.g. vendor, id and version. You can see in the POM there is

<jdkToolchain>
    <version>(,1.8]</version>
</jdkToolchain>

which says use JDK which version is up to 1.8 inclusive (JDK8 effectively)

and

<jdkToolchain>
    <version>[9,)</version>
</jdkToolchain>

which means any JDK which version is higher or equal to 9 (JDK11, JDK17, JDK18…)

The consumers don't need to deal with this at all (if they are not using Maven Toolchain themselves). They just need to use appropriate JVM which understands the byte code level of the classes in the dependent JAR — in our case any JVM from version 8 including.

This Apache page is quite nice, This page describes version range

Let me summarize on an example.

Project B uses (depends on) Project A.

Roles involved:

  • Developer of Project B
  • Developer of Project A

If Project A is not using Toolchains and Project B is not using Toolchains => nobody needs to deal with Toolchains; Developer of Project A nor Developer of Project B.

If Project A is not using Toolchains and Project B is using Toolchains => Developer of Project A does not need to deal w/ Toolchains; Developer of Project B must deal with it.

If Project A is using Toolchains and Project B is not using Toolchains => Developer of Project A must deal with Toolchains; Developer of Project B does not need to deal with it.

If Both Projects are using Toolchains => Both of them must deal with it.

In other words: Toolchains are local to project and their usage is not transitive in the sense of Maven dependency. The only issue in our case is that when we decide to switch to Toolchains all contributors will be forced to do the same. Consumers are not affected.

Hope this helps.

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

The toolchain.xml is used locally by the developer and is not shared

That's what i was missing ! But if someone never set or used toolchains, then it won't work. I think it's not very intuitive, no?

For instance i don't have a ~/.m2/toolchains.xml on my machine, so checking out the project and trying to run it would fail.

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 7, 2022

For instance i don't have a ~/.m2/toolchains.xml on my machine, so checking out the project and trying to run it would fail.

Yes, it fails if the configuration file is missing or does not provide JDK requested by the project.

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

For instance i don't have a ~/.m2/toolchains.xml on my machine, so checking out the project and trying to run it would fail.

Yes, it fails if the configuration file is missing or does not provide JDK requested by the project.

That's what I'm afraid of, we have so many contributors for this project, I don't want to add more impediments by using toolchains

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 7, 2022

That's what I'm afraid of, we have so many contributors for this project, I don't want to add more impediments by using toolchains

Agree.

What we can do is to use single JDK build. I've checked that even JDK19 can still build release 8. Question remains about JDK23 which will be the next LTS; but lets forget about it for now, we have plenty of time.

You said that JDK11 is already mandatory due to some testing stuff or alike, don't you?

Give me go-ahead and I'll clean-up the POM. :-)

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

You said that JDK11 is already mandatory due to some testing stuff or alike, don't you?

It's mentioned in the contributing instructions, jdk 11 is required to run the spotless maven plugin.

You could rebase your PR, I have updated all the plugins today!

@gotson
Copy link
Collaborator

gotson commented Nov 7, 2022

Since I added Maven enforcer today to enforce minimum maven version maybe we can use that to enforce minimum jdk version? https://maven.apache.org/enforcer/enforcer-rules/requireJavaVersion.html

This will also need updating in that case https://github.com/xerial/sqlite-jdbc/blob/master/CONTRIBUTING.md#prerequisites

And we need to amend the CI job to drop jdk 8 and use jdk 11 instead for all the jobs that were on 8

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 7, 2022

Since I added Maven enforcer today to enforce minimum maven version maybe we can use that to enforce minimum jdk version? https://maven.apache.org/enforcer/enforcer-rules/requireJavaVersion.html

This will also need updating in that case https://github.com/xerial/sqlite-jdbc/blob/master/CONTRIBUTING.md#prerequisites

And we need to amend the CI job to drop jdk 8 and use jdk 11 instead for all the jobs that were on 8

All done. The only problem is with one test which seems to be crashing from something who-knows-whats-going-on. The tests seem to be fine, but the JVM did something bad…

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 7, 2022

We already required JDK11 to run Spotless. I think we could make JDK11 mandatory for building, and use the -release flag to build with target of 8.

Yes, the release flag for 8. Will do so tomorrow

@gotson
Copy link
Collaborator

gotson commented Nov 8, 2022

All done. The only problem is with one test which seems to be crashing from something who-knows-whats-going-on. The tests seem to be fine, but the JVM did something bad…

which one would that be? i see CI is green now

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 8, 2022

It started at 2022-11-07T19:13:21.9227372Z and the job in question is

2022-11-07T19:13:34.9620516Z ##[group]Run uraimo/run-on-arch-action@v2 
2022-11-07T19:13:34.9620859Z with:                                     
2022-11-07T19:13:34.9621065Z   arch: armv7                             
2022-11-07T19:13:34.9621308Z   distro: ubuntu_latest

It is the previous run as of writing this.

The log has the following message:

2022-11-07T19:21:10.4765795Z [ERROR] Error occurred in starting fork, check output in log                                                     
2022-11-07T19:21:10.4766310Z [ERROR] Process Exit Code: 1              
2022-11-07T19:21:10.4781159Z [ERROR] Crashed tests:                    
2022-11-07T19:21:10.4795018Z [ERROR] org.sqlite.architecture.TestCodingRulesTest                                                              
2022-11-07T19:21:10.4855714Z [ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?                         
2022-11-07T19:21:10.4896038Z [ERROR] Command was /bin/sh -c cd /work && /usr/lib/jvm/java-11-openjdk-armhf/bin/java -jar /work/target/surefire/surefirebooter3000460293369316902.jar /work/target/surefire 2022-11-07T19-17-44_960-jvmRun1 surefire3163602568984339717tmp surefire_015744727068334001731tmp

I have the complete log downloaded and can provide it to you if necessary.

Interesting thing is that the second run was successful. Another explanation could be issues on GitHub site. It got extremely slow and I faced lots of out-of-available-servers errors.

@gotson
Copy link
Collaborator

gotson commented Nov 8, 2022

It started at 2022-11-07T19:13:21.9227372Z and the job in question is

2022-11-07T19:13:34.9620516Z ##[group]Run uraimo/run-on-arch-action@v2 
2022-11-07T19:13:34.9620859Z with:                                     
2022-11-07T19:13:34.9621065Z   arch: armv7                             
2022-11-07T19:13:34.9621308Z   distro: ubuntu_latest

It is the previous run as of writing this.

The log has the following message:

2022-11-07T19:21:10.4765795Z [ERROR] Error occurred in starting fork, check output in log                                                     
2022-11-07T19:21:10.4766310Z [ERROR] Process Exit Code: 1              
2022-11-07T19:21:10.4781159Z [ERROR] Crashed tests:                    
2022-11-07T19:21:10.4795018Z [ERROR] org.sqlite.architecture.TestCodingRulesTest                                                              
2022-11-07T19:21:10.4855714Z [ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?                         
2022-11-07T19:21:10.4896038Z [ERROR] Command was /bin/sh -c cd /work && /usr/lib/jvm/java-11-openjdk-armhf/bin/java -jar /work/target/surefire/surefirebooter3000460293369316902.jar /work/target/surefire 2022-11-07T19-17-44_960-jvmRun1 surefire3163602568984339717tmp surefire_015744727068334001731tmp

I have the complete log downloaded and can provide it to you if necessary.

Interesting thing is that the second run was successful. Another explanation could be issues on GitHub site. It got extremely slow and I faced lots of out-of-available-servers errors.

If a rerun ends up passing, it should be fine :)

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 8, 2022

If a rerun ends up passing, it should be fine :)

LOL 😆 Sounds deterministic to me 😁

@gotson
Copy link
Collaborator

gotson commented Nov 8, 2022

If a rerun ends up passing, it should be fine :)

LOL 😆 Sounds deterministic to me 😁

should only care when it's effectively flaky

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 8, 2022

Yes, the release flag for 8. Will do so tomorrow

Now done.

@hadrabap
Copy link
Contributor Author

hadrabap commented Nov 8, 2022

So we have one last open question.

What shall I do next? Any ideas, improvements?

@gotson
Copy link
Collaborator

gotson commented Nov 8, 2022

So we have one last open question.

What shall I do next? Any ideas, improvements?

The module name ? I am fine with org.xerial.sqlitejdbc

@gotson
Copy link
Collaborator

gotson commented Nov 11, 2022

@hadrabap is there some rework you need to do, or is this ready for merging?

@hadrabap
Copy link
Contributor Author

@gotson From my side it's complete and ready for merge.

@gotson
Copy link
Collaborator

gotson commented Nov 11, 2022

@gotson From my side it's complete and ready for merge.

just tested this locally for the first time, but the generated jar doesn't contain a java9 folder and doesn't contain the module-info. What am i doing wrong?

My bad, i was looking at the wrong place -_-

@gotson gotson merged commit 5bf7566 into xerial:master Nov 11, 2022
@gotson gotson added the enhancement:build Enhancement specific to the build process label Nov 11, 2022
@hadrabap
Copy link
Contributor Author

just tested this locally for the first time, but the generated jar doesn't contain a java9 folder and doesn't contain the module-info. What am i doing wrong?

The java9 folder is just to separate JDK9 stuff from the default JDK8 compilation process.

Look for module-info.class in /META-INF/versions/9/ in the generated JAR.

@hadrabap
Copy link
Contributor Author

@gotson Well, we have issue with JavaDoc.

I have a fix for it. Where should I put it? New PR? Or do you want to handle it on your own?

The fix is simple: adding <release>8</release> to JavaDoc <configuration/>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement:build Enhancement specific to the build process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants