Issue 6273 - Apply eyeo python code style

third_party/libadblockplus/prepare_dependencies.py

+from __future__ import print_function
+
 import sys
 import os
 import shutil
 
+
 def duplicate(src, dst, symlink=False):

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

It seems that symlink parameter is not passed, should it be removed?

[sergei, 2018/01/22 10:10:32]

BTW, what about moving this function into common file?

[anton, 2018/01/22 10:18:41]

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

[anton, 2018/01/22 10:18:41]

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

[sergei, 2018/01/22 12:04:02]

Acknowledged, maybe it makes sense.

-  if (os.path.exists(dst)):
-    if (os.path.isdir(dst)):
-      print('Deleting %s' % dst)
-      shutil.rmtree(dst);
+    if os.path.exists(dst):
+        if os.path.isdir(dst):
+            print('Deleting {}'.format(dst))
+            shutil.rmtree(dst)
+        else:
+            # symlink
+            os.unlink(dst)

[sergei, 2018/01/22 12:04:02]

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.

+
+    dst_parent = os.path.abspath(os.path.join(dst, os.pardir))
+    if not os.path.exists(dst_parent):
+        print('Creating parent directory {} for {}'.format(dst_parent, dst))
+        os.makedirs(dst_parent)
+
+    if symlink:
+        print('Symlinking {} to {}'.format(src, dst))
+        os.symlink(src, dst)
     else:
-      # symlink
-      os.unlink(dst)
-      
-  dst_parent = os.path.abspath(os.path.join(dst, os.pardir))
-  if not os.path.exists(dst_parent):
-    print('Creating parent directory %s for %s' % (dst_parent, dst))
-    os.makedirs(dst_parent)
+        print('Copying {} to {}'.format(src, dst))
+        shutil.copytree(src, dst)
 
-  if symlink:
-    print('Symlinking %s to %s' % (src, dst))
-    os.symlink(src, dst)
-  else:
-    print('Copying %s to %s' % (src, dst))
-    shutil.copytree(src, dst)
 
 def prepare_deps():
-  cwd = os.getcwd()
-  libadblockplus_root = os.path.join(cwd, 'src', 'third_party', 'libadblockplus', 'third_party')
-  
-  # googletest
-  # Chromium's googletest has a bit different directories structure ('src' as subdirectory)
-  #googletest_src = os.path.join(cwd, 'src', 'third_party', 'googletest')
-  #googletest_dst = os.path.join(libadblockplus_root, 'googletest')
-  #duplicate(googletest_src, googletest_dst)
+    cwd = os.getcwd()
+    libadblockplus_third_party = os.path.join(cwd,
+                                              'src', 'third_party',
+                                              'libadblockplus', 'third_party')
 
-  # gyp
-  # Chromium's gyp can't be used because of compile error:
-  # *** No rule to make target `shell/src/Main.cpp', needed by `local/armeabi-v7a/objs/abpshell/shell/src/Main.o'.  Stop.
-  # See. https://hg.adblockplus.org/gyp/rev/fix-issue-339
-  #gyp_src = os.path.join(cwd, 'src', 'tools', 'gyp')
-  #gyp_dst = os.path.join(libadblockplus_root, 'gyp')
-  #duplicate(gyp_src, gyp_dst)
+    # googletest
+    #
+    # Chromium's googletest has a bit different directories structure
+    # ('src' as subdirectory)
 
-  # back-up /v8/testing/gtest as it will be deleted while preparing v8
-  gtest_bak_src = os.path.join(libadblockplus_root, 'v8', 'testing', 'gtest')
-  gtest_bak_dst = os.path.join(libadblockplus_root, 'gtest.bak')
-  shutil.move(gtest_bak_src, gtest_bak_dst)
+    # gyp
+    #
+    # Chromium's gyp can't be used because of compile error:
+    # *** No rule to make target `shell/src/Main.cpp',
+    # needed by `local/armeabi-v7a/objs/abpshell/shell/src/Main.o'.  Stop.
+    # See. https://hg.adblockplus.org/gyp/rev/fix-issue-339
 
-  # v8
-  v8_src = os.path.join(cwd, 'src', 'v8')
-  v8_dst = os.path.join(libadblockplus_root, 'v8')
-  duplicate(v8_src, v8_dst)
+    # back-up /v8/testing/gtest as it will be deleted while preparing v8
+    gtest_bak_src = os.path.join(libadblockplus_third_party,
+                                 'v8', 'testing', 'gtest')
+    gtest_bak_dst = os.path.join(libadblockplus_third_party, 'gtest.bak')
+    shutil.move(gtest_bak_src, gtest_bak_dst)
 
-  # restore /v8/testing/gtest from backup
-  shutil.move(gtest_bak_dst, gtest_bak_src)
+    # v8
+    v8_src = os.path.join(cwd, 'src', 'v8')
+    v8_dst = os.path.join(libadblockplus_third_party, 'v8')
+    duplicate(v8_src, v8_dst)
 
-  # v8/base/trace_event/common
-  trace_common_src = os.path.join(cwd, 'src', 'base', 'trace_event', 'common')
-  trace_common_dst = os.path.join(libadblockplus_root, 'v8', 'base', 'trace_event', 'common')
-  duplicate(trace_common_src, trace_common_dst)
+    # restore /v8/testing/gtest from backup
+    shutil.move(gtest_bak_dst, gtest_bak_src)
 
-  # v8/build
-  build_src = os.path.join(cwd, 'src', 'build')
-  build_dst = os.path.join(libadblockplus_root, 'v8', 'build')
-  duplicate(build_src, build_dst)
+    # v8/testing/gtest

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

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.

[anton, 2018/01/22 10:18:41]

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

+    #
+    # Chromium's gtest can't be used becuase of compile error:
+    # fatal error: third_party/googletest/src/googletest/include/gtest/gtest_prod.h: No such file or directory
+    # #include "third_party/googletest/src/googletest/include/gtest/gtest_prod.h"
+    # so we have to back-up gtest above and restore it
+    # after prepring of 'v8' directory
 
-  # v8/testing/gtest
-  # Chromium's gtest can't be used becuase of compile error:
-  # fatal error: third_party/googletest/src/googletest/include/gtest/gtest_prod.h: No such file or directory
-  # #include "third_party/googletest/src/googletest/include/gtest/gtest_prod.h"
-  # so we have to back-up gtest above and restore it after prepring of 'v8' directory
-  #gtest_src = os.path.join(cwd, 'src', 'testing', 'gtest')
-  #gtest_dst = os.path.join(libadblockplus_root, 'v8', 'testing', 'gtest')
-  #duplicate(gtest_src, gtest_dst)
+    for path in [

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

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.

[anton, 2018/01/22 10:18:41]

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:02]

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.

[sergei, 2018/01/24 17:23:40]

No objections, could you please file an issue for that?

+        ('base', 'trace_event', 'common'),
+        ('build',),
+        ('tools', 'gyp'),
+        ('tools', 'clang'),
+        ('third_party', 'icu'),
+        ('third_party' , 'jinja2'),
+        ('third_party', 'markupsafe')  
+    ]:
+        src = os.path.join(cwd, 'src', *path)
+        dst = os.path.join(libadblockplus_third_party, 'v8', *path)
+        duplicate(src, dst)
 
-  # v8/tools/gyp
-  tools_gyp_src = os.path.join(cwd, 'src', 'tools', 'gyp')
-  tools_gyp_dst = os.path.join(libadblockplus_root, 'v8', 'tools', 'gyp')
-  duplicate(tools_gyp_src, tools_gyp_dst)
+    return 0
 
-  # v8/tools/clang
-  clang_src = os.path.join(cwd, 'src', 'tools', 'clang')
-  clang_dst = os.path.join(libadblockplus_root, 'v8', 'tools', 'clang')
-  duplicate(clang_src, clang_dst)
 
-  # v8/third_party/icu
-  icu_src = os.path.join(cwd, 'src', 'third_party', 'icu')
-  icu_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'icu')
-  duplicate(icu_src, icu_dst)
-
-  # v8/third_party/jinja2
-  jinja2_src = os.path.join(cwd, 'src', 'third_party', 'jinja2')
-  jinja2_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'jinja2')
-  duplicate(jinja2_src, jinja2_dst)
-
-  # v8/third_party/markupsafe
-  markupsafe_src = os.path.join(cwd, 'src', 'third_party', 'markupsafe')
-  markupsafe_dst = os.path.join(libadblockplus_root, 'v8', 'third_party', 'markupsafe')
-  duplicate(markupsafe_src, markupsafe_dst)
-
-  return 0  
-
-def main(argv):
-  return prepare_deps();
-
-if '__main__' == __name__:
-  try:
-    sys.exit(main(sys.argv[1:]))
-  except KeyboardInterrupt:
-    sys.stderr.write('interrupted\n')
-    sys.exit(1)
+if __name__ == '__main__':
+    try:
+        sys.exit(prepare_deps())

[Vasily Kuznetsov, 2018/01/29 12:39:12]

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:53]

Acknowledged.

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

+    except KeyboardInterrupt:
+        sys.exit('interrupted')