RFC: gclient: supporting patching DEPS with recursedeps

[Paweł Hajdan, Jr.]

I uploaded a doc with possible approaches to tackling https://bugs.chromium.org/p/chromium/issues/detail?id=643346 .

Feedback is welcome.

Paweł

[Dirk Pranke]

I'm not sure if people actually saw this doc or not, but we do need to figure out what we want to do here.

Restating what Paweł wrote: currently, the ANGLE team has an issue where they can test patches to ANGLE that don't affect DEPS files just fine, but if their patch touches their DEPS file, it doesn't work. We need to fix this, but it's not particularly obvious what the best way to do that is.

The most useful (but hardest) answer is probably to make bot_update and/or gclient properly understand the interaction of patches and when to sync and recurse into DEPS files, so that you fetch a repo, apply a patch to it, and then re-sync any DEPS in that repo, regardless of where the repo is.

However, it's possible that there are other things we can do, which is why we want more feedback.

Ryan, Aaron, I'd specifically like your feedback here, but of course feedback from anyone else is welcome, too.

-- Dirk

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPZ7HjMCPp-nn4OSkXCROJzaraU34F%2BsMsp_4TmeY-2BPA%40mail.gmail.com.

[Aaron Gable]

Thanks for the direct ping. I've left a comment on the doc expressing a condensed version of my opinion. Happy to have discussion there or over VC (although note that I go back on vacation again on Thursday).

On Mon, Aug 7, 2017 at 3:49 PM Dirk Pranke <dpr...@chromium.org> wrote:

I'm not sure if people actually saw this doc or not, but we do need to figure out what we want to do here.

Restating what Paweł wrote: currently, the ANGLE team has an issue where they can test patches to ANGLE that don't affect DEPS files just fine, but if their patch touches their DEPS file, it doesn't work. We need to fix this, but it's not particularly obvious what the best way to do that is.

The most useful (but hardest) answer is probably to make bot_update and/or gclient properly understand the interaction of patches and when to sync and recurse into DEPS files, so that you fetch a repo, apply a patch to it, and then re-sync any DEPS in that repo, regardless of where the repo is.

However, it's possible that there are other things we can do, which is why we want more feedback.

Ryan, Aaron, I'd specifically like your feedback here, but of course feedback from anyone else is welcome, too.

-- Dirk

On Thu, Aug 3, 2017 at 9:22 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I uploaded a doc with possible approaches to tackling https://bugs.chromium.org/p/chromium/issues/detail?id=643346 .

Feedback is welcome.

Paweł

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

[Ryan Tseng]

I'm still a little confused about the flows that this is targeting (ie which CL can't be tested right now?).  It sounds like a problem that could be solved with passing in strategic --revision flags.  Otherwise this makes gclient even more complex and scary.  Dirk let me know when you're around to discuss this.

[Dirk Pranke]

It's not clear to me that a solution involving --revision flags can actually work. For example, how would this work in the CQ, for a patch that changes third_party/ANGLE/DEPS to get a new version of dEQP plus updates code in ANGLE to depend on that change?

I spent some time talking to Ryan in person and I think he understands the problem better now; I'm not sure if he would agree with me above, or if there's something I'm still not seeing, though.

-- Dirk

[Aaron Gable]

It works for everything. Like, for example, a two-sided change which updates ANGLE's DEPS, files in ANGLE which rely on that, *and* files in chromium which rely on that:

bot_update.py --revsion src@refs/changes/56/123456/1 --revision src/third_party/ANGLE@refs/changes/90/567890/3

This is the future we need to be headed towards: no "sync then apply patch then sync again". No custom deps or anything like that. Just extract repos and change refs from the build properties, then sync directly to those.

On Wed, Aug 9, 2017 at 11:49 AM Dirk Pranke <dpr...@chromium.org> wrote:

It's not clear to me that a solution involving --revision flags can actually work. For example, how would this work in the CQ, for a patch that changes third_party/ANGLE/DEPS to get a new version of dEQP plus updates code in ANGLE to depend on that change?

I spent some time talking to Ryan in person and I think he understands the problem better now; I'm not sure if he would agree with me above, or if there's something I'm still not seeing, though.

-- Dirk

On Tue, Aug 8, 2017 at 4:37 PM, Ryan Tseng <hin...@chromium.org> wrote:

I'm still a little confused about the flows that this is targeting (ie which CL can't be tested right now?).  It sounds like a problem that could be solved with passing in strategic --revision flags.  Otherwise this makes gclient even more complex and scary.  Dirk let me know when you're around to discuss this.

On Mon, Aug 7, 2017 at 2:48 PM, Dirk Pranke <dpr...@chromium.org> wrote:

I'm not sure if people actually saw this doc or not, but we do need to figure out what we want to do here.

Restating what Paweł wrote: currently, the ANGLE team has an issue where they can test patches to ANGLE that don't affect DEPS files just fine, but if their patch touches their DEPS file, it doesn't work. We need to fix this, but it's not particularly obvious what the best way to do that is.

The most useful (but hardest) answer is probably to make bot_update and/or gclient properly understand the interaction of patches and when to sync and recurse into DEPS files, so that you fetch a repo, apply a patch to it, and then re-sync any DEPS in that repo, regardless of where the repo is.

However, it's possible that there are other things we can do, which is why we want more feedback.

Ryan, Aaron, I'd specifically like your feedback here, but of course feedback from anyone else is welcome, too.

-- Dirk

On Thu, Aug 3, 2017 at 9:22 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I uploaded a doc with possible approaches to tackling https://bugs.chromium.org/p/chromium/issues/detail?id=643346 .

Feedback is welcome.

Paweł

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

[Dirk Pranke]

So, in that case the user creates a chromium src.git CL that updates //DEPS to point to an uncommitted Gerrit CL in ANGLE? How would that work in the CQ?

-- Dirk

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

[Dirk Pranke]

Whoops, hit send too soon. I left out: what if the user isn't ready to commit that change into Chromium? Why should the user have to create a Chromium change at all?

-- Dirk

[Ryan Tseng]

Yes dirk explained it to me yesterday.  The problem is:

CL for angle.git contains:

* Local file changes

* DEPS.chromium changes

And they want to be able to CQ this CL

I don't see any way to do this other than the last suggestion, which is to teach bot_update to partially recurse deps before patching DEPS.chromium, and then finish recursing deps after.

On Wed, Aug 9, 2017 at 10:55 AM, Dirk Pranke <dpr...@chromium.org> wrote:

[Aaron Gable]

No, the chromium CL wouldn't include the DEPS change yet (and even if it did, it would be overridden by the --revision flag). It just has code which relies on the new changes in ANGLE.

The user doesn't *have* to create a chromium change at all. I was simply providing an example of a more complex situation which could be handled by using --revision flags. It would by no means be the default -- most of the time you'd just have a single --revision flag pointing at one of the nested dependencies.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

[Aaron Gable]

bot_update shouldn't have to partially recursedeps at all. If it uses --revision, it will get that pointed-to revision no matter what, ignoring whatever is in the actual parent DEPS file. So it syncs chromium to HEAD, syncs all its other dependencies to whatever they're supposed to be, syncs ANGLE to the /refs/changes/foo ref, and then continues syncing its dependencies just like normal.

The whole point is that there is no apply-patch step. It simply fetches the change ref just like it would fetch a commit on master, and then continues from there.

[Ryan Tseng]

The --revision proposal requires bot_update or recipes to parse DEPS.chromium (arbitrary python) to read the revision though.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

[Ryan Tseng]

Actually the recipe doesn't have access to DEPS.chromium other than a gerrit ref, I'm not sure I follow how it is supposed to acquire the new revision.

[Aaron Gable]

What? I don't follow. The --revision flag is being applied (in this example) to src/third_party/ANGLE, which is in chromium/src/DEPS. bot_update doesn't need to parse anything: it just passes that through to gclient, which is already parsing all of the DEPS files.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

[Aaron Gable]

Let's have a VC :)

[Aaron Gable]

Ok, had a quick sync with Ryan, Dirk, and Robbie on VC, and we're now on the same page. Pawel: I'm putting a hopefully-clear version of what we talked about in your doc (you have most of it already, in the "--revision" section). Please let me know if pieces of it need to be clearer or if there are any questions/comments.

[Paweł Hajdan, Jr.]

I accepted your suggestion and replied back on the doc.

To me the "no patching" solution looks very similar to the "--revision" one. Please point out any major differences I could have missed. We may mean the same thing, as "--revision" was already based on existing discussion on the bugs and your comments.

The non-obvious part is how do we make recipes pass the right flags. I was under impression we can already test ANGLE patches against chromium/src. Please consider clarifying what are advantages of your suggestion over what we currently have. I have a guess, but wanted to make sure we're on the same page.

Paweł

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.