-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Refactor/default pcre2 #3321
Refactor/default pcre2 #3321
Conversation
- Deleted AM_CONDITIONAL macros from configure.ac that had no functional impact on the build system.
Updated the build system and related source files to use libpcre2 as the default regex library instead of the deprecated libpcre. This change ensures future compatibility and aligns with the library's maintenance status. To build with the old libpcre, the `--with-pcre` configuration parameter can be specified.
Identified an issue where the macOS GitHub runner no longer includes the libpcre2 library by default. Updated the workflow configuration to explicitly add libpcre2 as a dependency, ensuring successful builds and compatibility with the updated build system. This change prevents build failures on macOS environments and aligns the runner's setup with project requirements.
Added AC_MSG_NOTICE macros to pcre2.m4 to enhance debugging output. This change aims to identify the cause of build failures on macOS runners in GitHub Actions, which do not occur locally or on other platforms (Linux, Windows). The added verbosity will help trace the build process and inspect variable values for inconsistencies in the macOS runner environment.
Introduced PCRE2_CFLAGS, PCRE2_LDADD, and PCRE2_LDFLAGS in all relevant Makefile.am files to align with the existing PCRE_* variable usage. This change addresses potential issues with linking and configuration for builds on macOS GitHub runners. These modifications aim to resolve the build failure observed exclusively in the macOS environment while maintaining compatibility across other platforms. Testing will confirm if this adjustment corrects the issue.
Enhanced the `configure.ac` script to provide clearer and more readable output for PCRE and PCRE2 settings during configuration. This change improves usability by ensuring that the configuration process displays relevant details in a structured and user-friendly format. This update aligns with the broader PCRE to PCRE2 migration effort, making the build configuration process more transparent and consistent.
|
Yay! :) |
- Marked the conversion operator in `Pcre2MatchContextPtr` as `explicit` to improve type safety and prevent unintended implicit conversions. - Ensured consistent use of `nullptr` instead of `NULL` for better readability and modern C++ compliance. These changes enhance code clarity, maintainability, and adherence to modern C++ best practices.
|
Legacy code. :) |
I think we should have keep legacy pcre as there are many users who use for example nas systems, routers with 3rd party software which are partialy close software and where you cannot update things or stop people building pcre3 and pcre2 withing 1 build for example nginx. Where you can compile security with pcre2 and nginx with pcre3. Chaos... Its great as it is already, blazing fast with millions of happy users with multi platform. So kick others out ?? Keep option legacy or pcre2, just set default pcre2 to conclude tests, dont need to remove excellent compatibility with 99,9% devices that can run nginx on the market. |
Well, we definitely didn't say we will finish old PCRE support of any engines.
No. Where did you see that?
This is what this PR does.
Again: we didn't say we will remove this legacy code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Seems good to me, thank you. My only note is the output of the configure script: if I chose specifically the
But when I don't choose anything (or choose
It would be nice to get the same syntax. Now I merge this PR, but please take a look at the |
Hmm... it's very interesting that the merge workflow's cppcheck was failed: But this one's was success: https://github.com/owasp-modsecurity/ModSecurity/actions/runs/13435232244/job/37535916649?pr=3321 |
What
Refactor the build system and the relevant source files to make the PCRE2 library the default build target. If you wish to continue building with the legacy PCRE library, you must explicitly specify it using the following command:
configure --with-pcre.
Additional build system cleanups include:
Removed unnecessary AM_CONDITIONAL lines from configure.ac.
Why
The legacy PCRE library will be deprecated and eventually removed. The first step in this process is to make PCRE2 the default build dependency, replacing the old PCRE library.