-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Guava + JPMS #7094
base: master
Are you sure you want to change the base?
Guava + JPMS #7094
Conversation
guava/src/module-info.java
Outdated
module com.google.common { | ||
requires java.base; | ||
requires java.logging; | ||
requires jdk.unsupported; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious as to which parts of Guava require jdk.unsupported
?
Not sure if it's correct to explicitly add a module requirement on it, since this is not a module dictated in the JLS, so it could be changed/removed/altered in some incompatible way in future Java releases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@norrisjeremy I believe this is because of sun.misc.Unsafe
; I'm not sure there is any way around requires jdk.unsupported
at least for the time being. Once Unsafe is refactored away this can be removed. We're 10 years in to the launch of JPMS and jdk.unsupported
is still around, and it is supported on all current Java versions up to latest LTS, so it should be okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I suppose.
But not sure if you have seen JDK-8323072 & it's associated draft JEP, which I believe may land in Java 23 later this year.
I think the doomsday clock for sun.misc.Unsafe
is advancing ever closer to midnight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@norrisjeremy Oh, agreed, for sure; but this PR just focuses on JPMS for now. JPMS and Unsafe are both sensitive changes that should be done soon, no question.
Java 22 comes out this month, in about 10 days. So you're right to be concerned for Java 23, but Guava has some time to address that before it becomes a problem. JPMS, meanwhile, has been out since Java 9 and is currently unsupported by Guava, so this PR advances on that front.
Even if Unsafe is deprecated in Java 23 (I don't foresee it getting removed completely yet), this changeset will let Guava work at Java 9 - Java 22 with fully modular Java. Then, working to refactor/fix the Unsafe
usages becomes easier, because Guava is shipping MRJARs, so Unsafe
can safely be used for earlier JVMs only, with later JVMs using classes that override with other intrinsics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
Btw, finally adding JPMS support is greatly appreciated btw. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@norrisjeremy Thank you likewise for the reviews :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does guava require unsafe, or can it safely fallback? If it doesn't require unsafe, we could use requires static here I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sineaggi If I understand correctly, the implications of that are basically the modular equivalent of a compile-time dependency, right? So users could effectively opt-in by adding requires jdk.unsupported
in their own module?
In any case, great catch. You're right: the testsuite passes in full with only requires static
.
@cpovirk Curious about your thoughts on behavior defaults here. By the way: technically it is legal to have a slightly different module-info.java
for different META-INF/versions/
within a single JAR. The public API surface of the module graph can't change, but including things from java
or jdk
was specifically imagined to be possible, so that newer class suites can leverage the latest JVM features while remaining compatible with strong module encapsulation:
Modular multi-release JAR files in JEP-238: Multi-Release JARs
(Fwiw, jar --validate
checks the consistency of these MRJAR APIs, the JPMS Attic is running this on Guava already)
That should mean that it is possible, in theory, to build with requires
or requires static
for jdk.unsupported
based on the active JVM. It would add some build complexity, but it's possible. So I think that means the options are:
-
requires jdk.unsupported
: All users are opting in toUnsafe
whether they like it or not; this is a drawback but the benefit is that the library performs as expected out of the box (i.e. it implies no performance hit from falling back fromUnsafe
) -
requires static jdk.unsupported
: All users can opt-in to use ofUnsafe
; when they upgrade Guava, depending on the shape of their workload, they may see a performance hit, but only if they are using fully modular Java, which would be a new use case at that library version anyway. Otherwise, on the classpath, there is no isolation and so there is no difference. -
Varied by JVM level: Perhaps, for users of Java 9 or even Java 11, we use
requires jdk.unsupported
, so that the library performs as expected for the vast majority of "legacy" use cases, which happen to begin building with Guava on themodulepath
simply because it ships amodule-info.java
. For Java 11+, themodule-info.java
specifiesrequires static jdk.unsupported
, and we trust more modern Java users (who are building in a fully modular manner, remember, classpath is still fine) to addrequires jdk.unsupported
if they need it.
Thoughts? Maybe this is overthinking it, I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, it would be helpful to see the actual performance hit that should realistically be expected; I'm midway through breaking the benchmark suite out and modernizing with JMH, so perhaps we'll look to some numbers from there or Caliper to help inform this. I think a big question mark is exactly what performance drop would be expected with the loss of Unsafe
as a default.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, perhaps adding requires static jdk.unsupported
to avoid a hard requirement on it at runtime would work?
I believe users that are performance sensitive and also desiring the ability to then also use Guava on the module-path instead of class-path could still make it work by adding --add-exports jdk.unsupported/sun.misc=com.google.common --add-opens jdk.unsupported/sun.misc=com.google.common
to their application.
New complication to this: |
37fdcc6
to
825394f
Compare
- chore: apply 'Harden Runner' auditing to all ci tasks - chore: apply `persist-credentials: false` to checkout tasks - chore: publish dependency graph and add dependency review check - chore: add codeql scan job (temp) Bumps [actions/checkout](https://github.com/actions/checkout) from 3.6.0 to 4.1.1. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3.6.0...b4ffde6) Bumps [actions/dependency-review-action](https://github.com/actions/dependency-review-action) from 2.5.1 to 4.1.3. - [Release notes](https://github.com/actions/dependency-review-action/releases) - [Commits](actions/dependency-review-action@0efb1d1...9129d7d) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major - dependency-name: actions/dependency-review-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: StepSecurity Bot <[email protected]> Signed-off-by: Sam Gammon <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This change refactors the main CI workflow into two new workflows, `on.pr.yml` and `on.push.yml`, which each call into the exiting CI job as a reusable workflow. This has the nice benefit of putting all tests, checks, builds, etc., on one screen during development on GitHub, allows customization of the PR vs. push flow, and yet keeps behavior fully consistent between the two. - chore: move ci jobs to `workflow_call` trigger - chore: add entrypoint jobs for PR and Push events - chore: cleanup permissions and dispatch checks/tests Signed-off-by: Sam Gammon <[email protected]>
This changeset switches the StepSecurity hardening action to enforced mode, where previously it was running in `audit` mode. Now, audit logs have been gathered and it is time to seal off the list of accessible network endpoints for a given job. - chore: gather and apply network endpoints for each job - chore: move to `block` mode for `egress-policy` in `step-security/harden-runner` Signed-off-by: Sam Gammon <[email protected]>
This changeset adds SLSA 3+ provenance support to the workflow. The main CI run has now been split into two: `ci.build.yml`, which only builds the library and is provenance-capable, and `ci.test.yml`, which is the previous CI logic. The regular build logic is applied only on push, and can be applied on PRs too, with publish of provenance material turned off. The test suite is invoked from PRs. The workflows have been split into build/test phases to avoid publishing provenance data and GitHub artifacts for build matrix outputs. JARs are uniform across OS targets, so there is no need to gather and publish for more than Ubuntu. - feat: add slsa support to build workflow - chore: split `test` into `build` and `test` workflows - chore: use new workflows (build/test) from push/pr triggers Signed-off-by: Sam Gammon <[email protected]>
Fails the build if any downloaded dependencies fail their checksum verification. - chore: add `--strict-checksums` flag to `mvnw` calls in ci
- chore: don't rebuild javadoc during tests in ci - chore: don't run with gpg enabled in ci Signed-off-by: Sam Gammon <[email protected]>
Adds two build parameters - `publishing.repository.snapshots`: Snapshot repo to deploy to - `publishing.repository.releases`: Releases repo to deploy to Both default to their current values, Sonatype. This small inert change allows a fork to easily publish to a different repository without resorting to a code change. Signed-off-by: Sam Gammon <[email protected]>
This changeset adds the Maven Sigstore plugin for use during publishing to Sonatype and other public repositories. - chore: add sigstore plugin to build Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
This changeset adds full support for modular Java builds in Guava, and in libraries which depend on Guava. The Guava JAR for JRE now structures as a Multi-Release JAR, with a module definition situated in `META-INF/versions/9/`. Guava remains compatible with JDK 8. - feat: add `module-info.java` to `guava` module - chore: update `guava` to build MRJAR - chore: adjust dev version → `1.0-HEAD-[jre|android]-SNAPSHOT` - chore: upgrade maven compiler plugin → `3.12.1` Fixes and closes google#2970 Relates-To: elide-dev/jpms#1 Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
This change adds a `module-info.java` definition for the ancillary `failureaccess` and `testlib` libraries. Test Lib is kept as an `open module` so that it can be reflectively introspected. - feat(jpms): add `module-info.java` to `failureaccess` - feat(jpms): add `module-info.java` to `testlib` - fix: necessary fixes to get testsuite running on modular java Signed-off-by: Sam Gammon <[email protected]>
This changeset generally fixes all Javadoc warnings at JVM 21, and upgrades the LTS Javadoc release to 21. Slight code changes have been made to fix Javadoc issues; no logic has changed. - fix: various javadoc flag fixes - fix: deprecations and other javadoc warnings - chore: upgrade javadoc plugin to latest - chore: upgrade javadoc java spec version → 21 Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
What's left to get this merged? |
@Sineaggi probably at least a rebase and a cleanup round. I'm waiting for feedback but if I don't receive any in the next few days/week, I'll go ahead and rebase and cleanup anyway. If there's anything specific that needs attention please let me know of course |
Guava with JPMS is released and under testing via the javamodules.dev repository. I am using it in several projects with no problems. |
With Netty and other libraries expressing interest as well, I plan to come back to this shortly. |
Summary
This PR adds modular Java support to Guava, by adding a
module-info.java
definition to theguava
andguava-testlib
modules. This includes upstream work in the Error Prone Compiler, J2ObjC Annotations, and Checker Framework to land modules as well, rendering Guava able to build entirely on themodulepath
.Fixes and closes #2970
Current Status
modulepath
Warning
This branch of Guava is still in testing. See the JPMS attic and the tracking issue if you want to use it ahead of release.
PR Tree
On the way to a finished PR, there were a number of other changes and improvements that surfaced. These have been broken out into their own PRs for easier review; they are listed below as Downstream PRs.
PRs that are Included in this PR can be rebased away, either when they are merged or closed without merging, at the Guava team's choosing.
The following PRs relate to this one:
annotations
error-prone#4311annotations
module j2objc#2302Using Modular Guava
From a Java 9+ application, in
module-info.java
:Tip
To build your app without any Guava transitive dependencies on the classpath, make sure to exclude
jsr305
. You will also need to use the JPMS Attic repository until a release occurs.Maven:
Gradle (Groovy):
Gradle (Kotlin):
Gradle (Version Catalog + Kotlin):
Forcing Resolution
Sometimes, your build will resolve to a different version of Guava. This usually happens because of a transitive dependency. Here are some instructions for Gradle:
build.gradle.kts
:configurations.all { resolutionStrategy.force("com.google.guava:guava:33.0.0-jre-jpms") }
Changelog
Expand for full changelog