Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#4906 closed defect (fixed)

change cloning in ensure_dependecies

Reported by: sergz Assignee:
Priority: P2 Milestone:
Module: Automation Keywords:
Cc: kzar, sebastian, fhd, kvas, trev Blocked By:
Blocking: #4907 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29408728/
https://codereview.adblockplus.org/29452663/

Description (last modified by sebastian)

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.

Change History (18)

comment:1 Changed 3 years ago by sergz

  • Blocking 4907 added

comment:2 Changed 3 years ago by 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.

comment:3 Changed 3 years ago by sebastian

  • Cc kvas trev added

comment:4 Changed 3 years ago by sergz

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:5 Changed 3 years ago by 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?

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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.

Last edited 3 years ago by fhd (previous) (diff)

comment:8 Changed 3 years ago by sergz

  • Description modified (diff)

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

comment:9 follow-up: Changed 3 years ago by 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?

comment:10 in reply to: ↑ 9 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by 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.

Last edited 3 years ago by sergz (previous) (diff)

comment:12 follow-up: Changed 3 years ago by 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.

Last edited 3 years ago by sebastian (previous) (diff)

comment:13 in reply to: ↑ 12 Changed 3 years ago by 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.

comment:14 Changed 3 years ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:15 Changed 3 years ago by sergz

  • Review URL(s) modified (diff)

comment:16 Changed 3 years ago by abpbot

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

comment:17 Changed 3 years ago by sebastian

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:18 Changed 2 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Running ensure_dependencies.py still works as expected.

adblockpluschrome / 1846:44197859df84

Last edited 2 years ago by Ross (previous) (diff)
Note: See TracTickets for help on using tickets.