Copilot commented on code in PR #60519:
URL: https://github.com/apache/doris/pull/60519#discussion_r2766904264
##########
thirdparty/build-thirdparty.sh:
##########
@@ -551,6 +551,9 @@ build_snappy() {
check_if_source_exist "${SNAPPY_SOURCE}"
cd "${TP_SOURCE_DIR}/${SNAPPY_SOURCE}"
+ # Enable RTTI for snappy (required by Doris BE for SnappySlicesSource
inheritance)
+ sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
Review Comment:
The sed command to enable RTTI is executed every time the build function
runs. While this works because the source is only unpacked once, there's a
potential issue: if someone manually modifies CMakeLists.txt and then reruns
the build, the sed command could fail or have unexpected behavior if the
pattern no longer matches.
Consider making this more robust by either:
1. Checking if the modification has already been applied before running sed
2. Applying this change as a proper patch in download-thirdparty.sh (similar
to how other libraries are patched)
Example for option 1:
```bash
if grep -q '\-fno-rtti' CMakeLists.txt; then
sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
fi
```
Option 2 would be more consistent with how other libraries (like glog,
mysql, etc.) are patched in this codebase.
```suggestion
if grep -q '\-fno-rtti' CMakeLists.txt; then
sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
fi
```
##########
thirdparty/vars.sh:
##########
@@ -512,7 +512,7 @@ JSONCPP_SOURCE="jsoncpp-1.9.5"
JSONCPP_MD5SUM="d6c8c609f2162eff373db62b90a051c7"
# libuuid
-LIBUUID_DOWNLOAD="https://nchc.dl.sourceforge.net/project/libuuid/libuuid-1.0.3.tar.gz";
+LIBUUID_DOWNLOAD="https://downloads.sourceforge.net/project/libuuid/libuuid-1.0.3.tar.gz";
Review Comment:
The libuuid download URL has been changed from a specific mirror
(nchc.dl.sourceforge.net) to the generic SourceForge downloads URL
(downloads.sourceforge.net). While this is a good change for reliability, it is
not mentioned in the PR description or CHANGELOG. Consider either:
1. Adding this to the CHANGELOG as a separate bullet point
2. Mentioning it in the PR description
This helps with transparency and makes it easier to track what changed in
this version.
##########
thirdparty/vars.sh:
##########
@@ -98,10 +98,10 @@ GTEST_SOURCE=googletest-release-1.12.1
GTEST_MD5SUM="e82199374acdfda3f425331028eb4e2a"
# snappy
-SNAPPY_DOWNLOAD="https://github.com/google/snappy/archive/1.1.8.tar.gz";
-SNAPPY_NAME=snappy-1.1.8.tar.gz
-SNAPPY_SOURCE=snappy-1.1.8
-SNAPPY_MD5SUM="70e48cba7fecf289153d009791c9977f"
+SNAPPY_DOWNLOAD="https://github.com/google/snappy/archive/1.1.10.tar.gz";
+SNAPPY_NAME=snappy-1.1.10.tar.gz
+SNAPPY_SOURCE=snappy-1.1.10
+SNAPPY_MD5SUM="70153395ebe6d72febe2cf2e40026a44"
Review Comment:
The LICENSE-dist.txt file still references the old versions of snappy
(1.1.8) and xxhash (0.8.1) on lines 1497 and 1522 respectively. These should be
updated to reflect the new versions (snappy 1.1.10 and xxhash 0.8.3) being
introduced in this PR.
This file documents the versions of third-party dependencies included in the
distribution and should be kept in sync with actual dependency versions.
##########
thirdparty/patches/snappy-1.1.10-sign-compare.patch:
##########
@@ -0,0 +1,11 @@
+--- a/snappy.cc
++++ b/snappy.cc
+@@ -1290,7 +1290,7 @@ inline bool Copy64BytesWithPatternExtension(ptrdiff_t
dst, size_t offset) {
+ DeferMemCopy(&deferred_src, &deferred_length, from, len);
+ }
+ } while (ip < ip_limit_min_slop &&
+- (op + deferred_length) < op_limit_min_slop);
++ static_cast<ptrdiff_t>(op + deferred_length) <
op_limit_min_slop);
Review Comment:
A new patch file `snappy-1.1.10-sign-compare.patch` has been added, but
there is no corresponding patch application logic in
`thirdparty/download-thirdparty.sh`.
Looking at the existing pattern in download-thirdparty.sh (e.g., lines
252-269 for glog, lines 272-280 for mysql), you need to add a snappy patch
section after the unpacking phase. The section should follow this pattern:
```
# snappy patch
if [[ " ${TP_ARCHIVES[*]} " =~ " SNAPPY " ]]; then
if [[ "${SNAPPY_SOURCE}" == "snappy-1.1.10" ]]; then
cd "${TP_SOURCE_DIR}/${SNAPPY_SOURCE}"
if [[ ! -f "${PATCHED_MARK}" ]]; then
patch -p1 <"${TP_PATCH_DIR}/snappy-1.1.10-sign-compare.patch"
touch "${PATCHED_MARK}"
fi
cd -
fi
echo "Finished patching ${SNAPPY_SOURCE}"
fi
```
Without this, the patch file will not be applied during the build process,
and the sign-compare issue it's meant to fix will persist.
```suggestion
+ reinterpret_cast<ptrdiff_t>(op + deferred_length) <
op_limit_min_slop);
```
--
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]
