Issue 6273 - Apply eyeo python code style

Issue 6273 - Apply eyeo python code style

[anton, 2018-01-19 07:30:20]

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
os.path.join(cwd,
It would be better to rename it to `libadblockplus_third_party` too but it seems
to be unrelated changes.

[Vasily Kuznetsov, 2018-01-19 13:06:53]

Hi Anton,

The script looks pretty good. I have a bunch of comments, but most of them are
just suggestions. Feel free to keep things as they are if it makes more sense in
the overall context (I haven't seen the whole repo, so I might be making some
wrong assumptions).

Cheers,
Vasily

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:6: def duplicate(src, dst,
symlink=False):
It seems like `symlink` argument is not used in this script. It could be removed
(together with related code) unless some other script imports this function from
here.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:9: print('Deleting
{}'.format(dst))
There's no `from __future__ import print_function` at the top of the file so I
assume this is Python3-only code. If it's not, that import is needed.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:12: # symlink
Good idea with the comment, but it could also be a regular file, so it will turn
out misleading. Or why can we assume it will be a symlink?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
os.path.join(cwd,
Given how much we use `os.path.join` in this function, I wonder if it would make
sense to assign it to a variable, maybe something like `pjoin = os.path.join`.
Then all those lines would need less wrapping and read a bit easier. What do you
think?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:39: # googletest_src =
os.path.join(cwd, 'src', 'third_party', 'googletest')
This commented out code (as well as other below) with some info about why we
can't do that is confusing. Are these really helpful comments? Maybe we can just
delete them or move them to some kind of documentation file?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:91: # v8/tools/gyp
Lines 91-112 could be rolled into a loop -- all five triples of lines basically
do the same thing. This would make this part shorter and a bit easier to read at
a glance.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:125: if '__main__' ==
__name__:
Yoda-conditions are generally frowned upon in Python world. Since `if foo =
'bar':` is not valid anyway, we don't gain additional error protection this way
and only make readability somewhat worse. I would recommend rewriting this as
`if __name__ == '__main__':`.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:127:
sys.exit(main(sys.argv[1:]))
It seems like we could just call `prepare_deps()` directly here -- `main()` only
delegates to it and ignores the argument.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:127:
sys.exit(main(sys.argv[1:]))
Since `main()` always returns the result of `prepare_deps()` and
`prepare_deps()` seems to always return 0 (which is the default return code for
Python scripts), we don't need to call `sys.exit()` and pass the return value of
`main()` to it. Also that `return` in line 118 would not be needed.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:129:
sys.stderr.write('interrupted\n')
You can just use `sys.exit('interrupted')` to achieve the same effect as these
two lines.

[anton, 2018-01-19 13:17:15]

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:6: def duplicate(src, dst,
symlink=False):
On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> It seems like `symlink` argument is not used in this script. It could be
removed
> (together with related code) unless some other script imports this function
from
> here.

That's true that actually it's not used anywhere.
But i'd leave it as an illustration of that possibility.
What do you think guys?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:9: print('Deleting
{}'.format(dst))
On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> There's no `from __future__ import print_function` at the top of the file so I
> assume this is Python3-only code. If it's not, that import is needed.

I'm not Python expert but i'm having python 2.x installed and it works as-is
(without import).

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:12: # symlink
On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> Good idea with the comment, but it could also be a regular file, so it will
turn
> out misleading. Or why can we assume it will be a symlink?

originally i've tried to symlink the dependencies (they are copied to our
modules from Chromium (another path))
to save the storage. But it turned out that dirs structure have to be different,
so i had to use just copying.
However i still thin that some dependencies could be symlinked
(here is improvement ticket https://issues.adblockplus.org/ticket/6260).

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
os.path.join(cwd,
On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> Given how much we use `os.path.join` in this function, I wonder if it would
make
> sense to assign it to a variable, maybe something like `pjoin = os.path.join`.
> Then all those lines would need less wrapping and read a bit easier. What do
you
> think?

Great ideas. However we will save just about 7 characters and it seems to be not
very helpful.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:39: # googletest_src =
os.path.join(cwd, 'src', 'third_party', 'googletest')
On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> This commented out code (as well as other below) with some info about why we
> can't do that is confusing. Are these really helpful comments? Maybe we can
just
> delete them or move them to some kind of documentation file?

Agree, i tend to think we should remove the commented out lines and just leave
comments.
What do you think guys?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:91: # v8/tools/gyp
On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> Lines 91-112 could be rolled into a loop -- all five triples of lines
basically
> do the same thing. This would make this part shorter and a bit easier to read
at
> a glance.

did you mean iterating over array of 'gyp','clang','icu','jinja2' ?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:125: if '__main__' ==
__name__:
On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> Yoda-conditions are generally frowned upon in Python world. Since `if foo =
> 'bar':` is not valid anyway, we don't gain additional error protection this
way
> and only make readability somewhat worse. I would recommend rewriting this as
> `if __name__ == '__main__':`.

Acknowledged.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:127:
sys.exit(main(sys.argv[1:]))
On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> Since `main()` always returns the result of `prepare_deps()` and
> `prepare_deps()` seems to always return 0 (which is the default return code
for
> Python scripts), we don't need to call `sys.exit()` and pass the return value
of
> `main()` to it. Also that `return` in line 118 would not be needed.

Acknowledged.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:129:
sys.stderr.write('interrupted\n')
On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> You can just use `sys.exit('interrupted')` to achieve the same effect as these
> two lines.

Acknowledged.

[Vasily Kuznetsov, 2018-01-19 13:35:17]

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:9: print('Deleting
{}'.format(dst))
On 2018/01/19 13:17:15, anton wrote:
> On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > There's no `from __future__ import print_function` at the top of the file so
I
> > assume this is Python3-only code. If it's not, that import is needed.
> 
> I'm not Python expert but i'm having python 2.x installed and it works as-is
> (without import).

Yeah, in Python 2 it's parsed as:

    print ("bla bla bla")

The first part is a print statement, the second part is a string in parentheses,
so it works. But it's still cleaner to do the import, because if you have
`print("foo")` and then somebody decides to add something to it: `print("foo",
42)`, this changed line will be parsed as print statement and a tuple, not as a
function call with two arguments. Nobody will die as a result, but it's just
kind of poor hygiene ;)

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:12: # symlink
On 2018/01/19 13:17:15, anton wrote:
> On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > Good idea with the comment, but it could also be a regular file, so it will
> turn
> > out misleading. Or why can we assume it will be a symlink?
> 
> originally i've tried to symlink the dependencies (they are copied to our
> modules from Chromium (another path))
> to save the storage. But it turned out that dirs structure have to be
different,
> so i had to use just copying.
> However i still thin that some dependencies could be symlinked
> (here is improvement ticket https://issues.adblockplus.org/ticket/6260).

Acknowledged.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
os.path.join(cwd,
On 2018/01/19 13:17:14, anton wrote:
> On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > Given how much we use `os.path.join` in this function, I wonder if it would
> make
> > sense to assign it to a variable, maybe something like `pjoin =
os.path.join`.
> > Then all those lines would need less wrapping and read a bit easier. What do
> you
> > think?
> 
> Great ideas. However we will save just about 7 characters and it seems to be
not
> very helpful.

Acknowledged.

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:91: # v8/tools/gyp
On 2018/01/19 13:17:14, anton wrote:
> On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> > Lines 91-112 could be rolled into a loop -- all five triples of lines
> basically
> > do the same thing. This would make this part shorter and a bit easier to
read
> at
> > a glance.
> 
> did you mean iterating over array of 'gyp','clang','icu','jinja2' ?

Yeah, something like this:

    for path in [
        ('tools', 'gyp'),
        ('tools', 'clang),
        ('third_party', 'icu'),
        ...
    ]:
        src = os.path.join(cwd, 'src', *path)
        dst = os.path.join(libadblockplus_root, 'v8', *path)
        duplicate(src, dst)

[anton, 2018-01-19 13:37:49]

On 2018/01/19 13:35:17, Vasily Kuznetsov wrote:
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> File third_party/libadblockplus/prepare_dependencies.py (right):
> 
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:9: print('Deleting
> {}'.format(dst))
> On 2018/01/19 13:17:15, anton wrote:
> > On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > > There's no `from __future__ import print_function` at the top of the file
so
> I
> > > assume this is Python3-only code. If it's not, that import is needed.
> > 
> > I'm not Python expert but i'm having python 2.x installed and it works as-is
> > (without import).
> 
> Yeah, in Python 2 it's parsed as:
> 
>     print ("bla bla bla")
> 
> The first part is a print statement, the second part is a string in
parentheses,
> so it works. But it's still cleaner to do the import, because if you have
> `print("foo")` and then somebody decides to add something to it: `print("foo",
> 42)`, this changed line will be parsed as print statement and a tuple, not as
a
> function call with two arguments. Nobody will die as a result, but it's just
> kind of poor hygiene ;)
> 
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:12: # symlink
> On 2018/01/19 13:17:15, anton wrote:
> > On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > > Good idea with the comment, but it could also be a regular file, so it
will
> > turn
> > > out misleading. Or why can we assume it will be a symlink?
> > 
> > originally i've tried to symlink the dependencies (they are copied to our
> > modules from Chromium (another path))
> > to save the storage. But it turned out that dirs structure have to be
> different,
> > so i had to use just copying.
> > However i still thin that some dependencies could be symlinked
> > (here is improvement ticket https://issues.adblockplus.org/ticket/6260).
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
> os.path.join(cwd,
> On 2018/01/19 13:17:14, anton wrote:
> > On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > > Given how much we use `os.path.join` in this function, I wonder if it
would
> > make
> > > sense to assign it to a variable, maybe something like `pjoin =
> os.path.join`.
> > > Then all those lines would need less wrapping and read a bit easier. What
do
> > you
> > > think?
> > 
> > Great ideas. However we will save just about 7 characters and it seems to be
> not
> > very helpful.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:91: # v8/tools/gyp
> On 2018/01/19 13:17:14, anton wrote:
> > On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> > > Lines 91-112 could be rolled into a loop -- all five triples of lines
> > basically
> > > do the same thing. This would make this part shorter and a bit easier to
> read
> > at
> > > a glance.
> > 
> > did you mean iterating over array of 'gyp','clang','icu','jinja2' ?
> 
> Yeah, something like this:
> 
>     for path in [
>         ('tools', 'gyp'),
>         ('tools', 'clang),
>         ('third_party', 'icu'),
>         ...
>     ]:
>         src = os.path.join(cwd, 'src', *path)
>         dst = os.path.join(libadblockplus_root, 'v8', *path)
>         duplicate(src, dst)

Acknowledged.

[sergei, 2018-01-19 14:19:43]

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
os.path.join(cwd,
On 2018/01/19 13:35:17, Vasily Kuznetsov wrote:
> On 2018/01/19 13:17:14, anton wrote:
> > On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > > Given how much we use `os.path.join` in this function, I wonder if it
would
> > make
> > > sense to assign it to a variable, maybe something like `pjoin =
> os.path.join`.
> > > Then all those lines would need less wrapping and read a bit easier. What
do
> > you
> > > think?
> > 
> > Great ideas. However we will save just about 7 characters and it seems to be
> not
> > very helpful.
> 
> Acknowledged.

What about calling os.path.join only when it's really needed and meanwhile use a
tuple/list?

https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:39: # googletest_src =
os.path.join(cwd, 'src', 'third_party', 'googletest')
On 2018/01/19 13:17:15, anton wrote:
> On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> > This commented out code (as well as other below) with some info about why we
> > can't do that is confusing. Are these really helpful comments? Maybe we can
> just
> > delete them or move them to some kind of documentation file?
> 
> Agree, i tend to think we should remove the commented out lines and just leave
> comments.
> What do you think guys?

Just remove everything and leave a comment if it's required explaining the
reason it's done for what it's done. Basically comment the existing code, not a
potential code.

[anton, 2018-01-22 07:12:41]

On 2018/01/19 14:19:43, sergei wrote:
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> File third_party/libadblockplus/prepare_dependencies.py (right):
> 
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:30: libadblockplus_root =
> os.path.join(cwd,
> On 2018/01/19 13:35:17, Vasily Kuznetsov wrote:
> > On 2018/01/19 13:17:14, anton wrote:
> > > On 2018/01/19 13:06:53, Vasily Kuznetsov wrote:
> > > > Given how much we use `os.path.join` in this function, I wonder if it
> would
> > > make
> > > > sense to assign it to a variable, maybe something like `pjoin =
> > os.path.join`.
> > > > Then all those lines would need less wrapping and read a bit easier.
What
> do
> > > you
> > > > think?
> > > 
> > > Great ideas. However we will save just about 7 characters and it seems to
be
> > not
> > > very helpful.
> > 
> > Acknowledged.
> 
> What about calling os.path.join only when it's really needed and meanwhile use
a
> tuple/list?
> 
>
https://codereview.adblockplus.org/29674555/diff/29674556/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:39: # googletest_src =
> os.path.join(cwd, 'src', 'third_party', 'googletest')
> On 2018/01/19 13:17:15, anton wrote:
> > On 2018/01/19 13:06:52, Vasily Kuznetsov wrote:
> > > This commented out code (as well as other below) with some info about why
we
> > > can't do that is confusing. Are these really helpful comments? Maybe we
can
> > just
> > > delete them or move them to some kind of documentation file?
> > 
> > Agree, i tend to think we should remove the commented out lines and just
leave
> > comments.
> > What do you think guys?
> 
> Just remove everything and leave a comment if it's required explaining the
> reason it's done for what it's done. Basically comment the existing code, not
a
> potential code.

Uploaded new patch set

[sergei, 2018-01-22 10:08:59]

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:8: def duplicate(src, dst,
symlink=False):
It seems that symlink parameter is not passed, should it be removed?

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:62: # v8/testing/gtest
I thought that v8/testing/gtest is not used in libadblockplus at all, so I would
propose to consider just remove anything related to it.

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:70: for path in [
Will it stop working if we remove this for loop? All these subdirectories are
duplicated already by duplicate(v8_src, v8_dst), aren't they?

It seems that the whole function could be just a duplication of chromium's V8
directory into libadblockplus/third_party/v8. The reason for that is that right
now libadblockplus/Makefile does not support building of merely V8's
libplatform.a.

[sergei, 2018-01-22 10:10:33]

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:8: def duplicate(src, dst,
symlink=False):
BTW, what about moving this function into common file?

[anton, 2018-01-22 10:18:42]

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:8: def duplicate(src, dst,
symlink=False):
On 2018/01/22 10:08:59, sergei wrote:
> It seems that symlink parameter is not passed, should it be removed?

Vasily noted it too.
Please ready my answer https://codereview.adblockplus.org/29674555/#msg3

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:8: def duplicate(src, dst,
symlink=False):
On 2018/01/22 10:10:32, sergei wrote:
> BTW, what about moving this function into common file?

Since it's not used anywhere else (and will not be used less most likely) i
don't think it worth it.

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:62: # v8/testing/gtest
On 2018/01/22 10:08:59, sergei wrote:
> I thought that v8/testing/gtest is not used in libadblockplus at all, so I
would
> propose to consider just remove anything related to it.

then why it's listed in dependencies
https://github.com/adblockplus/libadblockplus/blob/master/dependencies#L10 ?

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:70: for path in [
On 2018/01/22 10:08:59, sergei wrote:
> Will it stop working if we remove this for loop? All these subdirectories are
> duplicated already by duplicate(v8_src, v8_dst), aren't they?
> 
> It seems that the whole function could be just a duplication of chromium's V8
> directory into libadblockplus/third_party/v8. The reason for that is that
right
> now libadblockplus/Makefile does not support building of merely V8's
> libplatform.a.

They are copied not from v8_src but from different paths not in v8 directory.
Let's say gyp comes from src/tools/gyp

[sergei, 2018-01-22 12:04:03]

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:8: def duplicate(src, dst,
symlink=False):
On 2018/01/22 10:18:41, anton wrote:
> On 2018/01/22 10:08:59, sergei wrote:
> > It seems that symlink parameter is not passed, should it be removed?
> 
> Vasily noted it too.
> Please ready my answer https://codereview.adblockplus.org/29674555/#msg3

Acknowledged, maybe it makes sense.

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:15: os.unlink(dst)
Actually, according to the documentation "This follows symbolic links, so both
islink() and isdir() can be true for the same path." it seems it's never reached
on practice and unlink is just to remove a file, not specific for symlinks, so I
would propose to remove the comment.

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:70: for path in [
On 2018/01/22 10:18:41, anton wrote:
> On 2018/01/22 10:08:59, sergei wrote:
> > Will it stop working if we remove this for loop? All these subdirectories
are
> > duplicated already by duplicate(v8_src, v8_dst), aren't they?
> > 
> > It seems that the whole function could be just a duplication of chromium's
V8
> > directory into libadblockplus/third_party/v8. The reason for that is that
> right
> > now libadblockplus/Makefile does not support building of merely V8's
> > libplatform.a.
> 
> They are copied not from v8_src but from different paths not in v8 directory.
> Let's say gyp comes from src/tools/gyp

Acknowledged. I think the comments should be different.

Chromium code does not fetch V8 deps because in chromium project the
dependencies are taken from a root directory of a parent root project (see
v8/BUILD.gn), so in order to build V8 we have to copy them because we use are
still using gyp and gyp does not support such tricks, and even if it supported
libadblockplus Makefile does not support it right now either.

What concerns v8/testing/gtest, I guess the error message is different, it
should mention the path "testing/gtest/include/gtest/gtest_prod.h". We should
actually use the version of gtest specified in the DEPS file of the chromium's
V8, since that version is the same as in libadblockplus's V8 we may use the
backup, but it does not guarantee that after changing of a chromium's version it
will work, we will likely have to check out gtest of another version.

So, IMO the comments about not duplicated gyp and googletest should be removed
and the explanation why we do and why we may do what we are doing should be
added.

[anton, 2018-01-22 12:55:20]

On 2018/01/22 12:04:03, sergei wrote:
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> File third_party/libadblockplus/prepare_dependencies.py (right):
> 
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:8: def duplicate(src, dst,
> symlink=False):
> On 2018/01/22 10:18:41, anton wrote:
> > On 2018/01/22 10:08:59, sergei wrote:
> > > It seems that symlink parameter is not passed, should it be removed?
> > 
> > Vasily noted it too.
> > Please ready my answer https://codereview.adblockplus.org/29674555/#msg3
> 
> Acknowledged, maybe it makes sense.
> 
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:15: os.unlink(dst)
> Actually, according to the documentation "This follows symbolic links, so both
> islink() and isdir() can be true for the same path." it seems it's never
reached
> on practice and unlink is just to remove a file, not specific for symlinks, so
I
> would propose to remove the comment.
> 
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:70: for path in [
> On 2018/01/22 10:18:41, anton wrote:
> > On 2018/01/22 10:08:59, sergei wrote:
> > > Will it stop working if we remove this for loop? All these subdirectories
> are
> > > duplicated already by duplicate(v8_src, v8_dst), aren't they?
> > > 
> > > It seems that the whole function could be just a duplication of chromium's
> V8
> > > directory into libadblockplus/third_party/v8. The reason for that is that
> > right
> > > now libadblockplus/Makefile does not support building of merely V8's
> > > libplatform.a.
> > 
> > They are copied not from v8_src but from different paths not in v8
directory.
> > Let's say gyp comes from src/tools/gyp
> 
> Acknowledged. I think the comments should be different.
> 
> Chromium code does not fetch V8 deps because in chromium project the
> dependencies are taken from a root directory of a parent root project (see
> v8/BUILD.gn), so in order to build V8 we have to copy them because we use are
> still using gyp and gyp does not support such tricks, and even if it supported
> libadblockplus Makefile does not support it right now either.
> 
> What concerns v8/testing/gtest, I guess the error message is different, it
> should mention the path "testing/gtest/include/gtest/gtest_prod.h". We should
> actually use the version of gtest specified in the DEPS file of the chromium's
> V8, since that version is the same as in libadblockplus's V8 we may use the
> backup, but it does not guarantee that after changing of a chromium's version
it
> will work, we will likely have to check out gtest of another version.
> 
> So, IMO the comments about not duplicated gyp and googletest should be removed
> and the explanation why we do and why we may do what we are doing should be
> added.

This code reviews related to code style only, not v8 deps preparation or
anything else.
I'd suggest to create new ticket instead, related to gyp and gtest deps only

[sergei, 2018-01-24 17:23:41]

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:70: for path in [
On 2018/01/22 12:04:02, sergei wrote:
> On 2018/01/22 10:18:41, anton wrote:
> > On 2018/01/22 10:08:59, sergei wrote:
> > > Will it stop working if we remove this for loop? All these subdirectories
> are
> > > duplicated already by duplicate(v8_src, v8_dst), aren't they?
> > > 
> > > It seems that the whole function could be just a duplication of chromium's
> V8
> > > directory into libadblockplus/third_party/v8. The reason for that is that
> > right
> > > now libadblockplus/Makefile does not support building of merely V8's
> > > libplatform.a.
> > 
> > They are copied not from v8_src but from different paths not in v8
directory.
> > Let's say gyp comes from src/tools/gyp
> 
> Acknowledged. I think the comments should be different.
> 
> Chromium code does not fetch V8 deps because in chromium project the
> dependencies are taken from a root directory of a parent root project (see
> v8/BUILD.gn), so in order to build V8 we have to copy them because we use are
> still using gyp and gyp does not support such tricks, and even if it supported
> libadblockplus Makefile does not support it right now either.
> 
> What concerns v8/testing/gtest, I guess the error message is different, it
> should mention the path "testing/gtest/include/gtest/gtest_prod.h". We should
> actually use the version of gtest specified in the DEPS file of the chromium's
> V8, since that version is the same as in libadblockplus's V8 we may use the
> backup, but it does not guarantee that after changing of a chromium's version
it
> will work, we will likely have to check out gtest of another version.
> 
> So, IMO the comments about not duplicated gyp and googletest should be removed
> and the explanation why we do and why we may do what we are doing should be
> added.

> Anton:
> This code reviews related to code style only, not v8 deps preparation or
> anything else.
> I'd suggest to create new ticket instead, related to gyp and gtest deps only
No objections, could you please file an issue for that?

[anton, 2018-01-25 10:56:29]

On 2018/01/24 17:23:41, sergei wrote:
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> File third_party/libadblockplus/prepare_dependencies.py (right):
> 
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:70: for path in [
> On 2018/01/22 12:04:02, sergei wrote:
> > On 2018/01/22 10:18:41, anton wrote:
> > > On 2018/01/22 10:08:59, sergei wrote:
> > > > Will it stop working if we remove this for loop? All these
subdirectories
> > are
> > > > duplicated already by duplicate(v8_src, v8_dst), aren't they?
> > > > 
> > > > It seems that the whole function could be just a duplication of
chromium's
> > V8
> > > > directory into libadblockplus/third_party/v8. The reason for that is
that
> > > right
> > > > now libadblockplus/Makefile does not support building of merely V8's
> > > > libplatform.a.
> > > 
> > > They are copied not from v8_src but from different paths not in v8
> directory.
> > > Let's say gyp comes from src/tools/gyp
> > 
> > Acknowledged. I think the comments should be different.
> > 
> > Chromium code does not fetch V8 deps because in chromium project the
> > dependencies are taken from a root directory of a parent root project (see
> > v8/BUILD.gn), so in order to build V8 we have to copy them because we use
are
> > still using gyp and gyp does not support such tricks, and even if it
supported
> > libadblockplus Makefile does not support it right now either.
> > 
> > What concerns v8/testing/gtest, I guess the error message is different, it
> > should mention the path "testing/gtest/include/gtest/gtest_prod.h". We
should
> > actually use the version of gtest specified in the DEPS file of the
chromium's
> > V8, since that version is the same as in libadblockplus's V8 we may use the
> > backup, but it does not guarantee that after changing of a chromium's
version
> it
> > will work, we will likely have to check out gtest of another version.
> > 
> > So, IMO the comments about not duplicated gyp and googletest should be
removed
> > and the explanation why we do and why we may do what we are doing should be
> > added.
> 
> > Anton:
> > This code reviews related to code style only, not v8 deps preparation or
> > anything else.
> > I'd suggest to create new ticket instead, related to gyp and gtest deps only
> No objections, could you please file an issue for that?

https://issues.adblockplus.org/ticket/6315

[anton, 2018-01-29 06:03:27]

On 2018/01/25 10:56:29, anton wrote:
> On 2018/01/24 17:23:41, sergei wrote:
> >
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> > File third_party/libadblockplus/prepare_dependencies.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> > third_party/libadblockplus/prepare_dependencies.py:70: for path in [
> > On 2018/01/22 12:04:02, sergei wrote:
> > > On 2018/01/22 10:18:41, anton wrote:
> > > > On 2018/01/22 10:08:59, sergei wrote:
> > > > > Will it stop working if we remove this for loop? All these
> subdirectories
> > > are
> > > > > duplicated already by duplicate(v8_src, v8_dst), aren't they?
> > > > > 
> > > > > It seems that the whole function could be just a duplication of
> chromium's
> > > V8
> > > > > directory into libadblockplus/third_party/v8. The reason for that is
> that
> > > > right
> > > > > now libadblockplus/Makefile does not support building of merely V8's
> > > > > libplatform.a.
> > > > 
> > > > They are copied not from v8_src but from different paths not in v8
> > directory.
> > > > Let's say gyp comes from src/tools/gyp
> > > 
> > > Acknowledged. I think the comments should be different.
> > > 
> > > Chromium code does not fetch V8 deps because in chromium project the
> > > dependencies are taken from a root directory of a parent root project (see
> > > v8/BUILD.gn), so in order to build V8 we have to copy them because we use
> are
> > > still using gyp and gyp does not support such tricks, and even if it
> supported
> > > libadblockplus Makefile does not support it right now either.
> > > 
> > > What concerns v8/testing/gtest, I guess the error message is different, it
> > > should mention the path "testing/gtest/include/gtest/gtest_prod.h". We
> should
> > > actually use the version of gtest specified in the DEPS file of the
> chromium's
> > > V8, since that version is the same as in libadblockplus's V8 we may use
the
> > > backup, but it does not guarantee that after changing of a chromium's
> version
> > it
> > > will work, we will likely have to check out gtest of another version.
> > > 
> > > So, IMO the comments about not duplicated gyp and googletest should be
> removed
> > > and the explanation why we do and why we may do what we are doing should
be
> > > added.
> > 
> > > Anton:
> > > This code reviews related to code style only, not v8 deps preparation or
> > > anything else.
> > > I'd suggest to create new ticket instead, related to gyp and gtest deps
only
> > No objections, could you please file an issue for that?
> 
> https://issues.adblockplus.org/ticket/6315

What do you think about patch set #2?

[Vasily Kuznetsov, 2018-01-29 12:39:13]

I have one minor comment remaining, but basically it looks good. LGTM.

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:88: sys.exit(prepare_deps())
As I wrote before, you don't really need `sys.exit` here because Python scripts
set exit code to 0 by default. This also applies to the other scripts.

[anton, 2018-01-29 12:52:54]

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
File third_party/libadblockplus/prepare_dependencies.py (right):

https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
third_party/libadblockplus/prepare_dependencies.py:88: sys.exit(prepare_deps())
On 2018/01/29 12:39:12, Vasily Kuznetsov wrote:
> As I wrote before, you don't really need `sys.exit` here because Python
scripts
> set exit code to 0 by default. This also applies to the other scripts.

Acknowledged.

See patch set #3 (thought i've left few files unchanged where return code is not
always 0)

[diegocarloslima, 2018-01-29 15:13:23]

On 2018/01/29 12:52:54, anton wrote:
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> File third_party/libadblockplus/prepare_dependencies.py (right):
> 
>
https://codereview.adblockplus.org/29674555/diff/29676583/third_party/libadbl...
> third_party/libadblockplus/prepare_dependencies.py:88:
sys.exit(prepare_deps())
> On 2018/01/29 12:39:12, Vasily Kuznetsov wrote:
> > As I wrote before, you don't really need `sys.exit` here because Python
> scripts
> > set exit code to 0 by default. This also applies to the other scripts.
> 
> Acknowledged.
> 
> See patch set #3 (thought i've left few files unchanged where return code is
not
> always 0)

LGTM

[Vasily Kuznetsov, 2018-01-29 16:27:57]

> See patch set #3 (thought i've left few files unchanged where return code is
not
> always 0)

Even more LGTM!