scylladb / scylla-jmx

Scylla JMX proxy
GNU Affero General Public License v3.0
28 stars 51 forks source link

treewide: build jar which targets jdk-11 #207

Open tchaikov opened 1 year ago

tchaikov commented 1 year ago

since we've moved to OpenJDK-11, let's build the jar targeting JDK-11.

the --add-exports change was created after referencing https://docs.oracle.com/en/java/javase/16/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-2F61F3A9-0979-46A4-8B49-325BA0EE8B66 without this change, we'd have build failures like

[ERROR] /home/kefu/dev/scylladb/tools/jmx/src/main/java/com/scylladb/jmx/utils/APIBuilder.java:[48,19] package com.sun.jmx.interceptor is not visible
[ERROR]   (package com.sun.jmx.interceptor is declared in module java.management, which does not export it)

so that the created jar file won't be compatible with JDK-8, this helps us to identify the places where the JDK-8 is still used.

by passing -source to the javac compiler allows us to use the Java-11 features. despite that we don't have plan to use any of them, it is still a nice to have.

Refs #206 Signed-off-by: Kefu Chai kefu.chai@scylladb.com

tchaikov commented 1 year ago

as i am not an expert of Java, i am not quite sure about this change . will test it early tomorrow.

tchaikov commented 1 year ago

tested by running following commands

$ cd tools/jmx
$ ./reloc/build_reloc.sh --version $(<../../build/SCYLLA-PRODUCT-FILE)-$(sed 's/-/~/' <../../build/SCYLLA-VERSION-FILE)-$(<../../build/SCYLLA-RELEASE-FILE) --nodeps
$ ./scripts/scylla-jmx

these steps practically mirror how configure.py builds the jmx submodule.

tchaikov commented 8 months ago

@avelanarius hi Piotr, could you please help review this change?

mykaul commented 8 months ago

If we build with Java 11, can we run with Java 8? I'm not sure. If not, need to fix https://github.com/scylladb/scylla-jmx/blob/770c1f9fb06475fcad2337283e12188b5008a299/scripts/select-java#L21 and probably others?

tchaikov commented 8 months ago

@mykaul hi Yaniv, thank you for your inputs.

If we build with Java 11, can we run with Java 8?

to my best knowledge, no.

I'm not sure. If not, need to fix

https://github.com/scylladb/scylla-jmx/blob/770c1f9fb06475fcad2337283e12188b5008a299/scripts/select-java#L21

and probably others?

and the places of package dependencies. but before searching and replacing. i'd like have some reviews from our Java experts.

mykaul commented 8 months ago

I wonder if we can do all those changes without doing them in parallel on https://github.com/scylladb/scylla-tools-java ? (of course, if each were to run in its own container, we wouldn't have had this issue)

avikivity commented 8 months ago

I believe you can tell Java to build for an older version. But if we change our requires= to use java 11 then we can run on 11 too.

tchaikov commented 8 months ago

I believe you can tell Java to build for an older version. But if we change our requires= to use java 11 then we can run on 11 too.

if we build for an older version, in this question, it's JRE-8. then, that's practically partially reverts this very patch, which

  1. targets JRE-11: -target 11
  2. allow using newer language features introduced by JDK-11: -source 11

after a second thought, probably we will not benefit from either of these changes though, if

but i think the first item is still a nice-to-have. as we were suffering from the JRE-8 for a while, would be nice to just to be explicitly stop supporting it. and if we are still using jdk-8 somewhere, presumably, we will be aware of this after this change, as the JRE-8 should fail like:

Exception in thread "main" java.lang.UnsupportedClassVersionError: com/foo/Bar 
  has been compiled by a more recent version of the Java Runtime (class file version 55.0), 
  this version of the Java Runtime only recognizes class file versions up to 52.0
tchaikov commented 8 months ago

I wonder if we can do all those changes without doing them in parallel on https://github.com/scylladb/scylla-tools-java ? (of course, if each were to run in its own container, we wouldn't have had this issue)

the PR only changes the generated scylla-jmx-1.0.jar. and since we should always use jdk-11 to execute it before and after this change. so it should has nothing to do with the java tools hosted in scylla-tools-java .

mykaul commented 8 months ago

I wonder if we can do all those changes without doing them in parallel on https://github.com/scylladb/scylla-tools-java ? (of course, if each were to run in its own container, we wouldn't have had this issue)

the PR only changes the generated scylla-jmx-1.0.jar. and since we should always use jdk-11 to execute it before and after this change. so it should has nothing to do with the java tools hosted in scylla-tools-java .

In practice, it does - if we still support and deploy using Java 8, because this is what we have in the platform and the tools weren't changed to use 11 only. It seems to be that it needs to go hand in hand.