Update client build script to be source of truth for java related installations #395
Conversation
jbkyang-nvi
force-pushed
the
kyang-fix-java-build
branch
from
016ecc2
to
26eb66b
Compare
|
|
||
| # Copy over the jar to a specific location | ||
| mkdir -p ${JAR_INSTALL_PATH} | ||
| cp ${BUILD_HOME}/javacpp-presets/tritonserver/platform/target/tritonserver-platform-*shaded.jar ${JAR_INSTALL_PATH}/tritonserver-java-bindings.jar | ||
|
|
||
| rm -r ${BUILD_HOME}/javacpp-presets/ | ||
| rm -r /root/.m2/repository | ||
| if [ ${KEEP_BUILD_DEPENDENCIES} -eq 1 ]; then |
There was a problem hiding this comment.
This looks like the reverse. Shouldn't this be if it's not equal to 1, then delete?
There was a problem hiding this comment.
We have been pretty bad at following the guidelines for bash scripting. I would advocate for True == 0 instead of True == 1
There was a problem hiding this comment.
Great find! Let's start a conversation offline about this. This is news to me. I don't want to block this PR, but I think we'd want to decide this on a team level and have a PR to standardize all of our tests. Otherwise, it would be inconsistent and could lead to confusion/problems.
There was a problem hiding this comment.
Since this change has been propagated to the javacpp side, can we do this in a separate PR?
There was a problem hiding this comment.
Okay, but please let's do it ASAP. We do not want this getting forgotten and us being inconsistent.
| @@ -98,6 +102,10 @@ for OPTS; do | |||
| export INCLUDE_DEVELOPER_TOOLS_SERVER=0 | |||
| echo "Including developer tools server C++ bindings" | |||
| ;; | |||
| --keep-build-dependencies) | |||
| KEEP_BUILD_DEPENDENCIES=0 | |||
| echo "Including developer tools server C++ bindings" | |||
There was a problem hiding this comment.
If the below comment gets addressed, we may want to review the other code for things like this which will be inconsistent (e.g. if KEEP=0, then the statement should say we are excluding developer tools bindings).
Closes: #395 To be merged after bytedeco/javacpp-presets#1420
Closes: #395 To be merged after bytedeco/javacpp-presets#1420
Closes: #395 To be merged after bytedeco/javacpp-presets#1420
Related to: triton-inference-server/server#6296