Clean up gclient MergeDependencies

Clean up gclient MergeDependencies

Fix warnings with non-conflicted os deps which use FromImpl, and
update sorting order so conflicts are resolved the same way across a
checkout.

R=maruel@chromium.org
BUG=248168

[Isaac (away), 2013-12-17 05:59:28]

https://chromiumcodereview.appspot.com/101713003/diff/1/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/101713003/diff/1/gclient.py#newcode495
gclient.py:495: for _, val in reversed(os_dep_values):
A fancy way to do this is:

val_gen = (val for val in reversed(os_dep_values) if val)
target_os_deps[os_dep_key] = next(val_gen, None)

But this is 3x slower, and probably more confusing.

[Daniel Bratell, 2013-12-17 08:38:19]

So the problem was that some of the objects didn't behave as expected in hash
tables. I see. The chromium deps file doesn't use the From object so I didn't
encounter it.

I think you're worrying too much about performance in this code. It will be fast
enough even if you ran it a thousand times on ten times as big inputs. gclient
is slow but it's not python execution speed that is the problem but accesses to
the file system and use of external tools (and output sizes in some cases on
Windows). 

I'd worry more about readability and maintainability. It wasn't instant to
understand what the code tried to do (and that is my main problem with a lot of
code in many systems, it rarely hints at what it intends to do because the
initial developer knows what he intends to do and doesn't even think about
writing it down).

But lgtm (not an owner so just informative).

https://chromiumcodereview.appspot.com/101713003/diff/20001/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/101713003/diff/20001/gclient.py#newcod...
gclient.py:499: return dict(deps, **target_os_deps)
My pylint (not the one bundled) is noisily unhappy about ** constructs (because
they are hard to read/understand and maintain I guess; "Used * or ** magic"). In
other parts of gclient it looks like pylint's observations have been a factor
(comments to silence it). Is that a factor here as well?

[Ami GONE FROM CHROMIUM, 2013-12-17 17:50:38]

drive-by

FYI this patch removes the error log in a webrtc checkout.

(personally I think that anything /other/ than turning that logging.error() into
a fatal error is unlikely to stay unbroken for long :( )

[Ami GONE FROM CHROMIUM, 2014-02-27 22:20:51]

Isaac/M-A: ping?  This is still confusing people (e.g.
http://code.google.com/p/webrtc/issues/detail?id=2152)


On Tue, Dec 17, 2013 at 9:50 AM, <fischman@chromium.org> wrote:

> drive-by
>
> FYI this patch removes the error log in a webrtc checkout.
>
> (personally I think that anything /other/ than turning that
> logging.error() into
> a fatal error is unlikely to stay unbroken for long :( )
>
> https://chromiumcodereview.appspot.com/101713003/
>

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

[M-A Ruel, 2014-02-27 22:31:20]

The issue linked to is 403 for me.

+Robbie if he has an opinion. I don't mind so lgtm.

[Ami GONE FROM CHROMIUM, 2014-02-27 22:34:31]

On Thu, Feb 27, 2014 at 2:31 PM, <maruel@chromium.org> wrote:

> The issue linked to is 403 for me.
>

You mean https://code.google.com/p/chromium/issues/detail?id=248168 ?   I
can open it in an incognito window and it has no Restrict labels...

https://chromiumcodereview.appspot.com/101713003/
>

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

[M-A Ruel, 2014-02-27 22:36:38]

On 2014/02/27 22:34:31, Ami Fischman wrote:
> On Thu, Feb 27, 2014 at 2:31 PM, <mailto:maruel@chromium.org> wrote:
> 
> > The issue linked to is 403 for me.
> >
> 
> You mean https://code.google.com/p/chromium/issues/detail?id=248168 ?   I
> can open it in an incognito window and it has no Restrict labels...

I meant http://code.google.com/p/webrtc/issues/detail?id=2152.

[Ami GONE FROM CHROMIUM, 2014-02-27 22:39:39]

On Thu, Feb 27, 2014 at 2:36 PM, <maruel@chromium.org> wrote:

> I meant http://code.google.com/p/webrtc/issues/detail?id=2152.
>

Ah, cc'd you now.

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