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

update pcre support to pcre2 #203

Closed
komacke opened this issue Apr 16, 2023 · 9 comments · Fixed by #206
Closed

update pcre support to pcre2 #203

komacke opened this issue Apr 16, 2023 · 9 comments · Fixed by #206

Comments

@komacke
Copy link

komacke commented Apr 16, 2023

It seems that the fedora maintainers can't update to xastir 2.1.8 due to an issue with pcre:

https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&classification=Fedora&component=xastir&product=Fedora&product=Fedora%20EPEL

Could support be added so they can bump the package to the latest version?

@tvrusso
Copy link
Member

tvrusso commented Apr 16, 2023

Development energy for Xastir is pretty low right now, and apparently the PCRE2 API is very much different from the PCRE we use in Xastir, so while it is certainly possible that we could do this, it is not a trivial request and might not be a real fast change.

There is no obvious "pcre to pcre2" migration documentation, and the original author of DBFAWK (the part of Xastir that uses PCRE) hasn't been around for a long time. So whoever picks this up and works on it would first have to learn how DBFAWK uses pcre in detail, learn how to do the same thing in PCRE2's API, and do the coding to maintain backward compatibility (some systems that are NOT fedora continue to maintain PCRE 8.45 packages even though it's reached end of life already) with the aid of appropriate ifdefs and configure macros.

So:

  • I will take a look when I can
  • It would be phenomenally helpful if anyone else would like to pitch in
  • Don't hold your breath yet

@tvrusso
Copy link
Member

tvrusso commented Apr 16, 2023

Since I'm looking at it now and scoping out the type of work that will be required, I'm going to ramble on what I've found.

The old PCRE API is documented here:
https://www.pcre.org/original/doc/html/

The new PCRE2 API is here:
https://www.pcre.org/current/doc/html/

The good news is that ALL pcre function calls in Xastir are in the file src/awk.c (with some data structures from pcre used in src/awk.h), and we only call the functions pcre_maketables, pcre_study, pcre_compile, pcre_exec, and pcre_free.

Flow of awk_compile_program in awk.c is this:
for each rule in an "awk program" linked list:
- clear out any existing parse table saved in the rule
- allocate a table using pcre_maketables
- use pcre_compile to compile that rule's pattern and store the resulting regexp in the rule's "re" element
- optimize the regexp with pcre_study

The function awk_exec_program simply calls pcre_exec to check for matches with a compiled regexp.

There are functions similar to each of these in PCRE2, such as pcre2_maketables/pcre2_maketables_free, pcre2_compile/pcre2_compile_free, and pcre2_match. The "optimization" of pcre_study is apparently now done by pcre2_jit_compile.

It is not clear that there is a trivial substitution to be made here, but Xastir's use of PCRE is pretty minimal --- compile regexps, do matches, clean up --- so it shouldn't be a major surgery.

@tvrusso
Copy link
Member

tvrusso commented Apr 16, 2023

Also, per the linked fedora bugzilla reports, pcre has been deprecated in Fedora, not removed. The reports simply say "please consider" porting to pcre2, not "we cannot bump this package because you use a package we dropped." The only reported build failure for 2.1.8 was a year and a half ago, for an ARM build.

They want to drop PCRE, but haven't yet. https://bugzilla.redhat.com/show_bug.cgi?id=2127507 is still blocked by a lot of other packages.

This is worth doing, because someday PCRE will have to be dropped from package managers, but it's not the only reason that Xastir has not been updated by Fedora yet.

@df7cb
Copy link

df7cb commented Jul 25, 2023

In Debian, the PCRE3 removal is actually happening now: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=999925
Xastir already got removed from the testing distribution, i.e. if this doesn't get resolved in some way, it will not be part of the next release (trixie). I see there is a --without-pcre option that we might consider as a workaround until a proper fix is available.

@tvrusso
Copy link
Member

tvrusso commented Jul 25, 2023

That is indeed unfortunate.

The "--without-pcre" no longer exists. It was removed in commit 3a9b6e1, as it is now required for shapelib support.

You can, however, disable shapelib support (which breaks all use of ESRI shapefile maps) with "--without-shapelib". This will prevent configure from even looking for pcre.

It would be very nice if someone who has the time and energy would take a look at what it would take to make our PCRE usage compatible with PCRE2. I outlined the work that will need to be done in an earlier comment: we'll need to unravel exactly what our calls to PCRE are doing and what one has to do using the PCRE2 API to accomplish exactly the same thing. All of that stuff is in awk.c and awk.h, which will have to have all PCRE2 code ifdeffed for backward compatibility (Xastir is very, very conservative and we support very old OSen as much as possible, so one doesn't just replace our PCRE code with PCRE2 code, one must add new PCRE2 support and control which version is compiled/linked based on configure tests).

I am, unfortunately, not able to spend much time doing this sort of work right now and would appreciate any help that the community can offer.

The person who wrote our "dbfawk" code has not been active in Xastir development for many, many years (his last commit was in 2004). So fixing it will be a matter of reverse-engineering his work and recoding it to support the new PCRE2 API.

@tvrusso
Copy link
Member

tvrusso commented Jul 25, 2023

Some possibly helpful information about pcre1->pcre2 migration may be found in the various links at this pcre2project issue.

I have taken a few tentative steps to pull dbfawk, awk, and testdbfawk out of the Xastir tree and build them stand-alone. I should be able to refactor what's there a tiny bit to isolate the pcre calls and data a little more (notably the calls to "free" on various pieces of the awk_rule_ struct where pcre2 now has specialized "pcre2_somethingorother_free" calls).

From there I might be able to swap out pcre2 style usage for existing code and see if I can get testdbfawk to do the same thing that it does now. I will only have an hour or two a day to look at it in the coming wee, and I've used those up today.

Again, I appreciate any help I can get. No porting guide was ever developed when pcre2 was created, and as stated in the issue linked above, none will be forthcoming.

@tvrusso
Copy link
Member

tvrusso commented Jul 25, 2023

FWIW, I have a standalone version of awk.c, dbfawk, and testdbfawk that use pcre2 and produce exactly the same output on exactly the same input, so I have at least gotten most of it working in a hackish way.

Making this so that it is cleanly able to support both pcre1 and pcre2 (which is required for backward compatibility and stability on old installations) will be a bit more work, as there are some kludgy things one must do with respect to datatypes of pointers being passed to functions. Then there's all kinds of fun with configure.ac to work out.

I should be able to get to it for real this weekend.

Once I have testdbfawk working, the rest follows automagically, as testdbfawk does everything that Xastir does with the functions in awk.c and dbfawk.c, so Xastir itself will Just Work once awk.c and dbfawk.c are right.

@df7cb
Copy link

df7cb commented Jul 26, 2023

I appreciate Xastir being conservative (but please some day fix #152 😄), but PCRE2 has been around since 2015, isn't that enough?

@tvrusso tvrusso added this to To do in Release 2.2.0 Jul 26, 2023
@tvrusso tvrusso self-assigned this Jul 26, 2023
tvrusso added a commit to tvrusso/Xastir that referenced this issue Jul 26, 2023
Back when we had a hard-coded alternative to DBFAWK in map_shp.c, we
had to distinguish between a compilation where shapelib was found and
pcre wasn't, and when both were found.

In commit 3a9b6e1 the hard-coded rendering of shapefiles was removed,
and now configure refuses to compile in shapefile support if pcre is
not found.

It is now therefore unnecessary to have ifdefs that look for
"HAVE_LIBPCRE" because if "HAVE_LIBSHP" is defined then
configure *also* found a suitable PCRE.  It is enough to ifdef based
on HAVE_LIBSHP alone.

This commit removes all the ifdefs like "#if defined(HAVE_LIBSHP) &&
defined(HAVE_LIBPCRE)" or "#ifdef HAVE_LIBPCRE", replacing both with
"#ifdef HAVE_LIBSHP"

This commit is groundwork for really addressing issue Xastir#203
tvrusso added a commit to tvrusso/Xastir that referenced this issue Jul 26, 2023
When I add PCRE2 support, there are lines of Xastir's awk.c and awk.h
that will need to be completely overridden by new API calls and
declarations.

This commit adds configure.ac defines to flag that a legacy PCRE is
being used, and puts all calls to that legacy interface into an ifdef.

I have confirmed that doing this has absolutely no effect on a build
that already worked with legacy PCRE, and the "testdbfawk" program
works just fine to produce exactly the same output it always did on
input it always handled before.  That is, it properly parses the
DBFAWK patterns and actions, and produces valid assignment of Xastir's
various map rendering variables.  Xastir renders shapefiles
correctly (or rather, as correctly as it ever did).

More groundwork for issue Xastir#203
tvrusso added a commit to tvrusso/Xastir that referenced this issue Jul 26, 2023
This commit changes configure so that it first looks for pcre2, and if
it finds it it uses it.  Only if pcre2 cannot be found (either because
the header or library doesn't work) then the legacy pcre is searched.

All code that uses PCRE has been conditionalized to do the right thing
no matter which version is linked to.

I have tested it in both legacy and current versions.

Fixes issue Xastir#203
@tvrusso tvrusso linked a pull request Jul 26, 2023 that will close this issue
@tvrusso tvrusso moved this from To do to In progress in Release 2.2.0 Jul 27, 2023
Release 2.2.0 automation moved this from In progress to Done Jul 27, 2023
tvrusso added a commit that referenced this issue Jul 27, 2023
Issue 203 -- Add support for PCRE2

Closes #203
@df7cb
Copy link

df7cb commented Aug 4, 2023

Thank you! Updated Debian package is on the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Release 2.2.0
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants