Consistently honor make V=0 convention by kinkie · Pull Request #2467 · squid-cache/squid

kinkie · 2026-07-31T11:49:14Z

Only print the compile command if the V variable
is non-zero, honoring the same convention used by autotools.
Error messages, warnings, and other output from the compiler
will still be printed to the console.

Also filter testHeaders targets early, avoiding needless late skips
for non-header files.

Also apply some opportunistic portability enhancements,
using detected tools such as $(SED) in place of sed

rousskov

Great progress, thank you.

I adjusted PR description to avoid the implication that only .cc files were removed.

Remove .cc files early from the testing, reducing verbosity. Additionally, only print the compile command if the V variable is non-zero, honoring the same convention used by autotools for all other targets. Error messages, warnings, and other output from the compiler will still be printed to the console.

Autotools provides $(AM_V_at) which expands to either '@' or empty, and $(AM_V_P) which expands to either : or false depending on $(V) levels. Instead of using GNU Make's $(filter), use awk for filtering out non-hdrtest targets

…stracting sed in cf.data

Copilot

🟡 Not ready to approve

Several Makefile recipes introduce invalid shell constructs (notably $(AM_V_P) usage and a $(AM_V_GEN) line that attempts to execute a quoted string), which will break builds/tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR updates various Automake (*.am) build rules to better honor the make V=0 verbosity convention (reducing command echoing when not verbose), improves testHeaders target handling by filtering non-header prerequisites earlier, and makes a few portability tweaks by using detected tools (e.g., $(SED)).

Changes:

Prefixes many generated/copy steps with $(AM_V_GEN) / related Automake verbosity helpers to reduce noisy command output.
Updates testHeaders to skip non-header inputs earlier and only print the compile command when verbose.
Switches some hardcoded tool invocations to detected variables like $(SED) / $(SUBSTITUTE).

File summaries

File	Description
tools/helper-mux/Makefile.am	Uses `$(AM_V_GEN)` for generated helper and manpage targets.
test-suite/Makefile.am	Uses `$(AM_V_GEN)` for stub source/header copies.
src/TestHeaders.am	Filters `.hdrtest` targets earlier; changes when compile commands are echoed.
src/store/id_rewriters/file/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/security/cert_validators/fake/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/security/cert_generators/file/Makefile.am	Uses `$(AM_V_GEN)` for manpage substitution generation.
src/Makefile.am	Adds verbosity helpers to several generation/build steps; alters install-time echoing.
src/log/DB/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/icmp/Makefile.am	Uses `$(AM_V_GEN)` for copied-source rules.
src/http/url_rewriters/LFS/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/http/Makefile.am	Uses `$(AM_V_GEN)` and `$(SED)` for generated MethodType source.
src/error/Makefile.am	Uses `$(AM_V_GEN)` for generated categories source.
src/Common.am	Switches `subst_perlshell` to use detected `$(SED)`.
src/auth/ntlm/fake/Makefile.am	Updates commented example to include `$(AM_V_GEN)`.
src/auth/Makefile.am	Uses `$(AM_V_GEN)` for generated enum string-array sources.
src/auth/basic/POP3/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/auth/basic/DB/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/anyp/Makefile.am	Uses `$(AM_V_GEN)` / `$(SED)` for generated ProtocolType source.
src/acl/external/wbinfo_group/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/acl/external/SQL_session/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/acl/external/kerberos_sid_group/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
src/acl/external/delayer/Makefile.am	Uses `$(AM_V_GEN)` for Perl substitution and pod2man.
errors/Makefile.am	Adjusts translation/install recipe verbosity; introduces verbosity-conditional messaging.
doc/manuals/Substitute.am	Switches `SUBSTITUTE` to use detected `$(SED)`.

Review details

Suppressed comments (4)

errors/Makefile.am:94

Same $(AM_V_P) issue here: if it expands to empty, this becomes && echo ... and the install target will fail. Prefer a $(V) check that is safe when V is unset/0 and returns success when skipping the message.

		    $(AM_V_P) && echo "$(INSTALL_DATA) $(builddir)/$$l/$$page $(DESTDIR)$(DEFAULT_ERROR_DIR)/$$l"; \

errors/Makefile.am:97

Same $(AM_V_P) issue here: if it expands to empty, this becomes && echo ... and the install target will fail. Prefer a $(V) check that is safe when V is unset/0 and returns success when skipping the message.

		    $(AM_V_P) && echo "$(INSTALL_DATA) $(srcdir)/$$l/$$page $(DESTDIR)$(DEFAULT_ERROR_DIR)/$$l"; \

src/Makefile.am:693

$(AM_V_P) is not defined anywhere in this tree, so this line can expand to a leading && echo ... and break the shell. Use a $(V) check that is safe when V is unset/0 and returns success when skipping the message.

	  $(AM_V_P) && echo "$(INSTALL_DATA) squid.conf.default $(DESTDIR)$(DEFAULT_CONFIG_FILE)"; \

src/Makefile.am:699

$(AM_V_P) is not defined anywhere in this tree, so these lines can expand to a leading && echo ... and break the shell. Use a $(V) check that is safe when V is unset/0 and returns success when skipping the message.

	$(AM_V_P) && echo "$(INSTALL_DATA) squid.conf.default $(DESTDIR)$(DEFAULT_CONFIG_FILE).default"; \
	$(INSTALL_DATA) squid.conf.default $(DESTDIR)$(DEFAULT_CONFIG_FILE).default; \
	$(AM_V_P) && echo "$(INSTALL_DATA) squid.conf.documented $(DESTDIR)$(DEFAULT_CONFIG_FILE).documented"; \
	$(INSTALL_DATA) squid.conf.documented $(DESTDIR)$(DEFAULT_CONFIG_FILE).documented; \

Files reviewed: 24/24 changed files
Comments generated: 5
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot

🟡 Not ready to approve

Several touched manpage rules still hard-code pod2man instead of the configured $(POD2MAN), which can break builds when the detected tool is not on the runtime PATH.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (11)

tools/helper-mux/Makefile.am:22

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 helper-mux helper-mux.8

src/store/id_rewriters/file/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 storeid_file_rewrite storeid_file_rewrite.8

src/security/cert_validators/fake/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 security_fake_certverify security_fake_certverify.8

src/log/DB/Makefile.am:27

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 log_db_daemon log_db_daemon.8

src/http/url_rewriters/LFS/Makefile.am:31

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 url_lfs_rewrite url_lfs_rewrite.8

src/auth/basic/POP3/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 basic_pop3_auth basic_pop3_auth.8

src/auth/basic/DB/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 basic_db_auth basic_db_auth.8

src/acl/external/wbinfo_group/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 ext_wbinfo_group_acl ext_wbinfo_group_acl.8

src/acl/external/SQL_session/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 ext_sql_session_acl ext_sql_session_acl.8

src/acl/external/kerberos_sid_group/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 ext_kerberos_sid_group_acl ext_kerberos_sid_group_acl.8

src/acl/external/delayer/Makefile.am:25

This rule hard-codes pod2man instead of using the configured $(POD2MAN) tool. When pod2man is found in a non-default location (or POD2MAN is overridden at configure time), this recipe can fail even though documentation generation is enabled.

	$(AM_V_GEN) pod2man --section=8 ext_delayer_acl ext_delayer_acl.8

Files reviewed: 24/24 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

kinkie · 2026-08-02T22:06:30Z

🟡 Not ready to approve

Several touched manpage rules still hard-code pod2man instead of the configured $(POD2MAN), which can break builds when the detected tool is not on the runtime PATH.

While there are some opportunistic portability improvements, they are out of scope for this PR.

rousskov

I have not checked everything carefully, but I suspect that this PR is using AM_V_C and AM_V_CXX variables incorrectly and that the fixed version of this PR should not (explicitly) use those undocumented variables at all. Please see two specific change requests for details.

N.B. Some common AI tools appear to hallucinate a lot on this topic. Please test their suggestions carefully before posting them for human review.

rousskov

I am aware of only one blocking issue remaining.

rousskov · 2026-08-03T18:23:25Z


 test_cache_digest: test_cache_digest.o CacheDigest.o debug.o globals.o store_key_md5.o
-	$(CC) -o $@ $(LDFLAGS) $@.o CacheDigest.o debug.o globals.o store_key_md5.o $(STD_APP_LIBS)
+	$(AM_V_CC) $(CC) -o $@ $(LDFLAGS) $@.o CacheDigest.o debug.o globals.o store_key_md5.o $(STD_APP_LIBS)


AFAICT, this official code is wrong -- we should not use CC to link test_cache_digest. It is likely that we can remove this explicit build rule (possibly after some other adjustments). However, all that work lies outside this PR scope.

I support the current PR change -- it is correctly reporting the wrong tool. FWIW, I would also be OK with leaving this problematic line unchanged.

This comment does not request any PR changes.

rousskov requested changes Jul 31, 2026

View reviewed changes

Comment thread src/TestHeaders.am Outdated

Comment thread src/TestHeaders.am

Comment thread src/TestHeaders.am Outdated

kinkie requested a review from rousskov July 31, 2026 19:46

kinkie added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jul 31, 2026

rousskov requested changes Jul 31, 2026

View reviewed changes

Comment thread src/TestHeaders.am

Comment thread src/TestHeaders.am Outdated

Comment thread src/TestHeaders.am Outdated

kinkie added 3 commits August 1, 2026 09:49

Adopt autotools best practices, improve portability

ed1d716

Autotools provides $(AM_V_at) which expands to either '@' or empty, and $(AM_V_P) which expands to either : or false depending on $(V) levels. Instead of using GNU Make's $(filter), use awk for filtering out non-hdrtest targets

improve hdrtest filter

9b9788b

kinkie force-pushed the testheaders-optional-verbosity branch from 4ab941e to 9b9788b Compare August 1, 2026 08:49

kinkie added 9 commits August 1, 2026 09:16

Use AM_V_GEN in errors/

08c2f24

Use standard autotools verbosity patterns in translations

0cdd2ea

Silence more needlessly verbose output

583dce8

Optionally silence more pointless details

bf38c80

silence shell snippet in translate-warn

bfa065b

Honor V=0 insome file generation steps

a525bb0

Honor V=.. in several targets in src/. Also improve portability by ab…

cd20761

…stracting sed in cf.data

Adopt AM_V_GEN across the board

336bb32

Silence more GEN operations

426d325

kinkie changed the title ~~Reduce verbosity of "make testHeaders"~~ Consistently honor V=0 make convention Aug 1, 2026

kinkie changed the title ~~Consistently honor V=0 make convention~~ Consistently honor make V=0 convention Aug 1, 2026

fix typo

17edf83

kinkie requested a review from Copilot August 1, 2026 19:15

Copilot started reviewing on behalf of kinkie August 1, 2026 19:16 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

Comment thread errors/Makefile.am Outdated

Comment thread errors/Makefile.am

Comment thread src/TestHeaders.am

Comment thread src/Makefile.am

Comment thread src/anyp/Makefile.am Outdated

kinkie added 2 commits August 1, 2026 20:49

Fix improper use of AM_V_GEN

9404325

Restore explicit failure handling in src/anyp/Makefile.am

7358c02

kinkie requested review from Copilot and rousskov August 1, 2026 21:18

Copilot started reviewing on behalf of kinkie August 1, 2026 21:19 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

rousskov requested changes Aug 3, 2026

View reviewed changes

Comment thread src/Makefile.am Outdated

Comment thread src/Makefile.am Outdated

Comment thread src/Makefile.am Outdated

rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Aug 3, 2026

kinkie added 3 commits August 3, 2026 16:08

Ensure space between V macros and their respective commands

f9ed939

It's AM_V_CC, not AM_V_C

59c5add

Implement AM_V_BUILDCXX in src/

04830d9

rousskov requested changes Aug 3, 2026

View reviewed changes

remove AM_V_BUILDCXX, use AM_V_GEN instead

06dc088

Uh oh!

Conversation

kinkie commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (4)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (11)

Uh oh!

kinkie commented Aug 2, 2026

🟡 Not ready to approve

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kinkie commented Jul 31, 2026 •

edited

Loading