Closed Bug 1461383 Opened 2 years ago Closed 2 years ago

fix test_build.py for local builds

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

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.
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 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 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
https://hg.mozilla.org/mozilla-central/rev/73dd77918ce5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
See Also: → 1573845
You need to log in before you can comment on or make changes to this bug.