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

Switch from vendoring PCRE2 to downloading and building it #8363

Merged
merged 7 commits into from
Jul 3, 2022

Conversation

ridiculousfish
Copy link
Member

As discussed in #8355 and also in #2985, this removes the vendored PCRE2 sources. The FISH_USE_SYSTEM_PCRE2 CMake variable (if false) now downloads and build PCRE2 directly, using the FetchContent module.

This should probably go in after a 3.4 release.

FetchContent

CMake's FetchContent module works by checking out a git repo at configure time, by default into _deps/pcre2-src under the build directory. This directory is 18.5 MB. We then use add_subdirectory as normal to pull in PCRE2's CMake build.

Advantages

  • Updating to a later PCRE2 just means changing the commit hash
  • Sources are a lot smaller
  • Plays nicely with the Mac build, since we can build PCRE2 as universal (ARM + x86)

Disadvantages

  • This mode now requires GitHub to be accessible on the Internet for the initial configure
  • This mode requires CMake 3.11 (released March 2018)

I don't know how to evaluate the impact on Linux distros that use the vendored PCRE2 like Arch, but I think we want them to stop doing that anyways.

@faho
Copy link
Member

faho commented Oct 18, 2021

I don't know how to evaluate the impact on Linux distros that use the vendored PCRE2 like Arch, but I think we want them to stop doing that anyways.

They used to, by accident, but stopped doing that - archlinux/svntogit-community@7e8a186#diff-3e341d2d9c67be01819b25b25d5e53ea3cdf3a38d28846cda85a195eb9b7203a (and the bug report.

If a distribution did want to use our pcre2, they would now have to be okay with it being downloaded. Typically distributions don't allow network access on their builders, so they'd probably want a cache of some description. But then, typically distributions don't want vendored dependencies either, so that would be rather weird policy.

@krobelus
Copy link
Contributor

Looks great. I agree that this better go in after the release.

Small readme update below.
If we want to be extra nice towards users with old CMake, we can add this (additional) error message.

diff --git a/README.rst b/README.rst
index 4d63647d4..cb84aa665 100644
--- a/README.rst
+++ b/README.rst
@@ -147,3 +147,3 @@ Compiling fish requires:
 -  a curses implementation such as ncurses (headers and libraries)
--  PCRE2 (headers and libraries) - a copy is included with fish
+-  PCRE2 (headers and libraries) - optional, this will be downloaded if missing
 -  gettext (headers and libraries) - optional, for translation support
diff --git a/cmake/PCRE2.cmake b/cmake/PCRE2.cmake
index 281cf573c..2bb9cabc9 100644
--- a/cmake/PCRE2.cmake
+++ b/cmake/PCRE2.cmake
@@ -37,3 +37,6 @@ if(FISH_USE_SYSTEM_PCRE2)
 else()
-  include(FetchContent)
+  include(FetchContent RESULT_VARIABLE HAVE_FetchContent)
+  if (${HAVE_FetchContent} STREQUAL "NOTFOUND")
+    message(FATAL_ERROR "Please install PCRE2 headers, or CMake >= 3.11 so I can download PCRE")
+  endif()
   set(CMAKE_TLS_VERIFY true)

output when FISH_USE_SYSTEM_PCRE2=0 and CMake is too old

CMake Error at cmake/PCRE2.cmake:38 (include):
  include could not find requested file:

    FetchContent
Call Stack (most recent call first):
  CMakeLists.txt:156 (include)


CMake Error at cmake/PCRE2.cmake:40 (message):
  Please install PCRE2 headers, or CMake >= 3.11 so I can download PCRE
Call Stack (most recent call first):
  CMakeLists.txt:156 (include)

build_tools/make_pkg.sh Show resolved Hide resolved
cmake/PCRE2.cmake Show resolved Hide resolved
@faho
Copy link
Member

faho commented Oct 22, 2021

I've tested this and it seems to work nicely.

It should do for the cases where people don't have a pcre2 package installed and want to quickly install a fish.

Unless, of course, they don't have internet access when building. I'm not sure that's something we need to support, but if this could support tarballs and you could provide your own that might make it easier (tho in that case you could of course also build pcre2 and install it yourself, but that's system-wide).

