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

Add module-info.java to ship .jar as >=JDKv9 modular libraries #87

Merged
merged 5 commits into from
Nov 13, 2023
Merged

Conversation

brainstorm
Copy link
Contributor

@brainstorm brainstorm commented Aug 11, 2023

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Unit/integration tests
  • Code cleanup
  • Build system change
  • Documentation change

Pull request long description:

This change adds the Java module system exports so that third party projects (such as IGV, which also uses module-info.java when we transitioned it from Java 8 to Java 11) can see the methods and types that are required to interact with Crypt4GH.

This will facilitate:

  1. Integration on a future release of IGV (desktop), when and if htsjdk supports better (de)crypted stream support.
  2. Shipping it on Maven again, this time as a modern Java module, as pointed out in Resume publishing releases to maven central, please? #85.
  3. Not breaking module isolation by shipping shaded jars, discouraged after Java 9's module system.

/cc @AlexanderSenf @ohofmann

…nvolves renaming/moving/etc...). Also comment out compiler plugins and works toward publishability, warning: Required filename-based automodules detected: [blake2b-1.0.0.jar, commons-cli-1.5.0.jar, bkdf-0.6.0.jar, bcrypt-0.10.2.jar, scrypt-1.4.0.jar]. Please don't publish this project to a public artifact repository
… or switching between JDK11 and JDK20 fixed sth
@kjetilkl
Copy link
Collaborator

kjetilkl commented Sep 8, 2023

I have tested everything now to make sure modularization does not break any of our own code in other projects. It looks good! The only issue was that it seemed to interfere with Lombok somehow, but I was able to fix that by adding a configuration block to the maven-compiler-plugin in the pom.xml file.

            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-compiler-plugin</artifactId>
                <version>3.11.0</version>
                    <configuration>
                        <annotationProcessorPaths>
                          <annotationProcessorPath>
                                <groupId>org.projectlombok</groupId>
                                <artifactId>lombok</artifactId>
                                <version>1.18.28</version>
                            </annotationProcessorPath>
                        </annotationProcessorPaths>
                    </configuration>
            </plugin>

The reason we are using shading is that the JAR-file published under releases is being used as a stand-alone command line tool. So, we have included all its dependencies in the same JAR to simplify installation. But, I believe it should be possible to do both. The assets we publish to GitHub packages already include both the shaded JAR and the regular JAR, and the pom.xml file lists all the dependencies.

Our main project depends on libraries that we have developed across multiple GitHub organizations, and we are currently discussing whether we should consolidate these under a single new GroupID before we start publishing to Maven Central. This may or may not include updating package names also. But, hopefully, it should not be too long now.

@brainstorm
Copy link
Contributor Author

IIRC the shading introduces a few issues with the JDK>=9 module system... but as you pointed out, there are workarounds that exclude module-info.java from shaded JARs, i.e:

https://dev.to/cicirello/how-to-use-the-maven-shade-plugin-if-your-project-uses-java-platform-module-system-2cmh

I think that consolidating towards that GroupID you mention would help your org transition to the module system... how's that going btw?

@kjetilkl kjetilkl merged commit 7652b7e into uio-bmi:master Nov 13, 2023
2 of 3 checks passed
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