1393607 - --disable-tests fails in automation

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Tom Ritter [:tjr]]

It's failing with "No rule to package-tests" - I tracked this as far as the installer getting confused about what system it's on (I believe package-tests is called as part of building an RPM).

For now I just disabled the installer in automation.

For lack of a better place to put here, here is the whole log: https://ritter.vg/misc/stuff/mingw-installer-error-log.txt

Comment 1

[Molly Howell (she/her) (mostly inactive)]

(In reply to Tom Ritter [:tjr] from comment #0)
> It's failing with "No rule to package-tests" - I tracked this as far as the
> installer getting confused about what system it's on (I believe
> package-tests is called as part of building an RPM).

Could you expand on that? I'm not sure where that confusion would be showing up at, and I'm not set up to be able to reproduce this.

Comment 2

[Tom Ritter [:tjr]]

Ah(In reply to Matt Howell [:mhowell] from comment #1)
> (In reply to Tom Ritter [:tjr] from comment #0)
> > It's failing with "No rule to package-tests" - I tracked this as far as the
> > installer getting confused about what system it's on (I believe
> > package-tests is called as part of building an RPM).
> 
> Could you expand on that? I'm not sure where that confusion would be showing
> up at, and I'm not set up to be able to reproduce this.

Ah thanks! Disabling the installer did not solve this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4ae3e55e7cbdcbf7d6e74ef87f6f6148c2a7cb6&selectedJob=125689104 so I think I can't avoid fixing it.

Error is:

> INFO -  gmake[4]: *** No rule to make target 'package-tests'.  Stop.

I saw this in two places:

1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/installer/linux/rpm/mozilla.spec#81
This is the RPM builder

2) http://searchfox.org/mozilla-central/source/build/moz-automation.mk#62
And now I see this is the real source of the error now (pretty sure). So ignore that RPM stuff.

But it's trying to run package-tests and it's defined in http://searchfox.org/mozilla-central/source/testing/testsuite-targets.mk (and the RPM spec file but ignore that one)

I'm running with --disable-tests so presumably that's the problem... So I need to fix/change/disable something to fix --disable-tests I'm guessing?

Comment 3

[Molly Howell (she/her) (mostly inactive)]

Okay, thanks for the details. In that case, this doesn't look installer-related anymore. I'm going to shuffle this off to the build config component, that seems more relevant to what you're seeing.

Comment 4

[Tom Ritter [:tjr]]

So the problem is that in Makefile.in we only include $(topsrcdir)/testing/testsuite-targets.mk if ENABLE_TESTS - but in automation we expect the package-tests target to be available.

Comment 5

[Tom Ritter [:tjr]]

Attachment: Bug 1393607 Do not package tests if they are not enabled

Review commit: https://reviewboard.mozilla.org/r/172712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172712/

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 8901259 [details]
Bug 1393607 Do not package tests if they are not enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172712/diff/1-2/

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8901259 [details]
Bug 1393607 Do not package tests if they are not enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172712/diff/2-3/

Comment 8

[Tom Ritter [:tjr]]

Try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eef1994e5577bb3df26a86dbe52f197ba9e408d

Comment 9

[(not currently active) Ted Mielczarek]

Punting this review to mshal, who wrote all of the automation-targets stuff originally.

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8901259 [details]
Bug 1393607 Do not package tests if they are not enabled

https://reviewboard.mozilla.org/r/172712/#review179108

I like the idea of matching up ENABLE_TESTS with MOZ_AUTOMATION_PACKAGE_TESTS, since as you noticed, trying to set the latter while the former is disabled does not work. However, I don't think this is the right way to implement it. We can do one of the following:

1) In moz-automation.mk towards the top, just do (untested):

ifdef ENABLE_TESTS
# We can't package tests if they aren't enabled.
MOZ_AUTOMATION_PACKAGE_TESTS = 0
endif

However, this means we may be silently overriding MOZ_AUTOMATION_PACKAGE_TESTS, which could be confusing since the mozconfig would think it is 1.

2) Alternatively, we could just error out in the Makefile if MOZ_AUTOMATION_PACKAGE_TESTS is set and ENABLE_TESTS isn't (untested):

ifdef ENABLE_TESTS
ifeq (1,$(MOZ_AUTOMATION_PACKAGE_TESTS))
$(error MOZ_AUTOMATION_PACKAGE_TESTS is set but we have --disable-tests. Try setting MOZ_AUTOMATION_PACKAGE_TESTS=0 in your mozconfig or enabling tests.)
endif
endif

This would direct you to set MOZ_AUTOMATION_PACKAGE_TESTS=0 in your mozconfig to skip the package-tests step, but involves more manual work.

3) Unify ENABLE_TESTS with MOZ_AUTOMATION_PACKAGE_TESTS:
 - Remove MOZ_AUTOMATION_PACKAGE_TESTS in build/mozconfig.automation and all mozconfigs
 - add a block like this to moz-automation.mk:
ifdef ENABLE_TESTS
MOZ_AUTOMATION_PACKAGE_TESTS = 1
else
MOZ_AUTOMATION_PACKAGE_TESTS = 0
endif

This would make it impossible to build tests without packaging them, or try to package them without building them.  However, we currently have some build configurations (static analysis builds) that build tests but don't package them. I'm not sure if that's intentional, so this would require more investigation.

Right now I'm leaning towards 1) - let me know if that works for you. Feel free to ping me if it's not behaving as intended.

::: build/moz-automation.mk:38
(Diff revision 3)
>  
>  # Automation build steps. Everything in MOZ_AUTOMATION_TIERS also gets used in
>  # TIERS for mach display. As such, the MOZ_AUTOMATION_TIERS are roughly sorted
>  # here in the order that they will be executed (since mach doesn't know of the
>  # dependencies between them).
> -moz_automation_symbols = \
> +moz_automation_symbols =

This just lists the set of available post-automation steps. The way they are enabled or disabled is by setting eg: MOZ_AUTOMATION_PACKAGE_TESTS to 1 or 0. So, you don't need to ifdef it here to remove it - just set the flag to 0.

::: build/moz-automation.mk:65
(Diff revision 3)
>  automation/l10n-check: automation/package
>  automation/l10n-check: automation/installer
>  
>  automation/upload: automation/installer
>  automation/upload: automation/package
> +ifdef ENABLE_TESTS

Similarly, these dependencies can exist even if a step is disabled by setting a MOZ_AUTOMATION_* variable to 0. The automation/package-tests rule might still trigger, but that's ok because the actual rule in the end of the file will skip all action if the flag is disabled. So we don't need to wrap this dependency in an ifdef.

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 8901259 [details]
Bug 1393607 Do not package tests if they are not enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172712/diff/3-4/

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8901259 [details]
Bug 1393607 Do not package tests if they are not enabled

https://reviewboard.mozilla.org/r/172712/#review179532

LGTM - thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/017a3c4acb5c
Do not package tests if they are not enabled r=mshal

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/017a3c4acb5c