Closed Bug 851975 Opened 7 years ago Closed 7 years ago

ifdef ENABLE_TESTS in backend.mk always false

Categories

(Firefox Build System :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Well, this is embarrassing.

In backend.mk, we write out TEST_TOOL_DIRS as:

ifdef ENABLE_TESTS
TOOL_DIRS += <value>
endif

Unfortunately, backend.mk is included before config.mk. So, ENABLE_TESTS is *never* defined. So, essentially these directories have not been built since the moz.build switchover on February 28. Let's only hope there haven't been any regressions.

https://mxr.mozilla.org/mozilla-central/search?string=TEST_TOOL_DIRS reveals affected files. Patch will come shortly.
This is the simplest solution. We keep backend.mk static.

An alternative solution would be to move the include of backend.mk to after the inclusion of config.mk in rules.mk.

If config.status changes, a moz.build regeneration will be triggered. Thus, if ENABLE_TESTS changes, backend.mk should be updated automagically.

Broadcasting review to whoever gets it first.

https://tbpl.mozilla.org/?tree=Try&rev=212b3c4d231d

If we regressed tests while there wasn't test coverage, this could be very painful...
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #725950 - Flags: review?(ted)
Attachment #725950 - Flags: review?(mh+mozilla)
Attachment #725950 - Flags: review?(khuey)
Comment on attachment 725950 [details] [diff] [review]
Don't use ifdefs in backend.mk, v1

Review of attachment 725950 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch. :-/
Attachment #725950 - Flags: review?(ted) → review+
Attachment #725950 - Flags: review?(mh+mozilla)
Attachment #725950 - Flags: review?(khuey)
Pushed straight to central per philor's recommendation.

https://hg.mozilla.org/mozilla-central/rev/a9a25781850e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
OK, so this isn't as bad as I thought. This only has an impact if the Makefile.in was not including config.mk before rules.mk. I have yet to audit the full list, but /content/base/Makefile.in was definitely affected. (This is what caused the mochitest failure in bug 844635.)
Blocks: 846425
No longer blocks: 846825
Hmmm. I may have goofed here. I made it about halfway through auditing the Makefile.in seemingly affected by this and every one of them included autoconf.mk. Since autoconf.mk defines ENABLE_TESTS, this patch should have no effect.

Since I was fooling around with bug 844635 when I filed this bug, I suspect the state of my tree had something to do with this. In that bug, the auto-generated Makefile don't include autoconf.mk before rules.mk. And if autoconf.mk isn't included, ENABLE_TESTS isn't defined and thus this bug. I suspect I ran a |make| against a tree with auto-generated Makefiles, noticed the 'tests' directory wasn't being traversed, looked at backend.mk, and concluded "oh crap, ENABLE_TESTS is never defined before backend.mk is evaluated." *sigh*. It happens.

So, unless there was a Makefile.in not including autoconf.mk, all I did in this bug was fix the issue preventing bug 844635 from landing. Yay, I guess?

So, in the words of Frank Drebin [1], "there's nothing to see here."

[1] https://www.youtube.com/watch?v=5NNOrp_83RU
Try run for 212b3c4d231d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=212b3c4d231d
Results (out of 213 total builds):
    exception: 79
    success: 123
    warnings: 1
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-212b3c4d231d
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.