-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-14850: [R] Update ARROW_DEPENDENCY_SOURCE to default to AUTO #11881
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
Conversation
|
@github-actions crossbow submit all |
|
|
@github-actions crossbow -g r |
|
|
@github-actions crossbow submit -g r |
|
Revision: 1938ed9 Submitted crossbow builds: ursacomputing/crossbow @ actions-1256 |
nealrichardson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other thing we should do, if you haven't already, is document how to install all of the possible dependencies with system packages, which ones can't be installed as system packages, etc. This may be general C++ documentation, but we may also want an R-flavored version since we don't deal with all of the possible C++ dependencies in the R package (some are for features we don't enable, some are test/dev dependencies, etc.). Maybe the docs just start by pointing at ci/docker and all of the cpp docker files there: they install dependencies, set ARROW_DEPENDENCY_SOURCE=SYSTEM, and then also set a few other dependency sources as BUNDLED. Some dockerfiles even have comments explaining why (insufficient version, etc.).
| if [ "${ARROW_DEPENDENCY_SOURCE}" = "" ]; then | ||
| # TODO: BUNDLED is still default for now, but we plan to change it to AUTO | ||
| ARROW_DEPENDENCY_SOURCE=BUNDLED; export ARROW_DEPENDENCY_SOURCE | ||
| ARROW_DEPENDENCY_SOURCE=AUTO; export ARROW_DEPENDENCY_SOURCE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Can't leave this comment below but) on L162, we should revise the message, maybe just
echo "**** pkg-config not installed, setting ARROW_DEPENDENCY_SOURCE=BUNDLED"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
OK, will be sure to cover this in #11521 |
jonkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good (and will be especially good to check this in CI for a bit before the release).
I've looked through the build logs and both the bundled and system ones seems to be doing what we expect.
|
Benchmark runs are scheduled for baseline = 392a25f and contender = 04ed72d. 04ed72d is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
No description provided.