Issue 29670555: Issue 6264 - Can't prepare libadblockplus dependencies (Closed)
|
Created:
Jan. 16, 2018, 5:29 a.m. by anton Modified:
Jan. 19, 2018, 7:05 a.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 6264 - Can't prepare libadblockplus dependencies
Now i just delete "libadblockplus/third_party" to let 'ensure_dependencies' check out the sources from our repo (it's replaced with Chromium's repo while replacing our deps to Chromium deps - see issue ticket).
Patch Set 1 #
Total comments: 4
Patch Set 2 : Formatted with respect to eyeo python code style and tested with flake8 #
Total comments: 5
Patch Set 3 : Renamed "clear_cache.py" #
Total comments: 2
Patch Set 4 : Replaced 'clear_dependencies.py' with 'delete_dir.py' #Patch Set 5 : Deleting NDK directory before extracting. Formatted with respect to eyeo python code style #
Total comments: 7
Patch Set 6 : Renamed variable #
MessagesTotal messages: 32
https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... File third_party/libadblockplus_android/BUILD.gn (right): https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... third_party/libadblockplus_android/BUILD.gn:68: script = "clear_cache.py" "clear.py" is renamed to "clear_cache" just to remove misunderstanding as now there is another "clear" in libadblockplus (the last one clears deps and this one clear build cache).
https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... File third_party/libadblockplus/clear_dependencies.py (right): https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... third_party/libadblockplus/clear_dependencies.py:14: v8_gyp_launcher_src = os.path.join(libadblockplus_third_party, 'v8_gyp_launcher') Nit: We usually rely on PEP-8 for Python style guide. We have a style checker for Python (https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo). It would be cool to run the check on the whole file (and other Python files here), but at least I know that lines longer than 80 cars would trigger a warning. So maybe put parameters on separate lines here? https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... File third_party/libadblockplus_android/BUILD.gn (right): https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... third_party/libadblockplus_android/BUILD.gn:68: script = "clear_cache.py" On 2018/01/16 05:35:38, anton wrote: > "clear.py" is renamed to "clear_cache" just to remove misunderstanding as > now there is another "clear" in libadblockplus (the last one clears deps and > this one clear build cache). Acknowledged.
https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... File third_party/libadblockplus/clear_dependencies.py (right): https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... third_party/libadblockplus/clear_dependencies.py:14: v8_gyp_launcher_src = os.path.join(libadblockplus_third_party, 'v8_gyp_launcher') On 2018/01/16 07:14:14, shoniko wrote: > Nit: We usually rely on PEP-8 for Python style guide. We have a style checker > for Python (https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo). It > would be cool to run the check on the whole file (and other Python files here), > but at least I know that lines longer than 80 cars would trigger a warning. So > maybe put parameters on separate lines here? Thanks for letting me know. Should we use Chromium python code style (original repo) or eyeo?
On 2018/01/16 07:18:36, anton wrote: > https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... > File third_party/libadblockplus/clear_dependencies.py (right): > > https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... > third_party/libadblockplus/clear_dependencies.py:14: v8_gyp_launcher_src = > os.path.join(libadblockplus_third_party, 'v8_gyp_launcher') > On 2018/01/16 07:14:14, shoniko wrote: > > Nit: We usually rely on PEP-8 for Python style guide. We have a style checker > > for Python (https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo). It > > would be cool to run the check on the whole file (and other Python files > here), > > but at least I know that lines longer than 80 cars would trigger a warning. So > > maybe put parameters on separate lines here? > > Thanks for letting me know. > Should we use Chromium python code style (original repo) or eyeo? Good question. I think for the files you created we should use ours. It's in third_party folder, so we are not bound by Chrome rules as I see it.
On 2018/01/16 07:22:17, Oleksandr wrote: > On 2018/01/16 07:18:36, anton wrote: > > > https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... > > File third_party/libadblockplus/clear_dependencies.py (right): > > > > > https://codereview.adblockplus.org/29670555/diff/29670556/third_party/libadbl... > > third_party/libadblockplus/clear_dependencies.py:14: v8_gyp_launcher_src = > > os.path.join(libadblockplus_third_party, 'v8_gyp_launcher') > > On 2018/01/16 07:14:14, shoniko wrote: > > > Nit: We usually rely on PEP-8 for Python style guide. We have a style > checker > > > for Python (https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo). It > > > would be cool to run the check on the whole file (and other Python files > > here), > > > but at least I know that lines longer than 80 cars would trigger a warning. > So > > > maybe put parameters on separate lines here? > > > > Thanks for letting me know. > > Should we use Chromium python code style (original repo) or eyeo? > > Good question. I think for the files you created we should use ours. It's in > third_party folder, so we are not bound by Chrome rules as I see it. See patch set #2 I think it would be better to create separate issue to reformat all .py scripts (not related to this ticket) in separate ticket.
LGTM
It can be a solution but I think there is a better way, in particular without manipulations with files, so could we firstly discuss it in the issue? > Now i just delete "libadblockplus/third_party" to let 'ensure_dependencies' check out the sources from our repo (it's replaced with Chromium's repo while replacing our deps to Chromium deps - see issue ticket). Could you please explain when 'ensure_dependencies' is called and that it works? I'm afraid that each time one runs sync we remove v8 from libadblockplus, then clone it again from our repo, and then replace by V8 from chormium. https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... File third_party/libadblockplus/clear_dependencies.py (right): https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... third_party/libadblockplus/clear_dependencies.py:11: # Back-up needed files If we removed only libadblockplus/third_party/v8 then we would not need these backups, would we?
On 2018/01/16 11:51:00, sergei wrote: > It can be a solution but I think there is a better way, in particular without > manipulations with files, so could we firstly discuss it in the issue? Yes, i agree there should be better way, that's why i've created ticket: https://issues.adblockplus.org/ticket/6260. > > Could you please explain when 'ensure_dependencies' is called and that it works? It's called as GN hook on pattern `dependencies`: https://gitlab.com/adblockplus/chromium/blob/abp/DEPS#L1234 (in brief when `dependencies` file is updated, see GN DEPS patten docs). > I'm afraid that each time one runs sync we remove v8 from libadblockplus, then > clone it again from our repo, and then replace by V8 from chormium. That's right. It more simple as writing custom dependencies preparation logics and worked good for proof-of-concept.
On 2018/01/16 11:51:00, sergei wrote: > If we removed only libadblockplus/third_party/v8 then we would not need these > backups, would we? We remove 'libadblockplus/third_party' (line #28) as most of deps repos are replaced with original (not ours) and 'ensure_dependencies.py' will try to find revision from `dependencies` in original repos and obviously fails.
On 2018/01/16 12:10:27, anton wrote: > On 2018/01/16 11:51:00, sergei wrote: > > It can be a solution but I think there is a better way, in particular without > > manipulations with files, so could we firstly discuss it in the issue? > > Yes, i agree there should be better way, that's why i've created ticket: > https://issues.adblockplus.org/ticket/6260. > > > > > Could you please explain when 'ensure_dependencies' is called and that it > works? > > It's called as GN hook on pattern `dependencies`: > https://gitlab.com/adblockplus/chromium/blob/abp/DEPS#L1234 > (in brief when `dependencies` file is updated, see GN DEPS patten docs). > > > I'm afraid that each time one runs sync we remove v8 from libadblockplus, then > > clone it again from our repo, and then replace by V8 from chormium. > > That's right. It more simple as writing custom dependencies preparation logics > and worked good for proof-of-concept. What about having a special dependecies file referencing required revisions and cloning V8 with its dependencies from chromium file tree? It seems much easier and more descriptive to simply copy one file than manipulate a lot of trees with a danger to remove something. On 2018/01/16 12:13:49, anton wrote: > On 2018/01/16 11:51:00, sergei wrote: > > If we removed only libadblockplus/third_party/v8 then we would not need these > > backups, would we? > > We remove 'libadblockplus/third_party' (line #28) as most of deps repos are > replaced with original (not ours) and 'ensure_dependencies.py' will try > to find revision from `dependencies` in original repos and obviously fails. What are these repositories which are not in v8?
On 2018/01/16 12:30:40, sergei wrote: > On 2018/01/16 12:10:27, anton wrote: > > On 2018/01/16 11:51:00, sergei wrote: > > > It can be a solution but I think there is a better way, in particular > without > > > manipulations with files, so could we firstly discuss it in the issue? > > > > Yes, i agree there should be better way, that's why i've created ticket: > > https://issues.adblockplus.org/ticket/6260. > > > > > > > > Could you please explain when 'ensure_dependencies' is called and that it > > works? > > > > It's called as GN hook on pattern `dependencies`: > > https://gitlab.com/adblockplus/chromium/blob/abp/DEPS#L1234 > > (in brief when `dependencies` file is updated, see GN DEPS patten docs). > > > > > I'm afraid that each time one runs sync we remove v8 from libadblockplus, > then > > > clone it again from our repo, and then replace by V8 from chormium. > > > > That's right. It more simple as writing custom dependencies preparation logics > > and worked good for proof-of-concept. > What about having a special dependecies file referencing required revisions and > cloning V8 with its dependencies from chromium file tree? > It seems much easier and more descriptive to simply copy one file than > manipulate a lot of trees with a danger to remove something. > > On 2018/01/16 12:13:49, anton wrote: > > On 2018/01/16 11:51:00, sergei wrote: > > > If we removed only libadblockplus/third_party/v8 then we would not need > these > > > backups, would we? > > > > We remove 'libadblockplus/third_party' (line #28) as most of deps repos are > > replaced with original (not ours) and 'ensure_dependencies.py' will try > > to find revision from `dependencies` in original repos and obviously fails. > > What are these repositories which are not in v8? I got your point - googletest and gyp are not replaced by Chromium ones. So we can delete only `libadblockplus/third_party/v8` and avoid back-uping `v8_gyp_launcher` and `detect_v8_host_arch`, right?
On 2018/01/16 13:00:44, anton wrote: > I got your point - googletest and gyp are not replaced by Chromium ones. > So we can delete only `libadblockplus/third_party/v8` and avoid back-uping > `v8_gyp_launcher` and `detect_v8_host_arch`, right? right. JIC, what about just a dependencies file instead of copying/linking many V8 files?
> JIC, what about just a dependencies file instead of copying/linking many V8 > files? I have checked, unfortunately our ensure_dependencies is trying to be too smart again and does not allow to specify something like path/to/third_party/v8 = git:../path/to/chromium/v8@revision firstly it adds .git suffix, secondly it adds https://github.com/ prefix.
https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... File third_party/libadblockplus_android/clear_cache.py (right): https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... third_party/libadblockplus_android/clear_cache.py:1: import os Currently it looks like platform independent `rm -rf {}`, should it be renamed? https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... third_party/libadblockplus_android/clear_cache.py:16: sys.exit(main(sys.argv[1:])) What if remove sys.exit and `return 0` from `main`?
https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... File third_party/libadblockplus_android/clear_cache.py (right): https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... third_party/libadblockplus_android/clear_cache.py:1: import os On 2018/01/16 17:29:08, sergei wrote: > Currently it looks like platform independent `rm -rf {}`, should it be renamed? Acknowledged. https://codereview.adblockplus.org/29670555/diff/29670566/third_party/libadbl... third_party/libadblockplus_android/clear_cache.py:16: sys.exit(main(sys.argv[1:])) On 2018/01/16 17:29:08, sergei wrote: > What if remove sys.exit and `return 0` from `main`? I've tried to keep Chromium code style and it's usually done like that (see `src/build/landmines.py`, `src/third_party/depot_tools/update_depot_tools_toggle.py`, etc).
On 2018/01/16 17:29:08, sergei wrote:
> Currently it looks like platform independent `rm -rf {}`, should it be
renamed?
See patch set #3
https://codereview.adblockplus.org/29670555/diff/29671555/DEPS File DEPS (right): https://codereview.adblockplus.org/29670555/diff/29671555/DEPS#newcode1240 DEPS:1240: 'src/third_party/libadblockplus/clear_dependencies.py' Why not to call here python delete_dir.py src/third_party/libadblockplus/third_party/v8?
https://codereview.adblockplus.org/29670555/diff/29671555/DEPS File DEPS (right): https://codereview.adblockplus.org/29670555/diff/29671555/DEPS#newcode1240 DEPS:1240: 'src/third_party/libadblockplus/clear_dependencies.py' On 2018/01/17 09:49:03, sergei wrote: > Why not to call here > python delete_dir.py src/third_party/libadblockplus/third_party/v8? Because `delete_dir.py` relates to 'libadblockplus-android' (and clears native build cache) and `clear_dependencies.py` relates to 'libadblockplus'. They should be separated 1) logically 2) `delete_dir.py` is called 2 times (build with built-in v8 to generate .info) and then before build with Chromium's v8.
On 2018/01/17 09:49:03, sergei wrote: > https://codereview.adblockplus.org/29670555/diff/29671555/DEPS > File DEPS (right): > > https://codereview.adblockplus.org/29670555/diff/29671555/DEPS#newcode1240 > DEPS:1240: 'src/third_party/libadblockplus/clear_dependencies.py' > Why not to call here > python delete_dir.py src/third_party/libadblockplus/third_party/v8? taking into account that now we have to delete only 2 directory ('third_party/libadblockplus/third_party/v8') we can use `delete_dir.py` but i'd say we have to copy it from `libadblockplus-android` just like 'subproc.py'.
On 2018/01/17 11:15:11, anton wrote: > On 2018/01/17 09:49:03, sergei wrote: > > https://codereview.adblockplus.org/29670555/diff/29671555/DEPS > > File DEPS (right): > > > > https://codereview.adblockplus.org/29670555/diff/29671555/DEPS#newcode1240 > > DEPS:1240: 'src/third_party/libadblockplus/clear_dependencies.py' > > Why not to call here > > python delete_dir.py src/third_party/libadblockplus/third_party/v8? > > taking into account that now we have to delete only 2 directory > ('third_party/libadblockplus/third_party/v8') we can use `delete_dir.py` > but i'd say we have to copy it from `libadblockplus-android` just like > 'subproc.py'. Uploaded patch set #4
LGTM, though it seems that we could avoid copying of delete_dir.py if put it one level upper.
https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... File third_party/libadblockplus/download_ndk.py (right): https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:26: ndk_dir = os.path.join(libadblockplus_root, 'android-ndk-r12b') Previously we deleted whole 'third_party' including ndk dir. Now we have to delete it before extracting of zip. We could delete it with 'delete_dir.py' but i'd prefer to keep the logics in one file here.
On 2018/01/17 14:39:10, sergei wrote: > LGTM, though it seems that we could avoid copying of delete_dir.py if put it one > level upper. Yes, Diego mentioned it too so i've created separate ticket: https://issues.adblockplus.org/ticket/6286
On 2018/01/18 08:44:02, anton wrote: Since i've uploaded patch set #5 (because of found problem - ndk dir is not deleted (because we replaced deleting of 'third_party' to just 'third_party/v8')) it would be great to have your final approvals.
If it takes too long to test the changes then I think it can be approved for present. https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... File third_party/libadblockplus/download_ndk.py (right): https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:13: libadblockplus_root = os.path.join(cwd, I think it's rather libadblockplus_third_party_dir than libadblockplus_root. If you decide to rename the variable I would propose to `third_party_dir`. https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:15: 'libadblockplus', 'third_party') Why not to put it into cwd/src/third_party? https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:33: with zipfile.ZipFile(ndk_dst, 'r') as zf: Taking into account the number of files I would probably just call a subprocess unzip for the sake of performance. BTW, is there a doc for info.external_attr? I'm not sure that merely shifting is enough, perhaps we should also clear few bits.
https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... File third_party/libadblockplus/download_ndk.py (right): https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:15: 'libadblockplus', 'third_party') On 2018/01/18 09:16:50, sergei wrote: > Why not to put it into cwd/src/third_party? it's required for libadblockplus (and libadblockplus-android) first of all and no other deps require it. also there is third_party/android_tools/ndk dir so i'd like to keep it separate https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:33: with zipfile.ZipFile(ndk_dst, 'r') as zf: On 2018/01/18 09:16:50, sergei wrote: > Taking into account the number of files I would probably just call a subprocess > unzip for the sake of performance. BTW, is there a doc for info.external_attr? > I'm not sure that merely shifting is enough, perhaps we should also clear few > bits. zip python library does not keep permissions ("executable"), so i had to use this workaround.
https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... File third_party/libadblockplus/download_ndk.py (right): https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... third_party/libadblockplus/download_ndk.py:13: libadblockplus_root = os.path.join(cwd, On 2018/01/18 09:16:51, sergei wrote: > I think it's rather libadblockplus_third_party_dir than libadblockplus_root. If > you decide to rename the variable I would propose to `third_party_dir`. Done. See patch set #6
On 2018/01/18 09:49:12, anton wrote: > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > File third_party/libadblockplus/download_ndk.py (right): > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > third_party/libadblockplus/download_ndk.py:15: 'libadblockplus', 'third_party') > On 2018/01/18 09:16:50, sergei wrote: > > Why not to put it into cwd/src/third_party? > > it's required for libadblockplus (and libadblockplus-android) first of all and > no other deps require it. > also there is third_party/android_tools/ndk dir so i'd like to keep it separate > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > third_party/libadblockplus/download_ndk.py:33: with zipfile.ZipFile(ndk_dst, > 'r') as zf: > On 2018/01/18 09:16:50, sergei wrote: > > Taking into account the number of files I would probably just call a > subprocess > > unzip for the sake of performance. BTW, is there a doc for info.external_attr? > > I'm not sure that merely shifting is enough, perhaps we should also clear few > > bits. > > zip python library does not keep permissions ("executable"), so i had to use > this workaround. I also don't like the fact that we have duplicate files here (and also in other code reviews, such as in #6277). It makes it harder to maintain and easier to have inconsistencies. I would suggest putting all shared/common files in a separate folder under third_party dir. Could be something like third_party/common or third_party/util or third_party/shared (even prefixed by libadblockplus_)
On 2018/01/18 12:23:12, diegocarloslima wrote: > On 2018/01/18 09:49:12, anton wrote: > > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > > File third_party/libadblockplus/download_ndk.py (right): > > > > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > > third_party/libadblockplus/download_ndk.py:15: 'libadblockplus', > 'third_party') > > On 2018/01/18 09:16:50, sergei wrote: > > > Why not to put it into cwd/src/third_party? > > > > it's required for libadblockplus (and libadblockplus-android) first of all and > > no other deps require it. > > also there is third_party/android_tools/ndk dir so i'd like to keep it > separate > > > > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > > third_party/libadblockplus/download_ndk.py:33: with zipfile.ZipFile(ndk_dst, > > 'r') as zf: > > On 2018/01/18 09:16:50, sergei wrote: > > > Taking into account the number of files I would probably just call a > > subprocess > > > unzip for the sake of performance. BTW, is there a doc for > info.external_attr? > > > I'm not sure that merely shifting is enough, perhaps we should also clear > few > > > bits. > > > > zip python library does not keep permissions ("executable"), so i had to use > > this workaround. > > I also don't like the fact that we have duplicate files here (and also in other > code reviews, such as in #6277). It makes it harder to maintain and easier to > have inconsistencies. I would suggest putting all shared/common files in a > separate folder under third_party dir. Could be something like > third_party/common or third_party/util or third_party/shared (even prefixed by > libadblockplus_) This will be done in separate ticket https://issues.adblockplus.org/ticket/6286. Otherwise we will never finish this ticket and try to do all-in-one.
On 2018/01/18 12:25:34, anton wrote: > On 2018/01/18 12:23:12, diegocarloslima wrote: > > On 2018/01/18 09:49:12, anton wrote: > > > > > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > > > File third_party/libadblockplus/download_ndk.py (right): > > > > > > > > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > > > third_party/libadblockplus/download_ndk.py:15: 'libadblockplus', > > 'third_party') > > > On 2018/01/18 09:16:50, sergei wrote: > > > > Why not to put it into cwd/src/third_party? > > > > > > it's required for libadblockplus (and libadblockplus-android) first of all > and > > > no other deps require it. > > > also there is third_party/android_tools/ndk dir so i'd like to keep it > > separate > > > > > > > > > https://codereview.adblockplus.org/29670555/diff/29673555/third_party/libadbl... > > > third_party/libadblockplus/download_ndk.py:33: with zipfile.ZipFile(ndk_dst, > > > 'r') as zf: > > > On 2018/01/18 09:16:50, sergei wrote: > > > > Taking into account the number of files I would probably just call a > > > subprocess > > > > unzip for the sake of performance. BTW, is there a doc for > > info.external_attr? > > > > I'm not sure that merely shifting is enough, perhaps we should also clear > > few > > > > bits. > > > > > > zip python library does not keep permissions ("executable"), so i had to use > > > this workaround. > > > > I also don't like the fact that we have duplicate files here (and also in > other > > code reviews, such as in #6277). It makes it harder to maintain and easier to > > have inconsistencies. I would suggest putting all shared/common files in a > > separate folder under third_party dir. Could be something like > > third_party/common or third_party/util or third_party/shared (even prefixed by > > libadblockplus_) > > This will be done in separate ticket https://issues.adblockplus.org/ticket/6286. > Otherwise we will never finish this ticket and try to do all-in-one. Allright, so LGTM then
LGTM then 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
