-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47632: [CI][C++] Add a CI job for JNI on Linux #47746
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
Conversation
|
|
| @@ -89,7 +89,9 @@ RUN --mount=type=secret,id=github_repository_owner \ | |||
| --x-install-root=${VCPKG_ROOT}/installed \ | |||
| --x-manifest-root=/arrow/ci/vcpkg \ | |||
| --x-feature=azure \ | |||
| --x-feature=dev \ | |||
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 is for ARROW_BUILD_TESTS=ON.
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.
Do we really need to enable this for Python wheels?
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.
Yes, to reuse this image for build configuration for JNI.
I think that reusing this image reduces maintenance cost but you don't want to increase this image size, right?
I'll create a new image for build configuration for JNI.
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.
My main concern is that it makes vcpkg build times even larger when the cache isn't fresh or there is no cache.
| elif [ -n "${CMAKE_PRESET}" ]; then | ||
| cmake \ | ||
| --preset="${CMAKE_PRESET}" \ | ||
| ${ARROW_CMAKE_ARGS} \ | ||
| ${source_dir} |
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.
We want to provide preset for JNI build to share build options in apache/arrow and apache/arrow-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.
Why not pass --preset to the existing cmake call below?
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.
If we use it, all variables by --preset are overridden because it specify most variables explicitly. Explicit variables are used instead of variables set by --preset.
| if [ -x "$(command -v sudo)" ]; then | ||
| SUDO=sudo | ||
| else | ||
| if [ "$(id --user)" -eq 0 ]; then |
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.
We don't need to use sudo with the root user.
manylinux image doesn't provide /usr/bin/sudo but /opt/rh/gcc-toolset-12/root/usr/bin/sudo exists.
| if ! type storage-testbench >/dev/null 2>&1; then | ||
| exclude_tests+=("arrow-gcsfs-test") | ||
| fi | ||
| if ! type minio >/dev/null 2>&1; then | ||
| exclude_tests+=("arrow-s3fs-test") | ||
| fi |
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.
They don't exist in manylinux image.
ci/scripts/cpp_test.sh
Outdated
| if [ "${ARROW_USE_MESON:-OFF}" = "OFF" ] && [ "${ARROW_EMSCRIPTEN:-OFF}" = "OFF" ]; then | ||
| CMAKE_PREFIX_PATH="${CMAKE_INSTALL_PREFIX:-${ARROW_HOME}}" | ||
| if [ -n "${VCPKG_ROOT}" ] && [ -n "${VCPKG_TARGET_TRIPLET}" ]; then | ||
| CMAKE_PREFIX_PATH+=";${VCPKG_ROOT}/installed/${VCPKG_TARGET_TRIPLET}" | ||
| fi | ||
| cmake \ | ||
| -S ${source_dir}/examples/minimal_build \ | ||
| -B ${build_dir}/examples/minimal_build \ | ||
| -DCMAKE_PREFIX_PATH="${CMAKE_PREFIX_PATH}" | ||
| cmake --build ${build_dir}/examples/minimal_build | ||
| pushd ${source_dir}/examples/minimal_build | ||
| ${build_dir}/examples/minimal_build/arrow-example | ||
| popd | ||
| fi |
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 is a test for our CMake packages. If find_package(Arrow) doesn't work, this will detect it.
JNI build uses find_package(Arrow).
| @@ -19,6 +19,7 @@ | |||
| "re2", | |||
| "snappy", | |||
| "utf8proc", | |||
| "xsimd", | |||
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.
| @@ -176,7 +176,7 @@ class Process::Impl { | |||
| for (const auto& kv : process::environment::current()) { | |||
| env[kv.key()] = process::environment::value(kv.value()); | |||
| } | |||
| env["PATH"] = process::environment::value(current_exe.parent_path()); | |||
| env["PATH"] = process::environment::value(current_exe.parent_path().string()); | |||
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.
Recent Boost or old g++ (12.2.1) don't accept std::filesystem::path:
/arrow/cpp/src/arrow/testing/process.cc: In member function ‘arrow::Status arrow::util::Process::Impl::SetExecutable(const std::string&)’:
/arrow/cpp/src/arrow/testing/process.cc:179:74: error: call of overloaded ‘value(std::filesystem::__cxx11::path)’ is ambiguous
179 | env["PATH"] = process::environment::value(current_exe.parent_path());
| ^
In file included from /arrow/cpp/src/arrow/testing/process.cc:44:
/opt/vcpkg/installed/amd64-linux-static-release/include/boost/process/v2/environment.hpp:727:14: note: candidate: ‘boost::process::v2::environment::value::value(boost::process::v2::environment::value_view)’
727 | explicit value(value_view kv) : value_(kv.c_str()) {}
| ^~~~~
/opt/vcpkg/installed/amd64-linux-static-release/include/boost/process/v2/environment.hpp:723:5: note: candidate: ‘boost::process::v2::environment::value::value(string_type&&)’
723 | value( string_type&& source ) : value_(std::move(source)) {}
| ^~~~~
/opt/vcpkg/installed/amd64-linux-static-release/include/boost/process/v2/environment.hpp:722:5: note: candidate: ‘boost::process::v2::environment::value::value(const string_type&)’
722 | value( const string_type& source ) : value_(source) {}
| ^~~~~
We need to convert std::filesystem::path to std::string explicitly.
.github/workflows/cpp_extra.yml
Outdated
|
|
||
| permissions: | ||
| contents: read | ||
| packages: write |
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 is for using GitHub Packages as vcpkg cache.
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.
Can you add a comment about this?
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.
Done and moved to steps.jni-linux.permissions from the top-level permissions to reduce scope.
|
This is ready. This is a blocker of new Apache Arrow Java release. So this is needed for 22.0.0 C++ release. |
|
@raulcd Can we merge this for 22.0.0? |
Yes, I'll cherry-pick it on the maintenance branch once we merge it. |
|
Thanks. Let's merge this. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0c8ce5f. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change This is for preventing to break Apache Arrow Java JNI use case on Linux. ### What changes are included in this PR? * Add a CI job that uses build options for JNI use case * Install more packages in manylinux image that is also used by JNI build ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #47632 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
|
This is also a blocker of 22.0.0. |
Thanks for pointing that out! |
### Rationale for this change This is for preventing to break Apache Arrow Java JNI use case on Linux. ### What changes are included in this PR? * Add a CI job that uses build options for JNI use case * Install more packages in manylinux image that is also used by JNI build ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#47632 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
| # This is for testing find_package(Arrow). | ||
| # | ||
| # Note that this is not a perfect solution. We should improve this | ||
| # later. |
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.
It seems that this new check is breaking some C++ builds (CUDA, Thread Sanitizer). @kou
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.
Oh, sorry. I missed these failures: #47840
Rationale for this change
This is for preventing to break Apache Arrow Java JNI use case on Linux.
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?
No.