Skip to content
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

install.sh: introduce Java existance check #401

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

syuu1228
Copy link
Contributor

@syuu1228 syuu1228 commented Jul 4, 2024

Since we dropped Java check from unified installer and jmx, we need to do it on scylla-tools.

Related scylladb/scylladb#17969

@syuu1228 syuu1228 marked this pull request as ready for review July 4, 2024 23:38
@syuu1228
Copy link
Contributor Author

syuu1228 commented Jul 4, 2024

This is part of scylladb/scylladb#17969, need to merge first.

@denesb denesb requested a review from tchaikov July 8, 2024 06:51
done
}

# So far, scylla-jmx only works with Java 11 and Java 8, we prefer the newer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to s/scylla-jmx/scylla-tools/ . probably we could be more specific though, like "tools like nodetool and cassandra-stress". but "scylla-tools" is the name used by https://github.com/scylladb/scylla-tools-java?tab=readme-ov-file#scylla-tools .

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, in scylla-jmx, select-java actually serves for use cases

in this change, we only address the second use case, where we are installing scylla-tools using install.sh, but what about the first use case? please note, the existing check is performed by

java_ver_output=`"${JAVA:-java}" -version 2>&1`
, which just check if the java fulfills the needs, it does not pick a java runtime. so if user actually has openjdk-11 installed, but he/she doe not point /usr/bin/java to it, or export $JAVA so that it points to one of the supported java runtime, cassandra.in.sh fails.

Since we dropped Java check from unified installer and jmx, we need to
do it on scylla-tools.

Related scylladb/scylladb#17969
@syuu1228
Copy link
Contributor Author

also, in scylla-jmx, select-java actually serves for use cases

in this change, we only address the second use case, where we are installing scylla-tools using install.sh, but what about the first use case? please note, the existing check is performed by

java_ver_output=`"${JAVA:-java}" -version 2>&1`

, which just check if the java fulfills the needs, it does not pick a java runtime. so if user actually has openjdk-11 installed, but he/she doe not point /usr/bin/java to it, or export $JAVA so that it points to one of the supported java runtime, cassandra.in.sh fails.

We need some code to detect Java on scylla-tools's install.sh, since we are dropping it from unified installer.
But, as you descripbed, Java detection method between select-java and cassandra.in.sh are different, I think we should not use select-java here.
So I pushed new code to detect Java just like cassandra.in.sh, and dropped select-java.

fi

if ! builtin command -v $JAVA > /dev/null; then
echo "Please install openjdk-11 before running install.sh."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if user installs, for instance, openjdk-21 and have it in $PATH? and we are not checking for openjdk-11, but a random java executable in the $PATH. so even if this test passes, it does not imply that we have openjdk-11 around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants