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

Modular Java #16178

Closed
wants to merge 0 commits into from
Closed

Modular Java #16178

wants to merge 0 commits into from

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Mar 14, 2024

Summary

This changeset attempts to discover the minimal set of changes necessary to add module-info.java definitions to Protocol Buffers. Fixes and closes #16133.

This is a draft PR for now. I'm looking for feedback and to setup an integration testsuite downstream with popular Protobuf-based projects to make sure things work as expected.

PR Tree

This PR is applied on top of the following:

Relevant PRs + issues upstream

JAR Structure

JARs issued as part of the Protobuf Java release now have Multi-Release: true instead of Automatic-Module-Name within their MANIFEST.MF, making them eligible to be considered as Multi-Release JARs (or, MRJARs). Such JARs can include a module-info.class without interfering with bytecode targeting at JDK8 or earlier, where modules are not supported yet. These JARs are thus called "modular MRJARs."

For example, after building Protobuf Java with bazel build //java:release, vim bazel-bin/java/core/amended_lite_mvn-project.jar shows:

JAR structure:

META-INF/MANIFEST.MF
META-INF/
META-INF/versions/
META-INF/versions/9/
META-INF/versions/9/module-info.class
# ...

In the MANIFEST.MF:
Screenshot 2024-03-14 at 5 25 16 PM

Modular Java downstream

For projects that build with Protobuf Java on the classpath, no change is experienced by the end-user; for projects which build on modulepath:

module my.module {
  requires com.google.protobuf;
  requires com.google.protobuf.kotlin;  // optional
  requires com.google.protobuf.util;  // optional
}

Open module

Protobuf, Protobuf Util, and Protobuf Kotlin are expressed as open modules. This is the case because Protobuf Java is often used reflectively by library consumers. By default, all packages provided by Modular Java protobuf artifacts are open as a result.

Exports + Lite Variants

Protobuf Java Core, Protobuf Java Util, and Protobuf Kotlin all reside in one package path each, which is extremely lucky and convenient for Modular Java because there is no split package problem.

Kotlin Lite/Kotlin and Java Core/Java Lite are meant to be used exclusively within the classpath, but there is no enforcement possible for this case from the compiler or VM, at least not in a consistent way across environments. Building on the modulepath solves this because Kotlin Lite/Kotlin and Java Core/Java Lite share a module coordinate and export an identical package path.

Thus, only one of (Kotlin/Kotlin Lite) and (Java Core/Java Lite) can be used at a time when building on the modulepath. This guarantee is enforced by the compiler and by the JVM at runtime.

Changelog

  • feat(java): support for modular java
  • fix: transition to rules_kotlin

Copy link
Contributor Author

@sgammon sgammon left a comment

Choose a reason for hiding this comment

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

✅ Bazel 7.x passes
✅ Bazel 6.x passes
✅ Maven build fixed and passing all tests
Automatic-Module-Name replaced with module-info in all artifacts
✅ Artifacts retain pre-JDK9 bytecode targeting (expressed as MRJARs)
⚠️ Docs for use with Modular Java
⚠️ Downstream integration testing

.bazelrc Outdated
Comment on lines 36 to 42
# JPMS and Turbine do not get along yet.
build --nojava_header_compilation
build --incompatible_use_toolchain_resolution_for_java_rules
build --java_language_version=11
build --java_runtime_version=11
build --tool_java_language_version=11
build --tool_java_runtime_version=11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensures the build stays pinned at Java 11 even when Java toolchains kick in. Needed for Bazel 7.x support.

