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
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.
There was a problem hiding this comment.
Comment rewritten to cover both build systems: under the default BUILD_SYS=uspace the realtime .d files land in objects/ too and depends/ is never created, so on that build the old depclean was a complete no-op (it rm -rf'd a directory that never existed). Also dropped "userspace" from the modclean comment, and added a clause explaining why the recipe is a find and not $(RM) $(DEPS) $(RTDEPS) (the files that break the build are exactly the ones absent from $(DEPS), and $(DEPS) only exists when TRIVIAL_BUILD=no). The commit message and PR description are de-overclaimed the same way ("obvious remedy", not "documented"), and the description now makes the no-op point.
Revalidated on a uspace RIP tree: 535 objects/**/*.d (311 realtime), no depends/; make depclean takes that to 0; a follow-up make default recompiles 0 objects; and the stale-.d build abort still reproduces and is still cleared by depclean.
| # 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.
230fa08 to
c3e266f
Compare
depclean removed only 'depends'. Under BUILD_SYS=normal that directory
holds the realtime dependency files; under the default BUILD_SYS=uspace
'depends' is never created at all. Every other dependency file is
written next to its object as objects/**/*.d -- realtime included on a
uspace build -- and depclean never touched them. On the default build
that made depclean a complete no-op: it rm -rf'd a directory that never
existed.
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 obvious remedy is 'make depclean', and it did not help: 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.
The comment above modclean is corrected as well: 'clean' does remove the
dependency files today, because genclean deletes objects/ wholesale.
2 participants
Problem
src/Makefile'sdepcleantarget removes onlydepends. UnderBUILD_SYS=normalthat directory holds the realtime dependency files; underthe default
BUILD_SYS=uspaceit is never created at all. Every otherdependency file is written next to its object as
objects/**/*.d— seeTODEPS(Makefile:229) and the-MF "${@:.o=.d}"compile rules (Makefile:291,301, 309, 317 for userspace; 1359, 1369, 1384 for realtime) — and
depcleannever touched them. A run-in-place tree here keeps 535 of them, 311 of which
are realtime
objects/rt*.d.So on the default
uspacebuilddepcleanwas not merely incomplete — it wasa complete no-op: it
rm -rf'd adepends/directory that never existed.The target's own comment claimed otherwise ("
cleancleans everything butdependency files, and
depcleancleans them too"), and it is wrong in bothdirections:
cleandoes remove the dependency files, becausegencleandeletes
objects/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 obvious remedy is
make depclean— and it does not work. The only way outis
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 comments.It stays a
findrather than$(RM) $(DEPS) $(RTDEPS)on purpose: the filesthat break the build are exactly the ones whose source no longer exists, so
they are absent from
$(DEPS); and$(DEPS)is only defined whenTRIVIAL_BUILD=no. The pure-make form would not fix the bug.Removing the files costs no recompilation: nothing takes a
.das aprerequisite, and
UNREAD_DEPS(Makefile:255) is computed but never used. Theonly thing given up is header-dependency tracking until each object is next
rebuilt, which is precisely what asking for
depcleanmeans.Verification (uspace RIP tree, default build)
objects/**/*.din a fully-built tree, 311 of them realtimeobjects/rt*.d; nodepends/directory at all.make depclean: 535 -> 0,depends/still absent.make defaultimmediately after, on the up-to-date tree: 0.orecompiled.
.dabort reproduced verbatim — plantedobjects/hal/utils/halrmt.o: hal/utils/halrmt_GONE.c,make objects/hal/utils/halrmt.ofailed withNo rule to make target 'hal/utils/halrmt_GONE.c'(exit 2);make depcleanthen cleared it and thetarget built with no abort.