Add the ability to specify a target_os for gclient solutions

Add the ability to specify a target_os for gclient solutions

This ability is useful for versions of Chromium (i.e. Android) which want to
cross-compile to another platform that has a fixed set of custom dependencies.

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

[Peter Beverloo, 2012-04-19 03:50:48]

+maruel

We'll use this to be able to integrate Android-specific dependencies (which now
solely are available in a separate git solution) with the normal Chromium DEPS
file, and thus allowing WebKit to inherit the right versions based on the used
Chromium revision.

After all bots/systems/configurations have been updated, this will allow Chrome
on Android to get rid of the android_deps.git repository. It may also be
beneficial to Chrome OS.

[John Grabowski, 2012-04-19 05:29:04]

Overall like tons.
Give example about how the multi-solution android .gclient will become a single
solution with extra var in the CL description.

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py#newcode154
gclient.py:154: self._target_os = target_os
move this to a spot "right above" should_process to be more consistent with the
convention (vars initialized in same-ish order as args)

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py#newcode439
gclient.py:439: enforced_os = enforced_os + tuple([self.target_os])
tuple() seems redundant.  How about just
  enforced_os = enforced_os + [self.target_os]
or, even simpler, 
  enforced_os.append(self.target_os)

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py#newcode754
gclient.py:754: 'target_os', 'processed', 'hooks_ran', 'deps_parsed',
Somewhere you should document the idea behind target_os and how it is optional.
I don't know where, but somewhere.

https://chromiumcodereview.appspot.com/10127004/diff/2001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/2001/tests/gclient_test....
tests/gclient_test.py:289: 'solutions = [\n'
"""
for multi-line strings in python,
prefer triple-quotes.
Like this.
"""

https://chromiumcodereview.appspot.com/10127004/diff/2001/tests/gclient_test....
tests/gclient_test.py:312: self.assertEqual(4, len(self._get_processed()))
I don't understand this.  1 dep, plus 1 for unix, plus 1 for baz.  Doesn't that
make 3?  So why would _get_processed() be 4?

[Peter Beverloo, 2012-04-19 06:11:03]

Thank you for the review, John! Replies inline and a new patchset has been
uploaded.

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py#newcode154
gclient.py:154: self._target_os = target_os
On 2012/04/19 05:29:04, John Grabowski wrote:
> move this to a spot "right above" should_process to be more consistent with
the
> convention (vars initialized in same-ish order as args)

I disagree. There is a large comment block above _managed and _should_process
which does not apply to target_os. Since target_os is not mutable (only in the
.gclient file) I think this would be the most appropriate position.
Alternatively it could be around line 167.

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py#newcode439
gclient.py:439: enforced_os = enforced_os + tuple([self.target_os])
On 2012/04/19 05:29:04, John Grabowski wrote:
> tuple() seems redundant.  How about just
>   enforced_os = enforced_os + [self.target_os]
> or, even simpler, 
>   enforced_os.append(self.target_os)

enforced_os is a tuple, which are not mutable (so have no append method).
Appending a list to a tuple throws an error as this is not allowerd either..

https://chromiumcodereview.appspot.com/10127004/diff/2001/gclient.py#newcode754
gclient.py:754: 'target_os', 'processed', 'hooks_ran', 'deps_parsed',
On 2012/04/19 05:29:04, John Grabowski wrote:
> Somewhere you should document the idea behind target_os and how it is
optional.
> I don't know where, but somewhere.

Agreed. I added this to the first comment in the file, similar to how "hooks"
has done this.

https://chromiumcodereview.appspot.com/10127004/diff/2001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/2001/tests/gclient_test....
tests/gclient_test.py:289: 'solutions = [\n'
On 2012/04/19 05:29:04, John Grabowski wrote:
> """
> for multi-line strings in python,
> prefer triple-quotes.
> Like this.
> """

Only following the file's convention :-). I have updated the syntax to use the
three quotes, but I can't have the ending three quotes on a separate line since
trailing whitespace apparently is a syntax error.. This makes alignment seem a
bit wonky, but I'm indifferent to the change overall.

https://chromiumcodereview.appspot.com/10127004/diff/2001/tests/gclient_test....
tests/gclient_test.py:312: self.assertEqual(4, len(self._get_processed()))
On 2012/04/19 05:29:04, John Grabowski wrote:
> I don't understand this.  1 dep, plus 1 for unix, plus 1 for baz.  Doesn't
that
> make 3?  So why would _get_processed() be 4?

svn://example.com/foo is also being processed (top level dep). This is in line
with other uses of the function, i.e. the "first_3" variable on line ~134.

[M-A Ruel, 2012-04-19 13:29:03]

https://chromiumcodereview.appspot.com/10127004/diff/4/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/4/gclient.py#newcode151
gclient.py:151: deps_file, target_os, should_process):
It also doesn't make sense to make it per Dependency, so this shouldn't be done
here.

https://chromiumcodereview.appspot.com/10127004/diff/4/gclient.py#newcode846
gclient.py:846: self._enforced_os = tuple(set(enforced_os))
enforced_os already does what you want. It is used at line 442 and will work
properly.

https://chromiumcodereview.appspot.com/10127004/diff/4/gclient.py#newcode869
gclient.py:869: s.get('target_os', None),
enforced_os doesn't make sense to be per solution. As such, it should be outside
of the solutions list. So;

if 'enforced_os' in config_dict:
  self.enforced_os = config_dict['enforced_os']
  logging.debug('Enforcing deps_os: %s' % self.enforced_os)

[John Grabowski, 2012-04-19 18:16:37]

LGTM from me but definitely wait for M-A's LGTM

[Peter Beverloo, 2012-04-27 15:27:51]

maruel: amended, it's now (similar to "hooks") a list in the global scope of a
.gclient file.

[M-A Ruel, 2012-04-27 15:52:21]

https://chromiumcodereview.appspot.com/10127004/diff/11001/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/11001/gclient.py#newcode854
gclient.py:854: # Append any target OS that is not already being enforced to the
tuple.
Why not this two-liners instead? The only reason I'm only putting it on 2 lines
instead of 1 to improve readability and fit in 80 cols. :)

target_os = config_dict.get('target_os', [])
self._enforced_os = tuple(set(self._enforced_os).union(target_os))

[Peter Beverloo, 2012-04-27 15:55:49]

Updated the patch, thanks.
Good catch! I am not completely familiar with the Python language, so any advice
on the builtins is always welcome :).

[M-A Ruel, 2012-04-27 16:03:37]

lgtm with style nit.

https://chromiumcodereview.appspot.com/10127004/diff/13003/tests/gclient_test.py
File tests/gclient_test.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/13003/tests/gclient_test...
tests/gclient_test.py:289: solutions = [
style nit: If you look at the rest of this file, I tend to never use """foo"""
for data because it screws up readability IMHO. The irregular alignment make the
whole file much harder to quickly scan the test cases. And unreadable unit tests
are unit tests that are never improved. So in general, I prefer something like:
foo = (
  'bar\n'
  'baz\n'
  'booze\n'
)

It only requires a few vim keystroke to convert and the constant is not affected
by the file's EOL style for example.

[John Grabowski, 2012-04-27 16:06:26]

https://chromiumcodereview.appspot.com/10127004/diff/13003/tests/gclient_test.py
File tests/gclient_test.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/13003/tests/gclient_test...
tests/gclient_test.py:289: solutions = [
On 2012/04/27 16:03:37, Marc-Antoine Ruel wrote:
> style nit: If you look at the rest of this file, I tend to never use """foo"""
> for data because it screws up readability IMHO. The irregular alignment make
the
> whole file much harder to quickly scan the test cases. And unreadable unit
tests
> are unit tests that are never improved. So in general, I prefer something
like:
> foo = (
>   'bar\n'
>   'baz\n'
>   'booze\n'
> )
> 
> It only requires a few vim keystroke to convert and the constant is not
affected
> by the file's EOL style for example.

Ha!  I told pete the opposite.
I think """ is preferred in general for multi-line strings, but this is your
file...

[M-A Ruel, 2012-04-27 16:08:07]

On 2012/04/27 16:06:26, John Grabowski wrote:
> Ha!  I told pete the opposite.
> I think """ is preferred in general for multi-line strings, but this is your
> file...

$ grep "set fold" ~/.vimrc
set foldmethod=indent
set foldlevel=99


And I feel sooo much ownership on it. :)

[M-A Ruel, 2012-04-27 16:11:53]

lgtm with yet another nit

https://chromiumcodereview.appspot.com/10127004/diff/19001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://chromiumcodereview.appspot.com/10127004/diff/19001/tests/gclient_test...
tests/gclient_test.py:310: self.assertEqual(4, len(self._get_processed()))
you could also
self.assertEquals(['baz', 'unix'], sorted(obj.enforced_os))

[commit-bot: I haz the power, 2012-04-27 16:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/10127004/21001

[commit-bot: I haz the power, 2012-04-27 16:24:27]

Presubmit check for 10127004-21001 failed and returned exit status 1.

warning: code.google.com certificate with fingerprint
8f:05:fc:c2:e3:c9:2e:c6:88:8c:f7:62:f4:64:92:35:57:45:39:14 not verified (check
hostfingerprints or web.cacerts config setting)
Running presubmit commit checks ...
************* Module gclient_test
W0612:304,13:GclientTest.testTargetOS: Unused variable 'args'
Checking out rietveld...
Running push-basic.sh
Running save-description-on-failure.sh
Running basic.sh
Running upload-stale.sh
Running upload-local-tracking-branch.sh
Running patch.sh
Running submit-from-new-dir.sh
Running post-dcommit-hook-test.sh
Running hooks.sh
Running abandon.sh
Running upstream.sh

** Presubmit ERRORS **
Fix pylint errors first.

Presubmit checks took 441.4s to calculate.

[commit-bot: I haz the power, 2012-04-27 16:26:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/10127004/23001

[commit-bot: I haz the power, 2012-04-27 16:34:39]

Change committed as 134280