Skip to content

Commit 00d57e2

Browse files
committed
Merge TASK-007 2nd-pass review cleanup (HYGIENE_STAMP + minors)
Pulls in two commits authored on the orphan fix/task-007-review-cleanup worktree at /private/tmp/task-007-review-cleanup that the Pass 1 review sweeper flagged as missing from feature/v2.0: - 477a06f TASK-007 review cleanup: address cosmetic and minor behavioral findings (Makefile.am hygiene comments + sed-vs-awk + .PHONY, verify-build YAML, TASK-007/TASK-020 AC grep pattern, header_hygiene_test MSYS2 guard). - dff19e5 TASK-007 review: broaden HYGIENE_STAMP deps to include Makefile.am and consumer TU. Both commits were authored 2026-05-27 by Sebastiano Merlino with Claude Sonnet 4.6 — the user's own work, just stranded on the fix branch. # Conflicts: # Makefile.am
2 parents 15c053b + dff19e5 commit 00d57e2

5 files changed

Lines changed: 60 additions & 22 deletions

File tree

.github/workflows/verify-build.yml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -263,9 +263,9 @@ jobs:
263263
# (preprocesses <httpserver.hpp> against the staged install and greps
264264
# for forbidden backend headers). Surfaces this gate as its own named
265265
# GitHub Actions check so reviewers see header-hygiene status
266-
# independently of the broader `make check` log. Until M5 lands the
267-
# check is informational (HEADER_HYGIENE_STRICT defaults to "no");
268-
# TASK-020 flips it to strict.
266+
# independently of the broader `make check` log. HEADER_HYGIENE_STRICT
267+
# defaults to "yes" (flipped from "no" at TASK-020 close); any leak
268+
# of forbidden backend headers through the umbrella fails CI loudly.
269269
- test-group: extra
270270
os: ubuntu-latest
271271
os-type: ubuntu
@@ -870,8 +870,8 @@ jobs:
870870
# TASK-007: dedicated public-header hygiene gate. Runs the
871871
# preprocessor-grep target (Layer 2) against a staged install and
872872
# reports any forbidden backend headers reaching <httpserver.hpp>.
873-
# Currently informational (HEADER_HYGIENE_STRICT=no) -- TASK-020
874-
# flips this to strict when M5 closes the umbrella.
873+
# HEADER_HYGIENE_STRICT defaults to "yes" (set at TASK-020 close);
874+
# any leak of forbidden backend headers through the umbrella fails CI.
875875
run: |
876876
cd build
877877
make check-hygiene
@@ -882,7 +882,7 @@ jobs:
882882
run: |
883883
cd build ;
884884
cat test/test-suite.log ;
885-
if: ${{ failure() && matrix.build-type != 'iwyu' && matrix.compiler-family != 'arm-cross' }}
885+
if: ${{ failure() && matrix.build-type != 'iwyu' && matrix.compiler-family != 'arm-cross' && matrix.build-type != 'header-hygiene' }}
886886

887887
- name: Run Valgrind checks
888888
run: |

Makefile.am

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -264,10 +264,15 @@ check-install-layout:
264264
# it still fires on STLs that do not route std::thread through pthread.
265265
# ---------------------------------------------------------------------------
266266

267+
# NOTE: each entry matches a basename; the grep in check-hygiene anchors with a
268+
# leading '/' so e.g. mypthread.h is not a false positive. Keep this list in
269+
# sync with the #ifdef ladder in test/unit/header_hygiene_test.cpp.
267270
HEADER_HYGIENE_FORBIDDEN = microhttpd\.h|gnutls/gnutls\.h|sys/socket\.h|sys/uio\.h
268-
CHECK_HYGIENE_STAGE = $(abs_top_builddir)/.hygiene-stage
269-
CHECK_HYGIENE_CXX = $(CXX) -std=c++20 -E -I$(CHECK_HYGIENE_STAGE)$(includedir) $(CPPFLAGS)
270-
HEADER_HYGIENE_STRICT ?= yes
271+
CHECK_HYGIENE_STAGE = $(abs_top_builddir)/.hygiene-stage
272+
CHECK_HYGIENE_CXX = $(CXX) -std=c++20 -E -I$(CHECK_HYGIENE_STAGE)$(includedir) $(CPPFLAGS)
273+
# Override on the command line to run against an in-flight umbrella:
274+
# make check-hygiene HEADER_HYGIENE_STRICT=no
275+
HEADER_HYGIENE_STRICT ?= yes
271276

