Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-38043: [R] Enable all features by default on macOS #38195

Merged
merged 23 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b55fcc6
remove comment - see description for details
assignUser Oct 11, 2023
7d0091f
default to build with cloud support if libs are available
assignUser Oct 11, 2023
9804981
update opennssl search logic to also use pkg-config
assignUser Oct 11, 2023
107fe7e
test macos source build
assignUser Oct 11, 2023
845e607
revert - disable m1 package build
assignUser Oct 11, 2023
08b463f
fix needs
assignUser Oct 11, 2023
5c30461
fix typo
assignUser Oct 11, 2023
115968f
don't install mac deps on linux
assignUser Oct 11, 2023
10ea3a3
force source build in install.packages
assignUser Oct 11, 2023
0b5bd3e
fix configure if
assignUser Oct 11, 2023
9d40184
install crc32c from brew as a workaround for missing symbol when bundled
assignUser Oct 11, 2023
4ddbeaf
add crc32c to static bundled libs
assignUser Oct 11, 2023
f878645
Fix linux deps install
assignUser Oct 11, 2023
647f84c
Revert "revert - disable m1 package build"
assignUser Oct 11, 2023
ab8c7f8
use self-hosted 10.13 run to test macos source build
assignUser Oct 11, 2023
36537e2
don't install R on self-hosted
assignUser Oct 11, 2023
d8aff44
use checkout v3
assignUser Oct 11, 2023
8ba38a1
remove superflous crc
assignUser Oct 12, 2023
66401fc
restore previous behavior of linux min build but full on macos
assignUser Oct 12, 2023
db2ea4c
clarify step name
assignUser Oct 12, 2023
e2c05f8
fix core detection on mac
assignUser Oct 12, 2023
9d209a6
Merge branch 'main' into mac-source-cloud
assignUser Oct 12, 2023
16e72d4
Update r/tools/nixlibs.R
paleolimbot Oct 12, 2023
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
4 changes: 2 additions & 2 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -4086,6 +4086,7 @@ macro(build_crc32c_once)
${_CRC32C_STATIC_LIBRARY})
target_include_directories(Crc32c::crc32c BEFORE INTERFACE "${CRC32C_INCLUDE_DIR}")
add_dependencies(Crc32c::crc32c crc32c_ep)
list(APPEND ARROW_BUNDLED_STATIC_LIBS Crc32c::crc32c)
endif()
endmacro()

Expand Down Expand Up @@ -4316,8 +4317,7 @@ macro(build_google_cloud_cpp_storage)
absl::synchronization
absl::throw_delegate
absl::time
absl::time_zone
Crc32c::crc32c)
absl::time_zone)
endif()
endmacro()

Expand Down
1 change: 1 addition & 0 deletions dev/tasks/macros.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ on:
"arrow",
repos = c(getOption("arrow.dev_repo"), getOption("repos")),
verbose = TRUE,
type = "source",
INSTALL_opts = "--build"
)

Expand Down
31 changes: 16 additions & 15 deletions dev/tasks/r/github.packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -375,29 +375,30 @@ jobs:
read_parquet(system.file("v0.7.1.parquet", package = "arrow"))
print(arrow_info())

#TODO test macos source build?
test-linux-source:
test-source:
needs: source
name: Test linux source build
runs-on: ubuntu-latest
name: Test {{ '${{ matrix.platform.name }}' }} source build
assignUser marked this conversation as resolved.
Show resolved Hide resolved
runs-on: {{ '${{ matrix.platform.runs_on }}' }}
strategy:
fail-fast: false
matrix:
platform:
- {runs_on: "ubuntu-latest", name: "Linux"}
- {runs_on: ["self-hosted", "macos-10.13"] , name: "macOS"}
assignUser marked this conversation as resolved.
Show resolved Hide resolved
steps:
- name: Install R
if: matrix.platform.name == 'Linux'
uses: r-lib/actions/setup-r@v2
with:
install-r: false
{{ macros.github_setup_local_r_repo(false, false)|indent }}
{{ macros.github_checkout_arrow()|indent }}
{{ macros.github_checkout_arrow(action_v="3")|indent }}
assignUser marked this conversation as resolved.
Show resolved Hide resolved
- name: Install sccache
if: matrix.platform.name == 'Linux'
shell: bash
run: |
arrow/ci/scripts/install_sccache.sh unknown-linux-musl /usr/local/bin
- name: Install R package system dependencies
run: |
sudo arrow/ci/scripts/r_install_system_dependencies.sh
env:
ARROW_GCS: "ON"
ARROW_S3: "ON"
ARROW_SOURCE_HOME: arrow
- name: Install R package system dependencies (Linux)
if: matrix.platform.name == 'Linux'
run: sudo apt-get install -y libcurl4-openssl-dev libssl-dev
assignUser marked this conversation as resolved.
Show resolved Hide resolved
- name: Remove arrow/
run: |
rm -rf arrow/
Expand Down Expand Up @@ -426,7 +427,7 @@ jobs:

upload-binaries:
# Only upload binaries if all tests pass.
needs: [r-packages, test-linux-source, test-linux-binary]
needs: [r-packages, test-source, test-linux-binary]
name: Upload artifacts
runs-on: ubuntu-latest
steps:
Expand Down
21 changes: 15 additions & 6 deletions r/configure
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,25 @@ else
ARROW_USE_PKG_CONFIG="false"
fi

# find openssl on macos. macOS ships with libressl. openssl is installable
# with brew, but it is generally not linked. We can over-ride this and find
# openssl but setting OPENSSL_ROOT_DIR (which cmake will pick up later in
# the installation process). FWIW, arrow's cmake process uses this
# same process to find openssl, but doing it now allows us to catch it in
# nixlibs.R and throw a nicer error.
## Find openssl
# Arrow's cmake process uses this same process to find openssl,
# but doing it now allows us to catch it in
# nixlibs.R and activate S3 and GCS support for the source build.

# macOS ships with libressl. openssl is installable with brew, but it is
# generally not linked. We can over-ride this and find
# openssl by setting OPENSSL_ROOT_DIR (which cmake will pick up later in
# the installation process).
if [ "${OPENSSL_ROOT_DIR}" = "" ] && brew --prefix openssl >/dev/null 2>&1; then
export OPENSSL_ROOT_DIR="`brew --prefix openssl`"
export PKG_CONFIG_PATH="${OPENSSL_ROOT_DIR}/lib/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}"
fi
# Look for openssl with pkg-config for non-brew sources(e.g. CRAN) and Linux
assignUser marked this conversation as resolved.
Show resolved Hide resolved
if [ "${OPENSSL_ROOT_DIR}" = "" -a "${PKG_CONFIG_AVAILABLE}" = "true" ]; then
if ${PKG_CONFIG} --exists openssl; then
export OPENSSL_ROOT_DIR="`${PKG_CONFIG} --variable=prefix openssl`"
fi
fi

#############
# Functions #
Expand Down
9 changes: 3 additions & 6 deletions r/tools/nixlibs.R
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ try_download <- function(from_url, to_file, hush = quietly) {
}

not_cran <- env_is("NOT_CRAN", "true")
if (not_cran) {
# enable full featured builds for macOS in case of CRAN source builds.
paleolimbot marked this conversation as resolved.
Show resolved Hide resolved
if (not_cran || on_macos) {
# Set more eager defaults
if (env_is("LIBARROW_BINARY", "")) {
Sys.setenv(LIBARROW_BINARY = "true")
Expand Down Expand Up @@ -722,6 +723,7 @@ is_feature_requested <- function(env_varname, default = env_is("LIBARROW_MINIMAL
with_cloud_support <- function(env_var_list) {
arrow_s3 <- is_feature_requested("ARROW_S3")
arrow_gcs <- is_feature_requested("ARROW_GCS")

if (arrow_s3 || arrow_gcs) {
# User wants S3 or GCS support.
# Make sure that we have curl and openssl system libs
Expand All @@ -736,11 +738,6 @@ with_cloud_support <- function(env_var_list) {
cat("**** ", start_msg, " support ", msg, "; building with ", off_flags, "\n")
}

# Check the features
Copy link
Member

Choose a reason for hiding this comment

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

Why delete this comment? Is it no longer accurate or useful?

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 explained it in the commit message which seems redundant now as it doesn't show up unless you check ^^

The use of cmake here is correct in the fail fast way and matches the actual build, which the test compilation would not be.

On the other hand the test compilation does not require cmake and is used to get the binary so we would need cmake just to check for the deps which is an unnecessary dependency.

So I removed the comment because there is no change necessary!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow what you're saying here as to why it's redundant (or not used)? Could you explain a bit more for me?

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 it's reasonable to have some comment here explaing why this step exists; however, I think a wholistic review of comments on the entire script is better suited to #38236 (after rebase) (and the existing comment doesn't make this clear to me, at least).

# This duplicates what we do with the test program above when we check
# capabilities for using binaries. We could consider consolidating this
# logic, though these use cmake in order to match exactly what we do in the
# libarrow build, and maybe that increases the fidelity.
assignUser marked this conversation as resolved.
Show resolved Hide resolved
if (!cmake_find_package("CURL", NULL, env_var_list)) {
# curl on macos should be installed, so no need to alter this for macos
# TODO: check for apt/yum/etc. and message the right thing?
Expand Down
Loading