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] [CI] Clean up our snappy-sanitizer skipping behavior #31766

Closed
asfimport opened this issue Apr 27, 2022 · 6 comments
Closed

[R] [CI] Clean up our snappy-sanitizer skipping behavior #31766

asfimport opened this issue Apr 27, 2022 · 6 comments

Comments

@asfimport
Copy link
Collaborator

asfimport commented Apr 27, 2022

We have a number of locations where we skip parquet tests now that snappy is built by default + we use it by default when it is built.

One recent example of needing to do this is #13014

However, skipping tests like this is a little bit of misdirection, since we aren't really skipping these because | when snappy is not available like the helper suggests, just using that helper to also skip when we know we are in a sanitizer environment.

The ultimate answer to this, of course is to upstream the change google/snappy#148 though that's been sitting open for a few months still.

In the meantime, what if we took out these skips and instead used uncompressed parquet for reading and writting in some builds? This way we could make sure that snappy was not running during sanitizer tests, but still have test coverage for these code paths in other runs where we don't need to worry about this sanitizer error in snappy.

#13014 (comment) proposed one way to do this in this one case, but we should do it more generally for the other skips that we have had to add.

Reporter: Jonathan Keane / @jonkeane

Related issues:

Note: This issue was originally created as ARROW-16385. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Jacob Wujciak / @assignUser:
Given that snappy has a very slow release cadence and the patch has been untouched for month now, would it maybe make sense to rely on a fork with the patch merged (e.g. Antoine's)?

Opinions @pitrou @kou ?

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
My opinion is that we would generally like to avoid installing dependencies from forks, for auditability and trustability reasons, and also ongoing maintenance concerns.

@asfimport
Copy link
Collaborator Author

Kouhei Sutou / @kou:
I agree with @pitrou.

For this case, can we detect Snappy version and whether sanitizer is enabled or not? We'll be able to write more descriptive skip logic for this with these information. (We skip the test only with Snappy 1.1.8 && sanitizer is enabled.)

@asfimport
Copy link
Collaborator Author

Jacob Wujciak / @assignUser:
The patch was finally merged sadly a release just happened, so we will have to keep this behavior for a while longer.

@asfimport
Copy link
Collaborator Author

Apache Arrow JIRA Bot:
This issue was last updated over 90 days ago, which may be an indication it is no longer being actively worked. To better reflect the current state, the issue is being unassigned per project policy. Please feel free to re-take assignment of the issue if it is being actively worked, or if you plan to start that work soon.

paleolimbot pushed a commit that referenced this issue Oct 5, 2023
This PR modifies the build system of the R package to no longer rely on auto/homebrew. Instead this PR adds the infrastructure and code paths to use the same type of  pre-compiled libarrow binaries as we use for Linux. The main difference is the use of the binaries even on CRAN (as we previously also used binaries in form of brew bottles).

The addition of the new artifacts to tasks.yml should ensure that they get uploaded to the nightly repo as well as to the artifactory during the release (@ kou please confirm). 

A summary of the changes in this PR:
- update `r/configure` and `r/tools/nixlibs.R` to enable the source build on macOS and usage of precompiled binaries using the existing mechanism to test compile a program to detect the exisitng openssl version
- added tests for the changes in nixlibs.R
- update the binary allow-list
- Add the build jobs for libarrow binaries for arm64 and x86_64 macos with openssl 1.1 and 3.0 to the `r-binary-packages` job
- Use the binaries to build the nightly packages  
- bump snappy version to 1.1.10 (and patch it on 10.13) due to build issues with the current version. This also touches on a number of issues in regards to a sanitizer issue we have had for a long time: #32562 #31766
- Disable the centos binary test step: #37922

Follow up issues:
- #37921
- #37941 
- #37945
* Closes: #37923

Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this issue Oct 23, 2023
This PR modifies the build system of the R package to no longer rely on auto/homebrew. Instead this PR adds the infrastructure and code paths to use the same type of  pre-compiled libarrow binaries as we use for Linux. The main difference is the use of the binaries even on CRAN (as we previously also used binaries in form of brew bottles).

The addition of the new artifacts to tasks.yml should ensure that they get uploaded to the nightly repo as well as to the artifactory during the release (@ kou please confirm). 

A summary of the changes in this PR:
- update `r/configure` and `r/tools/nixlibs.R` to enable the source build on macOS and usage of precompiled binaries using the existing mechanism to test compile a program to detect the exisitng openssl version
- added tests for the changes in nixlibs.R
- update the binary allow-list
- Add the build jobs for libarrow binaries for arm64 and x86_64 macos with openssl 1.1 and 3.0 to the `r-binary-packages` job
- Use the binaries to build the nightly packages  
- bump snappy version to 1.1.10 (and patch it on 10.13) due to build issues with the current version. This also touches on a number of issues in regards to a sanitizer issue we have had for a long time: apache#32562 apache#31766
- Disable the centos binary test step: apache#37922

Follow up issues:
- apache#37921
- apache#37941 
- apache#37945
* Closes: apache#37923

Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
This PR modifies the build system of the R package to no longer rely on auto/homebrew. Instead this PR adds the infrastructure and code paths to use the same type of  pre-compiled libarrow binaries as we use for Linux. The main difference is the use of the binaries even on CRAN (as we previously also used binaries in form of brew bottles).

The addition of the new artifacts to tasks.yml should ensure that they get uploaded to the nightly repo as well as to the artifactory during the release (@ kou please confirm). 

A summary of the changes in this PR:
- update `r/configure` and `r/tools/nixlibs.R` to enable the source build on macOS and usage of precompiled binaries using the existing mechanism to test compile a program to detect the exisitng openssl version
- added tests for the changes in nixlibs.R
- update the binary allow-list
- Add the build jobs for libarrow binaries for arm64 and x86_64 macos with openssl 1.1 and 3.0 to the `r-binary-packages` job
- Use the binaries to build the nightly packages  
- bump snappy version to 1.1.10 (and patch it on 10.13) due to build issues with the current version. This also touches on a number of issues in regards to a sanitizer issue we have had for a long time: apache#32562 apache#31766
- Disable the centos binary test step: apache#37922

Follow up issues:
- apache#37921
- apache#37941 
- apache#37945
* Closes: apache#37923

Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
This PR modifies the build system of the R package to no longer rely on auto/homebrew. Instead this PR adds the infrastructure and code paths to use the same type of  pre-compiled libarrow binaries as we use for Linux. The main difference is the use of the binaries even on CRAN (as we previously also used binaries in form of brew bottles).

The addition of the new artifacts to tasks.yml should ensure that they get uploaded to the nightly repo as well as to the artifactory during the release (@ kou please confirm). 

A summary of the changes in this PR:
- update `r/configure` and `r/tools/nixlibs.R` to enable the source build on macOS and usage of precompiled binaries using the existing mechanism to test compile a program to detect the exisitng openssl version
- added tests for the changes in nixlibs.R
- update the binary allow-list
- Add the build jobs for libarrow binaries for arm64 and x86_64 macos with openssl 1.1 and 3.0 to the `r-binary-packages` job
- Use the binaries to build the nightly packages  
- bump snappy version to 1.1.10 (and patch it on 10.13) due to build issues with the current version. This also touches on a number of issues in regards to a sanitizer issue we have had for a long time: apache#32562 apache#31766
- Disable the centos binary test step: apache#37922

Follow up issues:
- apache#37921
- apache#37941 
- apache#37945
* Closes: apache#37923

Lead-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>
@pitrou
Copy link
Member

pitrou commented Nov 13, 2024

The patch has been upstreamed and released, closing.

@pitrou pitrou closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants