Handle conflicting os deps better in gclient.

Handle conflicting os deps better in gclient.

It's possible in gclient to list multiple operating systems on the
command line and while handling them all perfectly might not be possible
it's possible to make it better than today.

This patch makes sure None never overrides any previous value and it
adds some output to indicate what kind of dangerous choices it made.

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

[Daniel Bratell, 2013-09-16 15:11:24]

This solves a problem where a one os set a deps to None which made it never be
checked out.

This is currently true for "src/third_party/cld_2/src" in android so if you try
to do
gclient sync --deps_os=win,android (which we did at Opera)
you will get no cld_2.

The patch does one change (never let None override not-None) and it adds
diagnostic output to show possible checkout problems (right now the only one is
that lighttpd has different versions for mac and win).

[iannucci, 2013-09-16 17:43:30]

On 2013/09/16 15:11:24, Daniel Bratell wrote:
> This solves a problem where a one os set a deps to None which made it never be
> checked out.
> 
> This is currently true for "src/third_party/cld_2/src" in android so if you
try
> to do
> gclient sync --deps_os=win,android (which we did at Opera)
> you will get no cld_2.
> 
> The patch does one change (never let None override not-None) and it adds
> diagnostic output to show possible checkout problems (right now the only one
is
> that lighttpd has different versions for mac and win).

Does it make sense to pick the highest revision for conflicting revs? Otherwise
I think we're at the whim of python's dict() ordering (which is not stable).
Highest may not be right, but at least it's predictable (and the warning is
still good to have too).

[Isaac (away), 2013-09-16 21:58:18]

The cld_2 deps was incorrectly setup.  It should not have been removed from
android, and was fixed here: https://codereview.chromium.org/23851013/

[iannucci, 2013-09-16 22:54:40]

few more comments

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

https://codereview.chromium.org/23875029/diff/1/gclient.py#newcode493
gclient.py:493: if len(self.target_os) > 0:
what values can target_os have? can it be None?

Would it work to just do `if 'deps_os' in local_scope and self.target_os:`?

https://codereview.chromium.org/23875029/diff/1/gclient.py#newcode498
gclient.py:498: set_by_os[os_dep_key] = the_first_os
Hm... Isn't this loop doing exactly the same thing as fromkeys?

https://codereview.chromium.org/23875029/diff/1/gclient.py#newcode518
gclient.py:518: print('Conflicting dependencies for %s. %s wants %s '
this should be logging.warning, probably

[Daniel Bratell, 2013-09-17 08:11:58]

On 2013/09/16 17:43:30, iannucci wrote:
> Does it make sense to pick the highest revision for conflicting revs?
Otherwise
> I think we're at the whim of python's dict() ordering (which is not stable).
> Highest may not be right, but at least it's predictable (and the warning is
> still good to have too).

I believe the target_os list to be well ordered. It's generated by joining lists
in a tree structure and while I don't understand all the ways this code can be
used, it did seem to be a stable ordered list.

I considered revision ordering but with branches and git, it is not easy to
determine which revision is the one with the most recent code. Higher svn number
might still be from an old branch with a recent commit, and in git's it's even
worse since there is no version number ordering. So first os setting a property
to non-None wins is easy to understand and it's predictable.

On 2013/09/16 22:54:40, iannucci wrote:
> few more comments
> 
> https://codereview.chromium.org/23875029/diff/1/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/23875029/diff/1/gclient.py#newcode493
> gclient.py:493: if len(self.target_os) > 0:
> what values can target_os have? can it be None?

I don't think so. The fallback value is self._enforced_os which is set in
GClient.__init__ and while it can be set in four different ways, they all seem
to produce a list (or in once case a tuple). Possibly (but not even sure about
that) an empty list but not None.

> Would it work to just do `if 'deps_os' in local_scope and self.target_os:`?

I think so. I'll do that change.
 
> https://codereview.chromium.org/23875029/diff/1/gclient.py#newcode498
> gclient.py:498: set_by_os[os_dep_key] = the_first_os
> Hm... Isn't this loop doing exactly the same thing as fromkeys?

Doh, I was pondering which version would be most likely accepted. The idea was
not to keep both. :-)

