dhcpcd: pin the hook set so the image cannot depend on the build host - #139
Merged
Conversation
dhcpcd's configure chooses which dhcpcd-hooks to install by probing the BUILD HOST for ntpd/chronyd/systemd-timesyncd/ypbind unless it is given --with-hooks, and Buildroot's dhcpcd.mk passes none. A GitHub runner therefore shipped 50-ntp.conf while a developer box with timesyncd and no ntpd shipped 50-timesyncd.conf — both builds green. Found by diffing the master CI image against a local clean build. Fix: external.mk appends --with-hooks=ntp.conf --with-eghooks=yp.conf to DHCPCD_CONFIG_OPTS (legal: BR2_EXTERNAL .mk files are included after package/*/*.mk and the configure define expands at recipe time). The values reproduce the canonical CI image exactly; 'ntp.conf' is a hook stem (configure's find_hook matches NN-<stem>.conf; '50-ntp.conf' silently matches nothing). Verified: dhcpcd-dirclean + rebuild shows no host probes, config.mk HOOKSCRIPTS=50-ntp.conf, regenerated linux.img passes. scripts/check-linux-img.sh now asserts the exact hook set, fail-closed: passes on the master CI image, fails on the pre-fix local image naming 50-timesyncd.conf. docs/init-parity.md gains the row. Upstream-worthy follow-up: Buildroot's dhcpcd.mk should pass --with-hooks itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FYjrnzEEu4HzJSPWNAaRvm
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
scripts/check-linux-img.sh runs the new dhcpcd-hook assertion even when the debugfs rdump tree is invalid, producing misleading failures and should be guarded/skipped on dump failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Pins the dhcpcd hook set to make the DE10 image reproducible and independent of whatever daemons happen to be installed on the build host, and adds a CI-side guard to fail closed if the hook set drifts.
Changes:
- Append
--with-hooks=ntp.conf --with-eghooks=yp.conftoDHCPCD_CONFIG_OPTSviaexternal.mkto prevent host-probing from affecting installed hooks. - Add a
scripts/check-linux-img.shassertion that validates the exactdhcpcd-hooksfile set in the built image. - Document the pinned hook behavior in
docs/init-parity.md.
File summaries
| File | Description |
|---|---|
| scripts/check-linux-img.sh | Adds a fail-closed assertion for the expected dhcpcd-hooks set in linux.img. |
| external.mk | Forces a deterministic dhcpcd hook selection by passing explicit --with-hooks/--with-eghooks. |
| docs/init-parity.md | Documents the new host-independent dhcpcd hook pinning and its rationale. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
CI must also assert the exact example-hook set containing 50-yp.conf.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
…mp tree Copilot review on #139: on an unreadable debugfs dump the hook check reported 'no dhcpcd-hooks directory', masking the real fault. The assertion now lives inside the branch that has already proven the dumped tree is real, alongside the ssh_host_* scan. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FYjrnzEEu4HzJSPWNAaRvm
Copilot review on #139: --with-eghooks=yp.conf was pinned but unasserted, so a bump that stopped honouring it would change the canonical file set with a green build. /usr/share/dhcpcd/hooks must now be exactly 10-wpa_supplicant 15-timezone 29-lookup-hostname 50-yp.conf. Passes on the master CI image and on the pinned local image. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FYjrnzEEu4HzJSPWNAaRvm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The DE10 image's dhcpcd hook set depended on the build host. dhcpcd's own
configuredecides which hooks to install by probing the machine it runs on (which ntpd,which chronyd,/usr/lib/systemd/systemd-timesyncd,which ypbind) unless handed--with-hooks, and Buildroot'spackage/dhcpcd/dhcpcd.mkpasses none. So a GitHub runner shipped50-ntp.conf, and a developer machine with systemd-timesyncd and no ntpd shipped50-timesyncd.confinstead, with a green build both times. Found by diffing the master CI image (run 33648676862) against a local clean build: it was the single file-set difference.Fix (
external.mk): append--with-hooks=ntp.conf --with-eghooks=yp.conftoDHCPCD_CONFIG_OPTS. This is legal without patching Buildroot or dhcpcd: BR2_EXTERNAL.mkfiles are included afterpackage/*/*.mk, andDHCPCD_CONFIGURE_CMDSexpands$(DHCPCD_CONFIG_OPTS)when the recipe runs. The values reproduce the canonical CI image exactly (01-test,20-resolv.conf,30-hostname,50-ntp.confindhcpcd-hooks/;50-yp.confas the example under/usr/share/dhcpcd/hooks).50-ntp.confis the right hook: the image ships classicntpdfor stock parity, and the hook writes DHCP-offered servers into/etc/ntp.conf. Note the option takes hook stems:ntp.confmatches50-ntp.conf; passing50-ntp.confsilently matches nothing.Guard (
scripts/check-linux-img.sh, already in CI viaci-tests.sh): asserts the exact hook set, fail-closed. Passes on the master CI image; fails on the pre-fix local image with50-timesyncd.confnamed.Verified:
dhcpcd-dirclean+ rebuild logs no host probes andconfig.mkhasHOOKSCRIPTS=50-ntp.conf; the regeneratedlinux.imgpasses the checker.No defconfig change, so this does not conflict with #137. Upstream-worthy follow-up: Buildroot's
dhcpcd.mkshould pass--with-hooksitself.🤖 Generated with Claude Code
https://claude.ai/code/session_01FYjrnzEEu4HzJSPWNAaRvm