Copilot commented on code in PR #61369: URL: https://github.com/apache/doris/pull/61369#discussion_r2938286422
########## thirdparty/vars.sh: ########## @@ -552,6 +553,14 @@ PUGIXML_NAME=pugixml-1.15.tar.gz PUGIXML_SOURCE=pugixml-1.15 PUGIXML_MD5SUM="3b894c29455eb33a40b165c6e2de5895" +# paimon-cpp +# Using git clone since there's no official release yet +# We'll use a specific commit or tag for reproducibility +PAIMON_CPP_GIT_URL="https://github.com/alibaba/paimon-cpp.git"; +PAIMON_CPP_GIT_TAG="0a4f4e2e7967fdb0be180711bbe581a18eeeb2dd" +PAIMON_CPP_NAME=paimon-cpp +PAIMON_CPP_SOURCE=paimon-cpp Review Comment: PR metadata (linked #60296 description) says paimon-cpp is pinned to commit `fa80570a...`, but this cherry-pick pins `PAIMON_CPP_GIT_TAG` to `0a4f4e2e...`. Please reconcile the pinned revision (or update the PR description) to avoid building a different paimon-cpp than intended. ########## thirdparty/build-thirdparty.sh: ########## @@ -1971,6 +1971,68 @@ build_pugixml() { cp "${TP_SOURCE_DIR}/${PUGIXML_SOURCE}/src/pugiconfig.hpp" "${TP_INSTALL_DIR}/include/" } +# paimon-cpp +build_paimon_cpp() { + check_if_source_exist "${PAIMON_CPP_SOURCE}" + cd "${TP_SOURCE_DIR}/${PAIMON_CPP_SOURCE}" + + rm -rf "${BUILD_DIR}" + mkdir -p "${BUILD_DIR}" + cd "${BUILD_DIR}" + + # Darwin doesn't build GNU libunwind in this script, so don't force -lunwind there. + local paimon_linker_flags="-L${TP_LIB_DIR} -lbrotlienc -lbrotlidec -lbrotlicommon -llzma" + if [[ "${KERNEL}" != 'Darwin' ]]; then + paimon_linker_flags="${paimon_linker_flags} -lunwind" + fi + + CXXFLAGS="-Wno-nontrivial-memcall" \ + "${CMAKE_CMD}" -C "${TP_DIR}/paimon-cpp-cache.cmake" \ + -G "${GENERATOR}" \ + -DCMAKE_POLICY_VERSION_MINIMUM=3.5 \ + -DCMAKE_INSTALL_PREFIX="${TP_INSTALL_DIR}" \ + -DPAIMON_BUILD_SHARED=OFF \ + -DPAIMON_BUILD_STATIC=ON \ + -DPAIMON_BUILD_TESTS=OFF \ + -DPAIMON_ENABLE_ORC=ON \ + -DPAIMON_ENABLE_AVRO=OFF \ + -DPAIMON_ENABLE_LANCE=OFF \ + -DPAIMON_ENABLE_JINDO=OFF \ + -DPAIMON_ENABLE_LUMINA=OFF \ + -DPAIMON_ENABLE_LUCENE=OFF \ + -DCMAKE_EXE_LINKER_FLAGS="${paimon_linker_flags}" \ + -DCMAKE_SHARED_LINKER_FLAGS="${paimon_linker_flags}" \ + .. + "${BUILD_SYSTEM}" -j "${PARALLEL}" + "${BUILD_SYSTEM}" install + + # Install paimon-cpp internal dependencies with renamed versions + # These libraries are built but not installed by default + echo "Installing paimon-cpp internal dependencies..." + + # 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" + fi + + # Install xxhash, renamed to avoid conflict with Doris's xxhash + if [ -f "release/libxxhash.a" ]; then + cp -v "release/libxxhash.a" "${TP_INSTALL_DIR}/lib64/libxxhash_paimon.a" + fi + + # Install fmt v11 (from fmt_ep-install directory, renamed to avoid conflict with Doris's fmt v7) + if [ -f "fmt_ep-install/lib/libfmt.a" ]; then + cp -v "fmt_ep-install/lib/libfmt.a" "${TP_INSTALL_DIR}/lib64/libfmt_paimon.a" + fi + + # Install tbb (from tbb_ep-install directory, renamed to avoid conflict with Doris's tbb) + if [ -f "tbb_ep-install/lib/libtbb.a" ]; then + cp -v "tbb_ep-install/lib/libtbb.a" "${TP_INSTALL_DIR}/lib64/libtbb_paimon.a" + fi + + echo "Paimon-cpp internal dependencies installed successfully" Review Comment: The internal dependency “install” section silently skips copying if the expected static libs aren’t present, but still prints a success message. This can hide build layout changes and lead to later link failures that are harder to diagnose. Consider copying into `${TP_LIB_DIR}` (instead of hardcoding `lib64`) and failing (or at least warning) if any required internal libs are missing. ########## thirdparty/download-thirdparty.sh: ########## @@ -240,6 +262,57 @@ for TP_ARCH in "${TP_ARCHIVES[@]}"; do done echo "===== Unpacking all thirdparty archives...done" +# Clone and checkout git repositories +echo "===== Cloning git repositories..." +for TP_ARCH in "${TP_ARCHIVES[@]}"; do + if ! is_git_package "${TP_ARCH}"; then + continue + fi + + GIT_URL_VAR="${TP_ARCH}_GIT_URL" + GIT_TAG_VAR="${TP_ARCH}_GIT_TAG" + SOURCE_VAR="${TP_ARCH}_SOURCE" + + GIT_URL="${!GIT_URL_VAR}" + GIT_TAG="${!GIT_TAG_VAR}" + SOURCE_DIR="${TP_SOURCE_DIR}/${!SOURCE_VAR}" + + if [[ -z "${GIT_URL}" ]] || [[ -z "${GIT_TAG}" ]] || [[ -z "${!SOURCE_VAR}" ]]; then + echo "Warning: ${TP_ARCH} git configuration incomplete, skipping" + continue + fi + + if [[ ! -d "${SOURCE_DIR}" ]]; then + echo "Cloning ${TP_ARCH} from ${GIT_URL}..." + cd "${TP_SOURCE_DIR}" + if ! git clone "${GIT_URL}" "${!SOURCE_VAR}"; then + echo "Failed to clone ${TP_ARCH}" + exit 1 + fi + else + echo "${TP_ARCH} repository already exists, updating..." + cd "${SOURCE_DIR}" + git fetch origin || true + fi + + cd "${SOURCE_DIR}" + if ! git checkout "${GIT_TAG}" 2>/dev/null; then + echo "Tag ${GIT_TAG} not found, trying to fetch..." + is_shallow="$(git rev-parse --is-shallow-repository 2>/dev/null || echo false)" + if [[ "${is_shallow}" == "true" ]]; then + git fetch --unshallow origin || git fetch --depth=2147483647 origin + else + git fetch origin + fi + if ! git checkout "${GIT_TAG}"; then Review Comment: For git-based thirdparty sources, the repo can be left with local modifications after patching. Without resetting/cleaning the working tree, future tag/commit checkouts can fail (or silently keep the old tree). Consider running `git reset --hard` + `git clean -fdx` before checkout, and avoid `git fetch origin || true` so transient fetch failures don’t leave a stale revision while the script reports success. ########## thirdparty/download-thirdparty.sh: ########## @@ -601,6 +674,21 @@ if [[ " ${TP_ARCHIVES[*]} " =~ " AZURE " ]]; then echo "Finished patching ${AZURE_SOURCE}" fi +# patch paimon-cpp +if [[ " ${TP_ARCHIVES[*]} " =~ " PAIMON_CPP " ]]; then + cd "${TP_SOURCE_DIR}/${PAIMON_CPP_SOURCE}" + if [[ ! -f "${PATCHED_MARK}" ]]; then + if patch -p1 -N --batch --dry-run <"${TP_PATCH_DIR}/paimon-cpp-buildutils-static-deps.patch" >/dev/null 2>&1; then + patch -p1 -N --batch <"${TP_PATCH_DIR}/paimon-cpp-buildutils-static-deps.patch" + else + echo "Skip paimon-cpp patch: already applied or not applicable for current source" + fi + touch "${PATCHED_MARK}" Review Comment: The patch application for paimon-cpp is guarded by a single `patched_mark` file. Because git packages keep a stable source directory name, changing `PAIMON_CPP_GIT_TAG` later (or recloning into an existing dir) can skip required patches entirely. Also, a previously patched working tree can block `git checkout` to a new tag. Suggestion: either remove/ignore `patched_mark` for git repos and always dry-run/apply the patch after checkout, or make the marker include the checked-out commit hash and reset/clean the tree when the desired tag changes. -- 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]
