jonkeane commented on code in PR #37684:
URL: https://github.com/apache/arrow/pull/37684#discussion_r1339995461
##########
dev/tasks/macros.jinja:
##########
@@ -307,7 +307,8 @@ on:
stopifnot(packageVersion("arrow") == {{
'"${{needs.source.outputs.pkg_version}}"' }})
{% endmacro %}
-{%- macro github_setup_local_r_repo(get_nix, get_win) -%}
+{%- macro github_setup_local_r_repo(get_nix, get_win, get_mac=False) -%}
+# TODOf improve arg handling
Review Comment:
```suggestion
# TODO: improve arg handling
```
Typo?
##########
dev/tasks/r/github.packages.yml:
##########
@@ -56,6 +56,57 @@ 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" }
+ - { runs_on: ["self-hosted", "macOS", "arm64", "devops-managed"],
arch: "arm64" }
+ openssl: ['3.0', '1.1']
+
+ steps:
+ {{ macros.github_checkout_arrow(action_v="3")|indent }}
+ {{ macros.github_change_r_pkg_version(is_fork, '${{
needs.source.outputs.pkg_version }}')|indent }}
+ - name: Install Deps
+ if: {{ "${{ !contains(matrix.platform.runs_on, 'macos-10.13') }}" }}
+ run: |
+ brew install sccache ninja
+ brew install openssl@{{ '${{ matrix.openssl }}' }}
+ - name: Build libarrow
+ shell: bash
+ env:
+ {{ macros.github_set_sccache_envvars()|indent(8) }}
+ MACOSX_DEPLOYMENT_TARGET: "10.13"
+ ARROW_S3: ON
+ ARROW_GCS: ON
+ ARROW_DEPENDENCY_SOURCE: BUNDLED
+ CMAKE_GENERATOR: Ninja
+ LIBARROW_MINIMAL: false
+ run: |
+ sccache --start-server
+ export EXTRA_CMAKE_FLAGS="-DOPENSSL_ROOT_DIR=$(brew --prefix
openssl@{{ '${{ matrix.openssl }}' }})"
+ cd arrow
+ r/inst/build_arrow_static.sh
+ - name: Bundle libarrow
+ shell: bash
+ env:
+ PKG_FILE: arrow-{{ '${{ needs.source.outputs.pkg_version }}' }}.zip
+ VERSION: {{ '${{ needs.source.outputs.pkg_version }}' }}
+ run: |
+ cd arrow/r/libarrow/dist
+ # These files were created by the docker user so we have to sudo to
get them
Review Comment:
Am I missing something? This comment doesn't seem connected to the code
around it?
##########
dev/tasks/r/github.packages.yml:
##########
@@ -56,6 +56,57 @@ 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:
Does this 10.13 build for x86 cover all x86 needs (e.g. for macOS 12, 13,
14)?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1308,13 +1308,25 @@ macro(build_snappy)
set(SNAPPY_CMAKE_ARGS
${EP_COMMON_CMAKE_ARGS} -DSNAPPY_BUILD_TESTS=OFF
-DSNAPPY_BUILD_BENCHMARKS=OFF
"-DCMAKE_INSTALL_PREFIX=${SNAPPY_PREFIX}")
+ 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()
Review Comment:
Could you (add a) comment about why this is need?
##########
dev/tasks/r/github.packages.yml:
##########
@@ -178,18 +231,13 @@ jobs:
with:
working-directory: 'arrow'
extra-packages: cpp11
- - name: Install sccache
- if: startsWith(matrix.platform, 'macos')
- run: brew install sccache
- name: Build Binary
id: build
shell: Rscript {0}
env:
- NOT_CRAN: "true" # actions/setup-r sets this implicitly
+ NOT_CRAN: "false" # actions/setup-r sets this implicitly
Review Comment:
Is it possible (and if so, is it better to) unset this instead of marking it
as false? IIUC CRAN has this unset
##########
cpp/thirdparty/versions.txt:
##########
@@ -101,9 +101,8 @@
ARROW_RAPIDJSON_BUILD_VERSION=232389d4f1012dddec4ef84861face2d2ba85709
ARROW_RAPIDJSON_BUILD_SHA256_CHECKSUM=b9290a9a6d444c8e049bd589ab804e0ccf2b05dc5984a19ed5ae75d090064806
ARROW_RE2_BUILD_VERSION=2022-06-01
ARROW_RE2_BUILD_SHA256_CHECKSUM=f89c61410a072e5cbcf8c27e3a778da7d6fd2f2b5b1445cd4f4508bee946ab0f
-# 1.1.9 is patched to implement https://github.com/google/snappy/pull/148 if
this is bumped, remove the patch
Review Comment:
Is this comment still true? The patch above doesn't look like it's related,
but I might be missing something!
##########
dev/tasks/r/github.packages.yml:
##########
@@ -84,6 +135,8 @@ jobs:
env:
UBUNTU: {{ '"${{ matrix.ubuntu }}"' }}
{{ macros.github_set_sccache_envvars()|indent(8) }}
+ MACOSX_DEPLOYMENT_TARGET: "10.13"
Review Comment:
Is this needed here under `linux-cpp` / `C++ Binary Linux OpenSSL {{ '${{
matrix.openssl }}' }}`?
##########
r/tools/nixlibs-allowlist.txt:
##########
@@ -2,3 +2,4 @@ ubuntu
centos
redhat
rhel
+darwin
Review Comment:
Do we know if CRAN checks for downloads on macos? I know they do for some
system types (hence the existence of this allowlist). But so far this allowlist
was only platforms we knew weren't used by CRAN
##########
dev/tasks/r/github.packages.yml:
##########
@@ -291,7 +341,10 @@ jobs:
with:
name: r-pkg_centos7
path: arrow_*
+
test-centos-binary:
+ #arrow binary package not on ppm currently
Review Comment:
Is there a ticket or something we could link to for when we would remove
this?
--
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]
