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

Bug 5428: Missing pkg-config not treated as an error #1901

Closed
wants to merge 2 commits into from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Sep 17, 2024

pkg-config is required for building many parts of Squid.
But the PKG_PROG_PKG_CONFIG macro silently ignores
a missing binary. Causing errors far later at build time
which are often mistaken for issues with the library it was
used to detect.

Make pkg-config a mandatory build dependency now
that we rely on PKG_CHECK_MODULES to auto-detect
supported libraries.

@yadij
Copy link
Contributor Author

yadij commented Sep 17, 2024

Making this as cleared for merge since the issue was already triaged in the bug report and this fix agreed to be added.

@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Sep 17, 2024
@kinkie
Copy link
Contributor

kinkie commented Sep 17, 2024

whoops, we overlapped. Main difference is my PR (#1902 ) warns, instead of erring.
What is the best way forward?

@kinkie kinkie removed the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Sep 17, 2024
@kinkie
Copy link
Contributor

kinkie commented Sep 17, 2024

removed merge flag until we agree on which approach to take

configure.ac Outdated Show resolved Hide resolved
@yadij
Copy link
Contributor Author

yadij commented Sep 17, 2024

whoops, we overlapped. Main difference is my PR (#1902 ) warns, instead of erring. What is the best way forward?

IMO error is best for v7 since that version is moving to require it. Older versions the warning may be appropriate if you want to backport a fix.

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.

What required Squid features require pkg-config? If they exist, please update PR description to mention one or, better, two of them. Otherwise, we should not require pkg-config.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Sep 18, 2024
@yadij
Copy link
Contributor Author

yadij commented Sep 18, 2024

What required Squid features require pkg-config?

Short answer is:

We all agreed to make pkg-config a hard requirement for Squid v7 and I have been removing the code that allows libraries to be detected without it. The addition of this hard error reflects that decision.

Long(er) answer:

.. depends entirely on ones scope / interpretation of the word "required";

  • Developer? or Distributor? or System Admin / User?
  • Does one "require" Linux / BSD firewall integration for NAT / TPROXY?
  • Does one "require" OpenSSL or GnuTLS for HTTPS features?
  • Does one "require" libcap integration for root privileges (hello SELinux)?
  • Does one "require" make check to be operational?

... those are the obvious highlighted by grep PKG_CHECK configure.ac. The list is changing as more get refactored.

FWIW; There is already a diagnostic line printed about pkg-config being absent (albeit without a "WARNING" panic label). Nobody notices it when the library-specific side-effects grab attention by halting the build. Bug 5428 troubleshoot being just the latest case.

@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 Sep 18, 2024
@yadij yadij requested a review from rousskov September 18, 2024 04:31
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

let's ship it.
Missing pkg-config will result in unexpected build outcomes, better be prescriptive and ensure we have a fully working build environment

@rousskov
Copy link
Contributor

What required Squid features require pkg-config?

We all agreed to make pkg-config a hard requirement for Squid v7

We did not1.

Long(er) answer depends entirely on ones scope / interpretation of the word "required"

In this context, "X is required" means (roughly speaking) "there is no environment where ./configure ... && make can succeed without X".

... those are the obvious highlighted by grep PKG_CHECK configure.ac.

AFAICT, all modules checked by PKG_CHECK_MODULES() are not "required" (as defined above). The original change request stands.

Footnotes

  1. I hope we agree that ./configure should be testing modules (that normally ship with pkg-config configuration files) using PKG_CHECK_MODULES(). That design does not make pkg-config program presence a hard requirement for building Squid. PKG_CHECK_MODULES() works correctly in environments without pkg-config program; our ./configure --help even provides instructions on how to override what pkg-config would do.

@rousskov rousskov removed their request for review September 18, 2024 13:19
@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 Sep 18, 2024
@yadij
Copy link
Contributor Author

yadij commented Sep 18, 2024

Vetoed by Alex.

@yadij yadij closed this Sep 18, 2024
@kinkie
Copy link
Contributor

kinkie commented Sep 18, 2024

@yadij How about just changing AC_MSG_ERROR to AC_MSG_WARN?

@yadij yadij deleted the arc-autoconf-ng-42 branch September 21, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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