build: make depclean remove the userspace dependency files too - #4419
build: make depclean remove the userspace dependency files too#4419greatEndian wants to merge 1 commit into
Conversation
depclean removed only 'depends', which holds the realtime dependency
files. The userspace ones are written next to their objects as
objects/**/*.d (see TODEPS and the -MF "${@:.o=.d}" compile rules), and
survived it -- 535 of them in a typical run-in-place tree.
That matters when a source file is renamed or moved. A stale dependency
file still declares, say
objects/hal/utils/halrmt.o: hal/utils/halrmt.c
and gcc's -MP writes dummy targets for the *headers* only, never for the
main source, so once hal/utils/halrmt.c is gone nothing can satisfy that
prerequisite and the whole build stops with
make: *** No rule to make target 'hal/utils/halrmt.c',
needed by 'objects/hal/utils/halrmt.o'. Stop.
The documented remedy for stale dependencies is exactly this target, and
it did not work: the only way out was 'make clean' and a full rebuild.
Hit in practice on a run-in-place tree carried across the halrmt.c ->
halrmt.cc rename and the src/libnml/posemath -> src/libposemath move;
eight dependency files pointed at sources that no longer existed.
Removing the files costs no recompilation -- nothing has a .d as a
prerequisite, and UNREAD_DEPS is computed but never used -- so this only
gives up header-dependency tracking until each object is next rebuilt,
which is what asking for depclean means.
Verified by planting a dependency file naming a since-renamed source,
reproducing the failure verbatim, and confirming that depclean did not
clear it before this change and does after, with the build then
completing normally.
The comment above modclean is corrected as well: 'clean' does remove the
userspace dependency files today, because genclean deletes objects/
wholesale.
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
Mechanism checks out, and I reproduce the premise on a run-in-place tree: no depends/ directory at all, 533 objects/**/*.d surviving depclean. Nothing takes a .d as a prerequisite (they are only -included at Makefile:248 and Makefile:1436), so the no-forced-rebuild claim holds, and the find ... | xargs rm -f shape matches modclean and genclean directly above it in a tighter form, being scoped to objects and using -print0.
Three comments inline, none of them blocking. One more nit for the description rather than the diff: depclean appears nowhere in the tree outside src/Makefile, so "the documented remedy for stale dependency information" reads as an overclaim.
| # 'clean' removes the build products (including, via 'objects', the userspace | ||
| # dependency files), and 'depclean' removes the dependency files alone. |
There was a problem hiding this comment.
"userspace" undersells it: under BUILD_SYS=uspace, which is the default, objects/ holds the realtime dependency files as well (see the inline comment below). Dropping the word would keep this sentence true for both build systems.
| # Realtime dependency files live in 'depends'; userspace ones sit next to | ||
| # their objects as objects/**/*.d, so both have to go. Dropping them forces |
There was a problem hiding this comment.
This is only true for BUILD_SYS=normal. Under uspace the realtime dependency files land under objects/ too:
# Makefile:1292, BUILD_SYS=uspace
RTDEPS := $(sort $(RTOBJS:.o=.d))
# Makefile:1330, BUILD_SYS=normal
RTDEPS := $(sort $(patsubst objects/%.o,depends/%.d, $(RTOBJS)))On my uspace run-in-place tree there is no depends/ directory at all, and 310 of the 533 objects/**/*.d are objects/rt*. So on the default build depclean is not partially ineffective, it is a complete no-op: it removes a directory that was never created. That is a stronger case for the patch than the description makes, and since the point of the patch is to correct a comment that was wrong, this one should be right. Suggested wording:
| # Realtime dependency files live in 'depends'; userspace ones sit next to | |
| # their objects as objects/**/*.d, so both have to go. Dropping them forces | |
| # Dependency files land in two places: 'depends' for realtime under | |
| # BUILD_SYS=normal, and beside their objects as objects/**/*.d for | |
| # everything else, realtime included under BUILD_SYS=uspace, where | |
| # 'depends' is never created at all. Both have to go. |
The description splits the same way, counting all 535 as userspace.
| # target", and there is no way out of that short of a full 'make clean'. | ||
| depclean: | ||
| -rm -rf depends | ||
| -find objects -name '*.d' -print0 2>/dev/null | xargs -0 -r rm -f |
There was a problem hiding this comment.
Worth saying in the comment why this is a find and not $(RM) $(DEPS) $(RTDEPS), because the pure-make form is the obvious suggestion and it would not fix the bug: the files that break the build are exactly the ones whose source no longer exists, so they are absent from $(DEPS). DEPS is also only defined when TRIVIAL_BUILD=no. One clause here saves a review round.
2 participants
Problem
src/Makefile'sdepcleantarget removes onlydepends, which holds therealtime dependency files. The userspace ones are written next to their
objects as
objects/**/*.d— seeTODEPS(Makefile:229) and the-MF "${@:.o=.d}"compile rules (Makefile:291, 301, 309, 317) — and surviveit. A typical run-in-place tree keeps 535 of them.
The target's own comment claims otherwise ("
cleancleans everything butdependency files, and
depcleancleans them too"), and that comment is wrongin both directions:
cleandoes remove the userspace dependency files,because
gencleandeletesobjects/wholesale.Why it matters
A stale dependency file still declares its original source:
gcc's
-MPwrites dummy targets for the headers only, never for the mainsource, so once
hal/utils/halrmt.chas been renamed the prerequisite cannotbe satisfied and the entire build stops:
The documented remedy for stale dependency information is
make depclean—and it does not work. The only way out is
make cleanand a full rebuild.This is not hypothetical. It was hit on a run-in-place tree carried across two
upstream changes: the
halrmt.c->halrmt.ccrename, and thesrc/libnml/posemath->src/libposemathmove. Eight dependency files pointedat sources that no longer existed, and the tree could not be built at all.
Fix
Have
depcleanremoveobjects/**/*.das well, and correct the comment.Removing them costs no recompilation: nothing takes a
.das a prerequisite,and
UNREAD_DEPS(Makefile:255) is computed but never used. The only thinggiven up is header-dependency tracking until each object is next rebuilt,
which is precisely what asking for
depcleanmeans.Verification
failure verbatim, including the
-MPsubtlety (a first attempt that includeda dummy rule for the main source did not reproduce it, which is what
confirmed the mechanism).
make depcleanleft all 535objects/**/*.din place andthe build still aborted.
make depcleanremoved all 535, and the previously fataltarget built normally.
make defaultimmediately after a patcheddepcleanon an up-to-date tree recompiled 0 files.