Closed Bug 1393607 Opened 2 years ago Closed 2 years ago

--disable-tests fails in automation

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file)

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
(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.
Flags: needinfo?(tom)
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?
Flags: needinfo?(tom) → needinfo?(mhowell)
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.
Component: Installer → Build Config
Flags: needinfo?(mhowell)
Product: Firefox → Core
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.
Summary: Installer fails to build in MinGW build on Linux for Windows → --disable-tests fails in automation
Assignee: nobody → tom
Attachment #8901259 - Flags: review?(ted) → review?(mshal)
Punting this review to mshal, who wrote all of the automation-targets stuff originally.
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.
Attachment #8901259 - Flags: review?(mshal) → review-
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!
Attachment #8901259 - Flags: review?(mshal) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/017a3c4acb5c
Do not package tests if they are not enabled r=mshal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/017a3c4acb5c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.