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-34105: [R] Provide extra output for failed builds #37698

Closed
wants to merge 1 commit into from

Conversation

thisisnic
Copy link
Member

@thisisnic thisisnic commented Sep 13, 2023

Rationale for this change

Providing extra output for R package builds where the C++ build fails

What changes are included in this PR?

Update the system call to save output when building Arrow C++ from the R package and output it if it's failed

Are these changes tested?

No

Are there any user-facing changes?

Sure

@github-actions
Copy link

⚠️ GitHub issue #34105 has been automatically assigned in GitHub to PR creator.

"\n"
)
cat("**** Error building Arrow C++.", "\n")
if (!env_is("ARROW_R_DEV", "true")) {
Copy link
Member

Choose a reason for hiding this comment

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

See above: quietly <- !env_is("ARROW_R_DEV", "true")

Suggested change
if (!env_is("ARROW_R_DEV", "true")) {
if (quietly) {

cat("**** Error building Arrow C++.", "\n")
if (!env_is("ARROW_R_DEV", "true")) {
cat(status, fill = TRUE)
cat("Re-run with ARROW_R_DEV=true for more detailed debug information.")
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary anymore, is it?

Suggested change
cat("Re-run with ARROW_R_DEV=true for more detailed debug information.")

Comment on lines +479 to +480
stdout = ifelse(quietly, NULL, TRUE),
stderr = ifelse(quietly, NULL, TRUE)
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 certain I follow the logic, but I think this is what you want

Suggested change
stdout = ifelse(quietly, NULL, TRUE),
stderr = ifelse(quietly, NULL, TRUE)
# If running quietly, capture stdout/stderr so it doesn't print,
# else print to the console
stdout = ifelse(quietly, TRUE, ""),
stderr = ifelse(quietly, TRUE, "")

@amoeba
Copy link
Member

amoeba commented Sep 14, 2023

Those three suggestions look correct to me @nealrichardson, thanks for catching them.

@thisisnic I sent you a PR on your fork at thisisnic#10 with @nealrichardson 's suggestions and also one other fix.

To test this out, I needed a way to make the build fail which I did by setting the libarrow install dir (DEST_DIR) to a non-existent directory. This makes the libarrow build fail at the install step. I didn't test my re-creating other types of common build issues but I think since we're printing everything this is enough of a test for this PR.

The output of that run looks good to me and can be seen in my gist.

@amoeba
Copy link
Member

amoeba commented Sep 14, 2023

@thisisnic requested I just make a fresh PR so this one can be closed in favor of #37727.

@thisisnic thisisnic closed this Sep 14, 2023
@thisisnic
Copy link
Member Author

Thanks @amoeba

raulcd pushed a commit that referenced this pull request Sep 15, 2023
### Rationale for this change

This is a replacement for the previous PR #37698. The rationale for this PR is providing extra output for R package builds where the C++ build fails

### What changes are included in this PR?

Update the system call to save output when building Arrow C++ from the R package and output it if it's failed

### Are these changes tested?

No automated tests but the changes have been tested manually.

### Are there any user-facing changes?

Yes, but only for users building the R package from source which is hopefully not common.
* Closes: #34105

Lead-authored-by: Bryce Mecum <petridish@gmail.com>
Co-authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
thisisnic added a commit to thisisnic/arrow that referenced this pull request Sep 15, 2023
)

### Rationale for this change

This is a replacement for the previous PR apache#37698. The rationale for this PR is providing extra output for R package builds where the C++ build fails

### What changes are included in this PR?

Update the system call to save output when building Arrow C++ from the R package and output it if it's failed

### Are these changes tested?

No automated tests but the changes have been tested manually.

### Are there any user-facing changes?

Yes, but only for users building the R package from source which is hopefully not common.
* Closes: apache#34105

Lead-authored-by: Bryce Mecum <petridish@gmail.com>
Co-authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
)

### Rationale for this change

This is a replacement for the previous PR apache#37698. The rationale for this PR is providing extra output for R package builds where the C++ build fails

### What changes are included in this PR?

Update the system call to save output when building Arrow C++ from the R package and output it if it's failed

### Are these changes tested?

No automated tests but the changes have been tested manually.

### Are there any user-facing changes?

Yes, but only for users building the R package from source which is hopefully not common.
* Closes: apache#34105

Lead-authored-by: Bryce Mecum <petridish@gmail.com>
Co-authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
)

### Rationale for this change

This is a replacement for the previous PR apache#37698. The rationale for this PR is providing extra output for R package builds where the C++ build fails

### What changes are included in this PR?

Update the system call to save output when building Arrow C++ from the R package and output it if it's failed

### Are these changes tested?

No automated tests but the changes have been tested manually.

### Are there any user-facing changes?

Yes, but only for users building the R package from source which is hopefully not common.
* Closes: apache#34105

Lead-authored-by: Bryce Mecum <petridish@gmail.com>
Co-authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R] Provide extra output for failed builds
3 participants