Conversation
…error we treat warnings as errors).
server/pxf-service/src/scripts/pxf
Outdated
|
|
||
| # Detect Java major version and add --add-opens flags for Java 11+ | ||
| local java_major_version | ||
| java_major_version=$("$javaexe" -version 2>&1 | head -1 | sed -E 's/.*version "([0-9]+).*/\1/') |
There was a problem hiding this comment.
Examples:
1.8.0_472 -> 1
11.0.30 -> 11
17.0.14 -> 17
There was a problem hiding this comment.
if java9 java10 ？
There was a problem hiding this comment.
- I have found out that java can print some garbage to stdout... should filter it before
head -1. - Java 9 and 10:
openjdk version "9.0.4"-> 9
openjdk version "10.0.2-adoptopenjdk" 2018-07-17-> 10
And more interesting java 9+ supports--add-opens(JEP 261). However it seems to be optional until Java 16/17 (not sure)(JEP 396/403)
PS: java8 - doesn't support --add-opens that why I have added this IF statement
java --add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED
Unrecognized option: --add-opens=java.security.jgss/sun.security.krb5=ALL-UNNAMED
|
The code is ready for review (any comments are welcomed). However I will keep it in draft for manual testing with java 8 / non-openJDK (testing doesn't covers UPD: tested bash with |
server/pxf-service/src/scripts/pxf
Outdated
|
|
||
| # Detect Java major version and add --add-opens flags for Java 11+ | ||
| local java_major_version | ||
| java_major_version=$("$javaexe" -version 2>&1 | head -1 | sed -E 's/.*version "([0-9]+).*/\1/') |
There was a problem hiding this comment.
if java9 java10 ？
server/build.gradle
Outdated
| "-Xlint:empty", "-Xlint:finally", "-Xlint:overrides", "-Xlint:path", "-Xlint:-processing", "-Xlint:static", | ||
| "-Xlint:try", "-Xlint:fallthrough", "-Xlint:unchecked", "-Xlint:-options", "-Werror" | ||
| ] | ||
| options.compilerArgs += JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_17) ? ["-Xlint:-removal"] : [] |
There was a problem hiding this comment.
-Xlint:-removal Should we document which APIs will be replaced in the future?
There was a problem hiding this comment.
Currently it complains about
/Users/ostinru/development/cloudberry-pxf/server/pxf-api/src/main/java/org/apache/cloudberry/pxf/api/security/GSSCredentialProvider.java:22: warning: [removal] AccessController in java.security has been deprecated and marked for removal
import java.security.AccessController;
^
It is from JEP 411: Deprecate the Security Manager for Removal (Java 17).
And it will work up until JEP 486: Permanently Disable the Security Manager (Java 24). Probably, it will continue working.
It was added in 79be3d5 "Enabled Kerberos Constrained Delegation impersonation for secure clusters (#707)". And... it seems that we don't have these tests running.
Summary: I will add a comments about which warnings we should fix.
server/pxf-api/build.gradle
Outdated
|
|
||
| test { | ||
| if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_17)) { | ||
| jvmArgs = [ |
There was a problem hiding this comment.
jvmArgs+ is better ？
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| java: [ '8', '11', '17' ] |
There was a problem hiding this comment.
{ matrix.Java } I see the task is already running, which means both uppercase and lowercase are fine. However, it seems like the inconsistency in capitalization might look better if unified. Of course, this is just a suggestion.
There was a problem hiding this comment.
Thank you for thoughtful review!
2 participants
Add java 17 support to cloudberry-pxf:
cd server && ./gradlew wrapper --gradle-version 7.6--add-optionsto jvmArgs to bypass JEP486 limitations-Xlint:-removal- do not treat removal warnings as warnings (it breaks build when run with-Werror)