kou commented on code in PR #37684:
URL: https://github.com/apache/arrow/pull/37684#discussion_r1343400911
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1308,13 +1308,28 @@ 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 unconditionaly enables Werror when building with clang this can lead
+ # to build failues 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")
+ list(APPEND SNAPPY_CMAKE_ARGS
+ "-DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${EP_CXX_FLAGS_${CONFIG}}
-Wno-error")
Review Comment:
```suggestion
foreach(CONFIG DEBUG MINSIZEREL RELEASE RELWITHDEBINFO)
list(APPEND SNAPPY_CMAKE_ARGS
"-DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${EP_CXX_FLAGS_${CONFIG}}
-Wno-error")
endforeach()
```
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -456,11 +456,18 @@ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR
CMAKE_CXX_COMPILER_ID STRE
# Don't complain about optimization passes that were not possible
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-pass-failed")
- # Avoid clang / libc++ error about C++17 aligned allocation on macOS.
- # See
https://chromium.googlesource.com/chromium/src/+/eee44569858fc650b635779c4e34be5cb0c73186%5E%21/#F0
- # for details.
if(APPLE)
+ # Avoid clang / libc++ error about C++17 aligned allocation on macOS.
+ # See
https://chromium.googlesource.com/chromium/src/+/eee44569858fc650b635779c4e34be5cb0c73186%5E%21/#F0
+ # for details.
set(CXX_ONLY_FLAGS "${CXX_ONLY_FLAGS} -fno-aligned-new")
+
+ if(CMAKE_HOST_SYSTEM_VERSION VERSION_LESS 20)
+ # Avoid C++17 std::get 'not available' issue on macOs 10.13
+ # This will be required until atleast R 4.4 is released and
+ # CRAN (hopefully) stops checking on 10.13
+ set(CXX_ONLY_FLAGS "${CXX_ONLY_FLAGS} -D_LIBCPP_DISABLE_AVAILABILITY")
Review Comment:
```suggestion
string(APPEND CXX_ONLY_FLAGS " -D_LIBCPP_DISABLE_AVAILABILITY")
```
##########
cpp/cmake_modules/snappy.diff:
##########
@@ -0,0 +1,30 @@
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index c3062e2..d946037 100644
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -66,13 +66,6 @@ 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")
+diff --git a/snappy.cc b/snappy.cc
+index d414718..5b0d0d6 100644
+--- a/snappy.cc
++++ b/snappy.cc
+@@ -83,6 +83,7 @@
+ #include <string>
+ #include <utility>
+ #include <vector>
++#include <functional>
Review Comment:
Could you open a PR to https://github.com/google/snappy for this?
This may be acceptable because
https://github.com/google/snappy#contributing-to-the-snappy-project includes
"C++11" and `std::less` is defined in `<functional>`:
https://en.cppreference.com/w/cpp/utility/functional/less
(@emkornfield may help us like https://github.com/google/snappy/pull/148 .)
##########
dev/tasks/r/github.packages.yml:
##########
@@ -56,6 +56,56 @@ jobs:
name: r-pkg__src__contrib
path: arrow/r/arrow_*.tar.gz
+ macos-cpp:
+ name: C++ Binary macOS OpenSSL {{ '${{ matrix.openssl }}' }} {{ '${{
matrix.platform.arch }}'}}
+ runs-on: {{ '${{ matrix.platform.runs_on }}'}}
+ needs: source
+ strategy:
+ fail-fast: false
+ matrix:
+ platform:
+ - { runs_on: ["self-hosted", "macos-10.13"], arch: "x86_64" }
Review Comment:
How about renaming `runs-on` from `runs_on` because `-` is used for word
separator in GitHub Actions?
##########
dev/tasks/tasks.yml:
##########
@@ -994,6 +994,10 @@ tasks:
- r-lib__libarrow__bin__linux-openssl-1.0__arrow-{no_rc_r_version}\.zip
- r-lib__libarrow__bin__linux-openssl-1.1__arrow-{no_rc_r_version}\.zip
- r-lib__libarrow__bin__linux-openssl-3.0__arrow-{no_rc_r_version}\.zip
+ -
r-lib__libarrow__bin__darwin-arm64-openssl-1.1__arrow-{no_rc_r_version}\.zip
+ -
r-lib__libarrow__bin__darwin-arm64-openssl-3.0__arrow-{no_rc_r_version}\.zip
+ -
r-lib__libarrow__bin__darwin-x86_64-openssl-1.1__arrow-{no_rc_r_version}\.zip
+ -
r-lib__libarrow__bin__darwin-x86_64-openssl-3.0__arrow-{no_rc_r_version}\.zip
Review Comment:
They will be uploaded to
`https://apache.jfrog.io/ui/native/arrow/r/14.0.0/libarrow/bin/darwin-{arm64,x86_64}-openssl-{1.1,3.0}/arrow-*.zip`.
Is it expected?
https://github.com/apache/arrow/blob/50de296b0b62e54ee3a67401f277afba9b9d0433/dev/release/binary-task.rb#L1931-L1933
##########
r/configure:
##########
@@ -175,12 +165,6 @@ find_arrow () {
# 2. Use pkg-config to find arrow on the system
_LIBARROW_FOUND="`${PKG_CONFIG} --variable=prefix --silence-errors
${PKG_CONFIG_NAME}`"
echo "*** Trying Arrow C++ found by pkg-config: $_LIBARROW_FOUND"
- elif brew --prefix ${PKG_BREW_NAME} > /dev/null 2>&1; then
- # 3. On macOS, look for Homebrew apache-arrow
- # (note that if you have pkg-config, homebrew arrow may have already
been found)
- _LIBARROW_FOUND=`brew --prefix ${PKG_BREW_NAME}`
- echo "*** Trying Arrow C++ found by Homebrew: ${_LIBARROW_FOUND}"
- export
PKG_CONFIG_PATH="${_LIBARROW_FOUND}/lib/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}"
Review Comment:
Is dropping support for Homebrew's apche-arrow (not autobrew's apache-arrow)
intentional?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1308,13 +1308,28 @@ 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 unconditionaly enables Werror when building with clang this can lead
+ # to build failues 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")
+ list(APPEND SNAPPY_CMAKE_ARGS
+ "-DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${EP_CXX_FLAGS_${CONFIG}}
-Wno-error")
+ 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)
Review Comment:
```suggestion
set(SNAPPY_PATCH_COMMAND ${PATCH} -p1 -i
${CMAKE_CURRENT_LIST_DIR}/snappy.diff)
else()
set(SNAPPY_PATCH_COMMAND)
```
##########
dev/tasks/r/github.packages.yml:
##########
@@ -212,9 +263,10 @@ jobs:
repos = sub("file://", "file:", getOption("arrow.dev_repo")),,
INSTALL_opts = INSTALL_opts
)
-
+
Review Comment:
```suggestion
```
##########
dev/tasks/r/github.packages.yml:
##########
@@ -233,7 +285,8 @@ jobs:
with:
name: r-pkg{{ '${{ steps.build.outputs.path }}' }}
path: arrow_*
-
+
Review Comment:
```suggestion
```
##########
dev/tasks/macros.jinja:
##########
@@ -327,6 +328,17 @@ on:
path: repo/libarrow/bin/linux-openssl-{{ openssl_version }}
{% endfor %}
{% endif %}
+ {% if get_mac %}
+ {% for openssl_version in ["1.1", "3.0"] %}
+ {% for arch in ["x86_64", "arm64"] %}
Review Comment:
```suggestion
{% for arch in ["x86_64", "arm64"] %}
```
##########
dev/tasks/r/github.packages.yml:
##########
@@ -233,7 +285,8 @@ jobs:
with:
name: r-pkg{{ '${{ steps.build.outputs.path }}' }}
path: arrow_*
-
+
+
Review Comment:
```suggestion
```
##########
dev/tasks/macros.jinja:
##########
@@ -327,6 +328,17 @@ on:
path: repo/libarrow/bin/linux-openssl-{{ openssl_version }}
{% endfor %}
{% endif %}
+ {% if get_mac %}
+ {% for openssl_version in ["1.1", "3.0"] %}
+ {% for arch in ["x86_64", "arm64"] %}
+ - name: Get macOS {{ arch }} OpenSSL {{ openssl_version }} binary
+ uses: actions/download-artifact@v3
+ with:
+ name: r-lib__libarrow__bin__darwin-{{arch}}-openssl-{{ openssl_version }}
+ path: repo/libarrow/bin/darwin-{{ arch }}-openssl-{{ openssl_version }}
+ {% endfor %}
Review Comment:
```suggestion
{% endfor %}
```
##########
cpp/cmake_modules/snappy.diff:
##########
@@ -0,0 +1,30 @@
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index c3062e2..d946037 100644
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -66,13 +66,6 @@ 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")
Review Comment:
Can we remove this hunk because we already add `-Wno-error` in
`ThirdpartyToolchain.cmake`?
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -456,11 +456,18 @@ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR
CMAKE_CXX_COMPILER_ID STRE
# Don't complain about optimization passes that were not possible
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-pass-failed")
- # Avoid clang / libc++ error about C++17 aligned allocation on macOS.
- # See
https://chromium.googlesource.com/chromium/src/+/eee44569858fc650b635779c4e34be5cb0c73186%5E%21/#F0
- # for details.
if(APPLE)
+ # Avoid clang / libc++ error about C++17 aligned allocation on macOS.
+ # See
https://chromium.googlesource.com/chromium/src/+/eee44569858fc650b635779c4e34be5cb0c73186%5E%21/#F0
+ # for details.
set(CXX_ONLY_FLAGS "${CXX_ONLY_FLAGS} -fno-aligned-new")
+
+ if(CMAKE_HOST_SYSTEM_VERSION VERSION_LESS 20)
+ # Avoid C++17 std::get 'not available' issue on macOs 10.13
Review Comment:
```suggestion
# Avoid C++17 std::get 'not available' issue on macOS 10.13
```
##########
r/configure:
##########
@@ -298,6 +254,11 @@ set_pkg_vars () {
if [ "$ARROW_R_CXXFLAGS" ]; then
PKG_CFLAGS="$PKG_CFLAGS $ARROW_R_CXXFLAGS"
fi
+
+ if [ "$UNAME" = "Darwin" ] && [ $(expr `sw_vers -productVersion` : '10\.13'
) -gt 3 ]; then
Review Comment:
```suggestion
if [ "$UNAME" = "Darwin" ] && expr $(sw_vers -productVersion) : '10\.13';
then
```
##########
dev/tasks/r/github.packages.yml:
##########
@@ -291,7 +344,10 @@ jobs:
with:
name: r-pkg_centos7
path: arrow_*
+
Review Comment:
Could you remove trailing spaces?
##########
r/tools/nixlibs.R:
##########
@@ -168,17 +178,20 @@ select_binary <- function(os =
tolower(Sys.info()[["sysname"]]),
} else {
# No binary available for arch
cat(sprintf("*** Building on %s %s\n", os, arch))
- NULL
+ binary <- NULL
}
+ return(binary)
}
# This tests that curl and OpenSSL are present (bc we can include their
headers)
# and it checks for other versions/features and raises errors that we grep for
-test_for_curl_and_openssl <- "
+test_for_curl_and_openssl <-
+ "#ifndef __APPLE__
Review Comment:
```suggestion
test_for_curl_and_openssl <- "
#ifndef __APPLE__
```
--
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]
