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

Support PCRE2 #13771

Merged
merged 1 commit into from
May 21, 2024
Merged

Support PCRE2 #13771

merged 1 commit into from
May 21, 2024

Conversation

ellert
Copy link
Contributor

@ellert ellert commented Oct 2, 2023

This Pull request:

Makes it possible to use PCRE2 as an alternative to PCRE

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #11395

@Axel-Naumann
Copy link
Member

What we (you, the ROOT team, the users) need to discuss is whether we can deprecate TPRegex altogether, given that std::regex is now (since C++17 is required, as a side effect of kicking out old compilers) guaranteed to be available. Or reimplement it using std:: regex.

Thoughts?

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

Test Results

    10 files      10 suites   2d 4h 27m 22s ⏱️
 2 636 tests  2 635 ✅ 0 💤 1 ❌
25 131 runs  25 130 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit f89fc64.

♻️ This comment has been updated with latest results.

@ellert
Copy link
Contributor Author

ellert commented Oct 3, 2023

@andresailer
Copy link
Contributor

pcre2 10.40 (as mentioned in #11395) already comes with a pcre2-config.cmake (https://github.com/PCRE2Project/pcre2/blob/pcre2-10.40/cmake/pcre2-config.cmake.in) file. Can that be used instead of adding FindPCRE2.cmake ?

@ellert
Copy link
Contributor Author

ellert commented Oct 19, 2023

pcre2 10.40 (as mentioned in #11395) already comes with a pcre2-config.cmake (https://github.com/PCRE2Project/pcre2/blob/pcre2-10.40/cmake/pcre2-config.cmake.in) file. Can that be used instead of adding FindPCRE2.cmake ?

The packaged pcre2 in Fedora and RHEL/CentOS/Rocky/Alma does not ship the cmake files. They are only create/installed if pcre2 is built using cmake. The RPM for Fedora and RHEL are built using autotools, and therefore do not contain the cmake files.

@guitargeek
Copy link
Contributor

Not adding this to the 6.32 milestone, since even Fedora rawhide still has the old pcre for now.

@guitargeek
Copy link
Contributor

What we (you, the ROOT team, the users) need to discuss is whether we can deprecate TPRegex altogether, given that std::regex is now (since C++17 is required, as a side effect of kicking out old compilers) guaranteed to be available. Or reimplement it using std:: regex.

Thoughts?

I wouldn't go into this direction yet. We have a couple of problems with std::regexp, like these symbol clashes with other Python libraries:
#15309

Therefore, provided that it's tested, I'm in favor of merging this PR. I'm adding pcre2 to the CI images, so that we can test it:
root-project/root-ci-images#31

@root-project root-project deleted a comment from phsft-bot May 21, 2024
@root-project root-project deleted a comment from phsft-bot May 21, 2024
@guitargeek guitargeek closed this May 21, 2024
@guitargeek guitargeek reopened this May 21, 2024
@guitargeek guitargeek requested a review from dpiparo as a code owner May 21, 2024 04:03
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Amazing! We can see in the CI that it works:

  -- Looking for PCRE
  -- Found PCRE2: /usr/include (found version "10.32") 

@guitargeek guitargeek merged commit 4e77730 into root-project:master May 21, 2024
11 of 14 checks passed
@ellert ellert deleted the pcre2 branch May 21, 2024 14:38
@ferdymercury
Copy link
Contributor

Potential regression: #15986

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

Successfully merging this pull request may close these issues.

Migrate to pcre2
5 participants