Skip MigrateConfigToGit if gclient config is non trivial.

Skip MigrateConfigToGit if gclient config is non trivial.

At the moment it crashs if the .gclient config contains Import or Function statements.

[andresp-chromium, 2014-08-28 12:09:09]

andresp@chromium.org changed reviewers:
+ cmp@chromium.org, szager@chromium.org

[iannucci, 2014-08-28 16:36:38]

iannucci@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2014-08-28 16:36:38]

lgtm, though import statements in your .gclient is a Bad Idea.

Could you describe why you have those (e.g. what you use them for)?

[szager1, 2014-08-28 17:22:15]

not lgtm

https://codereview.chromium.org/513923006/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/513923006/diff/1/gclient.py#newcode1271
gclient.py:1271: if any(not hasattr(elem, 'targets') for elem in a.body):
Instead of this, I would prefer you modify the next line to be:

solutions = [elem for elem in a.body if elem.__class__ is ast.Assign and
'solutions' in [target.id for target in elem.targets]]

[andresp-chromium, 2014-08-29 14:15:20]

On 2014/08/28 17:22:15, szager1 wrote:
> not lgtm
> 
> https://codereview.chromium.org/513923006/diff/1/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/513923006/diff/1/gclient.py#newcode1271
> gclient.py:1271: if any(not hasattr(elem, 'targets') for elem in a.body):
> Instead of this, I would prefer you modify the next line to be:
> 
> solutions = [elem for elem in a.body if elem.__class__ is ast.Assign and
> 'solutions' in [target.id for target in elem.targets]]

We use a .gclient to import projects into google internal repo.


The .gclient uses os.environ to fetch revisions number and also since the same
revision and paths are used multiple times in the solution is defines solution
in a non-trivial way with method calls.

The new uploaded patch works for our use case, but it does not covers many other
non-trivial .gclients.

[iannucci, 2014-08-29 17:25:58]

On 2014/08/29 14:15:20, andresp wrote:
> On 2014/08/28 17:22:15, szager1 wrote:
> > not lgtm
> > 
> > https://codereview.chromium.org/513923006/diff/1/gclient.py
> > File gclient.py (right):
> > 
> > https://codereview.chromium.org/513923006/diff/1/gclient.py#newcode1271
> > gclient.py:1271: if any(not hasattr(elem, 'targets') for elem in a.body):
> > Instead of this, I would prefer you modify the next line to be:
> > 
> > solutions = [elem for elem in a.body if elem.__class__ is ast.Assign and
> > 'solutions' in [target.id for target in elem.targets]]
> 
> We use a .gclient to import projects into google internal repo.
> 
> 
> The .gclient uses os.environ to fetch revisions number and also since the same
> revision and paths are used multiple times in the solution is defines solution
> in a non-trivial way with method calls.
> 
> The new uploaded patch works for our use case, but it does not covers many
other
> non-trivial .gclients.

Just so you know, gclient supports --revision
path/to/dependency@arbitrary_revision, which is the only recommended way for
dealing with the situation you describe. Is there are reason that does not work?

[szager1, 2014-08-29 17:47:44]

lgtm

This should cover the great majority of cases.

[andresp-chromium, 2014-09-01 11:06:53]

On 2014/08/29 17:47:44, szager1 wrote:
> lgtm
> 
> This should cover the great majority of cases.

Thanks iannucci, I have fixed our internal use case to use --revision.
Seems to work as intended and I don't really need to submit this.

[iannucci, 2014-09-02 18:55:23]

On 2014/09/01 11:06:53, andresp wrote:
> On 2014/08/29 17:47:44, szager1 wrote:
> > lgtm
> > 
> > This should cover the great majority of cases.
> 
> Thanks iannucci, I have fixed our internal use case to use --revision.
> Seems to work as intended and I don't really need to submit this.

hooray! one less case for imports in .gclient! :D