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

Enforce YAJL mandatory dependency in configuration and codebase #3151

Open
wants to merge 3 commits into
base: v3/master
Choose a base branch
from

Conversation

eduar-hte
Copy link
Contributor

@eduar-hte eduar-hte commented May 24, 2024

The goal of this PR is to enforce YAJL being a mandatory dependencies during configuration and simplify the codebase. Work on this PR originated in discussions about mandatory dependencies in PR #3144 and initially in this comment.

Summary of the changes:

  • updated the m4 files that control how the library is configured to make the dependency mandatory so that if the library is not available, configuration will now fail.
  • removed #ifdef blocks for the scenario when the library is not present (and is thus unused).
  • updated configure.ac to log the library and its version in the mandatory section.

@airween
Copy link
Member

airween commented May 28, 2024

Thanks for this great PR again.

I know I suggested to remove the optional state of libxml and libyaml, but now I thought it again - I think we should check the most popular architectures that each of them has both libxml and libyaml support (also pcre2).

Eg. libmodsecurity is available on all platform in Debian, but libxml2 does not supported on ia64.

I don't think this is a big problem, but may be we have to consider it. (I just checked libxml2 and only in case of Debian).

May be we should ask the community - not just here, but on Slack.

What do you think?

@eduar-hte
Copy link
Contributor Author

Of course, I think it makes sense to check.

With regards to ia64 (Itanium), it was discontinued by Intel back in 2020. Debian last release with support for ia64 was Debian 7 (wheezy) from 2016. They're now on Debian 12 and have support for just their last three releases (external paid support extends to Debian 8). So I guess this in particular won't be an issue. :-)

@eduar-hte
Copy link
Contributor Author

A quick note on performance comparing the pcre & pcre2 builds (v3/master vs this branch), running the benchmark test with 100.000 requests (instead of 1.000.000). These were run on a couple of local environments, so they're provided just as a reference.

  • Mac mini m1
    • v3/master: 64.78 secs
    • this branch: 63.84 secs
  • Snapdragon 8cx Gen 3 (arm64) (Debian 11 -bullseye- container running on Windows Subsystem for Linux)
    • v3/master: 95.25 secs
    • this branch: 95.40 secs

I think these indicate that there's no significant difference in performance between these two versions of the library.

NOTE: I couldn't include a run on Windows from a different environment because work on PR #3132 only includes support to build with pcre2 already.

@airween
Copy link
Member

airween commented May 30, 2024

Of course, I think it makes sense to check.

With regards to ia64 (Itanium), it was discontinued by Intel back in 2020. Debian last release with support for ia64 was Debian 7 (wheezy) from 2016. They're now on Debian 12 and have support for just their last three releases (external paid support extends to Debian 8). So I guess this in particular won't be an issue. :-)

Sure, I don't think that's a real issue (moreover check the build results of the package in Debian SID here; rows with darker background means that architecture is supported, but not mandatory; so it's no problem if the build fails there).

I just thought we should think it over again, and perhaps we might ask the community - eg. ask them on Slack (or Twitter? 😄)

@airween
Copy link
Member

airween commented May 30, 2024

Thanks for these tests!

I think these indicate that there's no significant difference in performance between these two versions of the library.

yes, I didn't expect that there are any significant differences. But yes, it's good to see the fact.

NOTE: I couldn't include a run on Windows from a different environment because work on PR #3132 only includes support to build with pcre2 already.

Even though this PR is not actual at the moment, would you mind to create a separated PR where the PCRE2 will be the default regex engine?

@eduar-hte eduar-hte changed the title Enforce mandatory dependencies in configuration and codebase Enforce YAJL mandatory dependency in configuration and codebase May 30, 2024
@eduar-hte
Copy link
Contributor Author

I'm updating the PR to simplify it and just focus on YAJL. The changes to make libxml2 and pcre2 mandatory can be resumed when there's feedback on that.

Copy link

sonarcloud bot commented May 31, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

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

Successfully merging this pull request may close these issues.

None yet

2 participants