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 the 'compile' goal for 'compile-java9' #4314

Closed
wants to merge 4 commits into from
Closed

Conversation

cushon
Copy link
Collaborator

@cushon cushon commented Mar 9, 2024

and update to a newer version of bnd for multi-release jar support, see bndtools/bnd#2227.

and update to a newer version of bnd for multi-release jar support, see
bndtools/bnd#2227.
@cushon
Copy link
Collaborator Author

cushon commented Mar 9, 2024

@sgammon does this look reasonable to you?

Without this change I'm not seeing the compile-java9 step added in 0e95364 get executed, and the META-INF/versions/9/ classes aren't ending up in the artifact.

@cushon
Copy link
Collaborator Author

cushon commented Mar 9, 2024

Hmm, the version of bnd that supports mr-jars requires JDK 17:

Error:  Failed to execute goal biz.aQute.bnd:bnd-maven-plugin:7.0.0:bnd-process (generate-OSGi-manifest) on project error_prone_parent: Execution generate-OSGi-manifest of goal biz.aQute.bnd:bnd-maven-plugin:7.0.0:bnd-process failed: Unable to load the mojo 'bnd-process' in the plugin 'biz.aQute.bnd:bnd-maven-plugin:7.0.0' due to an API incompatibility: org.codehaus.plexus.component.repository.exception.ComponentLookupException: aQute/bnd/maven/plugin/BndMavenPlugin has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0

@cushon cushon mentioned this pull request Mar 9, 2024
@sgammon
Copy link
Contributor

sgammon commented Mar 9, 2024

@cushon Whoops! I can't believe that wasn't running. I thought I verified the JAR output. I'll check locally with this change applied and verify it with Guava downstream.

Re/BND, the MRJAR is assembled by the JAR step, and BND tools isn't aware of it. I don't think it needs to be aware of it. There are BND directives for generating module-info.java, but we have a definition so we don't need to generate one.

As far as BND loading MRJARs, I know that support is lacking there too, but the classes under META-INF/versions should be identical anyway and only consist of module-info.class.

@sgammon
Copy link
Contributor

sgammon commented Mar 9, 2024

@cushon Adding this directive to the BND tools fixes the build issue with classes in the MRJAR root:

-fixupmessages: ^Classes found in the wrong directory: .*

The final block in root pom.xml is:

              <bnd><![CDATA[
                Bundle-SymbolicName: com.google.$<replacestring;$<replacestring;${project.artifactId};^error_prone;errorprone>;_;.>
                Automatic-Module-Name: $<Bundle-SymbolicName>
                -exportcontents: com.google.errorprone*
                -noextraheaders: true
                -removeheaders: Private-Package
                -fixupmessages: ^Classes found in the wrong directory: .*
              ]]></bnd>

I'm just starting my day, so I'll be around. If you want me to push a PR to fix this let me know

@anthonyvdotbe
Copy link
Contributor

@cushon While you're at it, please remove requires java.base; from module-info.java. Every module requires java.base. Specifying it is like specifying import java.lang.*;.

@sgammon
Copy link
Contributor

sgammon commented Mar 9, 2024

@cushon A quick patch, I've verified it includes the classes and only the module-info.class (trying to prevent duplication of the actual classes)

diff --git a/annotations/pom.xml b/annotations/pom.xml
index 39a0627585..e68d412cbc 100644
--- a/annotations/pom.xml
+++ b/annotations/pom.xml
@@ -64,6 +64,9 @@
           </execution>
           <execution>
             <id>compile-java9</id>
+            <goals>
+              <goal>compile</goal>
+            </goals>
             <configuration>
               <source>9</source>
               <target>9</target>
@@ -82,6 +85,9 @@
               <Multi-Release>true</Multi-Release>
             </manifestEntries>
           </archive>
+          <excludes>
+            <exclude>/META-INF/versions/9/com/**/*.class</exclude>
+          </excludes>
         </configuration>
       </plugin>
     </plugins>
diff --git a/pom.xml b/pom.xml
index f7cad3c75b..7c0fc99120 100644
--- a/pom.xml
+++ b/pom.xml
@@ -153,6 +153,7 @@
                 -exportcontents: com.google.errorprone*
                 -noextraheaders: true
                 -removeheaders: Private-Package
+                -fixupmessages: ^Classes found in the wrong directory: .*
               ]]></bnd>
             </configuration>
           </execution>

@sgammon
Copy link
Contributor

sgammon commented Mar 9, 2024

@anthonyvdotbe I was under the impression that all Java modules had to depend on java.base? That's why it's there. Is that not the case?

@sgammon
Copy link
Contributor

sgammon commented Mar 9, 2024

I'm seeing the JDK 11 failure @cushon. I think I have a fix for that too.

@sgammon
Copy link
Contributor

sgammon commented Mar 9, 2024

Okay. Thank you @anthonyvdotbe, I learned something today.

The java.base module is not required to be stated in module-info.java. The JDK 11 failure happens because BND tries to load those classes, but we can make them inert instead:

This patch

  • Removes java.base
  • Fixes compile goal
  • Excludes Java 9 classes but includes module-info.class
  • Excludes META-INF from BND (see !META-INF...)
  • Applies the fixupmessages to BND
diff --git a/annotations/pom.xml b/annotations/pom.xml
index 39a0627585..e68d412cbc 100644
--- a/annotations/pom.xml
+++ b/annotations/pom.xml
@@ -64,6 +64,9 @@
           </execution>
           <execution>
             <id>compile-java9</id>
+            <goals>
+              <goal>compile</goal>
+            </goals>
             <configuration>
               <source>9</source>
               <target>9</target>
@@ -82,6 +85,9 @@
               <Multi-Release>true</Multi-Release>
             </manifestEntries>
           </archive>
+          <excludes>
+            <exclude>/META-INF/versions/9/com/**/*.class</exclude>
+          </excludes>
         </configuration>
       </plugin>
     </plugins>
diff --git a/annotations/src/main/java/module-info.java b/annotations/src/main/java/module-info.java
index 72215b7677..4c9077dd52 100644
--- a/annotations/src/main/java/module-info.java
+++ b/annotations/src/main/java/module-info.java
@@ -15,7 +15,6 @@
  */
 
 open module com.google.errorprone.annotation {
-  requires java.base;
   requires java.compiler;
   exports com.google.errorprone.annotations;
   exports com.google.errorprone.annotations.concurrent;
diff --git a/pom.xml b/pom.xml
index f7cad3c75b..3dc704b4f8 100644
--- a/pom.xml
+++ b/pom.xml
@@ -150,9 +150,10 @@
               <bnd><![CDATA[
                 Bundle-SymbolicName: com.google.$<replacestring;$<replacestring;${project.artifactId};^error_prone;errorprone>;_;.>
                 Automatic-Module-Name: $<Bundle-SymbolicName>
-                -exportcontents: com.google.errorprone*
+                -exportcontents: com.google.errorprone*,!META-INF.*
                 -noextraheaders: true
                 -removeheaders: Private-Package
+                -fixupmessages: ^Classes found in the wrong directory: .*
               ]]></bnd>
             </configuration>
           </execution>

cc / @cushon

google#4314 (comment)

* Remove unnecessary `requires java.base;`
* Excludes META-INF from BND (see !META-INF...)
@cushon cushon requested a review from cpovirk March 9, 2024 21:36
@cushon
Copy link
Collaborator Author

cushon commented Mar 9, 2024

Thanks @sgammon! I pushed another commit that includes the additional changes.

copybara-service bot pushed a commit that referenced this pull request Mar 9, 2024
#4314 (comment)

Fixes #4314

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4314 from cushon:module 604430d
PiperOrigin-RevId: 614272829
copybara-service bot pushed a commit that referenced this pull request Mar 9, 2024
#4314 (comment)

Fixes #4314

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4314 from cushon:module 604430d
PiperOrigin-RevId: 614272829
@copybara-service copybara-service bot closed this in ea5ef6d Mar 9, 2024
benkard pushed a commit to benkard/jgvariant that referenced this pull request Mar 12, 2024
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.google.errorprone:error_prone_core](https://errorprone.info) ([source](https://github.com/google/error-prone)) |  | minor | `2.25.0` -> `2.26.1` |
| [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | compile | minor | `2.25.0` -> `2.26.1` |

---

### Release Notes

<details>
<summary>google/error-prone</summary>

### [`v2.26.1`](https://github.com/google/error-prone/releases/tag/v2.26.1): Error Prone 2.26.1

[Compare Source](google/error-prone@v2.26.0...v2.26.1)

Changes:

-   Fix the module name of the annotations artifact: `com.google.errorprone.annotations` (google/error-prone@9d99ee7)

Full Changelog: google/error-prone@v2.26.0...v2.26.1

### [`v2.26.0`](https://github.com/google/error-prone/releases/tag/v2.26.0): Error Prone 2.26.0

[Compare Source](google/error-prone@v2.25.0...v2.26.0)

Changes:

-   The 'annotations' artifact now includes a `module-info.java` for Java Platform Module System support, thanks to [@&#8203;sgammon](https://github.com/sgammon) in [#&#8203;4311](google/error-prone#4311).
-   Disabled checks passed to `-XepPatchChecks` are now ignored, instead of causing a crash. Thanks to [@&#8203;oxkitsune](https://github.com/oxkitsune) in [#&#8203;4028](google/error-prone#4028).

New checks:

-   [`SystemConsoleNull`](https://errorprone.info/bugpattern/SystemConsoleNull): Null-checking `System.console()` is not a reliable way to detect if the console is connected to a terminal.
-   [`EnumOrdinal`](https://errorprone.info/bugpattern/EnumOrdinal): Discourage uses of `Enum.ordinal()`

Closed issues: [#&#8203;2649](google/error-prone#2649), [#&#8203;3908](google/error-prone#3908), [#&#8203;4028](google/error-prone#4028), [#&#8203;4311](google/error-prone#4311), [#&#8203;4314](google/error-prone#4314)

Full Changelog: google/error-prone@v2.25.0...v2.26.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
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.

4 participants