> https://codereview.chromium.org/23875029/diff/1/gclient.py#newcode518
> gclient.py:518: print('Conflicting dependencies for %s. %s wants %s '
> this should be logging.warning, probably

I tried changing it to logging.warning but it turns out the default logging
level is logging.ERROR so the user wasn't told and this is more important than
most of the other messages sent out. So either use print, or change the default
message level to logging.WARNING (or both).

[Daniel Bratell, 2013-09-18 18:23:11]

iannucci, can you please take a new look.

[Ami GONE FROM CHROMIUM, 2013-11-27 17:39:05]

@ilevy: can you review this for
https://code.google.com/p/chromium/issues/detail?id=248168 ?

[Daniel Bratell, 2013-12-09 09:07:52]

On 2013/11/27 17:39:05, Ami Fischman wrote:
> @ilevy: can you review this for
> https://code.google.com/p/chromium/issues/detail?id=248168 ?

The bug triggers again now for src/third_party/libaddressinput if you ask for
both Windows and Android source. Then that lib will be missing and WIndows won't
compile.

ilevy, can you please take a look?

[Isaac (away), 2013-12-09 10:33:12]

OK I took a look and sketched out some example code which I think is cleaner. 
Let me know what you think -- you don't have to use it verbatim.

https://codereview.chromium.org/23875029/diff/10001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode491
gclient.py:491: if 'deps_os' in local_scope and self.target_os:
self.target_os is a recursive function, let's save to a local variable.

target_os_list = self.target_os
if ....

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode493
gclient.py:493: the_first_os = self.target_os[0]
It seems like we're going to have two decision paths: 1 os dep, and multiple. 
So let's explicitly separate those.  For just 1, all we need is
target_os_deps.update(os_deps). 

For more than 1 OS, let's split this into two parts.
a) data processing
b) decision making.

target_os_list = self.target_os
if 'deps_os' in local_scope and target_os_list:
  os_deps_f = lambda os_key : local_scope['deps_os'].get(os_key, {})
  if len(target_os_list) == 1:
    os_deps = os_deps_f(target_os_list[0])
  elif len(target_os_list) > 1:
    <new logic here>.....

  target_os.update(os_deps)



os_deps_d = {}  #  deps -> [(os, val), ...]
for os_key in target_os_list:
  for os_dep_key, os_dep_value in os_deps_f(os_key).iteritems():
    os_deps_d.setdefault(os_dep_key, []).append((os_key, os_dep_value))

os_deps = {}
for dep_key, os_deps_vals in os_deps_d.iteritems():
  if all(val is None for_, val in os_deps_vals) and len(os_deps_vals) ==
len(target_os_list):
    os_deps[dep_key] = None
  else:
    non_nones = sorted(dep_tup for dep_tup in os_deps_vals if dep_tup is not
None)
    if len(non_nones) > 1:
      logging.error('Choosing alphabetically.  You better know what you're
doing!').
    if non_nones:
       os_deps[deps_key] = non_nones[0][1]

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode515
gclient.py:515: print('Conflicting dependencies for %s. %s wants %s '
Use logging calls instead of print

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode517
gclient.py:517: '%s will be used since %s was listed before %s.' %
Order is completely arbitrary in this list since target_os is passed through
set() multiple times.  I'd say we should just fail if multiple targets oses
specify conflicting revisions, which is just weird.

[Isaac (away), 2013-12-09 10:40:25]

two follow ups:
- Looking at the code I sketched out -- 1 target_os dep is the same logic as
multiple, so we don't need to special case it after all.  (I was thinking the
logic for removing deps would be different).
- Seems fine to log an error instead of failing when deciding between multiple
revisions specified.

[Daniel Bratell, 2013-12-11 18:09:48]

Updated the code as sketched.

https://codereview.chromium.org/23875029/diff/10001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode491
gclient.py:491: if 'deps_os' in local_scope and self.target_os:
On 2013/12/09 10:33:13, Isaac wrote:
> self.target_os is a recursive function, let's save to a local variable.
> 
> target_os_list = self.target_os
> if ....

To make the code faster?

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode517
gclient.py:517: '%s will be used since %s was listed before %s.' %
On 2013/12/09 10:33:13, Isaac wrote:
> Order is completely arbitrary in this list since target_os is passed through
> set() multiple times.  I'd say we should just fail if multiple targets oses
> specify conflicting revisions, which is just weird.

The problem with that is that it won't be the person that makes the mistake that
will be affected but someone else innocent. It's possible that both conflicting
versions would have "worked", but instead there will be no build at all. It's
always more pleasurable punishing the one causing the conflict than the innocent
victim. :-)

But I'll raise an error. If it happens, hopefully the victim knows how to
manually edit DEPS files.

[Isaac (away), 2013-12-11 18:58:27]

More comments just about the core algorithm

https://codereview.chromium.org/23875029/diff/10001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode491
gclient.py:491: if 'deps_os' in local_scope and self.target_os:
On 2013/12/11 18:09:49, Daniel Bratell wrote:
> On 2013/12/09 10:33:13, Isaac wrote:
> > self.target_os is a recursive function, let's save to a local variable.
> > 
> > target_os_list = self.target_os
> > if ....
> 
> To make the code faster?

yes, also because in theory the order could change on successive calls.
#premature optimization

https://codereview.chromium.org/23875029/diff/13001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/13001/gclient.py#newcode518
gclient.py:518: if len(os_dep_value) != len(target_os_list):
What does this loop do?  Can you add a comment?

https://codereview.chromium.org/23875029/diff/13001/gclient.py#newcode525
gclient.py:525: possible_values = set([x[1] for x in os_dep_value if x[1] is not
None])
line length, but you can remove the unneeded brackets and get under 80.
i.e. set(x[1] for .... not None)

https://codereview.chromium.org/23875029/diff/13001/gclient.py#newcode529
gclient.py:529: raise gclient_utils.Error('Inconsistent checkout instructions.')
I gave conflicting suggestions here.  in the first I suggest raising an error
and in the follow I said logging would be OK.  I think it's fine to remove this
and log the choices.  If we do pick one, we need to do so deterministically --
see the 'sorted' call in the sketch.

[Daniel Bratell, 2013-12-12 08:53:25]

On 2013/12/11 18:58:27, Isaac wrote:
> More comments just about the core algorithm
> 
> https://codereview.chromium.org/23875029/diff/10001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/23875029/diff/10001/gclient.py#newcode491
> gclient.py:491: if 'deps_os' in local_scope and self.target_os:
> On 2013/12/11 18:09:49, Daniel Bratell wrote:
> > On 2013/12/09 10:33:13, Isaac wrote:
> > > self.target_os is a recursive function, let's save to a local variable.
> > > 
> > > target_os_list = self.target_os
> > > if ....
> > 
> > To make the code faster?
> 
> yes, also because in theory the order could change on successive calls.
> #premature optimization
> 
> https://codereview.chromium.org/23875029/diff/13001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/23875029/diff/13001/gclient.py#newcode518
> gclient.py:518: if len(os_dep_value) != len(target_os_list):
> What does this loop do?  Can you add a comment?

Done.

> https://codereview.chromium.org/23875029/diff/13001/gclient.py#newcode525
> gclient.py:525: possible_values = set([x[1] for x in os_dep_value if x[1] is
not
> None])
> line length, but you can remove the unneeded brackets and get under 80.
> i.e. set(x[1] for .... not None)

