Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/docker/centos-7-cpp.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ RUN \
make \
openssl-devel \
openssl11-devel \
patch \
wget \
which

Expand Down
1 change: 1 addition & 0 deletions ci/docker/conda-cpp.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ RUN mamba install -q -y \
compilers \
doxygen \
libnuma \
patch \
python=${python} \
ucx \
ucx-proc=*=cpu \
Expand Down
1 change: 1 addition & 0 deletions ci/docker/ubuntu-20.04-cpp-minimal.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ RUN apt-get update -y -q && \
git \
libssl-dev \
libcurl4-openssl-dev \
patch \
python3-pip \
tzdata \
wget && \
Expand Down
1 change: 1 addition & 0 deletions ci/docker/ubuntu-20.04-cpp.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ RUN apt-get update -y -q && \
ninja-build \
nlohmann-json3-dev \
npm \
patch \
pkg-config \
protobuf-compiler \
python3-dev \
Expand Down
1 change: 1 addition & 0 deletions ci/docker/ubuntu-22.04-cpp-minimal.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ RUN apt-get update -y -q && \
git \
libssl-dev \
libcurl4-openssl-dev \
patch \
python3-pip \
tzdata \
wget && \
Expand Down
1 change: 1 addition & 0 deletions ci/docker/ubuntu-22.04-cpp.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ RUN apt-get update -y -q && \
ninja-build \
nlohmann-json3-dev \
npm \
patch \
pkg-config \
protobuf-compiler \
protobuf-compiler-grpc \
Expand Down
1 change: 1 addition & 0 deletions ci/docker/ubuntu-24.04-cpp-minimal.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ RUN apt-get update -y -q && \
git \
libssl-dev \
libcurl4-openssl-dev \
patch \
python3-pip \
tzdata \
tzdata-legacy \
Expand Down
1 change: 1 addition & 0 deletions ci/docker/ubuntu-24.04-cpp.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ RUN apt-get update -y -q && \
ninja-build \
nlohmann-json3-dev \
npm \
patch \
pkg-config \
protobuf-compiler \
protobuf-compiler-grpc \
Expand Down
2 changes: 1 addition & 1 deletion ci/scripts/r_docker_configure.sh
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ fi

# Install rsync for bundling cpp source and curl to make sure it is installed on all images,
# cmake is now a listed sys req.
$PACKAGE_MANAGER install -y rsync cmake curl
$PACKAGE_MANAGER install -y rsync cmake curl patch

# 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)}'
23 changes: 3 additions & 20 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1353,26 +1353,9 @@ macro(build_snappy)
set(SNAPPY_CMAKE_ARGS
${EP_COMMON_CMAKE_ARGS} -DSNAPPY_BUILD_TESTS=OFF -DSNAPPY_BUILD_BENCHMARKS=OFF
"-DCMAKE_INSTALL_PREFIX=${SNAPPY_PREFIX}")
# Snappy unconditionally enables -Werror when building with clang this can lead
# to build failures by way of new compiler warnings. This adds a flag to disable
# Werror to the very end of the invocation to override the snappy internal setting.
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
foreach(CONFIG DEBUG MINSIZEREL RELEASE RELWITHDEBINFO)
list(APPEND
SNAPPY_CMAKE_ARGS
"-DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${EP_CXX_FLAGS_${CONFIG}} -Wno-error"
)
endforeach()
endif()

if(APPLE AND CMAKE_HOST_SYSTEM_VERSION VERSION_LESS 20)
# On macOS 10.13 we need to explicitly add <functional> to avoid a missing include error
# This can be removed once CRAN no longer checks on macOS 10.13
find_program(PATCH patch REQUIRED)
set(SNAPPY_PATCH_COMMAND ${PATCH} -p1 -i ${CMAKE_CURRENT_LIST_DIR}/snappy.diff)
else()
set(SNAPPY_PATCH_COMMAND)
endif()
# See comments in snappy.diff.
find_program(PATCH patch REQUIRED)
set(SNAPPY_PATCH_COMMAND ${PATCH} -p1 -i ${CMAKE_CURRENT_LIST_DIR}/snappy.diff)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kou Do we require patch to be present on all platforms, especially Windows? Or should I make this more specific?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, to answer myself: our emscripten builds don't have access to a patch executable...
https://github.com/ursacomputing/crossbow/actions/runs/10388720694/job/28764995095#step:7:826

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even some Linux builds such as test-r-rocker-r-ver-latest don't seem to have patch either. I'll try to make it optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, unfortunately the patch is required. We either have to make sure that patch is available on all images, or do things differently :-/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. patch is required as you noticed.
I think that another approach is better. See also: #43688 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH, patch is useful and could be needed for other bundled builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should fix in upstream instead of using patch as much as possible :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this, but as you know Snappy rarely takes outside contributions...


if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
# ignore linker flag errors, as Snappy sets
Expand Down
36 changes: 34 additions & 2 deletions cpp/cmake_modules/snappy.diff
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#
# https://github.com/google/snappy/pull/172

# 1. Snappy unconditionally enables -Werror when building with clang this can lead
# to build failures by way of new compiler warnings. This adds a flag to disable
# -Werror to the very end of the invocation to override the snappy internal setting.
# 2. Snappy unconditionally disables RTTI, which is incompatible with some other
# build settings (https://github.com/apache/arrow/issues/43688).
# 3. On macOS 10.13 we need to explicitly add <functional> to avoid a missing
# include error. This can be removed once CRAN no longer checks on macOS 10.13
# (https://github.com/google/snappy/pull/172).

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c3062e2..21b49a7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -66,20 +66,9 @@ else(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wextra")
endif(NOT CMAKE_CXX_FLAGS MATCHES "-Wextra")

- # Use -Werror for clang only.
- if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
- if(NOT CMAKE_CXX_FLAGS MATCHES "-Werror")
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror")
- endif(NOT CMAKE_CXX_FLAGS MATCHES "-Werror")
- endif(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-
# Disable C++ exceptions.
string(REGEX REPLACE "-fexceptions" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions")
-
- # Disable RTTI.
- string(REGEX REPLACE "-frtti" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-rtti")
endif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")

# BUILD_SHARED_LIBS is a standard CMake variable, but we declare it here to make
diff --git a/snappy.cc b/snappy.cc
index d414718..5b0d0d6 100644
--- a/snappy.cc
Expand Down
3 changes: 3 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1889,6 +1889,9 @@ services:
command: >
/bin/bash -c "
git config --global --add safe.directory /arrow &&
python3 -m venv /build/pyvenv &&
source /build/pyvenv/bin/activate &&
pip install -U pip setuptools &&
pip install arrow/dev/archery[lint] &&
archery lint --all --no-clang-tidy --no-iwyu --no-numpydoc --src /arrow"

Expand Down