Closed
Bug 1461383
Opened 6 years ago
Closed 6 years ago
fix test_build.py for local builds
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(1 file)
2.00 KB,
patch
|
chmanchester
:
review+
mshal
:
feedback+
|
Details | Diff | Splinter Review |
test_build.py fails on local builds with messages like: make[4]: Entering directory '/tmp/tmp65BjCH' make[4]: *** No rule to make target 'buildid.h'. Stop. make[4]: Leaving directory '/tmp/tmp65BjCH' /home/froydnj/src/gecko-dev.git/config/faster/rules.mk:76: recipe for target '/tmp/tmp65BjCH/buildid.h' failed make[3]: *** [/tmp/tmp65BjCH/buildid.h] Error 2 make[3]: *** Waiting for unfinished jobs.... make[4]: Entering directory '/tmp/tmp65BjCH' make[4]: *** No rule to make target 'source-repo.h'. Stop. make[4]: Leaving directory '/tmp/tmp65BjCH' /home/froydnj/src/gecko-dev.git/config/faster/rules.mk:76: recipe for target '/tmp/tmp65BjCH/source-repo.h' failed make[3]: *** [/tmp/tmp65BjCH/source-repo.h] Error 2 make[3]: Leaving directory '/tmp/tmp65BjCH/faster' Makefile:155: recipe for target 'faster' failed The tests pass in automation, however, because automation always defines the rules for buildid.h and source-repo.h in the toplevel Makefile.in. For local builds, however, those rules are not defined to avoid build churn. Let's ensure that the necessary rules are defined during testing as well.
Assignee | ||
Comment 1•6 years ago
|
||
We talked about this over IRC, so I figured you were a reasonable candidate to review this. I'm not *totally* sure about the commit message; I'm not entirely sure how the GENERATED_FILES bits in the root moz.build for buildid.h and source-repo.h play into this...
Attachment #8975542 -
Flags: review?(mshal)
Comment 2•6 years ago
|
||
Comment on attachment 8975542 [details] [diff] [review] fix test_build.py for local builds Redirecting to chmanchester. Although this approach works, I'm not really sure if it's the right way to do it. It looks like faster/rules.mk has the install manifests depend on buildid.h + source-repo.h in [1], which triggers the rule at [2], and that fails because we don't have a buildid.h rule generated in backend.mk for the faster-make backend. It seems to me like the FORCE dependency in the top-level Makefile that makes this work is a happy accident, and we probably need to revisit the assumptions in faster/rules.mk so that they reflect how buildid.h is now built as a result of bug 1298328. (ie: do we still need buildid.h/source-repo.h as explicit dependencies here now that they are generated differently? Or do we need to update the rule in [2] to accommodate them? Or should buildid.h+source-repo.h rules be generated in the faster-make backend?). Unfortunately I don't know the answer to this, so hopefully chmanchester does :) [1] https://dxr.mozilla.org/mozilla-central/rev/45ec8fd380dd2c308e79dbb396ca87f2ce9b3f9c/config/faster/rules.mk#82 [2] https://dxr.mozilla.org/mozilla-central/rev/45ec8fd380dd2c308e79dbb396ca87f2ce9b3f9c/config/faster/rules.mk#73 >diff --git a/Makefile.in b/Makefile.in >index f08bb62..b573c8d 100644 >--- a/Makefile.in >+++ b/Makefile.in >@@ -30,16 +30,23 @@ DIST_GARBAGE = config.cache config.log config.status* config-defs.h \ > netwerk/necko-config.h xpcom/xpcom-config.h xpcom/xpcom-private.h \ > .mozconfig.mk > > ifndef MOZ_PROFILE_USE > # Automation builds should always have a new buildid, but for the sake of not > # re-linking libxul on every incremental build we do not enforce this for > # developer builds. > ifneq (,$(MOZ_AUTOMATION)$(MOZ_BUILD_DATE)) >+needs_buildid_rule := 1 >+endif >+# Tests need a new buildid always. >+ifdef TEST_MOZBUILD >+needs_buildid_rule := 1 >+endif >+ifdef needs_buildid_rule If we do go with this approach, I think this could be simplified by doing: ifneq (,$(MOZ_AUTOMATION)$(MOZ_BUILD_DATE)$(TEST_MOZBUILD)) Since the ifneq construction is effectively an 'if A or B or C'
Attachment #8975542 -
Flags: review?(mshal)
Attachment #8975542 -
Flags: review?(cmanchester)
Attachment #8975542 -
Flags: feedback+
Comment 3•6 years ago
|
||
Comment on attachment 8975542 [details] [diff] [review] fix test_build.py for local builds Review of attachment 8975542 [details] [diff] [review]: ----------------------------------------------------------------- I don't have great answers for mshal's questions about faster-make and buildid.h, but removing those buildid.h/source-repo.h dependencies in faster/rules.mk isn't entirely straightforward. This patch is a fine workaround (I agree with Mike's comment about avoiding the new variable), and in the context of the test I don't think it's doing any harm.
Attachment #8975542 -
Flags: review?(cmanchester) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/73dd77918ce5 fix test_build.py for local builds; r=chmanchester,f=mshal
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73dd77918ce5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•