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

Conversation

assignUser
Copy link
Member

@assignUser assignUser commented Oct 11, 2023

Rationale for this change

Previously GCS/S3 support would need to be explicitly enabled in source builds (when they are build without NOT_CRAN). As we want the macos binaries to be fully featured we should turn the features on when the dependencies exists.

What changes are included in this PR?

This PR enables this behavior for macOS only, on Linux setting NOT_CRAN or LIBARROW_MINIMAL=false is still required.

Are these changes tested?

Crossbow and locally (thanks @paleolimbot )

Using cmake here is good as it mirrors the actual build so we fail fast.
Replacing the test compile with the cmake check would require cmake even
if we use pre-compiled binary which we don't want.
@assignUser
Copy link
Member Author

assignUser commented Oct 11, 2023

@github-actions crossbow submit r-binary-packages

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@assignUser
Copy link
Member Author

assignUser commented Oct 11, 2023

@github-actions crossbow submit r-binary-packages

@github-actions
Copy link

Revision: 9d40184

Submitted crossbow builds: ursacomputing/crossbow @ actions-3b4a613597

Task Status
r-binary-packages Github Actions

@github-actions
Copy link

Revision: 4ddbeaf

Submitted crossbow builds: ursacomputing/crossbow @ actions-28d06e21a7

Task Status
r-binary-packages Github Actions

@paleolimbot
Copy link
Member

(After r-binary-packages passes I'll give a shot locally!)

@assignUser
Copy link
Member Author

Some weirdness going on with the linux deps but mac works!

Arrow package version: 13.0.0.100000311

Capabilities:
               
acero      TRUE
dataset    TRUE
substrait FALSE
parquet    TRUE
json       TRUE
s3         TRUE
gcs        TRUE
utf8proc   TRUE
re2        TRUE
snappy     TRUE
gzip       TRUE
brotli     TRUE
zstd       TRUE
lz4        TRUE
lz4_frame  TRUE
lzo       FALSE
bz2        TRUE
jemalloc   TRUE
mimalloc   TRUE

sudo was run in clean env as `-E` was missing,  so nothing was ever installed.
As we don't need test bench etc we can just call apt directly.
@paleolimbot
Copy link
Member

Ok I'll give it a go!

@assignUser
Copy link
Member Author

assignUser commented Oct 12, 2023

Thanks for testing @jonkeane !

but setting this to use more cores might be nice + possible then.

In the follow up #38236 I automatically set not_cran when we are in a dev build, which should make the dev experience on especially linux but overall much smoother as you also won't have to fiddle with allow lists or such to get the binaries.

I am a bit hesitant to remove the 2 core cap by default for release builds as we could still have to build from source on cran (BDR special checks? Condition for use of binaries ...). Mac users will get the cran binary anyway and through the allow list a majority of the linux users will also get binaries, so I think the reward doesn't justify the risk of a policy violation. (I also think that the 2 core think is a philosophical stance not necessarily limited to builds on cran, at least going by recent r dev ml threads...)

but the linking step seemed to use all of my cores

😱 I didn't change anything there so I am pretty sure that was already happening (on linuc), so it looks like you are right and the short burst is within tolerance?

Thanks @paleolimbot !

I think my local 11.7 build fails because it's mixing homebrew and bundled dependencies

Hm that should not be a problem normally though unless there is some version mismatch? The 10.13 runner also has brew and even has the deps installed but no problem arises... as this is a definitive dev use case I think it's ok to have people manually set the deps source var if the problem happens. Could you try with ARROW_DEPENDENCY_SOURCE= BUNDELD?

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 12, 2023
@assignUser
Copy link
Member Author

@github-actions crossbow submit -g r

@github-actions
Copy link

Revision: 9d209a6

Submitted crossbow builds: ursacomputing/crossbow @ actions-431cdc81fd

Task Status
conda-linux-aarch64-cpu-r42 Azure
conda-linux-aarch64-cpu-r43 Azure
conda-linux-x64-cpu-r42 Azure
conda-linux-x64-cpu-r43 Azure
conda-osx-arm64-cpu-r42 Azure
conda-osx-arm64-cpu-r43 Azure
conda-osx-x64-cpu-r42 Azure
conda-osx-x64-cpu-r43 Azure
conda-win-x64-cpu-r41 Azure
r-binary-packages Github Actions
test-fedora-r-clang-sanitizer Azure
test-r-arrow-backwards-compatibility Github Actions
test-r-depsource-bundled Azure
test-r-depsource-system Github Actions
test-r-dev-duckdb Github Actions
test-r-devdocs Github Actions
test-r-gcc-11 Github Actions
test-r-gcc-12 Github Actions
test-r-install-local Github Actions
test-r-install-local-minsizerel Github Actions
test-r-library-r-base-latest Azure
test-r-linux-as-cran Github Actions
test-r-linux-rchk Github Actions
test-r-linux-valgrind Azure
test-r-minimal-build Azure
test-r-offline-maximal Github Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-debian-gcc-release-custom-ccache Azure
test-r-rhub-ubuntu-gcc-release-latest Azure
test-r-rstudio-r-base-4.1-opensuse153 Azure
test-r-rstudio-r-base-4.2-centos7-devtoolset-8 Azure
test-r-rstudio-r-base-4.2-focal Azure
test-r-ubuntu-22.04 Github Actions
test-r-versions Github Actions
test-ubuntu-r-sanitizer Azure

@paleolimbot
Copy link
Member

Could you try with ARROW_DEPENDENCY_SOURCE= BUNDELD?

I'll debug my source build separately from this PR

I am a bit hesitant to remove the 2 core cap

That primarily affects the development experience, and we're not planning to have that be the primary means by which any MacOS developer gets an Arrow build. If it turns out that it is, we can fix that incrementally after the release.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 12, 2023
Co-authored-by: Jonathan Keane <[email protected]>
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 12, 2023
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

With a green nightlies run (minus any "device out of space" errors), I think this particular chunk of the larger build system refactor is good to go! Thank you!

@@ -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.

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).

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 12, 2023
@assignUser assignUser merged commit 4bf777a into apache:main Oct 13, 2023
32 of 33 checks passed
@assignUser assignUser removed the awaiting merge Awaiting merge label Oct 13, 2023
@assignUser assignUser deleted the mac-source-cloud branch October 13, 2023 04:48
raulcd pushed a commit that referenced this pull request Oct 13, 2023
### Rationale for this change

Previously GCS/S3 support would need to be explicitly enabled in source builds (when they are build without `NOT_CRAN`). As we want the macos binaries to be fully featured we should turn the features on when the dependencies exists. 

### What changes are included in this PR?

This PR enables this behavior for macOS only, on Linux setting `NOT_CRAN` or  `LIBARROW_MINIMAL=false` is still required.

### Are these changes tested?

Crossbow and locally (thanks @ paleolimbot )
* Closes: #38043

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 4bf777a.

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…8195)

### Rationale for this change

Previously GCS/S3 support would need to be explicitly enabled in source builds (when they are build without `NOT_CRAN`). As we want the macos binaries to be fully featured we should turn the features on when the dependencies exists. 

### What changes are included in this PR?

This PR enables this behavior for macOS only, on Linux setting `NOT_CRAN` or  `LIBARROW_MINIMAL=false` is still required.

### Are these changes tested?

Crossbow and locally (thanks @ paleolimbot )
* Closes: apache#38043

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
assignUser added a commit that referenced this pull request Oct 25, 2023
…38236)

### Rationale for this change

We currently need to manually download the latest nightly build as the ".9000' dev version never matches any nightly builds.

### What changes are included in this PR?

Check the nightly  repo html listing for the latest version matching the local major version.
Refactor  nixlibs.R and integrate the functionality of winlibs.R  into it to simplify maintenance and prepare for the future of in-line windows libarrow builds.

These changes are build on #38195 so once that is merged I will rebase this.

### Are these changes tested?

Crossbow
* Closes: #27839

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 25, 2023
…ons. (apache#38236)

### Rationale for this change

We currently need to manually download the latest nightly build as the ".9000' dev version never matches any nightly builds.

### What changes are included in this PR?

Check the nightly  repo html listing for the latest version matching the local major version.
Refactor  nixlibs.R and integrate the functionality of winlibs.R  into it to simplify maintenance and prepare for the future of in-line windows libarrow builds.

These changes are build on apache#38195 so once that is merged I will rebase this.

### Are these changes tested?

Crossbow
* Closes: apache#27839

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Nov 13, 2023
…ons. (apache#38236)

### Rationale for this change

We currently need to manually download the latest nightly build as the ".9000' dev version never matches any nightly builds.

### What changes are included in this PR?

Check the nightly  repo html listing for the latest version matching the local major version.
Refactor  nixlibs.R and integrate the functionality of winlibs.R  into it to simplify maintenance and prepare for the future of in-line windows libarrow builds.

These changes are build on apache#38195 so once that is merged I will rebase this.

### Are these changes tested?

Crossbow
* Closes: apache#27839

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…8195)

### Rationale for this change

Previously GCS/S3 support would need to be explicitly enabled in source builds (when they are build without `NOT_CRAN`). As we want the macos binaries to be fully featured we should turn the features on when the dependencies exists. 

### What changes are included in this PR?

This PR enables this behavior for macOS only, on Linux setting `NOT_CRAN` or  `LIBARROW_MINIMAL=false` is still required.

### Are these changes tested?

Crossbow and locally (thanks @ paleolimbot )
* Closes: apache#38043

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…ons. (apache#38236)

### Rationale for this change

We currently need to manually download the latest nightly build as the ".9000' dev version never matches any nightly builds.

### What changes are included in this PR?

Check the nightly  repo html listing for the latest version matching the local major version.
Refactor  nixlibs.R and integrate the functionality of winlibs.R  into it to simplify maintenance and prepare for the future of in-line windows libarrow builds.

These changes are build on apache#38195 so once that is merged I will rebase this.

### Are these changes tested?

Crossbow
* Closes: apache#27839

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
raulcd pushed a commit that referenced this pull request Nov 28, 2023
…38236)

### Rationale for this change

We currently need to manually download the latest nightly build as the ".9000' dev version never matches any nightly builds.

### What changes are included in this PR?

Check the nightly  repo html listing for the latest version matching the local major version.
Refactor  nixlibs.R and integrate the functionality of winlibs.R  into it to simplify maintenance and prepare for the future of in-line windows libarrow builds.

These changes are build on #38195 so once that is merged I will rebase this.

### Are these changes tested?

Crossbow
* Closes: #27839

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…8195)

### Rationale for this change

Previously GCS/S3 support would need to be explicitly enabled in source builds (when they are build without `NOT_CRAN`). As we want the macos binaries to be fully featured we should turn the features on when the dependencies exists. 

### What changes are included in this PR?

This PR enables this behavior for macOS only, on Linux setting `NOT_CRAN` or  `LIBARROW_MINIMAL=false` is still required.

### Are these changes tested?

Crossbow and locally (thanks @ paleolimbot )
* Closes: apache#38043

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…ons. (apache#38236)

### Rationale for this change

We currently need to manually download the latest nightly build as the ".9000' dev version never matches any nightly builds.

### What changes are included in this PR?

Check the nightly  repo html listing for the latest version matching the local major version.
Refactor  nixlibs.R and integrate the functionality of winlibs.R  into it to simplify maintenance and prepare for the future of in-line windows libarrow builds.

These changes are build on apache#38195 so once that is merged I will rebase this.

### Are these changes tested?

Crossbow
* Closes: apache#27839

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R] Enable full feature libarrow build as default on macOS
5 participants