Copilot commented on code in PR #60730:
URL: https://github.com/apache/doris/pull/60730#discussion_r2801972289
##########
thirdparty/build-thirdparty.sh:
##########
@@ -2028,6 +2028,21 @@ build_paimon_cpp() {
# These libraries are built but not installed by default
echo "Installing paimon-cpp internal dependencies..."
+ # Install paimon-cpp Arrow deps used by paimon parquet static libs.
+ # Keep them in an isolated directory to avoid clashing with Doris Arrow.
+ local paimon_deps_dir="${TP_INSTALL_DIR}/paimon-cpp/lib64/paimon_deps"
+ mkdir -p "${paimon_deps_dir}"
+ for paimon_arrow_dep in \
+ libarrow.a \
+ libarrow_filesystem.a \
+ libarrow_dataset.a \
+ libarrow_acero.a \
+ libparquet.a; do
+ if [ -f "arrow_ep-install/lib/${paimon_arrow_dep}" ]; then
+ cp -v "arrow_ep-install/lib/${paimon_arrow_dep}"
"${paimon_deps_dir}/${paimon_arrow_dep}"
+ fi
+ done
+
# Install roaring_bitmap, renamed to avoid conflict with Doris's
croaringbitmap
if [ -f "release/libroaring_bitmap.a" ]; then
cp -v "release/libroaring_bitmap.a"
"${TP_INSTALL_DIR}/lib64/libroaring_bitmap_paimon.a"
Review Comment:
The paths hardcode both the destination libdir (`lib64`) and the Arrow EP
libdir (`arrow_ep-install/lib`). This makes the logic brittle if the build
outputs to `lib64` (or a different layout) in some environments/configurations.
Consider deriving the lib directory from existing build variables (or probing
`arrow_ep-install/lib64` vs `.../lib`) and using a single computed
`arrow_lib_dir`/`paimon_lib_dir` to avoid repeated assumptions.
```suggestion
# Determine paimon-cpp library directory (lib vs lib64)
local paimon_lib_root="${TP_INSTALL_DIR}/paimon-cpp"
local paimon_lib_dir="${paimon_lib_root}/lib"
if [ ! -d "${paimon_lib_dir}" ]; then
paimon_lib_dir="${paimon_lib_root}/lib64"
fi
local paimon_deps_dir="${paimon_lib_dir}/paimon_deps"
mkdir -p "${paimon_deps_dir}"
# Determine Arrow EP library directory (lib vs lib64)
local arrow_lib_dir="arrow_ep-install/lib"
if [ ! -d "${arrow_lib_dir}" ]; then
arrow_lib_dir="arrow_ep-install/lib64"
fi
for paimon_arrow_dep in \
libarrow.a \
libarrow_filesystem.a \
libarrow_dataset.a \
libarrow_acero.a \
libparquet.a; do
if [ -f "${arrow_lib_dir}/${paimon_arrow_dep}" ]; then
cp -v "${arrow_lib_dir}/${paimon_arrow_dep}"
"${paimon_deps_dir}/${paimon_arrow_dep}"
fi
done
# Install roaring_bitmap, renamed to avoid conflict with Doris's
croaringbitmap
# Determine top-level TP lib directory (lib vs lib64)
local tp_lib_dir="${TP_INSTALL_DIR}/lib"
if [ ! -d "${tp_lib_dir}" ]; then
tp_lib_dir="${TP_INSTALL_DIR}/lib64"
fi
if [ -f "release/libroaring_bitmap.a" ]; then
cp -v "release/libroaring_bitmap.a"
"${tp_lib_dir}/libroaring_bitmap_paimon.a"
```
##########
thirdparty/build-thirdparty.sh:
##########
@@ -2028,6 +2028,21 @@ build_paimon_cpp() {
# These libraries are built but not installed by default
echo "Installing paimon-cpp internal dependencies..."
+ # Install paimon-cpp Arrow deps used by paimon parquet static libs.
+ # Keep them in an isolated directory to avoid clashing with Doris Arrow.
+ local paimon_deps_dir="${TP_INSTALL_DIR}/paimon-cpp/lib64/paimon_deps"
+ mkdir -p "${paimon_deps_dir}"
+ for paimon_arrow_dep in \
+ libarrow.a \
+ libarrow_filesystem.a \
+ libarrow_dataset.a \
+ libarrow_acero.a \
+ libparquet.a; do
+ if [ -f "arrow_ep-install/lib/${paimon_arrow_dep}" ]; then
Review Comment:
The paths hardcode both the destination libdir (`lib64`) and the Arrow EP
libdir (`arrow_ep-install/lib`). This makes the logic brittle if the build
outputs to `lib64` (or a different layout) in some environments/configurations.
Consider deriving the lib directory from existing build variables (or probing
`arrow_ep-install/lib64` vs `.../lib`) and using a single computed
`arrow_lib_dir`/`paimon_lib_dir` to avoid repeated assumptions.
##########
thirdparty/build-thirdparty.sh:
##########
@@ -2028,6 +2028,21 @@ build_paimon_cpp() {
# These libraries are built but not installed by default
echo "Installing paimon-cpp internal dependencies..."
+ # Install paimon-cpp Arrow deps used by paimon parquet static libs.
+ # Keep them in an isolated directory to avoid clashing with Doris Arrow.
+ local paimon_deps_dir="${TP_INSTALL_DIR}/paimon-cpp/lib64/paimon_deps"
+ mkdir -p "${paimon_deps_dir}"
+ for paimon_arrow_dep in \
+ libarrow.a \
+ libarrow_filesystem.a \
+ libarrow_dataset.a \
+ libarrow_acero.a \
+ libparquet.a; do
+ if [ -f "arrow_ep-install/lib/${paimon_arrow_dep}" ]; then
+ cp -v "arrow_ep-install/lib/${paimon_arrow_dep}"
"${paimon_deps_dir}/${paimon_arrow_dep}"
Review Comment:
This loop silently skips missing static libs, which can turn into
harder-to-debug link failures later (especially if Arrow’s built targets differ
by version/options). If these archives are required, fail fast when any
expected file is missing (e.g., emit an error and return non-zero). If they’re
optional, at least log an explicit warning per missing library so the build
output explains what happened.
```suggestion
cp -v "arrow_ep-install/lib/${paimon_arrow_dep}"
"${paimon_deps_dir}/${paimon_arrow_dep}"
else
echo "Warning: expected Arrow static library
'arrow_ep-install/lib/${paimon_arrow_dep}' not found; paimon-cpp static linking
may fail if this archive is required." >&2
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
