-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(build): update pcre2 autoconf macro and dependencies #2813
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
Conversation
2b947f3
to
1e3fbc3
Compare
I have a less intrusive patch for this, if needed. |
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.
Only commenting on one of three aspects for now.
Is this of any value, or should I close it? |
@fzipi , I haven't looked at this since your comment #2813 (comment) . I had understood that you were going to make some changes. But perhaps I misunderstood you? |
Will rebase first and split the change, you are right. |
Fixes debian builds using pcre2 on extended architectures. Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
e350ea4
to
97d13ac
Compare
When will be Modsecurity 2.9.7 will be released ? Since we need PCRE2 support for our Apache Modsecurity Binary. |
Hello @manojkavali ,
The v2.9.7 release has been published.
|
Would be great if the fix gets into v3. PCRE2 is also not found on SUSE. |
Finally getting back to this. FIrst off, I'm not clear on the precise scope of the proposed fix. In general, ModSecurity .m4 files are structured to attempt to find a dependency automatically, but to also allow the path to be specified via the configure option. Is it the case that A) one can still successfully specify the pcre2 location via the option or B) even that fallback option does not work. A second concern is that the this PR introduces a second .m4 file for pcre2. The pattern thus far has been for a single .m4 file to deal with a particular dependency. Even if there is no problem with the two stepping on each other, I expect it's undesirable to deviate from the existing convention unless it's truly necessary. Finally, the introduction of the include 'config.h' requires more information. What is this file exactly? Are the required pcre2 definitions housed in this file in some of those more esoteric environments? I'm a little uneasy about making presumptions about what a "config.h" file is and including it this way. For one thing, what if at some point a "config.h" file is created within ModSecurity's /src directory at some point in the future. What is some other component or dependency has a "config.h" file. |
I'm not able to reproduce it anymore. Add the
But, the information is not taken from
The commit from @fzipi fix(build): add libpcre2-8 to possible names resolves the issue and |
This PR fixes pcre2 builds for extended architectures. In summary:
Signed-off-by: Felipe Zipitria felipe.zipitria@owasp.org
Fixes #2810