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

Drop vendored convenience copy of pcre2 #8355

Closed
faho opened this issue Oct 14, 2021 · 8 comments
Closed

Drop vendored convenience copy of pcre2 #8355

faho opened this issue Oct 14, 2021 · 8 comments

Comments

@faho
Copy link
Member

faho commented Oct 14, 2021

Ever since we've been using pcre2 as the regex backend for string, we've been shipping a vendored copy so people don't have to install it.

We've not been fantastic at keeping this up-to-date - we ship 10.36, the current is 10.38, released about 2 weeks ago. Updating it requires a procedure we've been calling "pcrectomy": https://github.com/fish-shell/fish-shell/wiki/PCREctomy. This is because we don't want to ship a bunch of files not needed by us in the source tarball.

I would like to reconsider (#2985) dropping this copy.

Now, this depends of course on the idea that installing pcre2 isn't a huge burden - distro packages will depend on it anyway (because they don't want to use the vendored copy), some systems even have it preinstalled (e.g. arch has it as a dependency of systemd, their sole supported init system!).

It would be possible to drop it after 3.4, in which case we should still update it now. Or we drop it before, or not at all.

Thoughts?

@faho faho added the RFC label Oct 14, 2021
@zanchey
Copy link
Member

zanchey commented Oct 15, 2021

We had a pretty extensive discussion in #2985 in this regard.

I don't actually think it matters that we're not that up to date - it's a fairly mature library with limited changes in each version. Even the security issues don't really affect fish.

In particular, removing it makes the macOS builds much more complicated.

@faho
Copy link
Member Author

faho commented Oct 15, 2021

In particular, removing it makes the macOS builds much more complicated.

What would the steps roughly be? Note that we don't even use the vendored copy in the macOS runner for Github Actions - we install pcre2 via homebrew. So could you make the fish packages by linking that one statically?

Or @floam mentioned in #2985:

It would be trivial to include a OS X libpcre2 pkg installer (maybe the one directly generated by MacPorts) inside the main pkg file as a subcomponent and marked dependency.

(if it turned out that basically everyone used homebrew or macports we could also conceivably drop our pre-built macOS packages entirely. I'm assuming some people use the .app or .pkg?)

I don't actually think it matters that we're not that up to date

The context here is that our vendored version is missing a change that made the tests fail. That kinda does matter, because it changed the dialect and we didn't keep up-to-date on that.

@krobelus
Copy link
Member

If we decide to keep it then maybe make the build system download it. Then we don't need complicated PCREctomy to update.

@floam
Copy link
Member

floam commented Oct 15, 2021

I think making the build system pull in a git submodule if some flag is passed to request a static binary could be a solution. Plenty of projects live happily with that mechanism.

@floam
Copy link
Member

floam commented Oct 15, 2021

So could you make the fish packages by linking that one statically?

Yes. Homebrew's pcre2 formula installs .a files.

floam@M1 /opt/homebrew/lib (master) [124]> ls *pcre*
libpcre.1.dylib@        libpcre16.dylib@        libpcre2-32.a@          libpcre2-posix.3.dylib@ libpcre32.dylib@        libpcreposix.a@
libpcre.a@              libpcre2-16.0.dylib@    libpcre2-32.dylib@      libpcre2-posix.a@       libpcrecpp.0.dylib@     libpcreposix.dylib@
libpcre.dylib@          libpcre2-16.a@          libpcre2-8.0.dylib@     libpcre2-posix.dylib@   libpcrecpp.a@
libpcre16.0.dylib@      libpcre2-16.dylib@      libpcre2-8.a@           libpcre32.0.dylib@      libpcrecpp.dylib@
libpcre16.a@            libpcre2-32.0.dylib@    libpcre2-8.dylib@       libpcre32.a@            libpcreposix.0.dylib@

@ridiculousfish
Copy link
Member

ridiculousfish commented Oct 16, 2021

The macOS installer package needs this and can't depend on Homebrew. CMake does have support for downloading external packages. Maybe start by making a PR to switch to that mechanism if a CMake flag is passed, and we'll see how it works? I'll try it.

@floam
Copy link
Member

floam commented Oct 16, 2021

The macOS installer package needs this and can't depend on Homebrew.

Well, that would be talking about depending on Homebrew to build the fish binary for the installer package, not on using it. Are we sure that's so terrible? And actually, requiring pcre2, not specifically Homebrew. macports or "make install" can also install the .a for static linking, no?

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Oct 17, 2021
Now that PCRE2 is dynamically fetched and built, we can remove the vendored
directory.

Fixes fish-shell#8355
@ridiculousfish
Copy link
Member

ridiculousfish commented Oct 17, 2021

Good point regarding the .a files, however the Homebrew libs are thin (ARM64 or x86-64, not both) so they can't be used to build a universal fish. I think it's rather less risky to build PCRE2 ourselves, so the Mac build is insulated from Homebrew changes. FetchContent worked well, I opened a PR.

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Jul 3, 2022
Now that PCRE2 is dynamically fetched and built, we can remove the vendored
directory.

Fixes fish-shell#8355
ridiculousfish added a commit that referenced this issue Jul 3, 2022
This merge commit incorporates changes to download and build PCRE2 if
not found on the system, removing the vendored sources.

Fixes #8355
@ridiculousfish ridiculousfish added this to the fish 3.6.0 milestone Jul 3, 2022
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

5 participants