search

scylladb / scylla-tools-java

Apache Cassandra, supplying tools for Scylla
Apache License 2.0
53 stars 85 forks source link

build.xml: install and use java-11 when building #368

Closed tchaikov closed 2 months ago

tchaikov commented 11 months ago

before this change, we rely on JAVA_HOME to build for java11 target. but user might set JAVA_HOME to a java-8, in this case the javac from openjdk-8 would fail to compile, because it does not understand the --release option:

    [javac] Compiling 1 source file to /home/piotrs/src/scylla2/tools/java/build/classes/main/META-INF/versions/11
    [javac] javac: invalid flag: --release
    [javac] Usage: javac <options> <source files>
    [javac] use -help for a list of possible options

in this change, we use the JAVA11_HOME environmental variable exposed by build_reloc.sh, so that ant is able to build with the proper java even if user sets JAVA_HOME to java-8.

tchaikov commented 11 months ago

since we have FTBFS when developer happens to point JAVA_HOME to a java-1.8 installation. it'd be nice to fix this in the building system. so, this change resurrects #345.

tchaikov commented 11 months ago

once this change is merged, and scylla picks up the submodule change, i will update https://github.com/scylladb/scylladb/blob/master/tools/toolchain/Dockerfile#L16 to remove 

ENV JAVA8_HOME=/usr/lib/jvm/java-1.8.0-openjdk
nyh commented 11 months ago

To be honest, I don't understand why you need to fix anything in this project, which is mostly a fork of Cassandra and could be built like Cassandra.

Personally, to compile scylla-tools-java I use the command:

ant realclean
JAVA_HOME=/usr/lib/jvm/java-11 JAVA8_HOME=/usr/lib/jvm/java-1.8.0 ant jar

I have a README to remind me of that. You don't have to change the build.xml to explicitly override JAVA_HOME and JAVA8_HOME.

To compile more recent Cassandra, I need to use an even more elaborate command to have it avoid Java 17, but it also works:

JAVA_HOME=/usr/lib/jvm/java-11 JRE_HOME=/usr/lib/jvm/java-11/jre PATH=$JAVA_HOME:$JRE_HOME/bin:$PATH CASSANDRA_USE_JDK11=true ant -Duse.jdk11=true

What worries me is that I don't know if your changes, while improving the situation on your build environment, break it for other people with different build environments you didn't consider (I'm not saying it will - I'm just saying it worries me).

tchaikov commented 11 months ago

wanted to address the pain of those who fail to build this project. A README in one's $HOME directory or memory does not address this for developers who happen to run into this problem. I think the code is more reproducible than document. As always, i believe that developers' time is precious.

tchaikov commented 11 months ago

I'm ok with this, but if we finally remove the java 8 compilation, why not just call JAVA11_HOME JAVA_HOME and just verify we have the appropriate level javac in build.xml?

Yeah. The original goal was to setup the right path for JAVA_HOME, but when I was reading your comment, I thought we could go further. IMHO, JAVA11_HOME is more explicit than JAVA_HOME. But I don't feel strong either way. I am not sure how to verify this in build.xml though. Just wanted to address the pain of FTBFS. The cleanup was but a bonus.

nyh commented 11 months ago

wanted to address the pain of those who fail to build this project. A README in one's $HOME directory or memory does not address this for developers who happen to run into this problem. I think the code is more reproducible than document. As always, i believe that developers' time is precious.

I agree with that, just make sure that after your changes it actually works for everyone, not just for you :-) Since I already figured out how to build the existing project, I'm worried I'll need to learn new tricks to build it with your changes, if it doesn't just work out of the box:

Please correct me if I'm wrong, your patch doesn't just make "ant" work automatically - I still need to set JAVA11_HOME instead of the JAVA_HOME I had to set before. Right? If this is the case, I am not sure how it addresses the pain of people who build this project... Or maybe I'm missing something and just plain "ant" should work after your patch?

tchaikov commented 11 months ago

wanted to address the pain of those who fail to build this project. A README in one's $HOME directory or memory does not address this for developers who happen to run into this problem. I think the code is more reproducible than document. As always, i believe that developers' time is precious.

I agree with that, just make sure that after your changes it actually works for everyone, not just for you :-) Since I already figured out how to build the existing project, I'm worried I'll need to learn new tricks to build it with your changes, if it doesn't just work out of the box:

no. it's not a breaking change. at least i think it is not.

Please correct me if I'm wrong, your patch doesn't just make "ant" work automatically - I still need to set JAVA11_HOME instead of the JAVA_HOME I had to set before. Right? If this is the case, I am not sure how it addresses the pain of people who build this project... Or maybe I'm missing something and just plain "ant" should work after your patch?

the goal is to make the following workflow more smooth:

  1. git clone scylladb and its submodules
  2. install-dependencies.sh
  3. use ./configure.py to setup the building system
  4. ninja -j14

one does not need to setup JAVA_HOME or JAVA11_HOME. because the build.ninja generated by configure.py calls into tools/java/build_reloc.sh, which sets this up for us, and ant picks the environmental variable up. even his/her JAVA11_HOME or JAVA_HOME points to the shining java-17.0.9, the tree should still build on recent release of ubuntu or fedora.

i guess the developers who are already familiar with ant should be able to find their way out after reading the error message in the cover letter =)

EDIT, corrected the last step of the workflow.

