GNOME Bugzilla – Bug 740573
drop embedded pcre copy
Last modified: 2018-05-24 17:15:02 UTC
The internal copy of libpcre is at version 8.31, which is outdated, buggy and vulnerable. Updating to a more recent version is difficult because glib doesn't contain all the neccessary unicode data tables, see bug 689791 and bug 684222. The original reason for copying libpcre into glib was that it is patched to use the glib unicode data tables instead of its own. However, it turns out that libgio-2.0 links to libselinux, which in turn links to libpcre. This means that all gtk+/gnome applications actually have two copies of libpcre, more than negating any savings from the shared unicode tables. So I think we should just drop the embedded pcre copy and always use the system one, and bump the required version to the latest one (8.36).
fwiw, I'd love to kick pcre to the curb. However, this would also mean that we need to pull in pcre when jhbuilding (or when anyone is building glib by hand). This would be very annoying. Too bad we can't toss GRegex out...
I'm all for dropping GRegex, but we're not breaking API/ABI, are we? So we need to make a decision to either update the embedded copy of 8.1 (already over 2.5 years old at this time), or drop the embedded copy and always use the system one. I don't think requiring its presence during jhbuild is any worse than any of all the other libraries the gnome stack depends on. I guess this would be master only, but really distros should be advised to use system pcre also for recent stable glib versions via the configure flag.
Ping? It's still early in the cycle, perfect time for stuff like this :-)
(In reply to Christian Persch from comment #3) > Ping? It's still early in the cycle, perfect time for stuff like this :-) Step 1 would be to make GLib versions compiled against a modern pcre pass the tests, for which see my patches on Bug #733325.
(In reply to Simon McVittie from comment #4) > Step 1 would be to make GLib versions compiled against a modern pcre pass > the tests, for which see my patches on Bug #733325. Merged. A potential step 2 is to make --with-pcre=system the default, but still allow --with-pcre=internal.
Created attachment 304883 [details] [review] Use system PCRE unless --with-pcre=internal is given --- Straw-man patch.
Review of attachment 304883 [details] [review]: Would need an entry in the release notes in the README, so that integrators can keep using the internal copy if they want to, but to me it looks good.
Created attachment 318259 [details] [review] Use system PCRE unless --with-pcre=internal is given --- Now with NEWS and README. Is this suitable? Depends on Bug #759808 being fixed, otherwise current distributions can't use the system PCRE anyway.
Created attachment 318262 [details] [review] [untested jhbuild patch] sysdeps-3.20: add libpcre
Created attachment 318265 [details] [review] [untested gnome-continuous patch] manifest: force use of the bundled libpcre for GLib --- I couldn't immediately see how to make Continuous use a system GLib, and in any case it might be useful to have jhbuild and Continuous exercise both code paths between them.
(In reply to Simon McVittie from comment #10) > Created attachment 318265 [details] [review] [review] > [untested gnome-continuous patch] manifest: force use of the bundled libpcre > for GLib > > --- > > I couldn't immediately see how to make Continuous use a system GLib, s/GLib/PCRE/? It'd be a change in the Yocto/OE base to update it, likely straightforward. > and in > any case it might be useful to have jhbuild and Continuous exercise both > code paths between them. In the short term, OK - in the long term, the goal of Continuous is to look like how we expect a distribution core to look, so if that's a split PCRE I'd like Continuous to look that way too.
Review of attachment 318265 [details] [review]: LGTM, please push.
Review of attachment 318262 [details] [review]: This patch is fine on its own, but considering that vte is also trying to use pcre2, and vte actually requires the very latest (currently unreleased pcre2 right now), I don't think sysdeps is a good place for this. I had it in core-deps until https://git.gnome.org/browse/jhbuild/commit/?id=e022a89bcafea1d0a5a4bb144308adfa479ecd89 due to the vte dependency bump. I would simply put it back where it was (but make sure vte continues to use --without-pcre2 until we can update to 10.21).
(In reply to Colin Walters from comment #11) > s/GLib/PCRE/? Oops, yes, that's what I meant. > In the short term, OK - in the long term, the goal of Continuous is to look > like how we expect a distribution core to look, so if that's a split PCRE > I'd like Continuous to look that way too. Sorry, I don't know how the Yocto/OE base for Continuous works. Alternatively, since PCRE seems to have behaviour changes that affect the GRegex tests on a somewhat regular basis (even in the bugfix-only branch), it might be useful to use the latest from svn to get some early-warning. (Does Continuous still support svn like jhbuild does?) (In reply to Michael Catanzaro from comment #13) > considering that vte is also trying to > use pcre2, and vte actually requires the very latest (currently unreleased > pcre2 right now), I don't think sysdeps is a good place for this. > > I had it in core-deps until > https://git.gnome.org/browse/jhbuild/commit/ > ?id=e022a89bcafea1d0a5a4bb144308adfa479ecd89 due to the vte dependency bump. I saw that commit in the history. I also saw <!-- This should be moved to sysdeps once it's widely-available --> and applied that reasoning to the already-widely-available PCRE 3.x, which is why I added it to sysdeps and not core-deps. > I would simply put it back where it was (but make sure vte continues to use > --without-pcre2 until we can update to 10.21). PCRE 3.x (libpcre.pc, -lpcre, pcre_whatever()) and PCRE 10.x (libpcre2-8.pc, -lpcre2-8, pcre2_whatever()) are independent and parallel-installable - even more so than GTK 2 and GTK 3, because GTK 3 doesn't use function names like gtk3_label_new(). So I think we need to treat them as entirely separate libraries, and not let how we treat one influence how we treat the other.
Yes, makes sense, sysdeps is better then. :)
Review of attachment 318259 [details] [review]: Looks good to me.
Comment on attachment 318259 [details] [review] Use system PCRE unless --with-pcre=internal is given 82c2461, for 2.47.5
Comment on attachment 318262 [details] [review] [untested jhbuild patch] sysdeps-3.20: add libpcre 1532da6
Comment on attachment 318265 [details] [review] [untested gnome-continuous patch] manifest: force use of the bundled libpcre for GLib bd83f3f
(In reply to Christian Persch from comment #0) > So I think we should just drop the embedded pcre copy and always use the > system one, and bump the required version to the latest one (8.36). Deliberately not closing the bug: this has not yet been done. It might be a good idea to release 2.47.5 with system pcre by default, but with the --with-pcre=internal escape hatch still present, before making system pcre mandatory. Colin: please change the Yocto base layer to include pcre when convenient, then Attachment #318265 [details] can be reverted.
Hi, (I am posting this here since 759808 was closed...) Hmm, the situation applies to 8.37 as well for the regex test program, which 759808 addressed, not just 8.38, at least for me on Windows. I can't say anything about 8.36 or before, as I don't have builds for them on my Windows box at this point. If people don't mind, I could lower the check macro in the test program to PCRE_MINOR to >= 37, or I could try to build PCRE version-by-version until we step low enough for the new error checks. I will drop the *_PCRE configs in the Visual Studio project files once we stop bundling PCRE with the GLib sources. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #21) > Hmm, the situation applies to 8.37 as well for the regex test program, which > 759808 addressed, not just 8.38, at least for me on Windows. I can't say > anything about 8.36 or before Looking at http://vcs.pcre.org/pcre?view=revision&revision=1539 it seems that this actually changed in 8.37, so it seems correct to change the check from +#if PCRE_MAJOR > 8 || (PCRE_MAJOR == 8 && PCRE_MINOR >= 38) to +#if PCRE_MAJOR > 8 || (PCRE_MAJOR == 8 && PCRE_MINOR >= 37) If you do that, please cherry-pick it onto glib-2-46 too. We didn't notice this in Debian, because Debian's pcre3 package went from 8.35 to 8.38 without ever shipping the intervening versions.
(In reply to Simon McVittie from comment #22) > (In reply to Fan, Chun-wei from comment #21) > > Hmm, the situation applies to 8.37 as well for the regex test program, which > > 759808 addressed, not just 8.38, at least for me on Windows. I can't say > > anything about 8.36 or before > > Looking at http://vcs.pcre.org/pcre?view=revision&revision=1539 it seems > that this actually changed in 8.37, so it seems correct to change the check > from > > +#if PCRE_MAJOR > 8 || (PCRE_MAJOR == 8 && PCRE_MINOR >= 38) > > to > > +#if PCRE_MAJOR > 8 || (PCRE_MAJOR == 8 && PCRE_MINOR >= 37) > > If you do that, please cherry-pick it onto glib-2-46 too. > > We didn't notice this in Debian, because Debian's pcre3 package went from > 8.35 to 8.38 without ever shipping the intervening versions. I attached a patch to bug #760683 (comment #7) which will fix this issue.
Hi Simon, > If you do that, please cherry-pick it onto glib-2-46 too. I just did that, pushed the commits as 407a4e9 on master and 8c00a00 on glib-2-46. --- Hi Iane, Apparently your patch in the other bug depends on other items in there, so I can't use yours directly, sorry. But thanks though. --- With blessings, thank you!
The attached patches are marked as committed. Is there any thing left to do for this bug or can it be closed?
(In reply to Jeremy Bicha from comment #25) > The attached patches are marked as committed. Is there any thing left to do > for this bug or can it be closed? Er, see the title of the bug? (In reply to Christian Persch from comment #0) > So I think we should just drop the embedded pcre copy and always use the > system one This hasn't yet happened.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/962.