Closed Bug 595926 Opened 11 years ago Closed 10 years ago

Purge last traces of REQUIRES from the build system


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: khuey, Assigned: capella)


(Whiteboard: [good first bug])


(1 file, 1 obsolete file)

Things like as well as a few uses of REQUIRES in mozilla proper are still lying around the tree.  Would be nice to kill them.
I see REQUIRES usage all over security/ (presumably for security/coreconf/, 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)?
We don't want to touch security/coreconf or security/nss.  There are still a few references in the rest of the tree though.
Attached patch Patch (v1) (obsolete) — Splinter Review
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:


   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
Assignee: nobody → markcapella
Try run for 8412f7a8c599 is complete.
Detailed breakdown of the results available here:
Results (out of 282 total builds):
    exception: 4
    success: 237
    warnings: 39
    failure: 2
Builds (or logs if builds failed) available at:
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?
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 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 ...
Attachment #602776 - Flags: review?(philringnalda)
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.
Attachment #602776 - Flags: review?(philringnalda) → review?(khuey)
Ah... my mistake ... I should have gone back to the requester and/or checked the owners/peers : ...
Comment on attachment 602776 [details] [diff] [review]
Patch (v1)

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

There's also some stuff in config/ too, I think.  Stuff like JPEG_REQUIRES, etc.
Attachment #602776 - Flags: review?(khuey) → review+
Attached patch Patch (v2)Splinter Review
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
Attachment #602776 - Attachment is obsolete: true
Attachment #603900 - Flags: review?(khuey)
Comment on attachment 603900 [details] [diff] [review]
Patch (v2)

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

Thanks for the patch!
Attachment #603900 - Flags: review?(khuey) → review+
Whiteboard: [good first bug] → [autoland-try][good first bug]
Whiteboard: [autoland-try][good first bug] → [autoland-in-queue][good first bug]
Autoland Patchset:
	Patches: 603900
	Branch: mozilla-central => try
Try run started, revision 36189206b464. To cancel or monitor the job, see:
Try run for 36189206b464 is complete.
Detailed breakdown of the results available here:
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
Whiteboard: [autoland-in-queue][good first bug] → [good first bug]
Keywords: checkin-needed
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.