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.

Sign up for GitHub

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

Jump to bottom

[R] Source build should check if offline #39697

Closed
nealrichardson opened this issue Jan 19, 2024 · 0 comments · Fixed by #39699
Closed

[R] Source build should check if offline #39697

nealrichardson opened this issue Jan 19, 2024 · 0 comments · Fixed by #39699
Assignees
@nealrichardson
Labels
Component: R Type: enhancement
Milestone
16.0.0

Comments

@nealrichardson
Copy link
Member

Describe the enhancement requested

A source package installation will fail by default if you're offline. It will now correctly fall back to building libarrow from source if a binary isn't available, but the cmake build will try and fail to download thirdparty dependencies. So if we're truly offline, we need to detect that before calling cmake and turn off all optional features. IMO it's not ideal to fall back to successfully building a minimal package if offline--it would be better to opt-in to the offline behavior and not silently get a build with features disabled--but CRAN seems to insist on this.

We can set download_ok <- try_download("https://apache.jfrog.io/artifactory/arrow/r/"), that's a lightweight endpoint that should have good uptime (and is not github, which CRAN has problems with). It would also be good to skip this behavior under certain conditions where the user has expressed a preference for a non-minimal build:

  • LIBARROW_BINARY something other than false or unset
  • LIBARROW_MINIMAL=false
  • Probably also should rename TEST_OFFLINE_BUILD to just OFFLINE_BUILD, and if that is true or false, you don't need to try downloading, just use that value (inverted ,since it's the opposite of download_ok).

Component(s)

R
@github-actions github-actions bot mentioned this issue Jan 19, 2024
Merged
assignUser pushed a commit that referenced this issue Jan 19, 2024
@nealrichardson
GH-39697: [R] Source build should check if offline (#39699)
05b8f36 
### Rationale for this change

CRAN.

### What changes are included in this PR?

See the commit messages

### Are these changes tested?

Existing CI should pass and not be affected. We should confirm that source builds get all features enabled. We should test on macbuilder with this package and with one where we've inserted `download.file <- function(...) stop()` at the top of nixlibs.R to simulate offline behavior.

### Are there any user-facing changes?

I hope there is only one user affected. 
* Closes: #39697

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@assignUser assignUser added this to the 16.0.0 milestone Jan 19, 2024
assignUser pushed a commit that referenced this issue Jan 19, 2024
@nealrichardson @assignUser
GH-39697: [R] Source build should check if offline (#39699)
4e24435 
CRAN.

See the commit messages

Existing CI should pass and not be affected. We should confirm that source builds get all features enabled. We should test on macbuilder with this package and with one where we've inserted `download.file <- function(...) stop()` at the top of nixlibs.R to simulate offline behavior.

I hope there is only one user affected.
* Closes: #39697

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@nealrichardson @dgreiss
apacheGH-39697: [R] Source build should check if offline (apache#39699)
121eb9c 
### Rationale for this change

CRAN.

### What changes are included in this PR?

See the commit messages

### Are these changes tested?

Existing CI should pass and not be affected. We should confirm that source builds get all features enabled. We should test on macbuilder with this package and with one where we've inserted `download.file <- function(...) stop()` at the top of nixlibs.R to simulate offline behavior.

### Are there any user-facing changes?

I hope there is only one user affected. 
* Closes: apache#39697

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@nealrichardson @zanmato1984
apacheGH-39697: [R] Source build should check if offline (apache#39699)
0112a1a 
### Rationale for this change

CRAN.

### What changes are included in this PR?

See the commit messages

### Are these changes tested?

Existing CI should pass and not be affected. We should confirm that source builds get all features enabled. We should test on macbuilder with this package and with one where we've inserted `download.file <- function(...) stop()` at the top of nixlibs.R to simulate offline behavior.

### Are there any user-facing changes?

I hope there is only one user affected. 
* Closes: apache#39697

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
thisisnic pushed a commit that referenced this issue Mar 8, 2024
@nealrichardson @thisisnic
GH-39697: [R] Source build should check if offline (#39699)
14296ca 
### Rationale for this change

CRAN.

### What changes are included in this PR?

See the commit messages

### Are these changes tested?

Existing CI should pass and not be affected. We should confirm that source builds get all features enabled. We should test on macbuilder with this package and with one where we've inserted `download.file <- function(...) stop()` at the top of nixlibs.R to simulate offline behavior.

### Are there any user-facing changes?

I hope there is only one user affected. 
* Closes: #39697

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@nealrichardson @thisisnic
apacheGH-39697: [R] Source build should check if offline (apache#39699)
c54c533 
### Rationale for this change

CRAN.

### What changes are included in this PR?

See the commit messages

### Are these changes tested?

Existing CI should pass and not be affected. We should confirm that source builds get all features enabled. We should test on macbuilder with this package and with one where we've inserted `download.file <- function(...) stop()` at the top of nixlibs.R to simulate offline behavior.

### Are there any user-facing changes?

I hope there is only one user affected. 
* Closes: apache#39697

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nealrichardson nealrichardson

Labels
Component: R Type: enhancement
Projects
None yet
Milestone
16.0.0
Development

Successfully merging a pull request may close this issue.

GH-39697: [R] Source build should check if offline nealrichardson/arrow
2 participants
@nealrichardson @assignUser