Committing Rules and Responsibilities
Gervase Markham
Mitchell asked for a "Committing Rules and Responsibilities" document,
to try and pull together information scattered across various wiki
pages, tinderbox headers and so on. Here's a second draft:
https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities
Comments very welcome :-) Hopefully one day every tinderbox will just
have two links at the top - this one, and a link to a list of rules
specific to that tree.
Gerv
Dave Townsend
It is probably worth making some note about what comments are
appropriate for merges including the cases where you are merging a
series of changes from a different tree, merging because you lost the
push race and merging a backout.
Mark Banner
Merging because you lost a push race shouldn't be necessary, especially
now that hg has the --rebase command.
Standard8
Mark Banner
> have two links at the top - this one, and a link to a list of rules
> specific to that tree.
Comments mainly from a comm-central/Thunderbird perspective.
- comm-central has a different try server, see
https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer
- Reviews. Here we really need to pull together the existing review
requirements for different modules/code. We should at least do
mozilla-central and comm-central.
For comm-central, I've pulled the requirements together here:
https://developer.mozilla.org/en/comm-central#comm-central_tree_rules
For mozilla-central, I have no idea where these are. There was one for
toolkit somewhere on mozilla.org but I've lost the link now and where
the summary for the rest of it was, I've got no idea. I guess people
just use module-owners most of the time.
- Check Tinderbox. comm-central apps (and others) don't have a sheriff
rota, #developers is also the wrong pointer for most of them. I suggest
just pointing to look at the header at the top of the tinderbox page.
- Extra Source Documents:
http://tinderbox.mozilla.org/Thunderbird3.0/
https://wiki.mozilla.org/MailNews:Tinderbox_Numbers_Explanation
https://developer.mozilla.org/en/comm-central
Standard8
L. David Baron
A few unrelated comments:
It would be useful to link "super-reviews" to the document
describing super-review rules.
As far as "checkin comments" go, it's probably also worth suggesting
that a checkin comment should describe what changes are being made,
not just the summary of what they fix, e.g.:
BAD: Fix bug 123456 - crash clicking button on www.example.com
GOOD: Null-check pres shell so we don't crash when a button
removes itself during its own onclick handler. (Bug 123456)
since the latter type of comments are much easier to spot as likely
causes of regressions or likely causes of fixing other bugs when
we're searching through checkin logs.
It would probably be good to indicate that tinderboxpushlog is
generally preferred to tinderbox where it is available.
(http://tests.themasta.com/tinderboxpushlog/ , etc.)
The "Check Tinderbox" section could probably use some pointers to
how to find out if orange is a known random orange.
"Once you have checked in, you need to watch the tree and make sure
the next cycle for every machine is green." isn't quite true
anymore, since we have multiple machines doing the same thing, and
we only need to wait for one of each. (For talos, those machines
show up in different columns; for other things we're putting more
than one machine in the same column.)
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Gervase Markham
Thanks for this - very helpful.
On 06/07/09 23:30, L. David Baron wrote:
> It would probably be good to indicate that tinderboxpushlog is
> generally preferred to tinderbox where it is available.
> (http://tests.themasta.com/tinderboxpushlog/ , etc.)
Preferred in what sense? It's not a superset of tinderbox, because it
doesn't have e.g. the header with the tree state and checkin rules. So I
can't just point everyone there instead.
> The "Check Tinderbox" section could probably use some pointers to
> how to find out if orange is a known random orange.
Where might one find that information? :-) It seems to me from looking
at the headers of the current tinderbox that it's currently scattered
randomly. For example, are we still collecting random oranges on a page
called "November 2008 Orange Compendium"?
> "Once you have checked in, you need to watch the tree and make sure
> the next cycle for every machine is green." isn't quite true
> anymore, since we have multiple machines doing the same thing, and
> we only need to wait for one of each. (For talos, those machines
> show up in different columns; for other things we're putting more
> than one machine in the same column.)
What algorithm can one use to reliably determine when one can stop
watching? All the tinderboxen have different titles, so I need guidance
on which differences are significant and which are not.
Gerv
Gervase Markham
> It is probably worth making some note about what comments are
> appropriate for merges including the cases where you are
> b) merging because you lost the push race
> c) merging a backout.
Would you care to suggest text for each of these three situations (or
two, if you accept Mark's point)?
Gerv
Boris Zbarsky
> On 06/07/09 23:30, L. David Baron wrote:
>> It would probably be good to indicate that tinderboxpushlog is
>> generally preferred to tinderbox where it is available.
>> (http://tests.themasta.com/tinderboxpushlog/ , etc.)
>
> Preferred in what sense?
In the sense of actually being usable to answer questions like "did my
checkin break the tree?" and "what failed with my checkin?" and "is my
checkin in the clear so I can stop watching the tree now?"
You know, those little things that are your responsibility to figure out
answers to after checking in.
> It's not a superset of tinderbox, because it
> doesn't have e.g. the header with the tree state and checkin rules.
There's been talk of adding "tree state" (if you mean open/closed), but
that's much less important now that we have the commit hook. I, for
one, no longer look at tinderbox before checking in: I look at
tinderboxpushlog and assume that if the tree is closed my push will just
fail due to the hook.
> So I can't just point everyone there instead.
I'm not sure why not....
> Where might one find that information? :-) It seems to me from looking
> at the headers of the current tinderbox that it's currently scattered
> randomly. For example, are we still collecting random oranges on a page
> called "November 2008 Orange Compendium"?
The canonical place to find random orange is on the "depends on" list of
the bug with alias "randomorange", last I checked.
>> "Once you have checked in, you need to watch the tree and make sure
>> the next cycle for every machine is green." isn't quite true
>> anymore, since we have multiple machines doing the same thing, and
>> we only need to wait for one of each. (For talos, those machines
>> show up in different columns; for other things we're putting more
>> than one machine in the same column.)
>
> What algorithm can one use to reliably determine when one can stop
> watching?
Using tinderboxpushlog, when the number of green builds of that type
after your checkin matches the number of boxes in the summary display
might work....
-Boris
Robert Kaiser
> It would probably be good to indicate that tinderboxpushlog is
> generally preferred to tinderbox where it is available.
> (http://tests.themasta.com/tinderboxpushlog/ , etc.)
It just doesn't work as reliable as the plain "dumb" tinderbox waterfall
in my experience, as it needs more requests from the network and more
local browser power.
Robert Kaiser
Gervase Markham
> In the sense of actually being usable to answer questions like "did my
> checkin break the tree?" and "what failed with my checkin?" and "is my
> checkin in the clear so I can stop watching the tree now?"
So the idea is, you watch the letters next to your checkin, and when
they all go from grey to green, you are off the hook? And if any go
orange, you investigate?
>> It's not a superset of tinderbox, because it doesn't have e.g. the
>> header with the tree state and checkin rules.
>
> There's been talk of adding "tree state" (if you mean open/closed), but
> that's much less important now that we have the commit hook. I, for one,
> no longer look at tinderbox before checking in: I look at
> tinderboxpushlog and assume that if the tree is closed my push will just
> fail due to the hook.
I didn't realise we had a commit hook. Then how do people check in to a
closed tree? Or does closed now mean "absolutely, totally closed" and
all other states are some form of "open"?
How do you, using the scheme above, notice if the tree has a rule "don't
check in without asking the sheriff" or "we now require approval for all
patches"?
>> Where might one find that information? :-) It seems to me from looking
>> at the headers of the current tinderbox that it's currently scattered
>> randomly. For example, are we still collecting random oranges on a
>> page called "November 2008 Orange Compendium"?
>
> The canonical place to find random orange is on the "depends on" list of
> the bug with alias "randomorange", last I checked.
Great - thank you :-)
<looks>
"Bug 438871 depends on 182 open bugs." Wow, that's a lot of bugs. Are
you saying that if you have an orange, you have to check each of the 182
to see if it's one of them?
> Using tinderboxpushlog, when the number of green builds of that type
> after your checkin matches the number of boxes in the summary display
> might work....
So for Linux on trunk, to get off the hook, you need 2 green Bs, 1 L, 3
Us, 2 Ns and five Ts? That sort of thing?
Gerv
Blake Kaplan
> So the idea is, you watch the letters next to your checkin, and when
> they all go from grey to green, you are off the hook? And if any go
> orange, you investigate?
That's how I do it.
> I didn't realise we had a commit hook. Then how do people check in to a
> closed tree? Or does closed now mean "absolutely, totally closed" and
> all other states are some form of "open"?
If your checkin comment contains the words "CLOSED TREE" in all caps, then the
hook lets you by. Generally, I say something like "a=sheriff for checkin into
a CLOSED TREE".
> "Bug 438871 depends on 182 open bugs." Wow, that's a lot of bugs. Are
> you saying that if you have an orange, you have to check each of the 182
> to see if it's one of them?
Well, I've been told that it is possible to search bugzilla ;-).
--
Blake Kaplan
Daniel Holbert
> "Bug 438871 depends on 182 open bugs." Wow, that's a lot of bugs. Are
> you saying that if you have an orange, you have to check each of the 182
> to see if it's one of them?
Generally, you can just open the metabug dependency list (linked at the
top of http://tinderbox.mozilla.org/Firefox/ ) and do a quick search on
that page for the name of the failing test file. (Most of those bugs
mention the name of the sporadic test in their title.)
~Daniel
Boris Zbarsky
> So the idea is, you watch the letters next to your checkin, and when
> they all go from grey to green, you are off the hook? And if any go
> orange, you investigate?
More or less; if not all the relevant letters appeared next to your
checkin (due to high push volume), you need to look at the ones next to
the checkin after yours too.
> I didn't realise we had a commit hook. Then how do people check in to a
> closed tree? Or does closed now mean "absolutely, totally closed" and
> all other states are some form of "open"?
You can check in to a closed tree if your checkin comment includes the
string "CLOSED TREE" (possibly only at the end; I don't know; I put it
at the end).
> How do you, using the scheme above, notice if the tree has a rule "don't
> check in without asking the sheriff"
I don't. In practice, we only have this happening on known-carpool days
that we discuss in advance, so it's not been an issue. I can see how it
might be a problem for someone who doesn't call in to all the meetings,
though.
> or "we now require approval for all patches"?
Similar.
> "Bug 438871 depends on 182 open bugs." Wow, that's a lot of bugs. Are
> you saying that if you have an orange, you have to check each of the 182
> to see if it's one of them?
Yes. Generally it's pretty easy to tell, as it happens...
>> Using tinderboxpushlog, when the number of green builds of that type
>> after your checkin matches the number of boxes in the summary display
>> might work....
>
> So for Linux on trunk, to get off the hook, you need 2 green Bs, 1 L, 3
> Us, 2 Ns and five Ts? That sort of thing?
Yeah. Though it's one U, one M, one E, and no Ns at all (N is just
"nightly", and only happens.... well, once a day last I checked).
-Boris
Gervase Markham
> More or less; if not all the relevant letters appeared next to your
> checkin (due to high push volume), you need to look at the ones next to
> the checkin after yours too.
So I'm still trying to work out how to define "the relevant letters"...
>>> Using tinderboxpushlog, when the number of green builds of that type
>>> after your checkin matches the number of boxes in the summary display
>>> might work....
>>
>> So for Linux on trunk, to get off the hook, you need 2 green Bs, 1 L,
>> 3 Us, 2 Ns and five Ts? That sort of thing?
>
> Yeah. Though it's one U, one M, one E, and no Ns at all (N is just
> "nightly", and only happens.... well, once a day last I checked).
...because I applied what I thought you said was the algorithm to come
up with those numbers, but you've told me I was wrong.
I counted the number of boxes in an OS column in the bottom right corner
(for Linux, Build: 2, Leak Test: 1, Unit Test: 3, etc.) and thought you
needed that many of the appropriate letter, which is how I got my list
above. How did you get your counts (1U, 1M, 1E, no Ns...)?
Or is it just that the algorithm is "watch it for a while, then go away,
and if the sheriff comes after you, you weren't watching for long
enough"? :-)
Although this feels like wading through treacle, we're getting there! :-)
Gerv
Axel Hecht
FWIW, I'm not sure how much time we should spend on the technical
details of that particular display.
It's a hack on top of a hack. The paradigm to display the results is
obviously right and compelling. But the visual details etc are subject
to change, as far as I'm concerned. We have various folks that work on a
builds database, and need it. Once we have it, tools like the builds
display are likely going to change at least in terms of their robustness
and in how the attribute builds to changes, and are going to be able to
detect builds for changes that are pending, or running.
Axel
Benjamin Smedberg
> I counted the number of boxes in an OS column in the bottom right corner
> (for Linux, Build: 2, Leak Test: 1, Unit Test: 3, etc.) and thought you
> needed that many of the appropriate letter, which is how I got my list
> above. How did you get your counts (1U, 1M, 1E, no Ns...)?
Those boxes at the bottom don't represent exactly what you think.
There's one build box, labeled "B" above.
There's three unit-test boxes. One is labeled "U". One is labeled "M" for
mochitest-only. One is labeled "E" for everything-else.
"N" builds are nightlies... they typically only happen once a day and if you
wait for them you'll be waiting a long time!
> Or is it just that the algorithm is "watch it for a while, then go away,
> and if the sheriff comes after you, you weren't watching for long
> enough"? :-)
The algorithm is "if you're unsure, ask on IRC".
--BDS
Boris Zbarsky
> ...because I applied what I thought you said was the algorithm to come
> up with those numbers, but you've told me I was wrong.
Yeah, so I oversimplified (and forgot that the three unit test boxes
look identical in that display).
I think Benjamin and Axel have covered everything else I was going to say.
-Boris
Gervase Markham
> The algorithm is "if you're unsure, ask on IRC".
Can we just define a time period which is likely to be sufficient 95% of
the time (given variations in build box load)? What would that be? 2 hours?
Gerv
Boris Zbarsky
> Can we just define a time period which is likely to be sufficient 95% of
> the time (given variations in build box load)? What would that be? 2 hours?
Some data points from the last several patches that this data is
available for (so from today; not very high landing volume; about 6
pushes in the last 6 hours). All times measured from time of checkin to
first box of that type finishing for the checkin.
Build done:
Linux: 52min, 39min, 11min, 10min, 11min
Mac: 44min, 1h 6 min, 51min, 1h 8min, 1h 4min
Win: 1h 24min, 58min, 1h 38min
Unit Tests done:
Linux 1h 43min, 1h 2min, 1h 44min
Mac: 2h 26min, 1h 51min, 1h 17min
Win: 2h 19min, 2h 7min, 2h 11min
Talos tp4 done:
Linux: 1h 12min, 1h 17min
Mac: 2h 6min, 1h 23min
Win: 2h 58min, 2h 53min
Talos tp3 done:
Linux: 2h 11min, 1h 58min
Mac: 2h 3min, 2h 3min
Win: 3h 29min, 4h 21min
Looks like your 95% number probably means 1.5 hours to make sure you
don't go red, 2.5 hours to make sure unit tests pass, 4 hours to make
sure talos doesn't crash on your changes.
That pretty much matches the folk wisdom I recall (2 hours to build on
Windows, 2 hours to run talos).
-Boris