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

Refactor/default pcre2 #3321

Merged
merged 10 commits into from
Mar 12, 2025

Conversation

gberkes
Copy link
Contributor

@gberkes gberkes commented Jan 6, 2025

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.

Gabor Berkes and others added 8 commits December 10, 2024 07:32
- 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.
Copy link

sonarqubecloud bot commented Jan 6, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Jan 9, 2025
@theseion
Copy link
Collaborator

Yay! :)

@gberkes gberkes closed this Feb 20, 2025
@gberkes gberkes reopened this Feb 20, 2025
- 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.
@gberkes
Copy link
Contributor Author

gberkes commented Feb 20, 2025

Yay! :)

Legacy code. :)

@mmmmkjjui3
Copy link

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.

@airween
Copy link
Member

airween commented Mar 6, 2025

@mmmmkjjui3,

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...

Well, we definitely didn't say we will finish old PCRE support of any engines.

Its great as it is already, blazing fast with millions of happy users with multi platform. So kick others out ??

No. Where did you see that?

Keep option legacy or pcre2, just set default pcre2 to conclude tests,

This is what this PR does.

dont need to remove excellent compatibility with 99,9% devices that can run nginx on the market.

Again: we didn't say we will remove this legacy code.

Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@airween
Copy link
Member

airween commented Mar 12, 2025

Seems good to me, thank you.

My only note is the output of the configure script: if I chose specifically the --with-pcre option, then I get this result:

   + PCRE                                          ....found 
     using pcre v8.39
      -lpcre -lpcre, -DWITH_PCRE  -DPCRE_HAVE_JIT

But when I don't choose anything (or choose --with-pcre2 - which is the default so it's equal with nothing), then I get:

   + PCRE                                          ....found 
     using pcre v
      , 
   + PCRE2                                          ....found v10.45

It would be nice to get the same syntax.

Now I merge this PR, but please take a look at the configure.ac script.

@airween airween merged commit 1a2b139 into owasp-modsecurity:v3/master Mar 12, 2025
50 checks passed
@airween
Copy link
Member

airween commented Mar 12, 2025

@airween
Copy link
Member

airween commented Mar 12, 2025

Hmm... it's very interesting that the merge workflow's cppcheck was failed:

Okay, the reason is simple: cppcheck has a newer version in last workflow.

The old one: 2.16.0

The new: 2.17.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants