change cloning in ensure_dependecies

[sergz]

Environment

We have some dependencies for v8 and dependecies file in libadblockplus is going to contain something like

third_party/v8 = v8-googlesource hg:b0526a8c076e git:97fafa7b1c277bcc627495318fe309ba9c2e3081
third_party/v8/base/trace_event/common = chromium-trace_event-common hg:afa6c508d4a7 git:06294c8a4a6f744ef284cd63cfe54dbf61eea290
third_party/v8/build = chromium-src-build hg:71b1606b07cf 3983535ba9ae6e2dc501e59fde158018d811f95c
third_party/v8/testing/gtest = googletest-github hg:324727ee9ad7 git:6f8a66431cb592dad629028a50b3dd418a408c87
third_party/v8/tools/gyp = gyp hg:9282ef983052 git:e7079f0e0e14108ab0dba58728ff219637458563
third_party/v8/tools/clang = chromium-src-tools-clang hg:24cee4531b21 git:286099f3928097ad1602710b550dfc1ff5c22142
third_party/v8/third_party/icu = chromium-deps-icu hg:dcc0a556c207 git:9cd2828740572ba6f694b9365236a8356fd06147

The important thing here is that we have dependencies for a dependency, e.g. third_party/v8/base/trace_event/common for third_party/v8.

However, in this case ensure_dependecies.py firstly creates third_party/v8 directory and downloads v8 dependencies and afterwards cannot clone v8.

How to solve it

Clone the repositories in alphabetic order, so that any parent directory is cloned first.

[sergz]

Just in case, I have fixed already ensure_dependecies.py locally but only for git and would be glad to share it if someone decides to work on it.

[trev]

I'm missing something from the description. We are usually creating our own copies of dependencies. So why do we want to force dependencies on V8 from outside rather than add ensure_dependencies.py and dependencies files to it and clone recursively as usual?

[sergz]

We are usually creating our own copies of dependencies.

Sorry, what do you mean? It is supposed that there will be our own copies of dependencies, just in case the issue is #4933.

So why do we want to force dependencies on V8 from outside rather than add ensure_dependencies.py and dependencies files to it and clone recursively as usual?

I merely wanted to avoid any commits in the mirror of V8 until it is really necessary, and bringing of ensure_dependencies.py and dependencies files into our mirror of V8 repository looks redundant here. Another thing is that it looks questionable to have the dependencies file which is specific for libadblockplus in V8 repository because V8 is actually not supposed to be used without other dependencies which are not mentioned in #4933. On the another hand the proposed change is not only for libadblockplus, it is a more reliable way of checking of the existence of a dependency and of cloning of the latter.

[fhd]

Note that this is meant for dependencies of V8 - transitive dependencies of libadblockplus. We have three options for fetching those:

1) Use V8's own dependency mechanism - Apparently we don't want to do that since that would download many things we don't need for embedding V8.

2) Use ensure_dependencies.py - We could do this either by implementing support for fetching transitive dependencies, or by placing a dependencies file in our fork of the v8 repo.

3) Make the libadblockplus build code fetch those dependencies at build time.

Assuming that (1) is indeed not a good option, we can go for either (2) or (3). Sergei wants to go for (2), but he doesn't want to commit the dependencies file to the v8 repo. I'm also not a big fan of doing that - we shouldn't have our fork diverge from upstream if possible.

[sergz]

I have updated the excerpt from dependencies file in the issue description to avoid any potential confusion.

[kzar]

Probably a dumb idea but could we have all those dependencies live directly inside the third_party/ directory and then have the build script set up the nested structure using symlinks before compiling?

[sergz]

Replying to kzar:

Probably a dumb idea but could we have all those dependencies live directly inside the third_party/ directory and then have the build script set up the nested structure using symlinks before compiling?

libadblockplus is also used on Windows and symlinks have some pitfalls there and are not a real option.

Basically, during the development I used a simple script, only fetching these dependencies (the 3rd option) and very soon I had noticed that if we continued with that script it would very soon duplicate the functionality which is already in ensure_dependencies.py.

So, I would say currently the choice is between two options. The first one is to have small and specific for libadblockplus diverges with commits adding ensure_dependencies.py and dependencies files and tags pointing to these commits in our fork of v8. To better understand that it's not a good option, please consider the following alternative: since v8 and buildtools are both equal dependencies for libadblockplus we could do the same trick for buildtools repository, namely to have small and specific for libadblockplus diverges with commits extending ensure_dependencies.py file and tags pointing to these commits in our buildtools repository. The second one is this issue, namely to make ensure_dependecies.py a bit more robust and use it.

[sergz]

It would be better to get some conclusion for this issue sooner because it's basically the only one issue really blocking now updating of v8. I'm ready for any hack merely for the sake of the progress because using of old v8 negatively affects many other parts.

[sebastian]

I don't have a strong opinion (yet).

But for reference, back when we used jsHydra, our ES6->ES5 transpiler, which depended on SpiderMonkey's jsshell, we had the build script download the jshell build on the fly during build. It seems the case here is very similar, but since I'm not familiar with the build scripts of libadblockplus, I cannot judge how appropriate the approach would be there. But perhaps it's not the worst option.

As for adding complexity and use cases to ensure_dependencies.py, if we go that route, I'd first like to have some test suite. We should have one anyway, but in particular if we add further functionality that is only used by very few repositories.

How exactly is this blocking the V8 update? Didn't you say that you have patched ensure_dependencies.py in libadblockplus already as a workaround? Not to mention that some options considered above wouldn't even involve changes to ensure_dependencies.py.

[sergz]

Replying to sebastian:

I don't have a strong opinion (yet).

But for reference, back when we used jsHydra, our ES6->ES5 transpiler, which depended on SpiderMonkey's jsshell, we had the build script download the jshell build on the fly during build. It seems the case here is very similar, but since I'm not familiar with the build scripts of libadblockplus, I cannot judge how appropriate the approach would be there. But perhaps it's not the worst option.

Yes, we can have a special script, but it will be duplicating ensure_dependencies.py and in contrast to jsHydra, which can be run during building phase, that fetching script should be run exactly between normal ensure_dependencies.py and generation of Makefile/VS solution. It just looks so wrong when we can use normal ensure_dependencies.py.

As for adding complexity and use cases to ensure_dependencies.py, if we go that route, I'd first like to have some test suite. We should have one anyway, but in particular if we add further functionality that is only used by very few repositories.

I'm totally for having tests but I think that their introduction should not be a part of this issue nor should delay it. BTW, there is even functionality which is, as far as I know, not used in repositories under adblockplus account, e.g. what I find very useful is to have a possibility to reference a dependency using an URL. Yet one moment here is that I don't consider the proposed changes as specific only for libadblockplus, I find them rather as an improvement of ensure_dependecies.py in general. If we are talking about only libadblockplus specific changes then they should be of course done not in buildtools, which is used in many projects, but rather in libadblockplus itself or at least in v8 repository, which is not so widely used.

How exactly is this blocking the V8 update? Didn't you say that you have patched ensure_dependencies.py in libadblockplus already as a workaround? Not to mention that some options considered above wouldn't even involve changes to ensure_dependencies.py.

locally means on my computer, I cannot patch it in libadblockplus without updating it in buildtools repo because it will overwrite itself on the next run. Right, there are other options but I think that what is proposed in this issue is a better way in comparison with other options, so just in case before proceeding with some another option I would like to ensure that the proposed way cannot be used.

[abpbot]

A commit referencing this issue has landed:
​Issue 4906 - change cloning in ensure_dependecies

[Ross]

Done. Running ensure_dependencies.py still works as expected.

adblockpluschrome / 1846:44197859df84