[SSSD] [sssd PR#5593][opened] BUILD: prefer PCRE2 over PCRE

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: opened

PR body:
"""
:relnote:This release deprecates pcre1 support. This support will be
removed completely in following releases.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""
Under valgrind:
```
ERROR: test_ipa_subdom_server
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-823040655

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Changes requested

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""
Under valgrind:
```
ERROR: dyndns-tests
ERROR: nestedgroups-tests
ERROR: dp_opt_tests
ERROR: test_ipa_subdom_server
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-823040655

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""
I fixed issues in `nestedgroups-tests` and `dp_opt_tests`.
Two other still fail.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-823831521

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""
Ok, it seems the issue with `dyndns-tests` is the following:
- this test wraps `execv()`
- thus child helpers doing fork()+execv() keep all memory content of original process
- at exit Valgrind doesn't "understand" that `_sss_regexp_t::pcre2_code*` is enough to free all memory allocated for `re` and reports it as `possibly lost`

As Sumit pointed out, there is [suppression pattern for pcre](https://github.com/SSSD/sssd/blob/29abf94e3aec2c54c76adf61318099097c41ea77/contrib/ci/sssd.supp#L61).
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-824233050

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Waiting for review

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: -Changes requested

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

sumit-bose commented:
"""
Hi,

it looks like only `PCRE_LIBS` and `PCRE_CLAGS` are used in `Makefile.am` but the configure check defines only `PCRE2_LIBS` and `PCRE2_CFLAGS` if version 2 is found.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-824993325

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Changes requested

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: -Waiting for review

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""

it looks like only `PCRE_LIBS` and `PCRE_CLAGS` are used in `Makefile.am` but the configure check defines only `PCRE2_LIBS` and `PCRE2_CFLAGS` if version 2 is found.

Hmm... indeed. Looks like an oversight in original introduction of pcre2... Thanks for the catch.

Interesting that it was compiling anyway somehow.

Anyway, I updated first patch to include `PCRE2_*` in `Makefile.am`
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825106727

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: -Changes requested

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Waiting for review

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Bugzilla

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""

it looks like only `PCRE_LIBS` and `PCRE_CLAGS` are used in `Makefile.am` but the configure check defines only `PCRE2_LIBS` and `PCRE2_CFLAGS` if version 2 is found.

Hmm... indeed. Looks like an oversight in original introduction of pcre2... Thanks for the catch.

Interesting that it was compiling and linking anyway somehow.

Anyway, I updated first patch to include `PCRE2_*` in `Makefile.am`
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825106727

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

sumit-bose commented:
"""

Post by alexey-tikhonov

it looks like only `PCRE_LIBS` and `PCRE_CLAGS` are used in `Makefile.am` but the configure check defines only `PCRE2_LIBS` and `PCRE2_CFLAGS` if version 2 is found.

Hmm... indeed. Looks like an oversight in original introduction of pcre2... Thanks for the catch.
Interesting that it was compiling and linking anyway somehow.
Anyway, I updated first patch to include `PCRE2_*` in `Makefile.am`

Hi,

thank you, this seem to work, now e.g. `-lpcre2-8` can be seen in the build logs. Since we cannot use both libraries at once I wonder if it would be more clear to only use `PCRE_LIBS` and `PCRE_CLAGS` and define them in the case libpcre2 is found as well?

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825454223

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""

Post by sumit-bose
Since we cannot use both libraries at once I wonder if it would be more clear to only use `PCRE_LIBS` and `PCRE_CLAGS` and define them in the case libpcre2 is found as well?

That definitely would be more clean.
But those flags are set by `PKG_CHECK_MODULES`. Do you propose to do something like `PCRE_LIBS = PCRE2_LIBS` "manually" in this case?
And btw I don't know what is the purpose of:
https://github.com/SSSD/sssd/blob/f5bbff2502b69dc5f37ba049025683fbd67d7bb3/src/external/libpcre.m4#L1
-- is the same required for `PCRE2_LIBS`..?

But in general, I wouldn't bother much, because:
- currently either `PCRE_*` or `PCRE2_*` has value, and empty vars in makefile doesn't seem to hurt
- we will remove support of PCRE-1 in next release entirely anyway
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825507678

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

sumit-bose commented:
"""
Hi,

I was thinking of

- [PCRE2],
+ [PCRE],

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825518878

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

alexey-tikhonov commented:
"""
Ok, I should read a doc on `PKG_CHECK_MODULES`, of course. Thanks. Updated.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825574238

[thalman]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

thalman commented:
"""
I tested the patch, looks good to me. Works as expected.
Thanks
ACK
"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-825735412

[thalman]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: -Waiting for review

[thalman]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Accepted

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Ready to push

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

pbrezina commented:
"""
Pushed PR: https://github.com/SSSD/sssd/pull/5593

* `master`
* f2bcf74c43a682897e586eeb775c4bfedd95bafa - sssd.supp: suppress false positive valgrind warning about 'pcre2_code' ptr
* 0fbe5af1f1e0b82cd36a8178e58d79b7dc357ab6 - util/regexp: regular talloc d-tor shouldn't fail
* 9aa6fb34bea891e0385a9fc77f0181d01984c212 - tests/test_nested_groups: mem leak fixed
* 31bcb6f032c326d62fb7ac5efcf2ff55c9acbe04 - tests/test_dp_opts: mem leak fixed
* 519d943424ee744ecf7418df6bf6f0688a7d9099 - util/regexp: local functions shall be static
* cbfccb173d1cfa631350778abbee82bca1fbc296 - BUILD: prefer PCRE2 over PCRE

"""

See the full comment at https://github.com/SSSD/sssd/pull/5593#issuecomment-826676666

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: +Pushed

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: -Accepted

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5593
Title: #5593: BUILD: prefer PCRE2 over PCRE

Label: -Ready to push

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5593
Author: alexey-tikhonov
Title: #5593: BUILD: prefer PCRE2 over PCRE
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5593/head:pr5593
git checkout pr5593