Or reduce the indentation level. I just saw your comment about being able to use
the same code for 1 os and 1+ os:es. I find it a bit hard to keep track of
comments in rietvald. It's a bit sub-optimal compared to the system we use
internally at Opera (https://github.com/jensl/critic/) but maybe I just have to
get used to it. Anyway, indentation level reduced, line length dropped from 80
to 78.

> https://codereview.chromium.org/23875029/diff/13001/gclient.py#newcode529
> gclient.py:529: raise gclient_utils.Error('Inconsistent checkout
instructions.')
> I gave conflicting suggestions here.  in the first I suggest raising an error
> and in the follow I said logging would be OK.  I think it's fine to remove
this
> and log the choices.  If we do pick one, we need to do so deterministically --
> see the 'sorted' call in the sketch.

Sorted added.

[Isaac (away), 2013-12-12 09:41:01]

Sorry one last round of nits.  This lgtm -- but needs approval from an OWNER.

https://codereview.chromium.org/23875029/diff/33001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/33001/gclient.py#newcode526
gclient.py:526: possible_values = set([x[1] for x in os_dep_value if x[1] is not
None])
remove brackets (it's best practice so python can use a generator instead of
constructing a list)

https://codereview.chromium.org/23875029/diff/33001/gclient.py#newcode527
gclient.py:527: if len(possible_values) == 0:
Usually written as "if not possible_values".  However simpler might be:

possible_values = set(x[1] for x in os_dep_value)
if len(possible_values) > 1:
  # log error
  possible_values.discard(None)
target_os_deps[os_dep_key] = sorted(possible_values)[0]

https://codereview.chromium.org/23875029/diff/33001/gclient.py#newcode534
gclient.py:534: logging.error('Conflicting dependencies for %s: %s.' %
Nit, best practice here is to use logging implicit string formatting.  While
less important in this case, it avoids computation on logging calls that fall
below the current error threshold.  In this case:

logging.error('Conflicting...', os_dep_key, os_dep_value)

[M-A Ruel, 2013-12-12 14:14:53]

It's the kind of tricky code I'd like to see unit tested. It could be extracted
into a function that can them be easily unit tests for know good values.

[Daniel Bratell, 2013-12-12 16:26:14]

Now better than ever. With tests.

https://codereview.chromium.org/23875029/diff/33001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/33001/gclient.py#newcode527
gclient.py:527: if len(possible_values) == 0:
On 2013/12/12 09:41:01, Isaac wrote:
> Usually written as "if not possible_values".  However simpler might be:
> 
> possible_values = set(x[1] for x in os_dep_value)
> if len(possible_values) > 1:
>   # log error
>   possible_values.discard(None)
> target_os_deps[os_dep_key] = sorted(possible_values)[0]

It's not an error (or even unusual) to have more than one value if you include
None. None is used to optimize checkouts so it's very common but can safely be
ignored. That is the reason I remove all Nones from the list (and that is the
reason it looks more complicated that you would expect).

https://codereview.chromium.org/23875029/diff/33001/gclient.py#newcode534
gclient.py:534: logging.error('Conflicting dependencies for %s: %s.' %
On 2013/12/12 09:41:01, Isaac wrote:
> Nit, best practice here is to use logging implicit string formatting.  While
> less important in this case, it avoids computation on logging calls that fall
> below the current error threshold.  In this case:
> 
> logging.error('Conflicting...', os_dep_key, os_dep_value)

gclient is super super slow but I don't think it's because of python string
operations. I happened to have Process Explorer open while running it. It's
scary to see the number of sub processes it creates while running, as well as
the very heavy git commands they run even when gclient sync is a NOP. There is
plenty of room for optimizations for someone.

But I've planned to learn the logging formatting anyway so this was an excuse.

[Daniel Bratell, 2013-12-13 07:53:16]

maruel (or some other code owner; dpranke?), does the last patch look good? It
has tests! It even passes the tests.

[M-A Ruel, 2013-12-13 15:33:35]

https://codereview.chromium.org/23875029/diff/93001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://codereview.chromium.org/23875029/diff/93001/tests/gclient_test.py#new...
tests/gclient_test.py:541: {'os1': { 'bar': 'os1_bar' }},
I think you meant:
{'os1': { 'bar': 'os1_bar' }, 'os2': { 'bar': 'os1_bar' }},

[Daniel Bratell, 2013-12-16 09:32:42]

Good catch. Added the missing line in the test and it passes.

Is that the last problem we know about?

[M-A Ruel, 2013-12-16 13:18:18]

https://codereview.chromium.org/23875029/diff/113001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/113001/gclient.py#newcode470
gclient.py:470: def UpdateWithOsDeps(deps, deps_os, target_os_list):
The method doesn't make it obvious what is input or output parameters. I'd
recommend to return a fresh new dict and not modify input arguments, it'll make
the whole thing easier to understand.

https://codereview.chromium.org/23875029/diff/113001/gclient.py#newcode544
gclient.py:544: Dependency.UpdateWithOsDeps(deps, local_scope['deps_os'],
target_os_list)
self.UpdateWithOSDeps(...

[Daniel Bratell, 2013-12-16 13:56:08]

Changed per request.

[M-A Ruel, 2013-12-16 14:26:54]

lgtm

[commit-bot: I haz the power, 2013-12-16 14:32:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/23875029/133001

[commit-bot: I haz the power, 2013-12-16 14:34:14]

Change committed as 240892

[Ami GONE FROM CHROMIUM, 2013-12-16 22:33:47]

post-commit drive-by

https://codereview.chromium.org/23875029/diff/133001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/133001/gclient.py#newcode500
gclient.py:500: logging.error('Conflicting dependencies for %s: %s.
(target_os=%s)',
It's interesting that this is logged even when the two dependencies are actually
the exact same (e.g. openssl in
https://code.google.com/p/webrtc/source/browse/trunk/DEPS?spec=svn5260&r=5260).

[M-A Ruel, 2013-12-16 23:20:52]

https://codereview.chromium.org/23875029/diff/133001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/133001/gclient.py#newcode500
gclient.py:500: logging.error('Conflicting dependencies for %s: %s.
(target_os=%s)',
On 2013/12/16 22:33:47, Ami Fischman wrote:
> It's interesting that this is logged even when the two dependencies are
actually
> the exact same (e.g. openssl in
>
https://code.google.com/p/webrtc/source/browse/trunk/DEPS?spec=svn5260&r=5260).

ok so the best would likely be to log only if:
if len(set(possible_Values)) > 1:

[Isaac (away), 2013-12-16 23:26:49]

https://codereview.chromium.org/23875029/diff/133001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/133001/gclient.py#newcode503
gclient.py:503: target_os_deps[os_dep_key] = sorted(possible_values)[0]
Also, this sorted call should be sorting by target_os and not value, so that the
choice stays consistent across rolls.  default should be preferred.  Kind of
complicated, unfortunately.

[Ami GONE FROM CHROMIUM, 2013-12-16 23:36:55]

On Mon, Dec 16, 2013 at 3:20 PM, <maruel@chromium.org> wrote:

> if len(set(possible_Values)) > 1:


More like:
if len(set([str(x) for x in possible_values])) > 1:
I think.

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

[M-A Ruel, 2013-12-16 23:42:00]

Why? None is a perfectly valid value in a set. And you don't need to render
a generator into a list with [].


2013/12/16 Ami Fischman <fischman@chromium.org>

> On Mon, Dec 16, 2013 at 3:20 PM, <maruel@chromium.org> wrote:
>
>> if len(set(possible_Values)) > 1:
>
>
> More like:
> if len(set([str(x) for x in possible_values])) > 1:
> I think.
>
>

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

[Isaac (away), 2013-12-16 23:49:02]

https://codereview.chromium.org/23875029/diff/133001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/23875029/diff/133001/gclient.py#newcode500
gclient.py:500: logging.error('Conflicting dependencies for %s: %s.
(target_os=%s)',
possible_values is already a set... I'm not sure why Ami's example of webrtc is
printing a message.  The reason to ignore None for the error isn't because None
is an invalid option for set, but because we can cleanly merge an os which
doesn't need a dep and one which needs the dep at a specific revision.

[Ami GONE FROM CHROMIUM, 2013-12-16 23:59:27]

Sorry for being brief: I str()'ified the suggestion b/c otherwise the
len/comparisons are seemingly by identity instead of equality.


On Mon, Dec 16, 2013 at 3:49 PM, <ilevy@chromium.org> wrote:

>
> https://codereview.chromium.org/23875029/diff/133001/gclient.py
> File gclient.py (right):
>
> https://codereview.chromium.org/23875029/diff/133001/gclient.py#newcode500
> gclient.py:500: logging.error('Conflicting dependencies for %s: %s.
> (target_os=%s)',
> possible_values is already a set... I'm not sure why Ami's example of
> webrtc is printing a message.  The reason to ignore None for the error
> isn't because None is an invalid option for set, but because we can
> cleanly merge an os which doesn't need a dep and one which needs the dep
> at a specific revision.
>
> https://codereview.chromium.org/23875029/
>

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

[Daniel Bratell, 2013-12-17 08:04:58]

On 2013/12/16 22:33:47, Ami Fischman wrote:
> post-commit drive-by
> 
> https://codereview.chromium.org/23875029/diff/133001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/23875029/diff/133001/gclient.py#newcode500
> gclient.py:500: logging.error('Conflicting dependencies for %s: %s.
> (target_os=%s)',
> It's interesting that this is logged even when the two dependencies are
actually
> the exact same (e.g. openssl in
>
https://code.google.com/p/webrtc/source/browse/trunk/DEPS?spec=svn5260&r=5260).

I wonder why.

Do you have the output saved? Maybe it wasn't actually android,unix that
conflicted but something else.

Being a set, with None filtered, you'd expect there to be only one string there
if they are actually identical.

[Ami GONE FROM CHROMIUM, 2013-12-17 17:48:41]

On Tue, Dec 17, 2013 at 12:04 AM, <bratell@opera.com> wrote:

> On 2013/12/16 22:33:47, Ami Fischman wrote:
>
>> post-commit drive-by
>>
>
>  https://codereview.chromium.org/23875029/diff/133001/gclient.py
>> File gclient.py (right):
>>
>
>  https://codereview.chromium.org/23875029/diff/133001/
>> gclient.py#newcode500
>> gclient.py:500: logging.error('Conflicting dependencies for %s: %s.
>> (target_os=%s)',
>> It's interesting that this is logged even when the two dependencies are
>>
> actually
>
>> the exact same (e.g. openssl in
>>
>
> https://code.google.com/p/webrtc/source/browse/trunk/
> DEPS?spec=svn5260&r=5260).
>
> I wonder why.
>

I think you figured it out in the reviewlog of
https://chromiumcodereview.appspot.com/101713003/ but FTR, the output was:
gclient(501) MergeWithOsDeps:Conflicting dependencies for
third_party/openssl: [('unix', <__main__.FromImpl object at 0x288e050>),
('android', <__main__.FromImpl object at 0x288e0d0>)]. (target_os=('unix',
'android'))

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