raboof commented on code in PR #7930:
URL: https://github.com/apache/geode/pull/7930#discussion_r2371368177
##########
build-tools/scripts/src/main/groovy/geode-java.gradle:
##########
@@ -183,7 +192,8 @@ artifacts {
javadoc {
destinationDir = file("$buildDir/javadoc")
- options.addStringOption('Xwerror', '-quiet')
+ // Disabled strict HTML checking for Java 17 compatibility
+ options.addStringOption('Xdoclint:none', '-quiet')
Review Comment:
would this be possible to fix in the future (not a blocker for this PR) or
is this inevitable on Java 17?
##########
geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/membership/api/CoreOnlyUsesMembershipAPIArchUnitTest.java:
##########
@@ -33,11 +33,25 @@
* geode classes in a single test requires 1.5g of heap.<br>
* This test class can be removed if and when we create an isolated Java
module that does
* not export internal membership classes.
+ *
+ * ARCHITECTURAL CHANGE NOTE: This test was updated to fix the "Layer 'api' is
empty, Layer
+ * 'internal' is empty"
+ * error. The original layered architecture approach failed because membership
classes were moved
+ * from geode-core
+ * to geode-membership module, leaving empty layers. The solution uses direct
dependency rules
+ * instead of layered
+ * architecture to enforce the same constraint: geode-core classes should not
directly access GMS
+ * internals.
Review Comment:
I think comments about 'changes' should go into the Commit, PR message or PR
comment. I don't think it belongs in the code: the code should just document
the 'new' state.
##########
geode-core/build.gradle:
##########
@@ -410,6 +410,11 @@ dependencies {
jmhImplementation('org.jctools:jctools-core')
}
+// Exclude legacy doclet that uses removed com.sun.javadoc API (Java 9+)
+compileTestJava {
+ exclude '**/UnitTestDoclet.java'
+}
+
Review Comment:
Woudn't it be better to remove this file entirely?
##########
build-tools/scripts/src/main/groovy/warnings.gradle:
##########
@@ -16,6 +16,6 @@
*/
tasks.withType(JavaCompile) {
- options.compilerArgs << '-Xlint:unchecked' << "-Werror"
- options.deprecation = true
+ options.compilerArgs << '-Xlint:-unchecked' << "-Werror" <<
'-Xlint:-deprecation' << '-Xlint:-removal'
+ options.deprecation = false
Review Comment:
Same comment here: seems like a reasonable step, but should/could it be a
future issue to fix all use of deprecated functions?
##########
build-tools/scripts/src/main/groovy/spotless.gradle:
##########
@@ -127,6 +127,9 @@ spotless {
include '**/*.gradle'
exclude '**/generated-src/**'
exclude '**/build/**'
+ // Exclude acceptance test gradle projects - these are standalone test
applications
+ // that need hardcoded dependency versions for testing Geode integration
+ exclude
'src/acceptanceTest/resources/gradle-test-projects/**/build.gradle'
Review Comment:
I guess it's not a big deal for now, but would it be possible to make these
pass spotless checks in the future? If so could we create a Jira ticket for
that?
##########
build-tools/scripts/src/main/groovy/geode-java.gradle:
##########
@@ -31,17 +31,26 @@ dependencies {
}
String javaVersion = System.properties['java.version']
-if (javaVersion.startsWith("1.8.0") &&
javaVersion.split("-")[0].split("_")[1].toInteger() < 121) {
- throw new GradleException("Java version 1.8.0_121 or later required, but was
" + javaVersion)
+def versionMajor = JavaVersion.current().majorVersion.toInteger()
+if (versionMajor < 17) {
+ throw new GradleException("Java version 17 or later required, but was " +
javaVersion)
}
// apply compiler options
gradle.taskGraph.whenReady({ graph ->
tasks.withType(JavaCompile).each { javac ->
javac.configure {
- sourceCompatibility '1.8'
- targetCompatibility '1.8'
+ sourceCompatibility '17'
+ targetCompatibility '17'
options.encoding = 'UTF-8'
+ options.compilerArgs.addAll([
+
'--add-exports=java.management/com.sun.jmx.remote.security=ALL-UNNAMED',
+ '--add-exports=java.base/sun.nio.ch=ALL-UNNAMED',
+ '--add-exports=jdk.unsupported/sun.misc=ALL-UNNAMED',
+ '--add-exports=jdk.unsupported/sun.reflect=ALL-UNNAMED',
+ '-Xlint:-removal',
+ '-Xlint:-deprecation'
+ ])
Review Comment:
Longer-term it would be great to be able to migrate to `--release 17`
instead of setting `sourceCompatibility` and `targetCompatibility`, but that
conflicts with `--add-exports`, so that's more of a future goal.
https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#standard-options
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/management/internal/rest/GradleBuildWithGeodeCoreAcceptanceTest.java:
##########
@@ -66,7 +66,7 @@ public void testBasicGradleBuild() {
copyDirectoryResource(projectDir, buildDir);
GradleConnector connector = GradleConnector.newConnector();
- connector.useBuildDistribution();
+ connector.useGradleVersion("7.3.3");
Review Comment:
Does this mean we should update it along with the gradle version for the
rest of the project? should we document this somewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
