Use ensure_dependencies.py instead of subrepositories

Use ensure_dependencies.py instead of subrepositories

[Wladimir Palant, 2015-03-13 21:02:54]

LGTM

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
File dependencies (right):

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
dependencies:3: buildtools = buildtools hg:d06704a466c5
Note that you might want to reuse the buildtools repository within adblockplus
instead - it's the right revision to build Adblock Plus.

[René Jeschke, 2015-03-13 21:03:12]

LGTM

(I assume ensure_dependencies.py isn't needed to be reviewed :D )

[Felix Dahlke, 2015-03-15 23:01:54]

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
File dependencies (right):

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
dependencies:3: buildtools = buildtools hg:d06704a466c5
On 2015/03/13 21:02:55, Wladimir Palant wrote:
> Note that you might want to reuse the buildtools repository within adblockplus
> instead - it's the right revision to build Adblock Plus.

That's what I'm doing actually - only including this here since it seems
ensure_dependencies.py needs this for self updating.

Could I remove this line and change the one above to this:

_self = adblockplus/adblockplus/buildtools/ensure_dependencies.py

and it'd self update properly?

Whether or not it works - I don't think it's a good idea to depend on a
particular transitive dependency's existence here, so I'd rather keep it as it
is or remove _self (seems to work, disabling self updating though).

[Wladimir Palant, 2015-03-16 11:59:33]

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
File dependencies (right):

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
dependencies:3: buildtools = buildtools hg:d06704a466c5
On 2015/03/15 23:01:54, Felix H. Dahlke wrote:
> Could I remove this line and change the one above to this:
> 
> _self = adblockplus/adblockplus/buildtools/ensure_dependencies.py
> 
> and it'd self update properly?

Yes, that's what you should do. You can always test the self-update by making a
minimal change to your ensure_dependencies.py - it should revert that change on
a dependency update.

> Whether or not it works - I don't think it's a good idea to depend on a
> particular transitive dependency's existence here, so I'd rather keep it as it
> is or remove _self (seems to work, disabling self updating though).

There is a reason why we use _self - people will forget to update
ensure_dependencies.py otherwise. Note that if
adblockplus/adblockplus/buildtools doesn't exist then nothing will fail - there
will be merely a warning. So IMHO definitely better than having an additional
dependency for no good reason or disabling self-update altogether.

[Felix Dahlke, 2015-03-17 14:49:09]

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
File dependencies (right):

http://codereview.adblockplus.org/5924365505921024/diff/5629499534213120/depe...
dependencies:3: buildtools = buildtools hg:d06704a466c5
On 2015/03/16 11:59:33, Wladimir Palant wrote:
> On 2015/03/15 23:01:54, Felix H. Dahlke wrote:
> > Could I remove this line and change the one above to this:
> > 
> > _self = adblockplus/adblockplus/buildtools/ensure_dependencies.py
> > 
> > and it'd self update properly?
> 
> Yes, that's what you should do. You can always test the self-update by making
a
> minimal change to your ensure_dependencies.py - it should revert that change
on
> a dependency update.
> 
> > Whether or not it works - I don't think it's a good idea to depend on a
> > particular transitive dependency's existence here, so I'd rather keep it as
it
> > is or remove _self (seems to work, disabling self updating though).
> 
> There is a reason why we use _self - people will forget to update
> ensure_dependencies.py otherwise. Note that if
> adblockplus/adblockplus/buildtools doesn't exist then nothing will fail -
there
> will be merely a warning. So IMHO definitely better than having an additional
> dependency for no good reason or disabling self-update altogether.

I still think it makes more sense to depend on buildtools directly for that, but
I don't have a particularly strong opinion. Going with using the one required by
adblockplus now - less stuff in the root directory at least :)

[René Jeschke, 2015-03-18 09:04:03]

LGTM

[Wladimir Palant, 2015-03-18 15:31:08]

LGTM