to TBR or not to TBR

Discussion:

Ojan Vafai

2008-10-10 18:37:44 UTC

I hope not to start a flame-war here, but I'd like to see written down
somewhere our team policy on committing code. There are some issues that
seem underspecified to me. These are of course just my feelings on these
issues. I hope that coming out of this discussion we can agree on a formal
policy.

1. Use the trybots: It's at the point where I think that no one should
*ever* commit code without at least looking at the results of the trybots,
unless it is an emergency fix for a closed tree (or of course if the trybots
are down). I imagine there won't be much disagreement here and we can just
add this to the appropriate documentation on the Sites page.
2. Don't
TBR: I see inconsistency with the team culture around what is
acceptable to TBR. My experience with the rest of Google is that the
*only* acceptable changes to TBR are ones that fix closed trees. I
would feel a lot more comfortable if we had a hard rule like that, but
I understand others feel differently. In either case, can we generate
a hard list of the things that are acceptable toTBR?
3. Watch the waterfall: Noone should ever commit code unless they can
stick around for the next hour to make sure they didn't cause regressions,
or unless they can ask someone else to monitor the tree for them and act
appropriately. Not doing one of those two things means that when your
checkin inadvertently breaks the build it falls on the shoulders of either
the sheriff or whoever happens to be online if it's after hours.

This all piggy-backs on Marc-Antione's email yesterday about keeping the
tree green. It is possible to keep the tree considerably more green than we
currently do and I think the above would be an enormous step in that
direction. Keeping the tree green makes our team globally more efficient and
keeps our sheriffs from hating their jobs. :)

Thoughts?

Ojan

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To post to this group, send email to chromium-***@googlegroups.com
To unsubscribe from this group, send email to chromium-dev+***@googlegroups.com
For more options, visit this group at http://groups.google.com/group/chromium-dev?hl=en
-~----------~----~----~----~------~----~------~--~---

t***@chromium.org

2008-10-10 19:24:39 UTC

Permalink

I agree with Nicholas's definition of what is allowed to TBR: things that
are not code changes. To expand on his list, I think this also includes
svn prop changes and string translation dumps (xtb file updates).

Historically, we have checked in code TBR on the merge branch during the
initial phases of getting it to compile and link. When we were using
gvn, this still meant you had to send an email. Whoever the TBR was sent
to is still responsible for reviewing the change and making sure it's
correct. Any feedback on the change still needs to be addressed in a
follow up change. The idea being that getting merge branch to
compile/link shouldn't block on the review since there are a lot of small
things to do. I don't think the intent was ever that the merge would be
reviewed only when landed.

I think we were a bit sloppy with this in the most recent merge. This is
possibly because it is tempting to just svn commit with TBR in the commit
description. This means no email gets sent and the patch is never
actually reviewed. Part of the difficulty is that Reitveld can't handle
large changes, so it requires manually emailing the viewvc link after the
commit.

I think when the next merge happens, we just need to be more careful of
this and try to make sure there's an email associated with every commit.
If we want to enforce it, we could make an svn commit hook. The svn hook
would make sure there's either a Reitveld URL or a TBR in the commit log.
If there's a TBR, it would send an email to the viewvc link.

tony

Post by Ojan Vafai
I hope not to start a flame-war here, but I'd like to see written down
somewhere our team policy on committing code. There are some issues that
seem underspecified to me. These are of course just my feelings on these
issues. I hope that coming out of this discussion we can agree on a formal
policy.
1. Use the trybots: It's at the point where I think that no one should
*ever* commit code without at least looking at the results of the trybots,
unless it is an emergency fix for a closed tree (or of course if the trybots
are down). I imagine there won't be much disagreement here and we can just
add this to the appropriate documentation on the Sites page.
2. Don't
TBR: I see inconsistency with the team culture around what is acceptable to TBR. My experience with the rest of Google is that the *only* acceptable changes to TBR are ones that fix closed trees. I would feel a lot more comfortable if we had a hard rule like that, but I understand others feel differently. In either case, can we generate a hard list of the things that are acceptable toTBR?
3. Watch the waterfall: Noone should ever commit code unless they can
stick around for the next hour to make sure they didn't cause regressions,
or unless they can ask someone else to monitor the tree for them and act
appropriately. Not doing one of those two things means that when your
checkin inadvertently breaks the build it falls on the shoulders of either
the sheriff or whoever happens to be online if it's after hours.

