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

Install patch on Linuxes for patching snappy #20

Closed
wants to merge 3 commits into from

Conversation

jonkeane
Copy link
Contributor

@jonkeane jonkeane commented Dec 9, 2021

No description provided.

@jonkeane
Copy link
Contributor Author

jonkeane commented Dec 9, 2021

The macos errors are unrelated and fixed in #19

@nealrichardson
Copy link
Contributor

I have a more meta concern about this. We got here because we wanted snappy on by default. But in order to have it on by default, it has to build successfully on CRAN, and that means it has to pass the sanitizer checks. To pass the sanitizer checks, we have to apply a patch. So now we can't install snappy if patch isn't available, so we've effectively added another dependency (not a burdensome one, but still). Is this the right tradeoff to make?

@jonkeane
Copy link
Contributor Author

The extra patch dependency is temporary (when google/snappy#148 is merged we can rip all of this out — I'm also eager to have clean sanitizer builds leading up to the Arrow release in case other code we depend on / write fails those checks, it's much easier to find and fix if we know it happens)

In principle we could (and this is exactly what would happen, without the arrow r-package change) skip the patch step on platforms that don't have patch and build + use snappy just fine (albeit, with a sanitizer error). We disable snappy on platforms that don't have patch (in the r package) to ensure we don't accidentally find ourselves in a situation where an unpatched version of snappy gets to a CRAN sanitizer machine.

I agree this is not great — though the benefit of having snappy on by default (cf https://issues.apache.org/jira/browse/ARROW-15047 just today) is high. Alternatively, we could patch and host the patched version of snappy ourselves and then we don't have to worry about if installing systems have patch which means we no longer have that extraneous dependency at build time.

@jonkeane
Copy link
Contributor Author

Alternatively, we could take the r package bit out, and if the cran systems don’t have patch installed and this error ask for forgiveness + point to the fact we’ve sent upstream fixes?

@jonkeane
Copy link
Contributor Author

jonkeane commented Dec 10, 2021

And for clarity, when I'm talking about taking out the extra-paranoid r-arrow bit, I mean deleting this:

# Snappy 1.1.9 is patched to implement https://github.com/google/snappy/pull/148 but some platforms don't have
# patch available, so disable snappy in those cases. If the snappy version is bumped, we should remove this.
if [ ! $(command -v patch) ]; then
  ARROW_WITH_SNAPPY=OFF
fi

Which will allow arrow to be installed with snappy for all platforms (with or without patch), however platforms that do not have patch will be subject to the address sanitizer issue

@nealrichardson
Copy link
Contributor

We don't need this anymore right?

@jonkeane
Copy link
Contributor Author

jonkeane commented Jan 7, 2022

Correct, I should have and thought I had already closed it earlier!

@jonkeane jonkeane closed this Jan 7, 2022
@nealrichardson nealrichardson deleted the jonkeane-patch-2 branch January 7, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants