landing two-sided patches

[Evan Martin]

Person A landed the WebKit side of a "two-sided patch" -- a change to
WebKit that needed an accompanying change -- leaving the Chrome
canaries red.
Person B grumbled to me that this was done wrong, but that it'd be ok.
Person C reverted the patch.

No blaming! I actually thought what person A had done was right, and
I almost did the same thing a day ago (Darin noticed and suggested I
use a default argument on a function to make it so the code would work
both with and without my patch).

Do we have docs on what the proper thing to do is?
In composing this email I found "Landing Two-sided Patches" in this document:
http://dev.chromium.org/developers/how-tos/webkit-merge-1
but I'd never read it because I haven't ever done a WebKit merge.

Is that the rule everyone who commits to WebKit should have known about?

[Stephen White]

I think the main thing is, if you're landing a two-sided patch, give the WebKit gardener a heads-up.  Then there should be no unexpected reverting.

One thing I've done in the past is to hide the WebKit changes behind an #ifdef, so that it works either with or without the Chrome-side change.  Then once the merge brings it to Chrome, land the Chrome-side and #define the symbol in the same CL.  Then, if all the bots are happy, land a WebKit change to remove the old code and the #ifdef.  That way, if there are problems, you can revert just the Chrome-side change without needing an emergency WebKit merge.

Granted, this is more work, and is only practical for small to medium-sized changes.  But it's probably the safest.

Stephen

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

--
All truth passes through three stages. First, it is ridiculed. Second, it is violently opposed. Third, it is accepted as being self-evident. -- Schopenhauer

[Stephen White]

On Fri, Feb 19, 2010 at 2:06 PM, Brett Wilson <bre...@google.com> wrote:

On Fri, Feb 19, 2010 at 10:46 AM, Stephen White
<senor...@chromium.org> wrote:
> I think the main thing is, if you're landing a two-sided patch, give the
> WebKit gardener a heads-up.  Then there should be no unexpected reverting.
> One thing I've done in the past is to hide the WebKit changes behind an
> #ifdef, so that it works either with or without the Chrome-side change.
>  Then once the merge brings it to Chrome, land the Chrome-side and #define
> the symbol in the same CL.  Then, if all the bots are happy, land a WebKit
> change to remove the old code and the #ifdef.  That way, if there are
> problems, you can revert just the Chrome-side change without needing an
> emergency WebKit merge.
> Granted, this is more work, and is only practical for small to medium-sized
> changes.  But it's probably the safest.

Where do you end up defining the macro in Chrome?

In my case, I used skia/config/SkUserConfig.h.

Stephen

[Jonathan Dixon]

Natural follow on question: what's the correct way to contact today's gardener?

The previously linked Gardener howto refers to https://www.google.com/calendar/hosted/google.com/embed?src=google.com_h1kjbmj...@group.calendar.google.com&ctz=America/Los_Angeles but I can't discern any useful contact info from that.

[Brett Wilson]

On Fri, Feb 19, 2010 at 11:20 AM, Jonathan Dixon <jo...@google.com> wrote:
> Natural follow on question: what's the correct way to contact today's
> gardener?
> The previously linked Gardener howto refers
> to https://www.google.com/calendar/hosted/google.com/embed?src=google.com_h1kjbmj...@group.calendar.google.com&ctz=America/Los_Angeles
> but I can't discern any useful contact info from that.

You have to click on an individual calendar entry and see the tiny
text where it says who is invited to that meeting.

Brett

[Drew Wilson]

BTW, care is not only required for two-sided patches.

On my previous gardening shift, I had someone inform me (after the fact) that they had landed a change upstream that might cause perf regressions downstream. The heads-up was appreciated, but since the patch was landed during GMT while I was fast asleep (and there was an unrelated build break), by the time we were able to roll to the regression and realized we needed to revert the patch, 100+ revisions had passed and I was left to pick up the pieces.

Luckily, everything cleaned up OK in the end, but the moral is if you're checking something in upstream that you're even slightly dubious about, get the gardener looped in first. Oh, and run your changes through the try bots. There's nothing worse for a gardener than to do a roll and find 20 unit tests broken because a chromium committer didn't test their code.

-atw "the 'w' stands for 'whiny'"

--

[Darin Fisher]

With a little cleverness, two-sided patches can almost always be avoided.

It is far better on everyone if a two-sided patch is broken up into 3 patches:

1) Add the new WebKit interface, preserving the old.

2) Change Chrome to use the new interface.

3) Modify WebKit to remove the old interface.

There are still going to be some cases where this is nearly impossible or

too costly.  If anyone needs help figuring out how to possibly break up

a two-sided patch, please let me know.  I'd love to help!

-Darin