Skip to content

Conversation

@yadij
Copy link
Contributor

@yadij yadij commented Dec 29, 2024

A simple way to build with custom libregex paths instead of
forcing users to manually set build environment flags.

Currently only supports (re)implementations of GNU libregex.

@yadij
Copy link
Contributor Author

yadij commented Dec 29, 2024

FTR: this build option is required to simplify MinGW cross-building of Squid for Windows.
It may be of use for other OS packagers wanting to link against non-default GNU libregex code.

@yadij yadij added the feature maintainer needs documentation updates for merge label Dec 29, 2024
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

@yadij, before posting new master/v7 PRs, please cooperate on merging PRs (authored by others) that are awaiting your actions for weeks and even months.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Dec 29, 2024
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Mar 13, 2025
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Mar 13, 2025
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

@yadij, if you want to request my review, please do so via the Reviewers dialog on the primary GitHub PR page (rather than just changing PR labels). We have discussed that before: GitHub does not send notifications when labels are changed, and GitHub "your review is requested" search is unaware of our custom labels. Our custom labels serve a different purpose than requesting reviews!

AC_CHECK_HEADERS([regex.h],,[LIBREGEX_LIBS=""])
AC_CHECK_FUNCS([ \
regcomp \
regexec \
Copy link
Contributor

@rousskov rousskov Mar 13, 2025

Choose a reason for hiding this comment

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

AFAICT, official code makes REGEXLIB empty when regexec is not found in libregex.
PR code may1 set LIBREGEX_LIBS to -lregex when regexec is not found in libregex.
Is that change intentional?

This concern may be related to the current build failure (but it is valid even if that build failure is triggered by other/unrelated PR bugs):

/usr/bin/ld: cannot find -lregex: No such file or directory

Perhaps it would be better for this PR to continue to use existing AC_CHECK_LIB() check logic when (and only when) PKG_CHECK_MODULES fails? Doing so would make it clear that this PR preserves existing behavior in no-pc "last resort" cases (while relying on .pc to do the right thing in all other cases)?

Footnotes

  1. The exact conditions for that "may" are not important for this question, but they include something like "no .pc file but regex.h is present".

AC_SUBST(REGEXLIB)
SQUID_AUTO_LIB(regex,[GNU Regex],[LIBREGEX])
SQUID_CHECK_LIB_WORKS(regex,[
PKG_CHECK_MODULES([LIBREGEX],[libregex],[:],[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

This PKG_CHECK_MODULES() addition means that, in environments with the corresponding .pc file, Squid no longer defaults to -lregex; Squid now defaults to whatever that .pc file dictates. Please mention this important change/improvement in release notes and commit message. It is more important than the added --with-regex support itself in many cases!..

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

Labels

feature maintainer needs documentation updates for merge S-waiting-for-author author action is expected (and usually required)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants