New issue
Advanced search Search tips

Issue 875037 link

Starred by 2 users
Status: Started
Owner:
ehmaldonado@chromium.org
Cc:
ehmaldonado@chromium.org
martiniss@chromium.org
thakis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 906114
Blocking:
issue 756686
issue 923240



Sign in to add a comment

gclient: can't use built-in variables when expanding conditions in hooks

Project Member Reported by dpranke@chromium.org, Aug 16 
See bug 873373 for context.

If you define a hook as 
{
  'name': 'foo',
  'action': ['foo.py', '--checkout-android={checkout_android}']
}

gclient crashes b/c checkout_android isn't in scope when expanding the string via format(). It looks like we're not putting the built-in vars into var dict correctly.

Comment 1 by ehmaldonado@chromium.org, Aug 16

Status: Assigned (was: Untriaged)
Shouldn't this be assigned?

Comment 2 by dpranke@chromium.org, Aug 16

Status: Started (was: Assigned)
Yes, sorry. I missed that part :). 'Started', actually, since I have started working on it.

Comment 3 by dpranke@chromium.org, Nov 8

Owner: ehmaldonado@chromium.org
Status: Fixed (was: Started)
ehmaldonaldo fixed this in https://chromium-review.googlesource.com/c/1312234 :).

Comment 4 by thakis@chromium.org, Nov 8

Cc: martiniss@chromium.org
ehmaldonado: Want to remove line 54 here https://chromium-review.googlesource.com/c/chromium/src/+/1309229/5/DEPS#54 , which was a workaround for htis bug?

Comment 5 by thakis@chromium.org, Nov 16

Blocking: 756686
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c56126056693dff05fe37f370fc1562cf0c17120

commit c56126056693dff05fe37f370fc1562cf0c17120
Author: Nico Weber <thakis@chromium.org>
Date: Fri Nov 16 15:10:10 2018

Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234

Bug: 875037
Change-Id: Ia4a6393237602b88bb0130d9f916972e930d95b3
Reviewed-on: https://chromium-review.googlesource.com/c/1339339
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608783}
[modify] https://crrev.com/c56126056693dff05fe37f370fc1562cf0c17120/DEPS
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/505928329b8b6d38ddc2e8ed3f22c9e9f8b0be37

commit 505928329b8b6d38ddc2e8ed3f22c9e9f8b0be37
Author: Ravi Mistry <rmistry@chromium.org>
Date: Fri Nov 16 17:35:02 2018

Revert "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234"

This reverts commit c56126056693dff05fe37f370fc1562cf0c17120.

Reason for revert: Breaking 11 autorollers into Chromium. See https://bugs.chromium.org/p/chromium/issues/detail?id=906114

Original change's description:
> Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234
> 
> Bug: 875037
> Change-Id: Ia4a6393237602b88bb0130d9f916972e930d95b3
> Reviewed-on: https://chromium-review.googlesource.com/c/1339339
> Reviewed-by: Hans Wennborg <hans@chromium.org>
> Commit-Queue: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#608783}

TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org

Change-Id: Ib8984eb1b295f9695758fc9700024927ba75d9ed
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 875037
Reviewed-on: https://chromium-review.googlesource.com/c/1340521
Reviewed-by: Ravi Mistry <rmistry@chromium.org>
Commit-Queue: Ravi Mistry <rmistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608835}
[modify] https://crrev.com/505928329b8b6d38ddc2e8ed3f22c9e9f8b0be37/DEPS
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 21 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ffe990d20bbcec3ffd31e41129bfc48fb9ad297

commit 1ffe990d20bbcec3ffd31e41129bfc48fb9ad297
Author: Nico Weber <thakis@chromium.org>
Date: Wed Nov 21 09:51:25 2018

Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234"

This is a reland of c56126056693dff05fe37f370fc1562cf0c17120

Original change's description:
> Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234
>
> Bug: 875037
> Change-Id: Ia4a6393237602b88bb0130d9f916972e930d95b3
> Reviewed-on: https://chromium-review.googlesource.com/c/1339339
> Reviewed-by: Hans Wennborg <hans@chromium.org>
> Commit-Queue: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#608783}

TBR=hans

Bug: 875037
Change-Id: Iac24d9ec462cd4d5720d78a52b7f867be3ab2ec7
Reviewed-on: https://chromium-review.googlesource.com/c/1344198
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609974}
[modify] https://crrev.com/1ffe990d20bbcec3ffd31e41129bfc48fb9ad297/DEPS
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 21 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/319965600784a41581478e6cef29f0a7d8db210e

commit 319965600784a41581478e6cef29f0a7d8db210e
Author: Nico Weber <thakis@chromium.org>
Date: Wed Nov 21 19:46:00 2018

Revert "Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234""

This reverts commit 1ffe990d20bbcec3ffd31e41129bfc48fb9ad297.

Reason for revert:
roll-dep is still broken with this (https://bugs.chromium.org/p/chromium/issues/detail?id=906114#c19)

Original change's description:
> Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234"
> 
> This is a reland of c56126056693dff05fe37f370fc1562cf0c17120
> 
> Original change's description:
> > Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234
> >
> > Bug: 875037
> > Change-Id: Ia4a6393237602b88bb0130d9f916972e930d95b3
> > Reviewed-on: https://chromium-review.googlesource.com/c/1339339
> > Reviewed-by: Hans Wennborg <hans@chromium.org>
> > Commit-Queue: Nico Weber <thakis@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#608783}
> 
> TBR=hans
> 
> Bug: 875037
> Change-Id: Iac24d9ec462cd4d5720d78a52b7f867be3ab2ec7
> Reviewed-on: https://chromium-review.googlesource.com/c/1344198
> Commit-Queue: Hans Wennborg <hans@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609974}

TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org

Change-Id: I1b2327216797da231e11427a29edece425cd35bd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 875037
Reviewed-on: https://chromium-review.googlesource.com/c/1347333
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610177}
[modify] https://crrev.com/319965600784a41581478e6cef29f0a7d8db210e/DEPS

Comment 10 by thakis@google.com, Jan 17 (6 days ago)

Blockedon: 906114
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 17 (6 days ago) 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5eaa7f0167358c9073637e636b80b20639e698bb

commit 5eaa7f0167358c9073637e636b80b20639e698bb
Author: Nico Weber <thakis@chromium.org>
Date: Thu Jan 17 03:41:55 2019

Reland "Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234""

This reverts commit 319965600784a41581478e6cef29f0a7d8db210e.

Reason for revert: This might work now, https://crbug.com/906114

Original change's description:
> Revert "Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234""
> 
> This reverts commit 1ffe990d20bbcec3ffd31e41129bfc48fb9ad297.
> 
> Reason for revert:
> roll-dep is still broken with this (https://bugs.chromium.org/p/chromium/issues/detail?id=906114#c19)
> 
> Original change's description:
> > Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234"
> > 
> > This is a reland of c56126056693dff05fe37f370fc1562cf0c17120
> > 
> > Original change's description:
> > > Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234
> > >
> > > Bug: 875037
> > > Change-Id: Ia4a6393237602b88bb0130d9f916972e930d95b3
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1339339
> > > Reviewed-by: Hans Wennborg <hans@chromium.org>
> > > Commit-Queue: Nico Weber <thakis@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#608783}
> > 
> > TBR=hans
> > 
> > Bug: 875037
> > Change-Id: Iac24d9ec462cd4d5720d78a52b7f867be3ab2ec7
> > Reviewed-on: https://chromium-review.googlesource.com/c/1344198
> > Commit-Queue: Hans Wennborg <hans@chromium.org>
> > Reviewed-by: Nico Weber <thakis@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#609974}
> 
> TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org
> 
> Change-Id: I1b2327216797da231e11427a29edece425cd35bd
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 875037
> Reviewed-on: https://chromium-review.googlesource.com/c/1347333
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Commit-Queue: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#610177}

TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 875037
Change-Id: I223dd1baf8efe4ba8ca3ba4ff2c87e30319501eb
Reviewed-on: https://chromium-review.googlesource.com/c/1417030
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623559}
[modify] https://crrev.com/5eaa7f0167358c9073637e636b80b20639e698bb/DEPS
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 18 (5 days ago) 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c09fc5c07e8c434ab8f003d4e70ed1be5cf083d1

commit c09fc5c07e8c434ab8f003d4e70ed1be5cf083d1
Author: Michael Moss <mmoss@chromium.org>
Date: Fri Jan 18 08:00:03 2019

Revert "Reland "Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234"""

This reverts commit 5eaa7f0167358c9073637e636b80b20639e698bb.

Reason for revert: Suspect this is breaking the release bots ('gclient setdep' error). 

Original change's description:
> Reland "Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234""
> 
> This reverts commit 319965600784a41581478e6cef29f0a7d8db210e.
> 
> Reason for revert: This might work now, https://crbug.com/906114
> 
> Original change's description:
> > Revert "Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234""
> > 
> > This reverts commit 1ffe990d20bbcec3ffd31e41129bfc48fb9ad297.
> > 
> > Reason for revert:
> > roll-dep is still broken with this (https://bugs.chromium.org/p/chromium/issues/detail?id=906114#c19)
> > 
> > Original change's description:
> > > Reland "Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234"
> > > 
> > > This is a reland of c56126056693dff05fe37f370fc1562cf0c17120
> > > 
> > > Original change's description:
> > > > Remove workaround made unnecessary by https://chromium-review.googlesource.com/c/1312234
> > > >
> > > > Bug: 875037
> > > > Change-Id: Ia4a6393237602b88bb0130d9f916972e930d95b3
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/1339339
> > > > Reviewed-by: Hans Wennborg <hans@chromium.org>
> > > > Commit-Queue: Nico Weber <thakis@chromium.org>
> > > > Cr-Commit-Position: refs/heads/master@{#608783}
> > > 
> > > TBR=hans
> > > 
> > > Bug: 875037
> > > Change-Id: Iac24d9ec462cd4d5720d78a52b7f867be3ab2ec7
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1344198
> > > Commit-Queue: Hans Wennborg <hans@chromium.org>
> > > Reviewed-by: Nico Weber <thakis@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#609974}
> > 
> > TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org
> > 
> > Change-Id: I1b2327216797da231e11427a29edece425cd35bd
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug: 875037
> > Reviewed-on: https://chromium-review.googlesource.com/c/1347333
> > Reviewed-by: Nico Weber <thakis@chromium.org>
> > Commit-Queue: Nico Weber <thakis@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#610177}
> 
> TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 875037
> Change-Id: I223dd1baf8efe4ba8ca3ba4ff2c87e30319501eb
> Reviewed-on: https://chromium-review.googlesource.com/c/1417030
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Commit-Queue: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#623559}

TBR=thakis@chromium.org,hans@chromium.org,martiniss@chromium.org,ehmaldonado@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 875037, 923240
Change-Id: Iadcca77053471e1b46637e55ee6a713b281835c5
Reviewed-on: https://chromium-review.googlesource.com/c/1420268
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Michael Moss <mmoss@chromium.org>
Auto-Submit: Michael Moss <mmoss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624032}
[modify] https://crrev.com/c09fc5c07e8c434ab8f003d4e70ed1be5cf083d1/DEPS

Comment 13 by thakis@google.com, Jan 18 (4 days ago)

Blocking: 923240

Comment 14 by thakis@google.com, Jan 18 (4 days ago)

Status: Started (was: Fixed)
Looks like this is still not working, see new blockee (issue 923240)

Comment 15 by mmoss@chromium.org, Jan 18 (4 days ago) 

Ugh, after looking into this some more, I think the issue is that bot had a very old version of depot_tools (although I'm not sure why yet):

depot_tools at 8d3925b164822e2660d3b985402a5681432b0285

Comment 16 by mmoss@chromium.org, Jan 18 (4 days ago) 

And it's because it's getting it from:

https://chrome-internal.googlesource.com/chrome/tools/build/internal.DEPS.git

Comment 17 by mmoss@chromium.org, Jan 18 (4 days ago) 

Actually, the command that failed (https://logs.chromium.org/logs/infra-internal/bb/official.infra.cron/chrome-branch/949/+/recipes/steps/update_src_DEPS/0/steps/gclient_setdep/0/stdout)  should be using gclient from the recipes checkout, which should be up-to-date. So I'm confused :/

Comment 18 by ehmaldonado@chromium.org, Jan 18 (4 days ago) 

The reason is that gclient needs a config to know what checkout_android is supposed to be.
So we should pass a config, and then setting checkout_android to true as a custom var, of implementation a flag that sets all checkout_* cars to true.

https://cs.chromium.org/chromium/tools/depot_tools/gclient.py?l=2894
►

Sign in to add a comment