Issue 4906 - change cloning in ensure_dependecies

[Wladimir Palant, 2017-04-10 15:07:02]

I consider this approach questionable, see
https://issues.adblockplus.org/ticket/4906#comment:5

[Sebastian Noack, 2017-05-30 07:22:39]

FWIW, I don't like the idea to reimplement the clone command, adding complexity
for a scenario which ensure_dependencies.py wasn't meant for, either. But I
don't have a better idea. So LGTM from my end. But I'd like to see Vasily and/or
Wladimir review this too.

[Wladimir Palant, 2017-05-31 11:04:45]

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies.py
File ensure_dependencies.py (right):

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies...
ensure_dependencies.py:324: for dir, sources in config.iteritems():
IMHO, sorting here should do:

  for dir, sources in sorted(config.iteritems()):

[Wladimir Palant, 2017-05-31 11:04:53]

If I understand the problem correctly, the scenario is that we have dependencies
going like that:

  v8 = ...
  v8/build = ...

If our code decided to check out v8/build before the v8 dependency, it will
create en empty v8 directory. In this scenario, not only will our current code
assume that v8 has been check out already, even if it doesn't - cloning to a
non-empty directory isn't allowed.

If that's the issue - why don't we solve it by changing the ordering so that v8
is *always* processed before v8/build? Doesn't that solve the problem?

[sergei, 2017-05-31 13:38:00]

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies.py
File ensure_dependencies.py (right):

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies...
ensure_dependencies.py:324: for dir, sources in config.iteritems():
On 2017/05/31 11:04:45, Wladimir Palant wrote:
> IMHO, sorting here should do:
> 
>   for dir, sources in sorted(config.iteritems()):

Tested, it does work in our case and it is sufficient. Should I switch to it?

Actually, I would rather prefer to have the proposed change and check items out
in the order they appear in the "dependencies" file and not in some random order
nor sorted by paths. That will really increase the flexibility because it will
allow to handle in addition some other potential cases, the sorting on the other
hand will make the tool too smart and less flexible.

[Sebastian Noack, 2017-05-31 14:46:09]

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies.py
File ensure_dependencies.py (right):

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies...
ensure_dependencies.py:324: for dir, sources in config.iteritems():
On 2017/05/31 13:38:00, sergei wrote:
> On 2017/05/31 11:04:45, Wladimir Palant wrote:
> > IMHO, sorting here should do:
> > 
> >   for dir, sources in sorted(config.iteritems()):
> 
> Tested, it does work in our case and it is sufficient. Should I switch to it?
> 
> Actually, I would rather prefer to have the proposed change and check items
out
> in the order they appear in the "dependencies" file and not in some random
order
> nor sorted by paths. That will really increase the flexibility because it will
> allow to handle in addition some other potential cases, the sorting on the
other
> hand will make the tool too smart and less flexible.

Now where I understand the problem, I don't see why anybody would want to
checkout a repo in a subdirectory before checking out the parent directory.
Neither can I think of a scenario in which this "flexibility" is necessary.

Not to mention that re-implementing git/hg clone just to make this scenario
work, isn't great, if we can either require the dependencies file to list the
repositories in the "right" order, or sort them to ensure a compatible order.

Wladimir, Vasily: What do you think?

[sergei, 2017-05-31 14:59:23]

It's superseded by https://codereview.adblockplus.org/29452663/

[Wladimir Palant, 2017-05-31 15:59:37]

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies.py
File ensure_dependencies.py (right):

https://codereview.adblockplus.org/29408728/diff/29408740/ensure_dependencies...
ensure_dependencies.py:324: for dir, sources in config.iteritems():
On 2017/05/31 14:46:09, Sebastian Noack wrote:
> Now where I understand the problem, I don't see why anybody would want to
> checkout a repo in a subdirectory before checking out the parent directory.
> Neither can I think of a scenario in which this "flexibility" is necessary.

Absolutely. This is a classic example where an unnecessarily complicated
approach is justified with "flexibility" that nobody really needs. We can always
do complicated later if we really need it.