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

Update build files for java8 build #9541

Closed
wants to merge 5 commits into from

Conversation

alanpaxton
Copy link
Contributor

@alanpaxton alanpaxton commented Feb 10, 2022

For RocksJava 7 we will move from requiring Java 7 to Java 8.

  • This simplifies the Makefile as we no longer need to deal with Java 7; so we no longer use javah.

  • Added a java-version target which is invoked by the java target, and which exits if the version of java being used is not 8 or greater.

  • Enforces java 8 as a minimum.

  • Fixed CMake build.

  • Fixed broken java event listener test, as the test was broken and the assertions in the callbacks were not causing assertions in the tests. The callbacks now queue up assertion errors for the main thread of the tests to check.

  • Fixed C++ dangling pointers in the test code.

Simplify the makefile as we no longer need to deal with java 7 (so no  longer use javah). Added a java-version target which is invoked by the java target, and which exits if the version of java being used is not 8 or greater.

This does not change the "-source 7" in make_config.mk, we must next ensure that all source code conforms to java8, and then we will change it to "-source 8".

To summarise, with this change, we are simply enforcing the use of a java 8 compiler to compile java 7 code.
The test was broken and callbacks were asserting.
The assertions in the callbacks were not causing assertions in the tests.
Fix the callbacks so that when they assert, they queue up assertion errors for the main thread of the tests to check.
Fix the test so it works correctly (C++ test code had dangling pointers).
@alanpaxton alanpaxton marked this pull request as draft February 10, 2022 12:36
@alanpaxton alanpaxton changed the title Eb 1045 java8 build Update build files for java8 build Feb 14, 2022
@alanpaxton alanpaxton marked this pull request as ready for review February 14, 2022 11:41
@alanpaxton
Copy link
Contributor Author

CMake build has passed CI, so I believe this is ready for review.

Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM - just one question about version checks when Java 17 is used (as it is semantically similar to Java 1.7)

@@ -309,13 +309,17 @@ set(JAVA_TESTCLASSPATH ${JAVA_JUNIT_JAR} ${JAVA_HAMCR_JAR} ${JAVA_MOCKITO_JAR} $
set(JNI_OUTPUT_DIR ${PROJECT_SOURCE_DIR}/java/include)
file(MAKE_DIRECTORY ${JNI_OUTPUT_DIR})

if(${Java_VERSION_MINOR} VERSION_LESS_EQUAL "7" AND ${Java_VERSION_MAJOR} STREQUAL "1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanpaxton Just wondering, as they have changed the version numbering scheme after Java 1.8, does this still work on say Java 11? How about Java 17 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values come from CMake's FindJava plugin. I just went and played with it. Java 11, 13, 15, 17 all have correct ${Java_VERSION_MAJOR} of 11,. 13, 15, 17 respectively. Java 1.6, 1.7 and 1.8 have ${Java_VERSION_MAJOR} STREQUAL "1" so I claim this test works as intended.

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@adamretter adamretter self-requested a review February 16, 2022 09:08
Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

It's ready to go :-)

@facebook-github-bot
Copy link
Contributor

@alanpaxton has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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.

4 participants