enhance: unify C++ build for all language bindings - #469
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tedxu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
feaf2a8 to
3928e02
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #469 +/- ##
==========================================
- Coverage 72.84% 70.32% -2.53%
==========================================
Files 133 133
Lines 13300 13282 -18
Branches 1975 1972 -3
==========================================
- Hits 9689 9341 -348
- Misses 3611 3941 +330
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b711be2 to
92d3825
Compare
1ec1e66 to
c9560a2
Compare
c9560a2 to
ee543d2
Compare
|
why
|
The test |
51668c4 to
a8dfe81
Compare
Consolidate three separate CI workflows (cpp-ci, python-ci, java-ci) into a single ci.yml with a shared build matrix. The C++ library is built once in two variants (test and release) and reused by all downstream jobs. Make all deps shared by default in conanfile.py to prevent JVM TLS exhaustion from static PT_TLS segments when loaded via dlopen, and to avoid duplicate allocator state between Arrow and aws-sdk-cpp. Add FFI_EXPORT_ONLY option to hide C++ symbols via linker version script for Python/Java release builds. Skip loon CLI in this mode since it requires C++ internals. Move ASAN definitions before test subdirectory in CMakeLists.txt so that RUNNING_WITH_ASAN is visible to test targets. Skip TestS3Options and TestClientBuilder's S3Options::Defaults() under ASAN to avoid false-positive bad-free in shared aws-sdk-cpp. Add os._exit atexit handler on Linux to prevent C++ static destructor crashes during Python interpreter shutdown. Other changes: - Remove redundant python-lib / java-lib Makefile targets - Add make dist target for relocatable tarball - Add loon RPATH and install support - Add rust/build.rs search path for shared deps in libs/ - Align WITH_FIU Makefile default to False (matches conanfile) Signed-off-by: Ted Xu <ted.xu@zilliz.com>
a8dfe81 to
0434edc
Compare
|
A few concerns about 🔴 macOS path produces a broken tarballThe RPATH-fixup branch is Linux-only: if [ "${OS}" = "linux" ] && command -v patchelf &>/dev/null; then
patchelf --set-rpath '\$ORIGIN/../lib' "${DIST_DIR}/bin/loon" 2>/dev/null || true
...
fiBut the script also detects 🔴 Silent failures under
|
| if sys.platform == "linux": | ||
| _orig_exit = sys.exit | ||
|
|
||
| def _patched_exit(code=0): |
There was a problem hiding this comment.
The sys.exit / atexit → os._exit hack in _ffi.py concerns me. A few points worth discussing before we merge:
What it actually does. The os._exit in the atexit handler bypasses Py_FinalizeEx and __cxa_finalize, which is what avoids the C++ static-destructor crash. The sys.exit monkey-patch is purely to preserve the exit code through
that path (without it, any sys.exit(N) would get rewritten to exit 0 by the os._exit(0) in the handler). So the "fix" is really two coupled hacks, not one.
Why I think this is the wrong layer to fix it.
- It masks a real bug, not a spurious one. A segfault during C++ static destruction means some global singleton in aws-sdk-cpp / Arrow / folly is touching state that was already torn down. That's a real ordering/lifetime bug
in our shutdown sequence, and it will bite us in any embedding that isn't CPython — most notably the JNI path, which loads the same libmilvus-storage.so and has none of this protection. So we haven't actually fixed the crash,
we've just hidden it from Python CI while leaving Java users exposed. - It mutates global interpreter state on import. Any process that does import milvus_storage now has its sys.exit rebound and an os._exit atexit handler registered, regardless of whether the user wanted that behavior. This is
especially unfriendly in notebooks, long-running services, and test runners where milvus_storage is one of many libraries sharing the interpreter. - It silently swallows normal shutdown side effects. os._exit skips stdio flushing, tempfile cleanup, logging.shutdown, thread joins, and every atexit handler registered after ours. For a library that may be imported inside
someone else's application, this is a pretty invasive default.
There was a problem hiding this comment.
What is the root case here?
There was a problem hiding this comment.
The shared linkage introduced this issue, and we have some known resource allocation left-overs: the entries in loon fs cache, and the clients initialized globally in aws sdk, etc. We need a exit recall anyway. I've tried releasing the mentioned resources but those are not the complete set.
| self.options["zstd"].shared = True | ||
| self.options["glog"].shared = True | ||
| self.options["gflags"].shared = True | ||
| # Ensure aws-c-* are shared to avoid duplicate allocator |
There was a problem hiding this comment.
already defined "aws-c-*/*:shared": True, in default_options. And just define all shared libs into default_options. Do not split the same logical into different function.
| // DefaultAWSCredentialsProviderChain triggers a bad-free inside the | ||
| // AWS CRT TLS init path when aws-sdk-cpp is a shared lib not built | ||
| // with ASAN. Skip the entire test under ASAN. | ||
| GTEST_SKIP() << "Skipped under ASAN (AWS CRT shared lib false positive)"; |
There was a problem hiding this comment.
won‘t get this ASAN in linux? This is just a false alarm on OSX.
There was a problem hiding this comment.
The test breaks only under Linux, not OSX, and the issue is introduced by DefaultAWSCredentialsProviderChain described in the comment.
| @@ -0,0 +1,291 @@ | |||
| name: CI | |||
There was a problem hiding this comment.
can use the orchestrator + reusable workflows to split the different logical into different yaml files?
ex.
.github/workflows/
ci.yml ← orchestration
_build.yml ← workflow_call，build debug and release so
_test-cpp.yml ← workflow_call，do cpp test
_test-python.yml ← workflow_call，dp python test
_test-java.yml ← workflow_call，do java test
_lint-python.yml ← workflow_call, do python lint
ci.yml
name: CI
on:
push:
paths: ['cpp/**', 'python/**', 'java/**', '.github/workflows/**']
pull_request:
paths: ['cpp/**', 'python/**', 'java/**', '.github/workflows/**']
jobs:
build:
uses: ./.github/workflows/_build.yml
test-cpp:
needs: build
uses: ./.github/workflows/_test-cpp.yml
test-python:
needs: build
uses: ./.github/workflows/_test-python.yml
test-java:
needs: build
uses: ./.github/workflows/_test-java.yml
lint-python:
uses: ./.github/workflows/_lint-python.yml
...
|
|
||
| - name: Run tests (azurite) | ||
| working-directory: ./cpp | ||
| run: source ./scripts/azurite_env.sh && ./build/Release/test/milvus_test |
| run: ./build/Release/test/milvus_test && ./build/Release/test/Test_FFI | ||
|
|
||
| - name: Run tests (minio) | ||
| working-directory: ./cpp |
3 participants
Build libmilvus-storage.so once with all deps shared and WITH_JNI
on by default, eliminating separate python-lib and java-lib targets
that each triggered a full C++ rebuild.
Build system changes:
blocks (Python's RTLD_LOCAL provides the same isolation)
TLS exhaustion from static PT_TLS segments
JAVA_HOME
force-load workaround (both unnecessary with shared deps)
CI changes (new unified ci.yml replaces cpp-ci.yml, python-ci.yml,
java-ci.yml):
test-python, and test-java jobs via artifact upload
Release changes (release.yml):
Test plan: