patchGNU Octave - Patches: patch #10292, Switch from PCRE to PCRE2

 
 

patch #10292: Switch from PCRE to PCRE2

Submitter:  Rafael Laboissière <rlaboiss>
Submitted:  Thu 10 Nov 2022 02:00:15 PM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 19 Nov 2022 12:50:59 PM UTC, comment #13: 

This seems to be working.

Thank you again for your contribution.

Closing as done.

Markus Mützel <mmuetzel>
Group administrator
Tue 15 Nov 2022 04:29:24 PM UTC, comment #12: 

Updated the dependencies in MXE Octave here:
https://hg.octave.org/mxe-octave/rev/a8cdb57c6ea6

Markus Mützel <mmuetzel>
Group administrator
Tue 15 Nov 2022 12:02:47 PM UTC, comment #11: 

I followed up with a change that installs the PCRE2 packages instead of the PCRE packages on the respective GitHub runners here:
https://hg.savannah.gnu.org/hgweb/octave/rev/c7b2ad1e51f3

This change is probably still worth an entry to the NEWS file (even if backwards compatibility is probably not a concern). Done here:
https://hg.savannah.gnu.org/hgweb/octave/rev/662499ba0d21

We could/should probably also switch to PCRE2 in MXE Octave. Even if that might have happened implicitly already, we should probably make that dependency explicit. I'll try to do that later if no one else beats me to it.

Markus Mützel <mmuetzel>
Group administrator
Mon 14 Nov 2022 06:39:57 PM UTC, comment #10: 

Just for your information, I applied the changes proposed by John to the source files for Octave 7.3.0 (actually, the Debian package for this version) and everything went fine (compilation and tests).

Rafael Laboissière <rlaboiss>
Sun 13 Nov 2022 08:53:06 PM UTC, comment #9: 

Thanks, Markus and John, for the improvements in my patch and for the pushed changesets.

This is an important improvement for the downstream distributors of Octave.

Rafael Laboissière <rlaboiss>
Sun 13 Nov 2022 05:34:04 PM UTC, comment #8: 

I pushed the following changesets, all adapted from the patch posted in comment #6 and all credited to Rafael.

http://hg.savannah.gnu.org/hgweb/octave/rev/ff5bac1b9372
http://hg.savannah.gnu.org/hgweb/octave/rev/fcd4bc97f5f7
http://hg.savannah.gnu.org/hgweb/octave/rev/d1165473e4b0

Thanks for making this change.

My changes restored support for the old PCRE library so that now on systems with both versions of PCRE installed, Octave's configure script will prefer PCRE2 but you can use the configure option --without-pcre2 to skip the check for PCRE2 and Octave will find the older PCRE library instead.  Or, if only one of the libraries is installed, Octave's configure script should find and use it.

If should be fairly easy to drop support for PCRE version 1 in the future if/when we choose to do that.

Marking as ready for test.

John W. Eaton <jwe>
Group administrator
Fri 11 Nov 2022 07:10:50 PM UTC, comment #7: 

I understand that the world is moving to completely replace pcre with pcre2, but I'd still prefer to have the option for now of continuing to use pcre if it is reasonable to do so.  I can look at making that change and would like to do it before committing this change.  So please let me make an attempt at that before pushing the patch.  Thanks.

John W. Eaton <jwe>
Group administrator
Fri 11 Nov 2022 07:02:01 PM UTC, comment #6: 

Thank you very much. This looks good to me. 👍

I'm attaching a slightly updated version of your patch that silences a compiler warning about a mismatching format string, fixes some minor whitespaces, and adds a note about this change to the NEWS file.

I'm planning on adapting our CI rules for this change (install pcre2 instead of pcre) and will probably push this sometime tomorrow unless there'll be contrary comments.


(file #53949)

Markus Mützel <mmuetzel>
Group administrator
Fri 11 Nov 2022 03:15:46 PM UTC, comment #5: 

Attached to this message is a revised patch. I addressed the issues raised by John and Markus. Below are some comments:

[John]

> Octave's public header files should not depend on macros like PCRE2_DATA_WIDTH, PCRE2_CODE_UNIT_WIDTH, or include the pcre2.h header directly.  Using PCRE (or any library) is an implementation detail that shouldn't be exposed to someone just wanting to use the regexp class.  That's why we used the void pointer previously and only included the header files in .cc files.

[Markus]

> * I see you moved including the header from the .cc file(s) to the .h file. Would it be possible keep them in their old locations? That would reduce the number of (unnecessary) symbols declared in user code. Or is there a reason for this change?

I kept the scheme for including pcre2.h pretty similar to the previous one. Now, there is no unnecessary symbols declared in the public header file lo-regexp.h. I dropped the inclusion of pcre.h from regexp.cc, without replacing it by the inclusion of pcre2.h, since this does not seem necessary.

[John]

> How do we know that a buffer size of 128 is sufficient for the call to pcre2_get_error_message?  Should we try to adapt that in case the message ends up being longer and pcre2_get_error_message returns a failure code?

I increased the buffer size to 256. This is what the upstream author does in its demo program pcre2demo.c. I guess that this should be ok.

[John]

> If we need a change for OCTAVE_CHECK_LIB, that should probably be discussed separately.

[Markus]

> Wrt changing OCTAVE_CHECK_LIB: It might be less intrusive to (temporarily) add those defines to CPPFLAGS for this test.

The suggestion made by Markus is implemented in the new patch.

[Markus]

> * In lines 227 and following of your patch: I don't understand why `start` and `end` are of type double to begin with. You changed it to type int. Shouldn't it be of type std::size_t (or maybe better yet of type PCRE2_SIZE) instead? I guess that would also apply to the respective properties of `regexp::match_element`. Maybe that should be a separate change. But feel free to include it here if you prefer.

IIUC, the variables `start` and `end` were defined as double in the original code because they are passed as arguments `s` and `e` of regexp::match_element, which are defined as double. I changed them to int in order to simplify the code but, yes, it should be PCRE2_SIZE.

[Markus]

> * In lines 288 and 289: Static_casting between integer and floating-point types might be risky for large values (that can be represented in one type but not the other). Should there be more thorough checks? (That point might be mute if we change the type of those properties and the corresponding constructor in `regexp::match_element`.)

My preference is to let this for a separate change, if you agree.

[Markus]

> * In the code, you #define `PCRE2_DATA_WIDTH` and `PCRE2_CODE_UNIT_WIDTH`. Should both also be defined for the configure check?

I think that I defined PCRE2_DATA_WIDTH because I have seen it in an PCRE2 example elsewhere. The code works without it. I dropped it from the patch.

[Markus]

> * Instead of repeating `pcre2_match_data_free` in several locations (which could easily be missed in a future change), could you use `unwind_action` instead? See, e.g., liboctave/util/oct-glob.cc:90:


>       void *glob_info = octave_create_glob_info_struct ();
>
>       unwind_action cleanup_glob_info_struct
>         ([=] () { octave_destroy_glob_info_struct (glob_info); });

I implemented your suggestion.


(file #53948)

Rafael Laboissière <rlaboiss>
Thu 10 Nov 2022 06:18:00 PM UTC, comment #4: 

I crossed with John's comment. So some of my remarks are duplicates of his.
Wrt changing OCTAVE_CHECK_LIB: It might be less intrusive to (temporarily) add those defines to CPPFLAGS for this test.

Markus Mützel <mmuetzel>
Group administrator
Thu 10 Nov 2022 05:53:48 PM UTC, comment #3: 

Thanks John and Markus for commenting on my patch. I will revise my changes and propose a new patch as soon as time permits

Rafael Laboissière <rlaboiss>
Thu 10 Nov 2022 04:53:27 PM UTC, comment #2: 

Thank you very much for the patch.

I haven't tested it yet. Just a couple of remarks:

  • I see you moved including the header from the .cc file(s) to the .h file. Would it be possible keep them in their old locations? That would reduce the number of (unnecessary) symbols declared in user code. Or is there a reason for this change?
  • In lines 227 and following of your patch: I don't understand why `start` and `end` are of type double to begin with. You changed it to type int. Shouldn't it be of type std::size_t (or maybe better yet of type PCRE2_SIZE) instead? I guess that would also apply to the respective properties of `regexp::match_element`. Maybe that should be a separate change. But feel free to include it here if you prefer.
  • In lines 288 and 289: Static_casting between integer and floating-point types might be risky for large values (that can be represented in one type but not the other). Should there be more thorough checks? (That point might be mute if we change the type of those properties and the corresponding constructor in `regexp::match_element`.)
  • In the code, you #define `PCRE2_DATA_WIDTH` and `PCRE2_CODE_UNIT_WIDTH`. Should both also be defined for the configure check?
  • Instead of repeating `pcre2_match_data_free` in several locations (which could easily be missed in a future change), could you use `unwind_action` instead? See, e.g., liboctave/util/oct-glob.cc:90:


      void *glob_info = octave_create_glob_info_struct ();

      unwind_action cleanup_glob_info_struct
        ([=] () { octave_destroy_glob_info_struct (glob_info); });



Markus Mützel <mmuetzel>
Group administrator
Thu 10 Nov 2022 02:34:41 PM UTC, comment #1: 

Thanks for providing these changes.

Octave's public header files should not depend on macros like PCRE2_DATA_WIDTH, PCRE2_CODE_UNIT_WIDTH, or include the pcre2.h header directly.  Using PCRE (or any library) is an implementation detail that shouldn't be exposed to someone just wanting to use the regexp class.  That's why we used the void pointer previously and only included the header files in .cc files.

If we need a change for OCTAVE_CHECK_LIB, that should probably be discussed separately.

How do we know that a buffer size of 128 is sufficient for the call to pcre2_get_error_message?  Should we try to adapt that in case the message ends up being longer and pcre2_get_error_message returns a failure code?

John W. Eaton <jwe>
Group administrator
Thu 10 Nov 2022 02:00:15 PM UTC, original submission:  

Here is a proposal for switching from PCRE to PCRE2. This should close Bug #61542.

The patch applies to the default branch of the repository. All 122 BISTs in regexp.cc pass successfully.

Notice that the compilation fall back to the older PCRE library, in the case where it is present and PCRE2 is not. This would make the code quite convoluted and it would represent too much effort for a minimal gain. Indeed, PCRE2 is now ubiquitous nowadays and the upstream authors have abandoned development and maintenance for PCRE. Major Linux distributions, like Debian and Fedora, have plans to completely switch from PCRE to PCRE2 in a future release.

Rafael Laboissière <rlaboiss>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #53949:  bug61542-pcre2-v3.patch added by mmuetzel (14KiB - text/plain)
file #53948:  bug61542-v2.patch added by rlaboiss (13KiB - text/x-patch)
file #53942:  bug61542.patch added by rlaboiss (14KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rlaboiss (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-11-19 mmuetzel StatusReady For Test Done
        Open/ClosedOpen Closed
    2022-11-13 jwe StatusIn Progress Ready For Test
    2022-11-11 mmuetzel Attached File- Added bug61542-pcre2-v3.patch, #53949
    2022-11-11 rlaboiss Attached File- Added bug61542-v2.patch, #53948
    2022-11-10 mmuetzel StatusNone In Progress
    2022-11-10 rlaboiss Attached File- Added bug61542.patch, #53942

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code