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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Guava + JPMS #7094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ consult the
## Snapshots and Documentation

Snapshots of Guava built from the `master` branch are available through Maven
using version `HEAD-jre-SNAPSHOT`, or `HEAD-android-SNAPSHOT` for the Android
using version `1.0-HEAD-jre-SNAPSHOT`, or `1.0-HEAD-android-SNAPSHOT` for the Android
flavor.

[Snapshot API Javadoc][guava-snapshot-api-docs] as well as
Expand Down Expand Up @@ -108,7 +108,7 @@ flavor.
options open in case of surprises (like, say, a serious security problem).

3. Guava has one dependency that is needed for linkage at runtime:
`com.google.guava:failureaccess:1.0.2`. It also has
`com.google.guava:failureaccess:1.0.3`. It also has
[some annotation-only dependencies][guava-deps], which we discuss in more
detail at that link.

Expand Down
2 changes: 1 addition & 1 deletion android/guava-bom/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<groupId>com.google.guava</groupId>
<artifactId>guava-bom</artifactId>
<version>HEAD-android-SNAPSHOT</version>
<version>1.0-HEAD-android-SNAPSHOT</version>
<packaging>pom</packaging>

<parent>
Expand Down
2 changes: 1 addition & 1 deletion android/guava-testlib/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-android-SNAPSHOT</version>
<version>1.0-HEAD-android-SNAPSHOT</version>
</parent>
<artifactId>guava-testlib</artifactId>
<name>Guava Testing Library</name>
Expand Down
2 changes: 1 addition & 1 deletion android/guava-tests/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-android-SNAPSHOT</version>
<version>1.0-HEAD-android-SNAPSHOT</version>
</parent>
<artifactId>guava-tests</artifactId>
<name>Guava Unit Tests</name>
Expand Down
4 changes: 2 additions & 2 deletions android/guava/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-android-SNAPSHOT</version>
<version>1.0-HEAD-android-SNAPSHOT</version>
</parent>
<artifactId>guava</artifactId>
<packaging>bundle</packaging>
Expand All @@ -21,7 +21,7 @@
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>failureaccess</artifactId>
<version>1.0.2</version>
<version>1.0.3</version>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
Expand Down
7 changes: 4 additions & 3 deletions android/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-android-SNAPSHOT</version>
<version>1.0-HEAD-android-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

I assume that you don't know of any specific harm that would arise if we were to modularize the android flavor, as well? There is probably some risk, since Android-focused tools are less likely to be aware of modules than JVM-focused tools, but I'd expect it to be pretty small at this point. The upside would be that cross-platform libraries (which may declare a dependency on guava-android even when most of their users run on the JVM) would still be able to use a properly modularized Guava.

I ask because our internal setup actually makes it easier for me to make the change in both places. This wouldn't require any changes on your part; in my internal copy of this PR, our internal setup already made the changes to android :) But in the unlikely event that anyone knows of reasons for me to undo that, I can do so.

Copy link
Contributor Author

@sgammon sgammon Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I assume that you don't know of any specific harm that would arise if we were to modularize the android flavor, as well?

I am not aware of any problem this would cause. Android supports up to JVM 21 bytecode, and IIRC modules are ignored past the javac phase.

There is probably some risk [for Android...]

Yeah, but keep in mind we are packaging the module within the version root at META-INF/versions/9/*, so a badly behaving tool would need to (1) be unaware of, or lack support for, modules, and would have to (2) scan JARs on the modulepath blindly, and would have to (3) violate JDK 8 spec (by not skipping that as an invalid class name), and would have to (4) have updated Guava to the latest version, implying it is maintained.

This fear keeps me up at night, too, but I think that actually affords quite a bit of protection. Even ancient tools which were released pre-JDK8 should have no issue since classes were not valid in META-INF/* at all back then. Thus, the risky part would be shipping the module-info.class at the root of the JAR, which probably isn't even that risky. That could be done in a few versions once confidence is in place, or when Java 9 becomes the minimum.

I ask because our internal setup actually makes it easier for me to make the change in both places

Ah, I see. If you want to apply the change uniformly, I am all for it. I stayed out of Android and GWT because I am not sure if there is other dark magic going on.

<packaging>pom</packaging>
<name>Guava Maven Parent</name>
<description>Parent for guava artifacts</description>
Expand Down Expand Up @@ -42,7 +42,7 @@
<module.status>integration</module.status>
<variant.jvmEnvironment>android</variant.jvmEnvironment>
<variant.jvmEnvironmentVariantName>android</variant.jvmEnvironmentVariantName>
<otherVariant.version>HEAD-jre-SNAPSHOT</otherVariant.version>
<otherVariant.version>1.0-HEAD-jre-SNAPSHOT</otherVariant.version>
<otherVariant.jvmEnvironment>standard-jvm</otherVariant.jvmEnvironment>
<otherVariant.jvmEnvironmentVariantName>jre</otherVariant.jvmEnvironmentVariantName>
</properties>
Expand Down Expand Up @@ -86,6 +86,7 @@
<module>guava-bom</module>
<module>guava-testlib</module>
<module>guava-tests</module>
<module>../futures/failureaccess</module>
</modules>
<build>
<!-- Handle where Guava deviates from Maven defaults -->
Expand Down Expand Up @@ -211,7 +212,7 @@
</plugin>
<plugin>
<artifactId>maven-jar-plugin</artifactId>
<version>3.2.0</version>
<version>3.4.0</version>
</plugin>
<plugin>
<artifactId>maven-javadoc-plugin</artifactId>
Expand Down
32 changes: 27 additions & 5 deletions futures/failureaccess/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>26.0-android</version>
<version>1.0-HEAD-jre-SNAPSHOT</version>
<relativePath>../../pom.xml</relativePath>
</parent>
<artifactId>failureaccess</artifactId>
<version>1.0.2</version>
<packaging>bundle</packaging>
<version>1.0.3</version>
<packaging>jar</packaging>
<name>Guava InternalFutureFailureAccess and InternalFutures</name>
<description>
Contains
Expand All @@ -22,14 +23,34 @@
</description>
<build>
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<executions>
<execution>
<id>compile-java9</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<release>9</release>
<multiReleaseOutput>true</multiReleaseOutput>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-jar-plugin</artifactId>
<configuration>
<archive>
<manifestFile>${project.build.outputDirectory}/META-INF/MANIFEST.MF</manifestFile>
<manifestEntries>
<Automatic-Module-Name>com.google.common.util.concurrent.internal</Automatic-Module-Name>
<Multi-Release>true</Multi-Release>
</manifestEntries>
</archive>
<excludes>
<exclude>/module-info.class</exclude>
<exclude>META-INF/versions/9/com/google/common/util/concurrent/internal/*.class</exclude>
</excludes>
</configuration>
</plugin>
<plugin>
Expand All @@ -55,7 +76,8 @@
</executions>
<configuration>
<instructions>
<Export-Package>com.google.common.util.concurrent.internal</Export-Package>
<_fixupmessages>^Classes found in the wrong directory: .*</_fixupmessages>
<Export-Package>com.google.common.util.concurrent.internal,!META-INF.*</Export-Package>
<Bundle-DocURL>https://github.com/google/guava/</Bundle-DocURL>
</instructions>
</configuration>
Expand Down
20 changes: 20 additions & 0 deletions futures/failureaccess/src/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright (C) 2024 The Guava Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

/**
* Guava: Future Internals.
*/
module com.google.common.util.concurrent.internal {
exports com.google.common.util.concurrent.internal;
}
2 changes: 1 addition & 1 deletion guava-bom/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<groupId>com.google.guava</groupId>
<artifactId>guava-bom</artifactId>
<version>HEAD-jre-SNAPSHOT</version>
<version>1.0-HEAD-jre-SNAPSHOT</version>
<packaging>pom</packaging>

<parent>
Expand Down
29 changes: 17 additions & 12 deletions guava-gwt/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-jre-SNAPSHOT</version>
<version>1.0-HEAD-jre-SNAPSHOT</version>
</parent>
<artifactId>guava-gwt</artifactId>
<name>Guava GWT compatible libs</name>
Expand Down Expand Up @@ -45,7 +45,7 @@
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>failureaccess</artifactId>
<version>1.0.2</version>
<version>1.0.3</version>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
Expand Down Expand Up @@ -134,15 +134,20 @@
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<excludes>
<!-- Yes, we want to exclude ForceGuavaCompilation 4 times: -->
<!-- (And we might as well exclude DummyJavadocClass 3 times (though it would be harmless to include).) -->
<!-- 1. Don't compile it (since that requires a *non-test* dep on gwt-user. -->
<exclude>**/ForceGuavaCompilation*</exclude>
<exclude>**/DummyJavadocClass*</exclude>
</excludes>
</configuration>
<executions>
<execution>
<id>default-compile</id>
<configuration>
<excludes>
<!-- Yes, we want to exclude ForceGuavaCompilation 4 times: -->
<!-- (And we might as well exclude DummyJavadocClass 3 times (though it would be harmless to include).) -->
<!-- 1. Don't compile it (since that requires a *non-test* dep on gwt-user. -->
<exclude>**/ForceGuavaCompilation*</exclude>
<exclude>**/DummyJavadocClass*</exclude>
</excludes>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-jar-plugin</artifactId>
Expand Down Expand Up @@ -262,7 +267,7 @@
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>failureaccess</artifactId>
<version>1.0.2</version>
<version>1.0.3</version>
<classifier>sources</classifier>
</dependency>
<dependency>
Expand Down
41 changes: 40 additions & 1 deletion guava-testlib/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-jre-SNAPSHOT</version>
<version>1.0-HEAD-jre-SNAPSHOT</version>
</parent>
<artifactId>guava-testlib</artifactId>
<name>Guava Testing Library</name>
Expand Down Expand Up @@ -71,6 +71,45 @@
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<executions>
<execution>
<id>default-compile</id>
<configuration>
<compilerArgs combine.children="append" combine.self="append">
<arg>-XDignore.symbol.file</arg>
</compilerArgs>
</configuration>
</execution>
<execution>
<id>compile-java9</id>
<phase>compile</phase>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<release>9</release>
<compileSourceRoots>
<compileSourceRoot>${project.basedir}/src</compileSourceRoot>
</compileSourceRoots>

<!--
JPMS needs access to the module sources to complete a modular Java build. We also need to override
the base compiler settings (in the root `pom.xml`) to enable MRJAR output.
-->
<compilerArgs combine.self="override" combine.children="append">
<arg>-sourcepath</arg>
<arg>${project.basedir}/src</arg>
<arg>--add-reads=com.google.common=ALL-UNNAMED</arg>
<arg>--add-reads=com.google.common.testlib=ALL-UNNAMED</arg>
<!-- https://errorprone.info/docs/installation#maven -->
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xlint:-removal</arg>
<arg>-Xlint:-options</arg>
</compilerArgs>
<multiReleaseOutput>true</multiReleaseOutput>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-source-plugin</artifactId>
Expand Down
30 changes: 30 additions & 0 deletions guava-testlib/src/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (C) 2024 The Guava Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

/**
* Guava Testlib
*/
open module com.google.common.testlib {
Copy link
Member

Choose a reason for hiding this comment

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

It seems fine to be more permissive for the test library here by using open module: The stakes are just lower for guava-testlib than for guava. So, at this point, I'm just curious as to whether this ends up being necessary for something in particular or whether you left it this way to be conservative in general. I think that our tests pass even without open, assuming that I tested it right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left it this way to be conservative in general :) although happy to drop as well.

requires java.logging;
requires com.google.common;
requires com.google.common.util.concurrent.internal;

exports com.google.common.collect.testing;
exports com.google.common.collect.testing.features;
exports com.google.common.collect.testing.google;
exports com.google.common.collect.testing.testers;
exports com.google.common.escape.testing;
exports com.google.common.testing;
exports com.google.common.util.concurrent.testing;
}
2 changes: 1 addition & 1 deletion guava-tests/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>com.google.guava</groupId>
<artifactId>guava-parent</artifactId>
<version>HEAD-jre-SNAPSHOT</version>
<version>1.0-HEAD-jre-SNAPSHOT</version>
</parent>
<artifactId>guava-tests</artifactId>
<name>Guava Unit Tests</name>
Expand Down
8 changes: 4 additions & 4 deletions guava/module.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"group": "com.google.guava",
"module": "failureaccess",
"version": {
"requires": "1.0.2"
"requires": "1.0.3"
}
},
{
Expand Down Expand Up @@ -96,7 +96,7 @@
"group": "com.google.guava",
"module": "failureaccess",
"version": {
"requires": "1.0.2"
"requires": "1.0.3"
}
},
{
Expand Down Expand Up @@ -162,7 +162,7 @@
"group": "com.google.guava",
"module": "failureaccess",
"version": {
"requires": "1.0.2"
"requires": "1.0.3"
}
},
{
Expand Down Expand Up @@ -228,7 +228,7 @@
"group": "com.google.guava",
"module": "failureaccess",
"version": {
"requires": "1.0.2"
"requires": "1.0.3"
}
},
{
Expand Down
Loading