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

Fix JPMS module setup (fixes #1315) #1402

Merged
merged 2 commits into from Oct 18, 2018

Conversation

@pietvandongen
Copy link
Contributor

commented Oct 16, 2018

I've followed the instructions from the Apache Maven Compiler Plugin team here: https://maven.apache.org/plugins/maven-compiler-plugin/examples/module-info.html

This should ensure compatibility with Java 6+ while providing module info for Java 9+.

Also, in gson/src/main/java/com/google/gson/Gson.java I've removed calls to a constructor only introduced in Java 1.7, which broke backwards compatibility.

@@ -705,7 +705,7 @@ public void toJson(Object src, Type typeOfSrc, JsonWriter writer) throws JsonIOE
} catch (IOException e) {
throw new JsonIOException(e);
} catch (AssertionError e) {
throw new AssertionError("AssertionError (GSON " + GsonBuildConfig.VERSION + "): " + e.getMessage(), e);
throw new AssertionError("AssertionError (GSON " + GsonBuildConfig.VERSION + "): " + e.getMessage());

This comment has been minimized.

Copy link
@inder123

inder123 Oct 16, 2018

Collaborator

why are you changing these?

This comment has been minimized.

Copy link
@pietvandongen

pietvandongen Oct 16, 2018

Author Contributor

Because that constructor was only introduced in 1.7, which would make it not compatible with 1.6.

See https://docs.oracle.com/javase/7/docs/api/java/lang/AssertionError.html#AssertionError(java.lang.String,%20java.lang.Throwable)

This comment has been minimized.

Copy link
@inder123

inder123 Oct 16, 2018

Collaborator

Ah! Thanks.

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Oct 16, 2018

Member

Store the exception in a local and call initCause(e) so as not to break the chain of exceptions before throwing.

This comment has been minimized.

Copy link
@pietvandongen

pietvandongen Oct 17, 2018

Author Contributor

Isn't that beyond the scope of this pull request?

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Oct 17, 2018

Member

Nope. You're already changing these so I would either leave them as-is to retain the cause or make them correct for Java 6 while retaining the cause.

This comment has been minimized.

Copy link
@pietvandongen

pietvandongen Oct 17, 2018

Author Contributor

OK, agreed. If I revert this change, the build will fail. I do not completely understand what exactly you expect me to do using initCause. Could you spell it out for me so I can commit the change?

This comment has been minimized.

Copy link
@inder123

inder123 Oct 17, 2018

Collaborator

I think this is what Jake meant:

AssertionError error = new AssertionError("AssertionError (GSON " + GsonBuildConfig.VERSION + "): " + e.getMessage());
error.initCause(e); 
throw error;

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Oct 17, 2018

Member

Precisely! Just so we don't actually lose the causal chain.

This comment has been minimized.

Copy link
@pietvandongen

pietvandongen Oct 18, 2018

Author Contributor

Ahh, got it, thanks! I've fixed it and pushed those changes.

@inder123 inder123 merged commit 5bbc768 into google:master Oct 18, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
<execution>
<id>default-compile</id>
<configuration>
<release>9</release>

This comment has been minimized.

Copy link
@inder123

inder123 Oct 18, 2018

Collaborator

Seems like this requires JDK 9 for compilation?
I can no longer compile with JDK 8. I get this error:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project gson: Fatal error compiling: invalid flag: --release -> [Help 1]

How do I fix this?

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Oct 18, 2018

Member

Module info require JDK 9 or newer. While 8 isn't EOL yet, it will be in 5 months. You can grab a JDK 11 build here: https://adoptopenjdk.net/

This comment has been minimized.

Copy link
@inder123

inder123 Oct 18, 2018

Collaborator

Thanks Jake. I have not upgraded beyond JDK 8 so far because of my focus on Android.
I guess time to upgrade!

This comment has been minimized.

Copy link
@inder123

inder123 Oct 18, 2018

Collaborator

Have you used JDK 9 or 10 with Android? And does it work?

This comment has been minimized.

Copy link
@JakeWharton

JakeWharton Oct 18, 2018

Member

JDK 11 is my default, yes. It mostly just works fine. And wherever it doesn't it's a bug in the tooling.

FredricMei added a commit to orientsec/gson that referenced this pull request Feb 11, 2019
Fix JPMS module setup (fixes google#1315) (google#1402)
* Fix JPMS module setup (fixes google#1315)

* Re-added cause to AssertionErrors
@agoeb agoeb referenced this pull request Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.