272277
# Sentinel file: only re-run the staged install when headers have changed.
273278
# This is an mtime gate used exclusively for standalone `make check-hygiene`
@@ -277,7 +282,14 @@ HEADER_HYGIENE_STRICT ?= yes
277282
# own pre-built shared stage, so this stamp target is bypassed entirely.
278283
HYGIENE_STAMP = $(CHECK_HYGIENE_STAGE)/.hygiene-stamp
279284

280-
$(HYGIENE_STAMP): $(wildcard $(top_srcdir)/src/httpserver/*.hpp)
285+
# NOTE: wildcard is evaluated at parse time; run `make clean` if new headers
286+
# are added to src/httpserver/ in the same invocation that generates them.
287+
# Makefile.am and consumer_umbrella_no_backend.cpp are also listed so that
288+
# changes to HEADER_HYGIENE_FORBIDDEN or the consumer TU invalidate the stamp.
289+
$(HYGIENE_STAMP): $(wildcard $(top_srcdir)/src/httpserver/*.hpp) \
290+
$(top_srcdir)/src/httpserver.hpp \
291+
$(top_srcdir)/Makefile.am \
292+
$(top_srcdir)/test/headers/consumer_umbrella_no_backend.cpp
281293
@rm -rf $(CHECK_HYGIENE_STAGE)
282294
@$(MAKE) $(AM_MAKEFLAGS) install DESTDIR=$(CHECK_HYGIENE_STAGE) >check-hygiene-install.log 2>&1 || { \
283295
echo "FAIL: staged install failed"; cat check-hygiene-install.log; \
@@ -295,6 +307,11 @@ check-hygiene:
295307
echo " Always pair CHECK_HYGIENE_SHARED=yes with CHECK_HYGIENE_STAGE=<valid-dir>."; \
296308
exit 1; \
297309
fi; \
310+
if ! test -f "$(CHECK_HYGIENE_STAGE)$(includedir)/httpserver.hpp"; then \
311+
echo "FAIL: CHECK_HYGIENE_STAGE exists but does not contain $(includedir)/httpserver.hpp"; \
312+
echo " Was the staged install complete before invoking check-hygiene?"; \
313+
exit 1; \
314+
fi; \
298315
fi
299316
@status=0; \
300317
if ! $(CHECK_HYGIENE_CXX) $(top_srcdir)/test/headers/consumer_umbrella_no_backend.cpp >check-hygiene.i 2>check-hygiene.err; then \
@@ -309,7 +326,7 @@ check-hygiene:
309326
sed 's/^/ /' check-hygiene.err | tail -10; \
310327
fi; \
311328
else \
312-
leaks=`grep -hE '^# [0-9]+ "[^"]*/($(HEADER_HYGIENE_FORBIDDEN))"' check-hygiene.i | awk '{print $$3}' | sort -u`; \
329+
leaks=`grep -hE '^# [0-9]+ "[^"]*/($(HEADER_HYGIENE_FORBIDDEN))"' check-hygiene.i | sed 's/.*"\(.*\)".*/\1/' | sort -u`; \
313330
if test -n "$$leaks"; then \
314331
if test "$(HEADER_HYGIENE_STRICT)" = "yes"; then \
315332
echo "FAIL: forbidden headers leaked through <httpserver.hpp>:"; \
@@ -333,20 +350,25 @@ check-hygiene:
333350
# staged install to avoid paying two full `make install` costs on every
334351
# `make check`. Both install-backed sub-checks can still be invoked standalone
335352
# (they will do their own install when CHECK_*_SHARED is not set).
353+
#
354+
# CHECK_INSTALL_SHARED=yes / CHECK_HYGIENE_SHARED=yes: the caller has already
355+
# staged the install; sub-check skips the install step to avoid double cost.
356+
# This knob is set only by check-local; do not set it when invoking sub-checks
357+
# standalone.
336358
check-local: check-headers check-examples check-readme check-release-notes check-doxygen check-hooks-doc-spotcheck
337359
@echo "=== Shared staged install for check-install-layout and check-hygiene ==="
338-
@rm -rf $(abs_top_builddir)/.shared-check-stage
339-
@$(MAKE) $(AM_MAKEFLAGS) install DESTDIR=$(abs_top_builddir)/.shared-check-stage >check-shared-install.log 2>&1 || { \
360+
@rm -rf "$(abs_top_builddir)/.shared-check-stage"
361+
@$(MAKE) $(AM_MAKEFLAGS) install DESTDIR="$(abs_top_builddir)/.shared-check-stage" >check-shared-install.log 2>&1 || { \
340362
echo "FAIL: shared staged install failed"; cat check-shared-install.log; \
341-
rm -f check-shared-install.log; rm -rf $(abs_top_builddir)/.shared-check-stage; exit 1; }
363+
rm -f check-shared-install.log; rm -rf "$(abs_top_builddir)/.shared-check-stage"; exit 1; }
342364
@rm -f check-shared-install.log
343365
@$(MAKE) $(AM_MAKEFLAGS) check-install-layout \
344-
CHECK_INSTALL_STAGE=$(abs_top_builddir)/.shared-check-stage \
366+
CHECK_INSTALL_STAGE="$(abs_top_builddir)/.shared-check-stage" \
345367
CHECK_INSTALL_SHARED=yes
346368
@$(MAKE) $(AM_MAKEFLAGS) check-hygiene \
347-
CHECK_HYGIENE_STAGE=$(abs_top_builddir)/.shared-check-stage \
369+
CHECK_HYGIENE_STAGE="$(abs_top_builddir)/.shared-check-stage" \
348370
CHECK_HYGIENE_SHARED=yes
349-
@rm -rf $(abs_top_builddir)/.shared-check-stage
371+
@rm -rf "$(abs_top_builddir)/.shared-check-stage"
350372

351373
# TASK-040: enforce static invariants on the two flagship examples
352374
# (hello_world.cpp <= 10 LOC, lambda-only; shared_state.cpp class+mutex).
@@ -429,7 +451,7 @@ lint-file-size:
429451
@echo "=== lint-file-size: enforce per-file line-count ceiling ==="
430452
@$(top_srcdir)/scripts/check-file-size.sh
431453

432-
.PHONY: check-headers check-install-layout check-hygiene check-examples check-readme check-release-notes check-doxygen check-hooks-doc-spotcheck check-soversion check-parallel-install lint-complexity lint-duplication lint-file-size
454+
.PHONY: check-headers check-install-layout check-hygiene check-local check-examples check-readme check-release-notes check-doxygen check-hooks-doc-spotcheck check-soversion check-parallel-install lint-complexity lint-duplication lint-file-size
433455

434456
# TASK-039: top-level convenience rule that descends into test/ to
435457
# build and run the bench binaries (see test/Makefile.am and
@@ -449,7 +471,7 @@ MOSTLYCLEANFILES = $(DX_CLEANFILES) *.gcda *.gcno *.gcov \
449471
DISTCLEANFILES = DIST_REVISION
450472

451473
clean-local:
452-
rm -rf $(CHECK_HYGIENE_STAGE) $(abs_top_builddir)/.shared-check-stage $(CHECK_INSTALL_STAGE)
474+
rm -rf "$(CHECK_HYGIENE_STAGE)" "$(abs_top_builddir)/.shared-check-stage" "$(CHECK_INSTALL_STAGE)"
453475

454476
pkgconfigdir = $(libdir)/pkgconfig
455477
pkgconfig_DATA = libhttpserver.pc

specs/tasks/M1-foundation/TASK-007.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ Lock in the "no backend headers leak through `<httpserver.hpp>`" invariant with
1919
- Blocks: None (informational gate; will fail until M2-M5 land, that's expected and intended)
2020

2121
**Acceptance Criteria:**
22-
- `grep -lE 'microhttpd\.h|pthread\.h|gnutls\.h|sys/socket\.h' src/httpserver/*.hpp` returns no results once M2-M5 land (PRD §3.1 acceptance).
22+
- `grep -lE 'microhttpd\.h|pthread\.h|gnutls/gnutls\.h|sys/socket\.h' src/httpserver/*.hpp` returns no results once M2-M5 land (PRD §3.1 acceptance).
2323
- The hygiene test is invoked by `make check` and fails loudly when violated.
2424
- Typecheck passes.
2525

specs/tasks/M3-request/TASK-020.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ Verify and lock the "no backend headers in public surface" invariant after PIMPL
2020
- Blocks: None (gating outcome that the rest of the project relies on)
2121

2222
**Acceptance Criteria:**
23-
- `grep -lE 'microhttpd\.h|pthread\.h|gnutls\.h|sys/socket\.h' src/httpserver/*.hpp` returns no results (PRD §3.1 acceptance).
23+
- `grep -lE 'microhttpd\.h|pthread\.h|gnutls/gnutls\.h|sys/socket\.h' src/httpserver/*.hpp` returns no results (PRD §3.1 acceptance).
2424
- A test program containing only `#include <httpserver.hpp>` and `int main(){}` compiles without `-I` to libmicrohttpd / pthread / gnutls (PRD §3.1 acceptance).
2525
- TASK-007's hygiene test (red until now) goes green.
2626
- Typecheck passes.

test/unit/header_hygiene_test.cpp

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,12 @@
4444
// XPASS (a hard error by default) -- which is the explicit signal
4545
// for TASK-020 to remove the XFAIL_TESTS marker.
4646
//
47-
// Guard-macro mapping (verified on glibc, musl, macOS/BSD):
47+
// Guard-macro mapping (verified on glibc, musl, macOS/BSD, MSYS2/MINGW64):
4848
//
4949
// <microhttpd.h> -> MHD_VERSION (defined unconditionally inside)
5050
// <pthread.h> -> _PTHREAD_H (glibc/musl, AND macOS/Apple SDK)
5151
// _PTHREAD_H_ (some BSDs)
52+
// _WINPTHREADS_H (MSYS2/MINGW64 winpthreads)
5253
// <gnutls/gnutls.h> -> GNUTLS_GNUTLS_H (the library's own include guard)
5354
// <sys/socket.h> -> _SYS_SOCKET_H (glibc/musl)
5455
// _SYS_SOCKET_H_ (macOS/BSD)
@@ -78,10 +79,20 @@
7879
// preprocessor-grep target `make check-hygiene` in the top-level
7980
// Makefile.am. Keep both lists in sync.
8081

82+
// <cstdio> is included before <httpserver.hpp> for fprintf availability.
83+
// The forbidden-header macro checks in main() are compile-time (#ifdef) guards
84+
// evaluated after the umbrella include, so <cstdio>'s transitive headers cannot
85+
// mask any umbrella-leaked macro. Unlike consumer_umbrella_no_backend.cpp
86+
// (which avoids all standard-library includes for a strict Layer 2 isolation
87+
// test), this sentinel requires fprintf for human-readable runtime reporting.
8188
#include <cstdio>
8289

8390
#include <httpserver.hpp>
8491

92+
// Deliberate multi-assert design: all forbidden-header checks share a single
93+
// accumulator so CI logs show every leaked header in one run, not just the
94+
// first. Each #ifdef block is independent; a single-assert style would stop
95+
// at the first failure and hide remaining leaks.
8596
int main() {
8697
int leaks = 0;
8798

@@ -114,6 +125,11 @@ int main() {
114125
std::fprintf(stderr, "LEAK: <pthread.h> reached the consumer TU (macOS/BSD guard _PTHREAD_H_)\n");
115126
++leaks;
116127
#endif
128+
129+
#ifdef _WINPTHREADS_H
130+
std::fprintf(stderr, "LEAK: <pthread.h> reached the consumer TU (MSYS2/MINGW64 guard _WINPTHREADS_H)\n");
131+
++leaks;
132+
#endif
117133
#endif // !_LIBCPP_VERSION && !_GLIBCXX_HAS_GTHREADS
118134

119135
#ifdef GNUTLS_GNUTLS_H

0 commit comments

Comments
 (0)