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 On Fri, Oct 10, 2008 at 1:04 PM, Marc-Antoine Ruel <[EMAIL PROTECTED]>wrote: > > 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 > > 2008/10/10 <[EMAIL PROTECTED]>: > > > > 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 > > > > > > On Fri, 10 Oct 2008, Nicolas Sylvain wrote: > > > >> On Fri, Oct 10, 2008 at 11:37 AM, Ojan Vafai <[EMAIL PROTECTED]> wrote: > >> > >> > 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 > >> > >> > >> > 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 [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/chromium-dev?hl=en -~----------~----~----~----~------~----~------~--~---
