595926 - Purge last traces of REQUIRES from the build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Things like http://mxr.mozilla.org/mozilla-central/source/config/autoconf.mk.in#423 as well as a few uses of REQUIRES in mozilla proper are still lying around the tree.  Would be nice to kill them.

Comment 1

[Nathan Froyd [:froydnj]]

I see REQUIRES usage all over security/ (presumably for security/coreconf/ruleset.mk), but the link above has gone stale and grepping for REQUIRES indicates that it's not really used anymore (still set in a couple of places)?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We don't want to touch security/coreconf or security/nss.  There are still a few references in the rest of the tree though.

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v1)

I took a stab at this, it's not currently assigned... I'll take it if it's ok.

   Searching for REQUIRES, I found 87 files. Ignoring 2 security/coreconf, 66 security/nss, 9 with REQUIRES in comments only, 5 with REQUIRES as partial/part of different VAR names, leaves us with 5 files requiring changes:

   config/rules.mk
   js/src/config/rules.mk
   mobile/android/components/build/makefile.in
   mobile/xul/components/build/makefile.in
   toolkit/library/makefile.in

   The attached patch builds clean locally and passes mochitest-plain with a few of the usual timeouts. Further testing might be required. (I don't have access to TRY server yet.)  -- mark

Comment 4

[Gregory Szorc [:gps]]

Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=8412f7a8c599

Comment 5

[Mozilla RelEng Bot]

Try run for 8412f7a8c599 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8412f7a8c599
Results (out of 282 total builds):
    exception: 4
    success: 237
    warnings: 39
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8412f7a8c599

Comment 6

[Mark Capella [:capella]]

Well, the change seemed simple enough, but the TRY build has errors. I'm not very good at interpreting the results of these things yet... is it safe to ask for a checkin? Or should I have it pushed to TRY again?

Comment 7

[Phil Ringnalda (:philor)]

Yeah, interpreting those results is one of our many opaque things :)

The first thing to remember is that for any push to try that does -p all or -u all to get every sort of test on every platform, the summary posted in the bug will be completely and utterly worthless, totally devoid of any meaning, with only one useful thing in the entire bug comment, the link to tbpl.

Once you load the tbpl page for your push, the non-success things break down into four groups:

* things which are not there - your push got warnings on 14 Jetpack runs which are hidden because they are expected to be mostly broken and you aren't required to have them pass, so they are hidden both on try and on mozilla-central/mozilla-inbound, and warnings on 7 Peptest runs which don't even run on any other trees and pretty much never pass, and warnings on 7 debug and 4 opt OS X 10.7 test suites that are hidden on try and everywhere else because they always fail and nobody can be bothered to fix them. 32 of 39 warnings down already.

* things which are not Android, and which intermittently fail without breaking the whole test suite - click the orange letter, suggestions pop up, carefully compare the summary of the suggested bugs to your failure, maybe even load the bug itself, and if they match, click the outline of a star next to the right suggested bug, click the "add a comment" link, submit, and the failure is starred as a known intermittent

* things which are not Android, which intermittently fail but break the whole test suite - you have to go through the starring, but you also need to retrigger the test, since you didn't get all the tests run, for which you need at least the same L1 access that we should have gotten you well before now so that you can push to try yourself

* things which are Android - this is where it gets really difficult, and there are two main approaches: either just retrigger all the failures until they turn green, however long that takes, or, ask me to look at whether any of them look out of the ordinary. Both approaches are about equally popular, and equally reasonable. The third, by far the most popular, is to just completely ignore Android results, but then when you get backed out because you actually did break Android and broke it in a way that your try push showed, I mercilessly mock you ;)

I retriggered your Android tests that didn't manage to finish, just because I do, but your patch is fine, they'll green up (eventually, given enough retriggering).

Comment 8

[Mark Capella [:capella]]

Comment on attachment 602776 [details] [diff] [review]
Patch (v1)

Phil, I've saved those instructions in three different places ... I would not enjoy being mocked mercilessly :-P

Thanks though! Great information to have ... I've asked/flagged you for the eventual review+ ... after that I can do a checkin-needed or whatever is required ...

Depending on who I'm working with and and what areas of the code, there seems to be different protocols ... some folks want me to autoland to TRY then checkin-needed their requested bug patches, others prefer to take the patch and move it along the rest of the way without me ...

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 602776 [details] [diff] [review]
Patch (v1)

We're confusing in any number of directions, among them who will and won't review where. Every now and then I'll cheat and review a build patch without actually being a module peer, but since I never really did understand REQUIRES, I won't claim to understand its last little remains, just that we certainly did seem to build and test fine without them.

Comment 10

[Mark Capella [:capella]]

Ah... my mistake ... I should have gone back to the requester and/or checked the owners/peers : https://wiki.mozilla.org/Modules/Core ...

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 602776 [details] [diff] [review]
Patch (v1)

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

There's also some stuff in config/autoconf.mk.in too, I think.  Stuff like JPEG_REQUIRES, etc.

Comment 12

[Mark Capella [:capella]]

Attachment: Patch (v2)

That's one of the (5) files I wasn't sure about in comment #3 with REQUIRES as partial/part of different VAR names ...

I've updated the patch and am asking for a new review ... then I can AUTOLAND it to TRY -- mark

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 603900 [details] [diff] [review]
Patch (v2)

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

Thanks for the patch!

Comment 14

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 603900
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=36189206b464
Try run started, revision 36189206b464. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=36189206b464

Comment 15

[Mozilla RelEng Bot]

Try run for 36189206b464 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=36189206b464
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-36189206b464

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f2d6d32de1c1

Comment 17

[Matt Brubeck (:mbrubeck)]

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