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 build failure with JDK 9 EA b148 #470
Conversation
587a943
to
70804ba
Compare
@@ -86,6 +86,8 @@ case "$JDK" in | |||
;; | |||
9-ea | 9-ea-stable) | |||
# see https://bugs.openjdk.java.net/browse/JDK-8131041 about "java.locale.providers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment refers to the line below the next line of code. Moving down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof you mean change order of comments? Unfortunately it can't be placed exactly above line or on line to which it refers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. What about concatenating MAVEN_OPTS step-by-step instead of using a long multi-line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof not sure what you mean - one comment is about java.locale.providers
that is not part of MAVEN_OPTS
. Maybe possible to construct whole command in variable step-by-step, but I'm afraid to run into problem with escaping of quotes. Following suggestion of @bjkail below I'm going to split this command on multiple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact will be better to get rid of this java.locale.providers
completely, but that's another story and I'll try to handle it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Something like this:
# Groovy version should be updated to get rid of "--add-opens" options (see https://twitter.com/CedricChampeau/status/807285853580103684)
MAVEN_OPTS="--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED"
MAVEN_OPTS=$MAVEN_OPTS mvn -V -B -e verify -Dbytecode.version=1.9
# see https://bugs.openjdk.java.net/browse/JDK-8131041 about "java.locale.providers"
MAVEN_OPTS=$MAVEN_OPTS -DargLine=-Djava.locale.providers=JRE,SPI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof I'm not sure that -DargLine
can be passed via MAVEN_OPTS
- usually last one is used to pass options that control Maven JVM, while -D
is argument to Maven invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof ouch, and what you wrote isn't gonna work as is, and looks like attempt to construct the whole command in variable.
Anyway command has been divided on two, where comments are above of respective parts - see updated commit. Seems quite acceptable to me given that actually all this should be gone eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Didn't we talk about removing at at all? Otherwise we should document the required JVM options in URLStreamHandlerRuntime
JavaDoc.
@marchof Well, there is separate ticket about removal - #471 , which anyway requires change in documentation - see page about implementation details, and IMO it will be nice to preserve information about it on this page. Also for instrumentation with companion classes reflection seems mandatory, so maybe we'll update code of agent to allow reflection. And since there is WDYT? |
@@ -86,6 +86,8 @@ case "$JDK" in | |||
;; | |||
9-ea | 9-ea-stable) | |||
# see https://bugs.openjdk.java.net/browse/JDK-8131041 about "java.locale.providers" | |||
# Groovy version should be updated to get rid of "--add-opens" options (see https://twitter.com/CedricChampeau/status/807285853580103684) | |||
MAVEN_OPTS="--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED" \ | |||
mvn -V -B -e verify -Dbytecode.version=1.9 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, but perhaps indent the mvn
line? The trailing \
on the MAVEN_OPTS
line can be difficult to spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjkail indeed, done
@Godin I'm ok with this fix to make the latest JDK 9 EA work again. |
According to https://twitter.com/CedricChampeau/status/807285853580103684 Groovy should be updated to 2.4.8