I agree with all this. To answer your question about "what is acceptable to
TBR". Everything that has code should not be TBR. On the other hand I would
personally not mind if someone changes the DEPS file, the test_fixable.txt
list or the VERSION file with a TBR checkin.
What do we do for branches? My understanding was that code reviews for
branches were not mandatory, since the code has to be reviewed anyway when
it's merge back to trunk. Is it true? How do you proceed for the webkit
merge branch?
Nicolas

Post by Ojan Vafai
1.
This all piggy-backs on Marc-Antione's email yesterday about keeping the
tree green. It is possible to keep the tree considerably more green than we
currently do and I think the above would be an enormous step in that
direction. Keeping the tree green makes our team globally more efficient and
keeps our sheriffs from hating their jobs. :)
Thoughts?
Ojan

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To post to this group, send email to chromium-***@googlegroups.com
To unsubscribe from this group, send email to chromium-dev+***@googlegroups.com
For more options, visit this group at http://groups.google.com/group/chromium-dev?hl=en
-~----------~----~----~----~------~----~------~--~---

Marc-Antoine Ruel

2008-10-10 20:04:56 UTC

Permalink

I'd like to add two points.

- If you haven't seen my original email, that's normal, it was on an
internal mailing list. It wasn't on a nice tone I won't repeat it
here.
- The try server is still not accessible externally. So saying to use
it externally is a bit moot. ;) That being said, I'm actively working
on open sourcing it. Once that is done, anyone can start one, even
locally.

M-A

Post by t***@chromium.org
I agree with Nicholas's definition of what is allowed to TBR: things that
are not code changes. To expand on his list, I think this also includes
svn prop changes and string translation dumps (xtb file updates).
Historically, we have checked in code TBR on the merge branch during the
initial phases of getting it to compile and link. When we were using
gvn, this still meant you had to send an email. Whoever the TBR was sent
to is still responsible for reviewing the change and making sure it's
correct. Any feedback on the change still needs to be addressed in a
follow up change. The idea being that getting merge branch to
compile/link shouldn't block on the review since there are a lot of small
things to do. I don't think the intent was ever that the merge would be
reviewed only when landed.
I think we were a bit sloppy with this in the most recent merge. This is
possibly because it is tempting to just svn commit with TBR in the commit
description. This means no email gets sent and the patch is never
actually reviewed. Part of the difficulty is that Reitveld can't handle
large changes, so it requires manually emailing the viewvc link after the
commit.
I think when the next merge happens, we just need to be more careful of
this and try to make sure there's an email associated with every commit.
If we want to enforce it, we could make an svn commit hook. The svn hook
would make sure there's either a Reitveld URL or a TBR in the commit log.
If there's a TBR, it would send an email to the viewvc link.
tony

Ojan Vafai

2008-10-10 22:50:35 UTC

Permalink

I have yet to hear much in the way of disagreement on any of these points.
But, I'm pretty sure there are naysayers as I've seen things being checked
in TBR that violate these proposed rules. An example, changes to vcproj
files and adding/deleting files. Do those count as code changes? To me they
do and should not be TBRed.
For the record, I agree that changes to test_fixable, version files and
translation files are fine to TBR. I'm a bit more hesitant about DEPS. We
don't need to make DEPS changes nearly as often as we used to now that we
have one module for most of the code and I've seen a number of broken builds
that are a result of DEPS changes. That said, I doubt that code review would
have avoided breaking the build in those cases.

I'd love to hear from people who disagree about these proposed policies so
we can compromise on something everyone is happy with.

At a tools level, a lot of my concern with respect to TBR would be mitigated
if the committer and the reviewer got nag mail for any checkins that don't
have a positive review, and even better, if checkins without positive
reviews showed up in reitveld as unclosed issues.

Ojan

Post by Marc-Antoine Ruel
I'd like to add two points.
- If you haven't seen my original email, that's normal, it was on an
internal mailing list. It wasn't on a nice tone I won't repeat it
here.
- The try server is still not accessible externally. So saying to use
it externally is a bit moot. ;) That being said, I'm actively working
on open sourcing it. Once that is done, anyone can start one, even
locally.
M-A

Post by Ojan Vafai
I hope not to start a flame-war here, but I'd like to see written down
somewhere our team policy on committing code. There are some issues

that