Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-38043: [R] Enable all features by default on macOS#38195
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
b55fcc67d0091f9804981107fe7e845e60708b463f5c30461115968f10ea3a30b5bd3e9d401844ddbeaff878645647f84cab8c7f836537e2d8aff448ba38a166401fcdb2ea4ce2c05f89d209a616e72d4File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -393,36 +393,38 @@ 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 | ||
| 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.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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.
Uh oh!There was an error while loading. Please reload this page. | ||
| - 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.
Uh oh!There was an error while loading. Please reload this page. | ||
| - name: Remove arrow/ | ||
| run: | | ||
| rm -rf arrow/ | ||
| - name: Enable parallel build | ||
| run: | | ||
| echo "MAKEFLAGS=-j$(nproc)" >> $GITHUB_ENV | ||
| - name: Install arrow from nightly repo | ||
| cores=`nproc || sysctl -n hw.logicalcpu` | ||
| echo "MAKEFLAGS=-j$cores" >> $GITHUB_ENV | ||
| - name: Install arrow source package | ||
| env: | ||
| # Test source build so be sure not to download a binary | ||
| LIBARROW_BINARY: "FALSE" | ||
| @@ -444,7 +446,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: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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 # | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -57,7 +57,8 @@ try_download <- function(from_url, to_file, hush = quietly) { | ||
| } | ||
| not_cran <- env_is("NOT_CRAN", "true") | ||
| if (not_cran) { | ||
| # enable full featured builds and binaries for macOS (or if the NOT_CRAN variable has been set) | ||
| if (not_cran || on_macos) { | ||
| # Set more eager defaults | ||
| if (env_is("LIBARROW_BINARY", "")) { | ||
| Sys.setenv(LIBARROW_BINARY = "true") | ||
| @@ -759,6 +760,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 | ||
| @@ -773,11 +775,6 @@ with_cloud_support <- function(env_var_list) { | ||
| cat("**** ", start_msg, " support ", msg, "; building with ", off_flags, "\n") | ||
| } | ||
| # Check the features | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why delete this comment? Is it no longer accurate or useful? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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? | ||
Uh oh!
There was an error while loading. Please reload this page.