nyh commented 11 months ago

As far as I know (please correct me if I'm wrong), ninja build/release/scylla doesn't call tools/java/build_reloc.sh. We have other scripts that do this, but not ninja build/release/scylla.

Anyway, if you don't care what "cd tools/java; ant" does, why did you want to modify build.xml at all? You could have done anything you wanted in the scripts you mentioned - including "unset JAVA_HOME" if you wanted to.

Anyway, I'm not against this patch - all that I'm saying, I guess, is that I don't understand it :-)

tchaikov commented 11 months ago

As far as I know (please correct me if I'm wrong), ninja build/release/scylla doesn't call tools/java/build_reloc.sh. We have other scripts that do this, but not ninja build/release/scylla.

sorry. i failed to copy the command line and instead typed the one i was using. it should be 

ninja  -j14

Anyway, if you don't care what "cd tools/java; ant" does, why did you want to modify build.xml at all? You could have done anything you wanted in the scripts you mentioned - including "unset JAVA_HOME" if you wanted to.

agreed. a minimal change does not need to touch build.xml. that'd be more or less what #345 does. but the more i read the comments in that PR the more i was tempted to go further: if we don't need this and that, why would we need to install and build foo and bar, and how about ... hence these changes.

Anyway, I'm not against this patch - all that I'm saying, I guess, is that I don't understand it :-)

tchaikov commented 11 months ago

v2:

elcallio commented 11 months ago

LGTM

tchaikov commented 11 months ago

v3:

tchaikov commented 11 months ago

this fails the build. see https://jenkins.scylladb.com/job/scylla-master/job/byo/job/byo_build_tests_dtest/1961/consoleText

[2023-12-22T06:59:52.909Z] [151/1070] cd tools/java && ./reloc/build_reloc.sh --version $(<../../build/SCYLLA-PRODUCT-FILE)-$(sed 's/-/~/' <../../build/SCYLLA-VERSION-FILE)-$(<../../build/SCYLLA-RELEASE-FILE) --nodeps 
[2023-12-22T06:59:52.909Z] FAILED: tools/java/build/scylla-tools-5.5.0~dev-0.20231222.7b1a0af13993.noarch.tar.gz 
[2023-12-22T06:59:52.909Z] cd tools/java && ./reloc/build_reloc.sh --version $(<../../build/SCYLLA-PRODUCT-FILE)-$(sed 's/-/~/' <../../build/SCYLLA-VERSION-FILE)-$(<../../build/SCYLLA-RELEASE-FILE) --nodeps 
[2023-12-22T06:59:52.909Z] unable to find jdk-11

we need to install openjdk-11-jdk-headless or up to fix it. i think, currently, the dbuild image only contains java-11-openjdk-headless at this moment, but openjdk-11-jdk-headless is missing in it.

tchaikov commented 3 months ago 
[2024-08-20T07:13:09.727Z] [99/631] cd tools/java && ./reloc/build_reloc.sh --version $(<../../build/SCYLLA-PRODUCT-FILE)-$(sed 's/-/~/' <../../build/SCYLLA-VERSION-FILE)-$(<../../build/SCYLLA-RELEASE-FILE) --nodeps 
[2024-08-20T07:13:09.727Z] FAILED: tools/java/build/scylla-tools-6.2.0~dev-0.20240820.8bdeaf72664c.noarch.tar.gz 
[2024-08-20T07:13:09.727Z] cd tools/java && ./reloc/build_reloc.sh --version $(<../../build/SCYLLA-PRODUCT-FILE)-$(sed 's/-/~/' <../../build/SCYLLA-VERSION-FILE)-$(<../../build/SCYLLA-RELEASE-FILE) --nodeps 
[2024-08-20T07:13:09.727Z] unable to find jdk-11
[2024-08-20T07:13:10.334Z]

https://jenkins.scylladb.com/job/scylla-master/job/byo/job/byo_build_tests_dtest/2451/consoleText . still the same.

tchaikov commented 3 months ago

anyway, i think there is no need to get this in, as

in short, for the same reason put at #207. i am not going to continue pushing forwards this PR anymore.

EDIT, because of https://jenkins.scylladb.com/job/scylla-master/job/next/8228/ , i think this change is still relevant.

tchaikov commented 2 months ago

@scylladb/scylla-maint hello maintainers, could you help install openjdk-11-jdk-headless to the dbuild image? we are seeing build failures in our CI due to the --release problem:

would be nice if we can fix it once and for all.

avikivity commented 2 months ago

I agree with the change, but I don't want to bring it in for a toolchain update.

Also, do we need -devel, or is the jdk enough?

avikivity commented 2 months ago

I see -devel and openjdk are interdependent.

avikivity commented 2 months ago

Turns out adding java-11-openjdk wasn't enough

tchaikov commented 2 months ago

Turns out adding java-11-openjdk wasn't enough

@avikivity adding java-11-openjdk is not enough, but openjdk-11-jdk-headless should suffice. the former runs a java application, the later builds it.

avikivity commented 2 months ago

openjdk-headless is a subset of openjdk-devel, no?

avikivity commented 2 months ago

It doesn't contain javac for instance. We need -devel.

avikivity commented 2 months ago

It doesn't work installing random stuff, and does work with this series. My conclusion is that something changed in how Fedora selects Java, and this series fixes it (although: it works on my workstation without the series).