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

[R] Add debug output to r/configure in case of failure #38756

Closed
assignUser opened this issue Nov 17, 2023 · 4 comments · Fixed by #38819
Closed

[R] Add debug output to r/configure in case of failure #38756

assignUser opened this issue Nov 17, 2023 · 4 comments · Fixed by #38819

Comments

@assignUser
Copy link
Member

Describe the enhancement requested

We have already enabled printing of the complete cmake log in case of a build issue. We should do something similar for the test compile command in r/configure (and potentially other places in that file that are ambious with their error messages?)

Component(s)

R

@paleolimbot
Copy link
Member

As noted on Zulip, a failed download would also be good to log by default!

@assignUser
Copy link
Member Author

#38769 has an interesting failure

  • the build failes because the downloaded cmake binary can't be found in the build script (this also happened on the cran builder when testing the source build) I don't know why? Maybe some config leads to the R temp dir being cleaned out when the bash script is started or something?
  • the source build/nixlibs.R fail but that doesn't kill the entire package install, which of course leads to confusion... I also saw this on the failed windows build where even though nixlib.R should have closed via q(status=1) the build continued.

@paleolimbot
Copy link
Member

I think there is somewhere in the either nixlibs.R or configure that is swallowing an error and continuing, which probably means that the MacOS check failure also happens earlier than the test compile stage (we just don't know what it is because we don't produce any output).

I think we should probably enable all output at this point: at one point we were trying to actively suppress output that would raise eyebrows with respect to CRAN policy, but I think we are better off enabling all the output by default. We sent them an email telling them what we do: since they already know, the best thing we can do to respect their time (and ours) is to give enough output to reasonably diagnose an install failure.

@assignUser
Copy link
Member Author

assignUser commented Nov 19, 2023

I think we should probably enable all output at this point

Yes totally agree

failure also happens earlier than the test compile stage

I actually don't think so (unless its in the unpack/unlink commands but those don't have errors or warning supressed so they should pop up). After the checksum validation nothing happens that could throw an error. There is code that checks/sets flags which could misbehave and cause the test compile error of course...

assignUser added a commit that referenced this issue Nov 21, 2023
### Rationale for this change

It  hinders debug-ability for users if the failing log doesn't include all info by default.

### What changes are included in this PR?

Add debug output to test compile command in r/configure and always display output with regards to the binary download.

### Are these changes tested?
crossbow, locally
* Closes: #38756

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
@assignUser assignUser added this to the 15.0.0 milestone Nov 21, 2023
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Nov 23, 2023
…pache#38819)

### Rationale for this change

It  hinders debug-ability for users if the failing log doesn't include all info by default.

### What changes are included in this PR?

Add debug output to test compile command in r/configure and always display output with regards to the binary download.

### Are these changes tested?
crossbow, locally
* Closes: apache#38756

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
@assignUser assignUser modified the milestones: 15.0.0, 14.0.2 Nov 27, 2023
raulcd pushed a commit that referenced this issue Nov 28, 2023
### Rationale for this change

It  hinders debug-ability for users if the failing log doesn't include all info by default.

### What changes are included in this PR?

Add debug output to test compile command in r/configure and always display output with regards to the binary download.

### Are these changes tested?
crossbow, locally
* Closes: #38756

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
assignUser added a commit to assignUser/arrow that referenced this issue Dec 1, 2023
…pache#38819)

### Rationale for this change

It  hinders debug-ability for users if the failing log doesn't include all info by default.

### What changes are included in this PR?

Add debug output to test compile command in r/configure and always display output with regards to the binary download.

### Are these changes tested?
crossbow, locally
* Closes: apache#38756

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 issue Feb 19, 2024
…pache#38819)

### Rationale for this change

It  hinders debug-ability for users if the failing log doesn't include all info by default.

### What changes are included in this PR?

Add debug output to test compile command in r/configure and always display output with regards to the binary download.

### Are these changes tested?
crossbow, locally
* Closes: apache#38756

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 a pull request may close this issue.

2 participants