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

Put module-info.class into Multi-Release JAR folder #2013

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Nov 6, 2021

Places the module-info.class into the version specific folder META-INF/versions/9. This should hopefully fix issues with build tools for older JDK versions (or Android) which expect only Java 6 compatible classes in the JAR:

If users still encounter issues afterwards, they probably have to update their build tool, see for example #1627 (comment).

When I tested the newly created JAR manually, Java was able to find the module descriptor file in the Multi-Release JAR without issues.

@google-cla google-cla bot added the cla: yes label Nov 6, 2021
@Marcono1234 Marcono1234 force-pushed the marcono1234/module-info-multi-release-jar branch from d7f7a39 to 9a4c0b1 Compare Nov 6, 2021
pom.xml Outdated
<groupId>org.apache.felix</groupId>
<artifactId>maven-bundle-plugin</artifactId>
<version>5.1.2</version>
<inherited>true</inherited>
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Nov 6, 2021

Choose a reason for hiding this comment

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

Have removed this because it looks like this is not used (unless I am overlooking something). The gson Maven submodule is using the bnd-maven-plugin.

Uses ModiTect to place module-info.class under Multi-Release JAR folder
`META-INF/versions/9`.
@Marcono1234 Marcono1234 force-pushed the marcono1234/module-info-multi-release-jar branch from 9a4c0b1 to 2d656a1 Compare Nov 8, 2021
@@ -68,23 +68,45 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.1</version>
<configuration>
<release>${javaVersion}</release>
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Nov 8, 2021

Choose a reason for hiding this comment

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

This change caused all Maven modules and test classes to be build using Java 1.6, which is why I removed @SafeVarargs from two test classes and adjusted ProtoTypeAdapter because Map.putIfAbsent was added in Java 8 while ConcurrentMap.putIfAbsent already existed in Java 1.6.

pom.xml Outdated
supporting compiling Java 1.6, see https://bugs.openjdk.java.net/browse/JDK-8028563 -->
<!-- When Gson increases lowest supported Java version, can
change this to an open version range (e.g. `[11,)`) -->
<version>[9,11]</version>
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Nov 9, 2021

Choose a reason for hiding this comment

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

Not completely sure why this works for the GitHub CI workflow (or rather why it is ignored?). For Surefire plugin it caused build failures, see https://github.com/Marcono1234/gson/runs/4146407481.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Nov 9, 2021

This is great.

Concerning Java 6, I think we could reasonably consider dropping support for that platform now. It has been 10 years since Java 7. So maybe we can drop Java 6, separately, and undo the changes in this PR that were required to support it.

@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Dec 27, 2021

Have resolved the merge conflict. Is there anything else you want to have changed?

Edit: It appears the fuzzer execution is failing now because the release version overwriting in google/oss-fuzz#6833 does not have any effect anymore. Maybe these Maven plugin config system properties only have an effect when not explicitly specified in the POM. However, a new workaround might be to overwrite the custom javaVersion property introduced here instead. That means, using -DjavaVersion=8 in the fuzzer build script instead.

pom.xml Outdated
@@ -28,7 +28,7 @@

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<java.version>1.6</java.version>
<javaVersion>6</javaVersion>
Copy link
Member

@eamonnmcmanus eamonnmcmanus Dec 29, 2021

Choose a reason for hiding this comment

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

Would it work if you set this to 7? If so then I think it is probably time for a separate PR to fix #2018 by changing the target version to 7 everywhere.

Copy link
Collaborator Author

@Marcono1234 Marcono1234 Dec 29, 2021

Choose a reason for hiding this comment

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

Do you mean to work around the fuzzer exception? That can probably rather be solved by adjusting the fuzzer script to use -DjavaVersion=8 instead (which should hopefully overwrite this property here then).

Changing this property to 7 would effectively change the complete build to use Java 7.

Copy link
Member

@eamonnmcmanus eamonnmcmanus Dec 30, 2021

Choose a reason for hiding this comment

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

Sorry, I was unclear. I was indeed suggesting that if changing the complete build to use Java 7 fixes this problem, then that's another reason to do it now. So if the CIFuzz check passes with that change, we can make a separate PR for just the Java version change, close #2018, and proceed with this PR.

Copy link
Collaborator Author

@Marcono1234 Marcono1234 Jan 1, 2022

Choose a reason for hiding this comment

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

The fuzzer seems to run successfully now.

@eamonnmcmanus eamonnmcmanus merged commit 6b96a38 into google:master Jan 1, 2022
4 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/module-info-multi-release-jar branch Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants