From 4c426a5382b63d657c90bf968e2ba95ddf1b223b Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Mon, 6 Dec 2021 15:03:32 -0600 Subject: [PATCH 1/8] patch snappy instead? --- cpp/build-support/snappy-UBSAN.patch | 13 +++++++++++++ cpp/cmake_modules/ThirdpartyToolchain.cmake | 4 ++++ 2 files changed, 17 insertions(+) create mode 100644 cpp/build-support/snappy-UBSAN.patch diff --git a/cpp/build-support/snappy-UBSAN.patch b/cpp/build-support/snappy-UBSAN.patch new file mode 100644 index 00000000000..015c8ba8da0 --- /dev/null +++ b/cpp/build-support/snappy-UBSAN.patch @@ -0,0 +1,13 @@ +diff --git a/snappy.cc b/snappy.cc +index 79dc0e8..2b5e662 100644 +--- a/snappy.cc ++++ b/snappy.cc +@@ -348,7 +348,7 @@ static inline bool Copy64BytesWithPatternExtension(char* dst, size_t offset) { + if (SNAPPY_PREDICT_TRUE(offset < 16)) { + if (SNAPPY_PREDICT_FALSE(offset == 0)) return false; + // Extend the pattern to the first 16 bytes. +- for (int i = 0; i < 16; i++) dst[i] = dst[i - offset]; ++ for (int i = 0; i < 16; i++) dst[i] = (dst - offset)[i]; + // Find a multiple of pattern >= 16. + static std::array pattern_sizes = []() { + std::array res; diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 66d04acae06..25452b1d869 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1025,6 +1025,9 @@ macro(build_snappy) "${SNAPPY_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}${SNAPPY_STATIC_LIB_NAME}${CMAKE_STATIC_LIBRARY_SUFFIX}" ) + + set(SNAPPY_PATCH_COMMAND "git" "--git-dir=." "apply" "--verbose" "--whitespace=fix" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") + set(SNAPPY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib @@ -1039,6 +1042,7 @@ macro(build_snappy) URL ${SNAPPY_SOURCE_URL} URL_HASH "SHA256=${ARROW_SNAPPY_BUILD_SHA256_CHECKSUM}" CMAKE_ARGS ${SNAPPY_CMAKE_ARGS} + PATCH_COMMAND ${SNAPPY_PATCH_COMMAND} BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}") file(MAKE_DIRECTORY "${SNAPPY_PREFIX}/include") From e33f7d5c34a67aad95e1d5057e49d0a3384a1d5f Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Mon, 6 Dec 2021 17:22:53 -0600 Subject: [PATCH 2/8] linter fix? --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 25452b1d869..5be34aa843b 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1026,7 +1026,9 @@ macro(build_snappy) ) - set(SNAPPY_PATCH_COMMAND "git" "--git-dir=." "apply" "--verbose" "--whitespace=fix" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") + set(SNAPPY_PATCH_COMMAND + "git" "--git-dir=." "apply" "--verbose" "--whitespace=fix" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch" + ) set(SNAPPY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} From b141103f3b33fff723a50248b803b274ed9c8994 Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 7 Dec 2021 08:20:28 -0600 Subject: [PATCH 3/8] use patch instead --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 5be34aa843b..3152c28b2a0 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1025,9 +1025,9 @@ macro(build_snappy) "${SNAPPY_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}${SNAPPY_STATIC_LIB_NAME}${CMAKE_STATIC_LIBRARY_SUFFIX}" ) - + # This can be removed when https://github.com/google/snappy/pull/148 is released set(SNAPPY_PATCH_COMMAND - "git" "--git-dir=." "apply" "--verbose" "--whitespace=fix" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch" + "patch" "snappy.cc" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch" ) set(SNAPPY_CMAKE_ARGS From e1599d3d6b873abcf5eabc184cd60959438a073a Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 7 Dec 2021 10:52:17 -0600 Subject: [PATCH 4/8] License, conditional patching --- cpp/build-support/snappy-UBSAN.patch | 17 +++++++++++++++++ cpp/cmake_modules/ThirdpartyToolchain.cmake | 8 +++++--- cpp/thirdparty/versions.txt | 1 + r/inst/build_arrow_static.sh | 6 ++++++ 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/cpp/build-support/snappy-UBSAN.patch b/cpp/build-support/snappy-UBSAN.patch index 015c8ba8da0..61856510bad 100644 --- a/cpp/build-support/snappy-UBSAN.patch +++ b/cpp/build-support/snappy-UBSAN.patch @@ -1,3 +1,20 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + diff --git a/snappy.cc b/snappy.cc index 79dc0e8..2b5e662 100644 --- a/snappy.cc diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 3152c28b2a0..32b21b76077 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1026,9 +1026,11 @@ macro(build_snappy) ) # This can be removed when https://github.com/google/snappy/pull/148 is released - set(SNAPPY_PATCH_COMMAND - "patch" "snappy.cc" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch" - ) + # Some platforms don't have patch, but this is probably ok to skip + find_program(patch "patch") + IF(patch) + set(SNAPPY_PATCH_COMMAND "patch" "snappy.cc" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") + ENDIF() set(SNAPPY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} diff --git a/cpp/thirdparty/versions.txt b/cpp/thirdparty/versions.txt index 19c19ced0fb..06fd74bab36 100644 --- a/cpp/thirdparty/versions.txt +++ b/cpp/thirdparty/versions.txt @@ -81,6 +81,7 @@ ARROW_RAPIDJSON_BUILD_VERSION=1a803826f1197b5e30703afe4b9c0e7dd48074f5 ARROW_RAPIDJSON_BUILD_SHA256_CHECKSUM=0b6b780b6c534bfb0b23d29910bfe361e486bcfeaf106db8bc8995792072905a ARROW_RE2_BUILD_VERSION=2021-02-02 ARROW_RE2_BUILD_SHA256_CHECKSUM=1396ab50c06c1a8885fb68bf49a5ecfd989163015fd96699a180d6414937f33f +# 1.1.9 is patched to implement https://github.com/google/snappy/pull/148 if this is bumped, remove the patch ARROW_SNAPPY_BUILD_VERSION=1.1.9 ARROW_SNAPPY_BUILD_SHA256_CHECKSUM=75c1fbb3d618dd3a0483bff0e26d0a92b495bbe5059c8b4f1c962b478b6e06e7 # There is a bug in GCC < 4.9 with Snappy 1.1.9, so revert to 1.1.8 for those (ARROW-14661) diff --git a/r/inst/build_arrow_static.sh b/r/inst/build_arrow_static.sh index 38db2bf3fcf..f3856255ab9 100755 --- a/r/inst/build_arrow_static.sh +++ b/r/inst/build_arrow_static.sh @@ -47,6 +47,12 @@ else ARROW_DEFAULT_PARAM="OFF" fi +# Snappy 1.1.9 is patched to implement https://github.com/google/snappy/pull/148 but some platforms don't have +# patch available, so disable snappy in those cases. If the snappy version is bumped, we should remove this. +if [ ! $(command -v patch) ]; then + ARROW_WITH_SNAPPY=OFF +fi + mkdir -p "${BUILD_DIR}" pushd "${BUILD_DIR}" ${CMAKE} -DARROW_BOOST_USE_SHARED=OFF \ From 33eb954f80ac13d06e0706dc93530810f6c2ec51 Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 7 Dec 2021 11:40:06 -0600 Subject: [PATCH 5/8] linter fix --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 32b21b76077..f7ab95ba76e 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1028,9 +1028,10 @@ macro(build_snappy) # This can be removed when https://github.com/google/snappy/pull/148 is released # Some platforms don't have patch, but this is probably ok to skip find_program(patch "patch") - IF(patch) - set(SNAPPY_PATCH_COMMAND "patch" "snappy.cc" "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") - ENDIF() + if(patch) + set(SNAPPY_PATCH_COMMAND "patch" "snappy.cc" + "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") + endif() set(SNAPPY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} From ce99254ef15bcea644f312a991b77ce2751ad793 Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 7 Dec 2021 12:44:38 -0600 Subject: [PATCH 6/8] add patch to our docker builds if it's not there already --- ci/scripts/r_docker_configure.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ci/scripts/r_docker_configure.sh b/ci/scripts/r_docker_configure.sh index 4d837d9d7f1..4ac693881fe 100755 --- a/ci/scripts/r_docker_configure.sh +++ b/ci/scripts/r_docker_configure.sh @@ -77,5 +77,19 @@ if [ "$ARROW_S3" == "ON" ] || [ "$ARROW_R_DEV" == "TRUE" ]; then fi fi +# Install patch if it doesn't already exist +if [ ! $(command -v patch) ]; then + if [ "`which dnf`" ]; then + dnf install -y patch + elif [ "`which yum`" ]; then + yum install -y patch + elif [ "`which zypper`" ]; then + zypper install -y patch + else + apt-get update + apt-get install -y patch + fi +fi + # Workaround for html help install failure; see https://github.com/r-lib/devtools/issues/2084#issuecomment-530912786 Rscript -e 'x <- file.path(R.home("doc"), "html"); if (!file.exists(x)) {dir.create(x, recursive=TRUE); file.copy(system.file("html/R.css", package="stats"), x)}' From 197eaabbe696790c45b746d65307acbf1a81310b Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 7 Dec 2021 12:55:57 -0600 Subject: [PATCH 7/8] oops, only patch 1.1.9 and not earlier versions --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index f7ab95ba76e..3d05d097a5e 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -620,6 +620,14 @@ else() "https://github.com/google/snappy/archive/${ARROW_SNAPPY_BUILD_VERSION}.tar.gz" "https://github.com/ursa-labs/thirdparty/releases/download/latest/snappy-${ARROW_SNAPPY_BUILD_VERSION}.tar.gz" ) + + # This can be removed when https://github.com/google/snappy/pull/148 is released + # Some platforms don't have patch, but this is probably ok to skip + find_program(patch "patch") + if(patch) + set(SNAPPY_PATCH_COMMAND "patch" "snappy.cc" + "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") + endif() endif() endif() @@ -1025,14 +1033,6 @@ macro(build_snappy) "${SNAPPY_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}${SNAPPY_STATIC_LIB_NAME}${CMAKE_STATIC_LIBRARY_SUFFIX}" ) - # This can be removed when https://github.com/google/snappy/pull/148 is released - # Some platforms don't have patch, but this is probably ok to skip - find_program(patch "patch") - if(patch) - set(SNAPPY_PATCH_COMMAND "patch" "snappy.cc" - "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") - endif() - set(SNAPPY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib From a9e77fde90c7094478760a3446d51636a84f2f52 Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 7 Dec 2021 13:42:06 -0600 Subject: [PATCH 8/8] always linting --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 3d05d097a5e..5ff3478769e 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -626,7 +626,7 @@ else() find_program(patch "patch") if(patch) set(SNAPPY_PATCH_COMMAND "patch" "snappy.cc" - "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") + "${CMAKE_SOURCE_DIR}/build-support/snappy-UBSAN.patch") endif() endif() endif()