Skip to content

Force Java version on Travis CI#6512

Closed
adamretter wants to merge 3 commits intofacebook:masterfrom
adamretter:travis/jdk-8
Closed

Force Java version on Travis CI#6512
adamretter wants to merge 3 commits intofacebook:masterfrom
adamretter:travis/jdk-8

Conversation

@adamretter
Copy link
Copy Markdown
Collaborator

In the .travis.yml file the jdk: openjdk7 element is ignored when language: cpp. So whatever version of the JDK that was installed in the Travis container was used - typically JDK 11.

To ensure our RocksJava builds are working, we now instead install and use OpenJDK 8. Ideally we would use OpenJDK 7, as RocksJava supports Java 7, but many of the newer Travis containers don't support Java 7, so Java 8 is the next best thing.

@pdillinger
Copy link
Copy Markdown
Contributor

Should we be using -source to javac to enforce a source code compatibility level?

@adamretter
Copy link
Copy Markdown
Collaborator Author

@pdillinger Hmm... good idea! Normally I am using a Java build tool (instead of Make) and so that stuff is taken care of for me. I will add it in now...

@pdillinger
Copy link
Copy Markdown
Contributor

I guess we could run into newer versions of javac not supporting such an old source level, but at least in the javac version I have handy, that appears to be a warning and not an error.

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@adamretter
Copy link
Copy Markdown
Collaborator Author

@pdillinger Hmm the JAVAC_ARGS isn't being picked up... I will take a further look...

@adamretter
Copy link
Copy Markdown
Collaborator Author

@pdillinger Okay I updated this PR, assuming the CI is good. Could you re-import it please?

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@adamretter has updated the pull request. Re-import the pull request

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@adamretter has updated the pull request. Re-import the pull request

@adamretter
Copy link
Copy Markdown
Collaborator Author

@pdillinger can you re-import please? I fixed the merge conflict

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@adamretter has updated the pull request. Re-import the pull request

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@adamretter has updated the pull request. Re-import the pull request

fi
- if [[ "${JOB_NAME}" == java_test || "${JOB_NAME}" == cmake* ]]; then
java -version && echo "JAVA_HOME=${JAVA_HOME}";
- |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

before_install is now redendant, right? Can't we remove it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yup

.travis.yml Outdated
export PATH=/usr/lib/jvm/java-8-openjdk-$(dpkg --print-architecture)/bin:$PATH
export JAVA_HOME=/usr/lib/jvm/java-8-openjdk-$(dpkg --print-architecture)
fi
java -version && echo "JAVA_HOME=${JAVA_HOME}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest

echo "JAVA_HOME=${JAVA_HOME}"
which java && java -version
which javac && javac -version

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Okie

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@adamretter has updated the pull request. Re-import the pull request

@adamretter
Copy link
Copy Markdown
Collaborator Author

@pdillinger Okay I pushed your suggestions.

@pdillinger
Copy link
Copy Markdown
Contributor

@pdillinger Okay I pushed your suggestions.

Let's hope the Travis gods are still happy ;)

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pdillinger added a commit to guyuqi/rocksdb that referenced this pull request Mar 12, 2020
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@pdillinger merged this pull request in 0772768.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants