851975 - ifdef ENABLE_TESTS in backend.mk always false

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Don't use ifdefs in backend.mk, v1

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...

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 725950 [details] [diff] [review]
Don't use ifdefs in backend.mk, v1

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

Ouch. :-/

Comment 3

[Gregory Szorc [:gps]]

Pushed straight to central per philor's recommendation.

https://hg.mozilla.org/mozilla-central/rev/a9a25781850e

Comment 4

[Gregory Szorc [:gps]]

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.)

Comment 5

[Gregory Szorc [:gps]]

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

Comment 6

[Mozilla RelEng Bot]

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/[e-mail]-212b3c4d231d