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

feat: support jpms in annotations module #2302

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Mar 7, 2024

Summary

This PR adds a module-info.java definition to the annotations package, and adjusts the build to produce a MRJAR. The result is a JAR artifact which (1) remains safely usable on JDK 1.8, and (2) helps enable modular Java builds for consumers of the annotations.

Downstream, Guava depends on these annotations, and many artifacts depend on Guava. As a result, many projects that don't otherwise use J2ObjC or ship for iOS rely on J2ObjC adopting a module definition like this one.

I've made the minimum suite of changes possible here to maintain both the make and maven-based builds. The two produce nearly identical JARs, save for the Maven Compiler Plugin stamping the JAR manifest differently with its own message. I've cleaned and performed a full rebuild of the project to ensure the JARs are properly usable.

For now, I will build and issue the JAR artifacts at the version 2.8-jpms, and make them available for downstream testing. I think releasing the annotations under a version postfix like that might help projects opt-in before this ships for real.

Changelog

  • feat: add module-info.java to annotations
  • chore: wire up build support for building java9 root in a multi-release jar
  • chore: apply java9 jpms changes to both maven and makefile build

Relates to: google/guava#2970

@sgammon
Copy link
Contributor Author

sgammon commented Mar 7, 2024

cc / @cpovirk

- feat: add `module-info.java` to `annotations`
- chore: wire up build support for building `java9` root in MRJAR
- chore: apply java9 jpms changes to both maven and makefile build

Signed-off-by: Sam Gammon <sam@elide.ventures>
annotations/.gitignore Show resolved Hide resolved
annotations/Makefile Show resolved Hide resolved
annotations/Makefile Show resolved Hide resolved
annotations/pom.xml Show resolved Hide resolved
annotations/pom.xml Show resolved Hide resolved
annotations/pom.xml Show resolved Hide resolved
annotations/src/main/java/module-info.java Show resolved Hide resolved
@sgammon
Copy link
Contributor Author

sgammon commented Mar 8, 2024

make -j$(nproc) && make test_all passes, tested in the environment shown below. If there are any notable cases to cover, I'm happy to test locally, or amend CI.

CPU: M-series Mac (aarch64)
OS: macOS Sonoma `14.3.1`, Java 17 (Azul Zulu).

> java -version
openjdk version "11.0.19" 2023-04-18 LTS
OpenJDK Runtime Environment Zulu11.64+19-CA (build 11.0.19+7-LTS)
OpenJDK 64-Bit Server VM Zulu11.64+19-CA (build 11.0.19+7-LTS, mixed mode)

Copy link
Collaborator

@tomball tomball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this contribution, and all your notes regarding it. I'll work with the Guava team when this is committed so their next release uses this version.

@sgammon
Copy link
Contributor Author

sgammon commented Mar 8, 2024

@tomball Thank you for the quick response! And for all of your hard work on J2ObjC. 😄 I'll keep an eye on this in case you need any changes from me.

@cpovirk
Copy link
Member

cpovirk commented Mar 8, 2024

Thanks, @sgammon and @tomball!

Before your PR yesterday, I had been playing with the alternative of replacing Guava's j2objc-annotations dependency with local copies of the annotations in each Guava package that needed them. That was looking OK up until I realized that it would mean something like 20 annotations (since J2ObjC provides 4 different annotations, most of them used from various Guava packages). We could do that, but it feels more bloated and clumsy (especially if we need to introduce more copies in more packages over time).

So it will be great to have j2objc-annotations ready for modules. (No doubt some users would still prefer for us to remove the dependency entirely :) But I am much more concerned with the module issue than I am with the "I don't want Maven to pull in a jar of 10 annotations by default" issue.)

@copybara-service copybara-service bot merged commit d3c2ca6 into google:master Mar 8, 2024
3 checks passed
@sgammon
Copy link
Contributor Author

sgammon commented Mar 8, 2024

@cpovirk If 10 annotation classes are still too much, I can offer a PR which links users to Proguard ;)

@sgammon
Copy link
Contributor Author

sgammon commented Mar 8, 2024

@tomball Thank you both for the quick review and merge. Made my day! 🥳

@cpovirk
Copy link
Member

cpovirk commented Mar 8, 2024

:)

In fairness, my sense is that some of the concerns are things like compliance overhead and the need to include another project if you're building from source. But a lot of objections do sound like cases that would be best served with Proguard....

@tomball
Copy link
Collaborator

tomball commented Mar 8, 2024 via email

@sgammon
Copy link
Contributor Author

sgammon commented Mar 8, 2024

@tomball Happy meals were exciting to me when you started coding in Java. So I will defer to you, this is why

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.

None yet

3 participants