Closed
Bug 851975
Opened 12 years ago
Closed 12 years ago
ifdef ENABLE_TESTS in backend.mk always false
Categories
(Firefox Build System :: General, defect, P1)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla22
People
(Reporter: gps, Assigned: gps)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Attachment #725950 -
Flags: review?(mh+mozilla)
Attachment #725950 -
Flags: review?(khuey)
Assignee | ||
Comment 3•12 years ago
|
||
Pushed straight to central per philor's recommendation.
https://hg.mozilla.org/mozilla-central/rev/a9a25781850e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 4•12 years ago
|
||
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.)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•