@zanchey
Copy link
Member

zanchey commented Oct 23, 2021

This will break the RHEL/CentOS 7 packages as it stands, because there's no PCRE2 on those platforms (it's in EPEL, but I don't think there's any way of making the fish repositories depend on EPEL). There's various workarounds but I'd prefer to wait until after 3.4.0.

@faho
Copy link
Member

faho commented Oct 23, 2021

This will break the RHEL/CentOS 7 packages as it stands, because there's no PCRE2 on those platforms (it's in EPEL, but I don't think there's any way of making the fish repositories depend on EPEL)

@zanchey Would it then not simply download pcre2 and build against that? Or is cmake too old?

There's various workarounds but I'd prefer to wait until after 3.4.0.

This is definitely for after 3.4.0.

@zanchey
Copy link
Member

zanchey commented Oct 23, 2021

The package-building workers don't have internet access, by design.

@floam
Copy link
Member

floam commented Oct 23, 2021

Would OpenSUSE be basically happy if this had been done using straight vanilla git submodules?

@zanchey
Copy link
Member

zanchey commented Oct 24, 2021

The way I have configured the builds at present is that a small script runs on a server I control which basically does everything in the release checklist - builds a tarball, creates a Debian package and an RPM spec, and then uploads them to the right place. This helps ensure that the end-to-end release process is working ok (with a few slight quirks). I'm not sure what would happen with submodules and git clones instead of tarball generation, and I haven't looked into it at all (but I presume it would be possible).

@krobelus
Copy link
Contributor

A real git submodule doesn't seem desirable because it messes with git bisect.
We might want a workaround to include the downloaded copy in the tarball (and maybe teach cmake to pick that one up)

long term we should ask distros to package PCRE2, as it makes little sense to package fish but not PCRE2

@ridiculousfish ridiculousfish added this to the next-major milestone Nov 7, 2021
ridiculousfish referenced this pull request Dec 20, 2021
We're 44% "shell" because it's counting all of pcre2's autocruft!
@ridiculousfish ridiculousfish force-pushed the pcre-external branch 2 times, most recently from 23f7d03 to 41d3d35 Compare July 3, 2022 01:39
This switches to using the CMake FetchContent path to dynamically download
and build PCRE2, allowing us to drop the vendored sources.

The FISH_USE_SYSTEM_PCRE2 CMake option is kept, but if false it now means
fetch-and-build PCRE2 rather than building vendored sources.

Note FetchContent was introduced in CMake 3.11. That is now a prerequisite
for building fish with FISH_USE_SYSTEM_PCRE2 disabled.
Now that PCRE2 is dynamically fetched and built, we can remove the vendored
directory.

Fixes fish-shell#8355
This ensures we don't link against a system installed libpcre2.
Comment in the script why not.
CMake's FetchContent package will check out a git repo and leave
permissions as read-only, causing rm to fail. Pass -f so that rm will
succeed.
We no longer vendor PCRE2 sources, instead we fetch them from the
official repo.
@ridiculousfish ridiculousfish merged commit 5c4f88f into fish-shell:master Jul 3, 2022
@ridiculousfish
Copy link
Member Author

ridiculousfish commented Jul 3, 2022

Bravely merging (after applying suggestions from review, thank you all).

@floam
Copy link
Member

floam commented Jul 9, 2022

We can make the clone a little lighter-weight and slow down the CMake generation step less with GIT_SHALLOW 1.

@zanchey
Copy link
Member

zanchey commented Jul 10, 2022

CentOS/RHEL 7 does have PCRE2 packages - they were added sometime after release. I'll add them to the spec file and it should be ok (there's another issue causing breakage on old PCRE2 at present).

@floam
Copy link
Member

floam commented Jul 10, 2022

GIT_SHALLOW 1 does lighten the load but for some reason not quite as significantly as I suspected it ought to. Looks like a bug being tracked by CMake. Still, it helps.

@ridiculousfish ridiculousfish deleted the pcre-external branch October 29, 2022 22:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants