Skip to content

Commit 477a06f

Browse files
etrclaude
andcommitted
TASK-007 review cleanup: address cosmetic and minor behavioral findings
- Makefile.am: add comment explaining HEADER_HYGIENE_FORBIDDEN anchor semantics (item 8); add wildcard parse-time warning comment and broaden HYGIENE_STAMP to include top-level src/httpserver.hpp (items 10, 29); normalize variable alignment; add inline override comment to HEADER_HYGIENE_STRICT (items 22, 19/23); expand check-local comment to list all sub-checks and document CHECK_*_SHARED knob (items 14, 21); replace awk with sed for robust path extraction (item 24); add check-local to .PHONY (item 25); quote abs_top_builddir in shell word positions (item 37); add staged-install content guard after test -d check (item 46) - verify-build.yml: exclude header-hygiene from 'Print tests results' step (item 42); update stale comments about HEADER_HYGIENE_STRICT default - TASK-007.md, TASK-020.md: fix gnutls acceptance-criteria pattern from gnutls\.h to gnutls/gnutls\.h (items 40, 26) - header_hygiene_test.cpp: add _WINPTHREADS_H guard for MSYS2/MINGW64 (item 48); document deliberate multi-assert design (item 18); document <cstdio> inclusion rationale vs consumer_umbrella_no_backend.cpp (item 5); update guard-macro table with MSYS2 variant Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 014ad36 commit 477a06f

5 files changed

Lines changed: 60 additions & 25 deletions

File tree

.github/workflows/verify-build.yml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -262,9 +262,9 @@ jobs:
262262
# (preprocesses <httpserver.hpp> against the staged install and greps
263263
# for forbidden backend headers). Surfaces this gate as its own named
264264
# GitHub Actions check so reviewers see header-hygiene status
265-
# independently of the broader `make check` log. Until M5 lands the
266-
# check is informational (HEADER_HYGIENE_STRICT defaults to "no");
267-
# TASK-020 flips it to strict.
265+
# independently of the broader `make check` log. HEADER_HYGIENE_STRICT
266+
# defaults to "yes" (flipped from "no" at TASK-020 close); any leak
267+
# of forbidden backend headers through the umbrella fails CI loudly.
268268
- test-group: extra
269269
os: ubuntu-latest
270270
os-type: ubuntu
@@ -827,8 +827,8 @@ jobs:
827827
# TASK-007: dedicated public-header hygiene gate. Runs the
828828
# preprocessor-grep target (Layer 2) against a staged install and
829829
# reports any forbidden backend headers reaching <httpserver.hpp>.
830-
# Currently informational (HEADER_HYGIENE_STRICT=no) -- TASK-020
831-
# flips this to strict when M5 closes the umbrella.
830+
# HEADER_HYGIENE_STRICT defaults to "yes" (set at TASK-020 close);
831+
# any leak of forbidden backend headers through the umbrella fails CI.
832832
run: |
833833
cd build
834834
make check-hygiene
@@ -839,7 +839,7 @@ jobs:
839839
run: |
840840
cd build ;
841841
cat test/test-suite.log ;
842-
if: ${{ failure() && matrix.build-type != 'iwyu' && matrix.compiler-family != 'arm-cross' }}
842+
if: ${{ failure() && matrix.build-type != 'iwyu' && matrix.compiler-family != 'arm-cross' && matrix.build-type != 'header-hygiene' }}
843843

844844
- name: Run Valgrind checks
845845
run: |

Makefile.am

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -222,10 +222,15 @@ check-install-layout:
222222
# route std::thread through pthread (e.g. MSVC's Microsoft STL).
223223
# ---------------------------------------------------------------------------
224224

225+
# NOTE: each entry matches a basename; the grep in check-hygiene anchors with a
226+
# leading '/' so e.g. mypthread.h is not a false positive. Keep this list in
227+
# sync with the #ifdef ladder in test/unit/header_hygiene_test.cpp.
225228
HEADER_HYGIENE_FORBIDDEN = microhttpd\.h|gnutls/gnutls\.h|sys/socket\.h|sys/uio\.h
226-
CHECK_HYGIENE_STAGE = $(abs_top_builddir)/.hygiene-stage
227-
CHECK_HYGIENE_CXX = $(CXX) -std=c++20 -E -I$(CHECK_HYGIENE_STAGE)$(includedir) $(CPPFLAGS)
228-
HEADER_HYGIENE_STRICT ?= yes
229+
CHECK_HYGIENE_STAGE = $(abs_top_builddir)/.hygiene-stage
230+
CHECK_HYGIENE_CXX = $(CXX) -std=c++20 -E -I$(CHECK_HYGIENE_STAGE)$(includedir) $(CPPFLAGS)
231+
# Override on the command line to run against an in-flight umbrella:
232+
# make check-hygiene HEADER_HYGIENE_STRICT=no
233+
HEADER_HYGIENE_STRICT ?= yes
229234

230235
# Sentinel file: only re-run the staged install when headers have changed.
231236
# This is an mtime gate used exclusively for standalone `make check-hygiene`
@@ -235,7 +240,10 @@ HEADER_HYGIENE_STRICT ?= yes
235240
# own pre-built shared stage, so this stamp target is bypassed entirely.
236241
HYGIENE_STAMP = $(CHECK_HYGIENE_STAGE)/.hygiene-stamp
237242

238-
$(HYGIENE_STAMP): $(wildcard $(top_srcdir)/src/httpserver/*.hpp)
243+
# NOTE: wildcard is evaluated at parse time; run `make clean` if new headers
244+
# are added to src/httpserver/ in the same invocation that generates them.
245+
# Also includes the top-level umbrella so a change there invalidates the stamp.
246+
$(HYGIENE_STAMP): $(wildcard $(top_srcdir)/src/httpserver/*.hpp) $(top_srcdir)/src/httpserver.hpp
239247
@rm -rf $(CHECK_HYGIENE_STAGE)
240248
@$(MAKE) $(AM_MAKEFLAGS) install DESTDIR=$(CHECK_HYGIENE_STAGE) >check-hygiene-install.log 2>&1 || { \
241249
echo "FAIL: staged install failed"; cat check-hygiene-install.log; \
@@ -253,6 +261,11 @@ check-hygiene:
253261
echo " Always pair CHECK_HYGIENE_SHARED=yes with CHECK_HYGIENE_STAGE=<valid-dir>."; \
254262
exit 1; \
255263
fi; \
264+
if ! test -f "$(CHECK_HYGIENE_STAGE)$(includedir)/httpserver.hpp"; then \
265+
echo "FAIL: CHECK_HYGIENE_STAGE exists but does not contain $(includedir)/httpserver.hpp"; \
266+
echo " Was the staged install complete before invoking check-hygiene?"; \
267+
exit 1; \
268+
fi; \
256269
fi
257270
@status=0; \
258271
if ! $(CHECK_HYGIENE_CXX) $(top_srcdir)/test/headers/consumer_umbrella_no_backend.cpp >check-hygiene.i 2>check-hygiene.err; then \
@@ -267,7 +280,7 @@ check-hygiene:
267280
sed 's/^/ /' check-hygiene.err | tail -10; \
268281
fi; \
269282
else \
270-
leaks=`grep -hE '^# [0-9]+ "[^"]*/($(HEADER_HYGIENE_FORBIDDEN))"' check-hygiene.i | awk '{print $$3}' | sort -u`; \
283+
leaks=`grep -hE '^# [0-9]+ "[^"]*/($(HEADER_HYGIENE_FORBIDDEN))"' check-hygiene.i | sed 's/.*"\(.*\)".*/\1/' | sort -u`; \
271284
if test -n "$$leaks"; then \
272285
if test "$(HEADER_HYGIENE_STRICT)" = "yes"; then \
273286
echo "FAIL: forbidden headers leaked through <httpserver.hpp>:"; \
@@ -284,24 +297,30 @@ check-hygiene:
284297
rm -f check-hygiene.i check-hygiene.err; \
285298
exit $$status
286299

287-
# check-local runs check-install-layout and check-hygiene against a single
288-
# shared staged install to avoid paying two full `make install` costs on
289-
# every `make check`. Both sub-checks can still be invoked standalone (they
300+
# check-local runs check-headers (prerequisite), check-install-layout and
301+
# check-hygiene (via recursive $(MAKE) with shared-stage variables) against a
302+
# single shared staged install to avoid paying two full `make install` costs on
303+
# every `make check`. Both sub-checks can still be invoked standalone (they
290304
# will do their own install when CHECK_*_SHARED is not set).
305+
#
306+
# CHECK_INSTALL_SHARED=yes / CHECK_HYGIENE_SHARED=yes: the caller has already
307+
# staged the install; sub-check skips the install step to avoid double cost.
308+
# This knob is set only by check-local; do not set it when invoking sub-checks
309+
# standalone.
291310
check-local: check-headers check-examples check-readme check-release-notes check-doxygen check-hooks-doc-spotcheck
292311
@echo "=== Shared staged install for check-install-layout and check-hygiene ==="
293-
@rm -rf $(abs_top_builddir)/.shared-check-stage
294-
@$(MAKE) $(AM_MAKEFLAGS) install DESTDIR=$(abs_top_builddir)/.shared-check-stage >check-shared-install.log 2>&1 || { \
312+
@rm -rf "$(abs_top_builddir)/.shared-check-stage"
313+
@$(MAKE) $(AM_MAKEFLAGS) install DESTDIR="$(abs_top_builddir)/.shared-check-stage" >check-shared-install.log 2>&1 || { \
295314
echo "FAIL: shared staged install failed"; cat check-shared-install.log; \
296-
rm -f check-shared-install.log; rm -rf $(abs_top_builddir)/.shared-check-stage; exit 1; }
315+
rm -f check-shared-install.log; rm -rf "$(abs_top_builddir)/.shared-check-stage"; exit 1; }
297316
@rm -f check-shared-install.log
298317
@$(MAKE) $(AM_MAKEFLAGS) check-install-layout \
299-
CHECK_INSTALL_STAGE=$(abs_top_builddir)/.shared-check-stage \
318+
CHECK_INSTALL_STAGE="$(abs_top_builddir)/.shared-check-stage" \
300319
CHECK_INSTALL_SHARED=yes
301320
@$(MAKE) $(AM_MAKEFLAGS) check-hygiene \
302-
CHECK_HYGIENE_STAGE=$(abs_top_builddir)/.shared-check-stage \
321+
CHECK_HYGIENE_STAGE="$(abs_top_builddir)/.shared-check-stage" \
303322
CHECK_HYGIENE_SHARED=yes
304-
@rm -rf $(abs_top_builddir)/.shared-check-stage
323+
@rm -rf "$(abs_top_builddir)/.shared-check-stage"
305324

306325
# TASK-040: enforce static invariants on the two flagship examples
307326
# (hello_world.cpp <= 10 LOC, lambda-only; shared_state.cpp class+mutex).
@@ -386,7 +405,7 @@ lint-file-size:
386405
@echo "=== lint-file-size: enforce per-file line-count ceiling ==="
387406
@$(top_srcdir)/scripts/check-file-size.sh
388407

389-
.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
408+
.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
390409

391410
# TASK-039: top-level convenience rule that descends into test/ to
392411
# build and run the bench binaries (see test/Makefile.am and
@@ -401,7 +420,7 @@ MOSTLYCLEANFILES = $(DX_CLEANFILES) *.gcda *.gcno *.gcov
401420
DISTCLEANFILES = DIST_REVISION
402421

403422
clean-local:
404-
rm -rf $(CHECK_HYGIENE_STAGE) $(abs_top_builddir)/.shared-check-stage $(CHECK_INSTALL_STAGE)
423+
rm -rf "$(CHECK_HYGIENE_STAGE)" "$(abs_top_builddir)/.shared-check-stage" "$(CHECK_INSTALL_STAGE)"
405424

406425
pkgconfigdir = $(libdir)/pkgconfig
407426
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

@@ -107,6 +118,11 @@ int main() {
107118
std::fprintf(stderr, "LEAK: <pthread.h> reached the consumer TU (macOS/BSD guard _PTHREAD_H_)\n");
108119
++leaks;
109120
#endif
121+
122+
#ifdef _WINPTHREADS_H
123+
std::fprintf(stderr, "LEAK: <pthread.h> reached the consumer TU (MSYS2/MINGW64 guard _WINPTHREADS_H)\n");
124+
++leaks;
125+
#endif
110126
#endif // !_LIBCPP_VERSION && !_GLIBCXX_HAS_GTHREADS
111127

112128
#ifdef GNUTLS_GNUTLS_H

0 commit comments

Comments
 (0)