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
CMake: Add support for NLS and CP tools #3675
Conversation
@keithc-ca Can you take a look at this as well? |
b7dc9c9
to
f08cae3
Compare
Updated to address comments, and fixed merge conflicts |
f08cae3
to
64cb8fc
Compare
Everything should be fixed up now |
@DanHeidinga can you take a look at this since @keithc-ca is out of the office |
set(Java_JAVA_EXECUTABLE ${JAVA_HOME}/bin/java) | ||
else() | ||
message(STATUS "BOOT_JDK is not set, default jdk will be used") | ||
include(FindJava) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather this errors out than pick a random java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is downstream from the openjdk autoconf. We should never get here without having set the bootjdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dnakamura If this gets dealt with, I think it's ready to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanHeidinga How would you feel about wrapping this behavior up in a flag? ie J9VM_FORCE_JDK_SEARCH or something. I'm thinking of the case of the jit linter. To run properly it needs to generate all the various header files. The easiest way to do that is to just run cmake outside of the full openjdk build. Its not clear what the proper value of BOOT_JDK should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You raise a good point. I hadn't thought of the jit linter.
I was about to say it's not unreasonable to force it to pick a JDK but it kind of is.... That extra barrier isn't needed.
64cb8fc
to
35d36a2
Compare
I've restarted the travis build - will merge if it passes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@dnakamura The linter job in the travis build is failing with:
Can you take a look? |
Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
Now that proper support for nls/cp tools are in cmake, the code is much simpler Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
35d36a2
to
073ec78
Compare
@keithc-ca Any further comments on this? |
travis build has passed - merging |
Signed-off-by: Devin Nakamura devinn@ca.ibm.com