Generalize patch_project to patch root conversion.

Generalize patch_project to patch root conversion.

IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency roll-back plan.

This patch is first of a series. It provides a cleaner and generic way to apply patches to non-root of first solution.

R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300141

[tandrii(chromium), 2016-04-22 15:59:10]

PTAL && WDYT

This should not only solve Gerrit issue in a non-intrusive way, but also provide
for potentially applying patches of any DEPS-ed in project to actual
project-solution (e.g. build_tools to chromium), which is now possibly only for
v8 in chromium.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
File recipe_modules/gclient/config.py (right):

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:162: p['v8'] = ('src/v8', 'HEAD')
for gerrit, this row would be duplicated with
  'v8/v8' = ('src/v8', 'HEAD')

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:163: p['angle/angle'] =
('src/third_party/angle', None)
this is how gerrit looks.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:164: p['blink'] = ('src/third_party/WebKit',
None)
machenbach@ i decided to specify the the full path here instead of omitting src
for consistency, as that's what code 15 lines above does anyway.

[Michael Achenbach, 2016-04-25 09:32:43]

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/api.py
File recipe_modules/gclient/api.py (right):

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/api....
recipe_modules/gclient/api.py:357: root, _ =
cfg.solutions[0].patch_projects.get(patch_project, ('', ''))
solutions[0] goes away when making the dict global.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
File recipe_modules/gclient/config.py (right):

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:41: #     'v8': ('v8', 'HEAD')
Stay in sync with below (includes src). Maybe provide a less intuitive example
where the solution and project names don't match.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:161: p = s.patch_projects
I'd make this global, i.e. c.patch_projects, as the solution name is part of the
first item of the tuples.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:164: p['blink'] = ('src/third_party/WebKit',
None)
On 2016/04/22 15:59:10, tandrii(chromium) wrote:
> machenbach@ i decided to specify the the full path here instead of omitting
src
> for consistency, as that's what code 15 lines above does anyway.

Does blink still exist in this context? Keep it around if the old code had it.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:168: path, revision =
s.patch_projects.get(c.PATCH_PROJECT, (None, None))
Move to bot_update ensure_checkout and maybe to gclient sync.

[tandrii(chromium), 2016-04-25 11:31:36]

Description was changed from

==========
(WIP) Generalize patch_project to patch root conversion.

COMMIT=False
R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

to

==========
Generalize patch_project to patch root conversion.

COMMIT=False
R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

[tandrii(chromium), 2016-04-25 11:34:50]

Description was changed from

==========
Generalize patch_project to patch root conversion.

COMMIT=False
R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

to

==========
Generalize patch_project to patch root conversion.

IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency
roll-back plan.

This patch is first of a series. It provides a cleaner and generic way to apply
patches to non-root of first solution.

COMMIT=False
R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

[tandrii(chromium), 2016-04-25 11:39:51]

PTAL

also see description and specifically the plan for deployment
https://bugs.chromium.org/p/chromium/issues/detail?id=605563#c3

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/api.py
File recipe_modules/gclient/api.py (right):

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/api....
recipe_modules/gclient/api.py:357: root, _ =
cfg.solutions[0].patch_projects.get(patch_project, ('', ''))
On 2016/04/25 09:32:42, Michael Achenbach wrote:
> solutions[0] goes away when making the dict global.

Done.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
File recipe_modules/gclient/config.py (right):

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:41: #     'v8': ('v8', 'HEAD')
On 2016/04/25 09:32:42, Michael Achenbach wrote:
> Stay in sync with below (includes src). Maybe provide a less intuitive example
> where the solution and project names don't match.

Done.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:161: p = s.patch_projects
On 2016/04/25 09:32:42, Michael Achenbach wrote:
> I'd make this global, i.e. c.patch_projects, as the solution name is part of
the
> first item of the tuples.

Done.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:164: p['blink'] = ('src/third_party/WebKit',
None)
On 2016/04/25 09:32:42, Michael Achenbach wrote:
> On 2016/04/22 15:59:10, tandrii(chromium) wrote:
> > machenbach@ i decided to specify the the full path here instead of omitting
> src
> > for consistency, as that's what code 15 lines above does anyway.
> 
> Does blink still exist in this context? Keep it around if the old code had it.

kept as is, but yes, i think i'll clean it up with all the todos in subsequent
CLs.

https://codereview.chromium.org/1917433002/diff/1/recipe_modules/gclient/conf...
recipe_modules/gclient/config.py:168: path, revision =
s.patch_projects.get(c.PATCH_PROJECT, (None, None))
On 2016/04/25 09:32:42, Michael Achenbach wrote:
> Move to bot_update ensure_checkout and maybe to gclient sync.

Done.

[Michael Achenbach, 2016-04-25 12:09:58]

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/example.expected/tryjob_v8.json (right):

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/example.expected/tryjob_v8.json:27: "--revision",
Hmm, wonder if this should maybe not change? I didn't even know that feature (or
forgot about it in case I added it), but it looks like we can pass in the v8
revision to use as a property and if we do, it should be used instead of HEAD,
right?

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/api.py (right):

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/api.py:151: self.set_patch_project_revision(cfg)
Guess this path has no coverage, but never mind...

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/api.py:377: cfg.revisions[path] = revision
This should maybe only be updated if not set already? Also wondering about cases
where bot_update is run several times, e.g. once with once without patch. The
second usually sets cfg.revisions[X] to the same as it got in the first call. We
don't want it to be set to HEAD again. E.g. this code:
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/config.py (right):

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/config.py:70: #    dependency.
Maybe mention that 'HEAD' and '' are the only two meaningful values. Or (in case
particular branches do work) just say that this value is passed verbatim to
bot_update's --revision unless it's ''.

[tandrii(chromium), 2016-04-25 13:37:52]

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/example.expected/tryjob_v8.json (right):

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/example.expected/tryjob_v8.json:27: "--revision",
On 2016/04/25 12:09:58, Michael Achenbach wrote:
> Hmm, wonder if this should maybe not change? I didn't even know that feature
(or
> forgot about it in case I added it), but it looks like we can pass in the v8
> revision to use as a property and if we do, it should be used instead of HEAD,
> right?

good point. Added a check to avoid ovewriting.

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/api.py (right):

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/api.py:377: cfg.revisions[path] = revision
On 2016/04/25 12:09:58, Michael Achenbach wrote:
> This should maybe only be updated if not set already? Also wondering about
cases
> where bot_update is run several times, e.g. once with once without patch. The
> second usually sets cfg.revisions[X] to the same as it got in the first call.
We
> don't want it to be set to HEAD again. E.g. this code:
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

Done.

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/config.py (right):

https://codereview.chromium.org/1917433002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/config.py:70: #    dependency.
On 2016/04/25 12:09:58, Michael Achenbach wrote:
> Maybe mention that 'HEAD' and '' are the only two meaningful values. Or (in
case
> particular branches do work) just say that this value is passed verbatim to
> bot_update's --revision unless it's ''.

Done.

[tandrii(chromium), 2016-04-25 13:55:27]

With latest fix, this CL causes NO changes to expectations in build. I'm not
sure if this means test coverage is bad or this CL is awesome.

[tandrii(chromium), 2016-04-25 14:08:00]

Description was changed from

==========
Generalize patch_project to patch root conversion.

IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency
roll-back plan.

This patch is first of a series. It provides a cleaner and generic way to apply
patches to non-root of first solution.

COMMIT=False
R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

to

==========
Generalize patch_project to patch root conversion.

IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency
roll-back plan.

This patch is first of a series. It provides a cleaner and generic way to apply
patches to non-root of first solution.

R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

[tandrii(chromium), 2016-04-25 14:09:26]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 14:09:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1917433002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1917433002/100001

[commit-bot: I haz the power, 2016-04-25 14:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-25 14:14:26]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)

[Michael Achenbach, 2016-04-25 14:15:33]

lgtm

[tandrii(chromium), 2016-04-25 14:15:47]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2016-04-25 14:15:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1917433002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1917433002/120001

[commit-bot: I haz the power, 2016-04-25 14:18:34]

Description was changed from

==========
Generalize patch_project to patch root conversion.

IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency
roll-back plan.

This patch is first of a series. It provides a cleaner and generic way to apply
patches to non-root of first solution.

R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563
==========

to

==========
Generalize patch_project to patch root conversion.

IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency
roll-back plan.

This patch is first of a series. It provides a cleaner and generic way to apply
patches to non-root of first solution.

R=phajdan.jr@chromium.org,machenbach@chromium.org
BUG=605563

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300141
==========

[commit-bot: I haz the power, 2016-04-25 14:18:35]

Committed patchset #7 (id:120001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=300141