WORKSPACE Outdated
@@ -62,6 +62,7 @@ maven_install(
repositories = [
"https://repo1.maven.org/maven2",
"https://repo.maven.apache.org/maven2",
"https://jpms.pkg.st/repository",
Copy link
Contributor Author

@sgammon sgammon Mar 14, 2024

Choose a reason for hiding this comment

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

This repository hosts JPMS artifacts for Guava before release, so we can test with them here in the Protobuf project. This change is therefore only temporary.

WORKSPACE Outdated
@@ -91,11 +92,11 @@ load("@rules_cc//cc:repositories.bzl", "rules_cc_dependencies")
rules_cc_dependencies()

# For `kt_jvm_library`
load("@io_bazel_rules_kotlin//kotlin:repositories.bzl", "kotlin_repositories")
load("@rules_kotlin//kotlin:repositories.bzl", "kotlin_repositories")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New version of rules_jvm_external that had to be upgraded for Bazel 7.x needs this repo expressed as rules_kotlin. Attempts to preserve the name and map with repo_mapping = {...} didn't work at first but it could be investigated further if changing the repo name is intolerable.

Comment on lines 22 to 35
JAVA9_OPTS = [
"-source 9",
"-target 9",
]

## Default JAR manifest entries to stamp on export.
DEFAULT_MANIFEST_ENTRIES = {
"Multi-Release": "true"
}

## Default visibility settings for Java module targets.
DEFAULT_JMOD_VISIBILITY = [
"//java:__subpackages__",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Java 9 module opts
  2. JAR manifests should carry Multi-Release: true when using META-INF/versions/...
  3. Java Module targets built in this PR are private to //java/...

Comment on lines 50 to 65
def protobuf_java_module(
name,
module,
module_deps = [],
deps = [],
jvm_version = JPMS_JDK_TARGET,
visibility = DEFAULT_JMOD_VISIBILITY,
**kwargs):
"""Builds a `module-info.java` definition for use with a Protobuf Java target.

This macro replaces a chain of `java_library` and `genrule` steps to coax Bazel
into placing the `module-info.class` at the right path in a versioned modular
MRJAR (Multi-Release JAR).

The name provided to this target is then meant to be used downstream as a
JAR-unwrapped classfile. For example:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New macro for building a Java Module for use in the Protobuf Java library; example of use:

protobuf_java_module(
    name = "my-module",
    module = "src/some/module-info.java",
)
java_library(
    name = "...",
    resources = [":my-module"],
    resource_strip_prefix = "src/some/",
)

The macro creates a target tree that looks like this:

java_library(
  name = "module",
  srcs = [module],
  javacopts = JAVA9_OPTS,
  deps = deps_rewritten + (deps or []),  # rewritten module deps + regular deps
  visibility = visibility,
)
genrule(
  name = "module-class",
  srcs = [":module"],
  command = "mkdir -p META-INF/versions/9 && unzip module.jar module-info.class > META-INF/versions/9/module-info.class",
)
genrule(
  name = "module-class-zip",
  srcs = [":module-class"]
  command = "zip -r - META-INF > module-java9.jar",
)
java_import(
  name = "module-jar",
  jars = [":module-java9.jar"]
)

# This target is used in the `resources = [ ... ]` attr of a `java_library`
alias(
  name = "name-of-module-macro-target",
  actual = "module-class",
)

# This (hidden) target is used internally by the macro to target `module_deps`
alias(
  name = "name-of-module-macro-target-jar",
  actual = "module-jar",
)

java/osgi/OsgiWrapper.java Outdated Show resolved Hide resolved
@@ -122,6 +126,8 @@ public Integer call() throws Exception {
analyzer.setProperty(Analyzer.BUNDLE_DOCURL, bundleDocUrl);
analyzer.setProperty(Analyzer.BUNDLE_LICENSE, bundleLicense);
analyzer.setProperty(Analyzer.REMOVEHEADERS, REMOVEHEADERS);
analyzer.setProperty(MULTI_RELEASE_MANIFEST_PROPERTY, "true");
analyzer.setProperty(FIXUP_MESSAGES_PROPERTY, FIXUP_MESSAGES_IGNORE_MRJAR);
Copy link
Contributor Author

@sgammon sgammon Mar 15, 2024

Choose a reason for hiding this comment

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

Adjusts BND arguments in the following manner

  • Automatic-Module-Name is removed
  • Multi-Release: true is added
  • -fixupmessages is passed to suppress a BND validation failure when classes reside in META-INF/versions, which is legal for MRJARs.

These same changes are applied to the BND tools invocation in Error Prone and Guava.

Copy link

Choose a reason for hiding this comment

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

Does bnd validation not support multi-release jars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sineaggi It just identifies META-INF/versions as "invalid classes," which of course they aren't. I'm happy to dig in as I find it to be a bit strange too.

Copy link
Contributor Author

@sgammon sgammon Apr 2, 2024

Choose a reason for hiding this comment

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

Ultimately Protobuf is not being "built" or re-exported with different class hierarchy at different versions; only the module descriptor remains as a compiled class within that root. So I am figuring that since META-INF and module-info are not valid class identifiers outside of MRJARs and modular JARs, maybe that's where Bnd is choking.

But you're right, I'm not sure, that's a guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Strange, because bnd is capable of generating its own module-info.java definitions; it is a feature described in their JPMS section. MRJARs were created mostly to allow the safe transition to JDK 9+, so I would imagine that if they support JPMS stuff they should support MRJARs.)

Comment on lines 147 to 143
} else {
List<String> err = analyzer.getErrors();
System.out.println("BND analyzer failed: " + err.toString());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the ability to print errors from BND tools during dev and felt it may be useful to keep.

java/pom.xml Outdated
Comment on lines 171 to 179
<version>1.23</version>
<dependencies>
<dependency>
<groupId>org.ow2.asm</groupId>
<artifactId>asm</artifactId>
<version>9.6</version>
</dependency>
</dependencies>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ASM upgraded → 9.6 to let Animal Sniffer work with JVM 21.

Comment on lines 10 to 13
"com.google.errorprone:error_prone_annotations:2.26.1",
"com.google.j2objc:j2objc-annotations:3.0.0",
"com.google.guava:guava:33.0.0-jre-jpms",
"com.google.guava:guava-testlib:33.0.0-jre",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the version bumps needed to use entirely modular Java with Error Prone, J2ObjC, etc. The Guava release version follows regular Guava releases until JPMS support lands.

@sgammon
Copy link
Contributor Author

sgammon commented Mar 15, 2024

I happen to be quite familiar with how Java Toolchains work in Bazel, and I find JPMS to be a little awkward to work with (the genrule pattern in this PR is a good example). Maybe rules_jpms would be a good thing, or I could file upstream to improve rules_java for this case.

java/osgi/OsgiWrapper.java Outdated Show resolved Hide resolved
@sgammon
Copy link
Contributor Author

sgammon commented Apr 3, 2024

@Sineaggi I cleaned up the --module_name flag and its usage; the other changes applied were from automatic formatting. I think it is ready for review again.

I'm not using this downstream yet in integration tests, but I have a harness going with modular Guava and GSON, and I'll add Protobuf to that testing cycle. Integration tests run against several projects downstream, many of which depend on Protobuf, so at least obvious issues should surface quickly.

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.

Add a module-info.java
2 participants