Issue 2539 - Move VCS abstraction part of ensure_dependencies into a separate module

Issue 2539 - Move VCS abstraction part of ensure_dependencies into a separate module

[Wladimir Palant, 2015-05-18 15:27:00]

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
File ensure_dependencies.py (right):

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:239: parser.add_argument("-s", "--self", metavar="path",
type=str, help="Update ensure_dependencies.py at this location")
This now allows two ways of setting up ensure_dependencies.py in a new
repository:

* Run |python -m buildtools.ensure_dependencies -s ensure_dependencies.py|
* Copy ensure_dependencies.py from buildtools repository and run |python
ensure_dependencies.py|

Both approaches will work only if self-update is enabled.

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/vcs.py
File vcs.py (right):

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/vcs....
vcs.py:9: import subprocess
This has been moved from ensure_dependencies.py without any changes beyond the
module imports.

[Sebastian Noack, 2015-05-19 11:52:41]

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
File ensure_dependencies.py (right):

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:197: if self_update and config.get("_self", {}).get("*",
"").lower() not in ("", "0", "false"):
The logic checking the "true"-ness of the value is redundant with the check for
SKIP_ENSURE_DEPENENDICES. Moving it to a function?

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:198: original_path = sys.path
This is useless, as you merely backup the reference to the list, which you don't
change anyway. If you want to backup it's content you have to copy the list:

  orig_path = sys.path[:]

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:212: with io.open(target, "rb") as handle:
Nit: You can just use the open() built-in function here.

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:212: with io.open(target, "rb") as handle:
Nit: "file" would be a more appropriate variable name, to match Python's
terminology. Feel free to change it in the vcs module as well while on it.

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:229: logging.warning("Failed to update %s, skipping
self-update" % __file__)
Pro-Tip: logging.warning(.., exc_info=True)

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
File script_compiler.py (right):

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:10: def multiline_repr(str):
Nit: Please avoid variable names conflicting with built-ins like "str".

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:11: return "'''%s'''" % str.replace('\\',
'\\\\').replace("'", "\\'")
Note that you only need to escape quotes if followed by at least two more
quotes. This is another feature of Python's triple-quote syntax beside allowing
newlines, and makes the resulting code a little more readable:

  re.sub(r"(\\|'(?=''))", r'\\\1', s)

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:19: yield '#!/usr/bin/env python\n'
I'd rather pass in the file object and use the print statement instead returning
generator here. That way you don't need a loop in the calling code, and don't
need to hard-code the newline character here.

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:22: path =
inspect.getsourcefile(reload(importlib.import_module(module)))
Any reason you reaload the module?

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:22: path =
inspect.getsourcefile(reload(importlib.import_module(module)))
For reference, there is also inspect.getsource(), however it fails for empty
modules. So I agree that it's better to use inspect.getsourcefile() and read the
file ourselves here.

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:29: import importlib
importlib isn't used in the generated file.

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/scri...
script_compiler.py:33: for module, source in modules:
IMO, both, the generated and the generating code would be simpler if you unroll
the loop and inline the main script:

  def get_module_source(module):
    filename = inspect.getsourcefile(importlib.import_module(module))
    with open(filename, 'rb') as file:
      return file.read()

  def compile_script(file, main, modules):
    print >>file, 'import imp'
    print >>file, 'import sys'
    print >>file
  
    for module in modules:
      print >>file, 'module = imp.new_module(%r)' % module
      print >>file, 'sys.modules[%r] = module' % module
      source = get_module_source(module)
      if source:
        print >>file, 'exec %s in module.__dict__' %
multiline_repr(get_module_source(module))
      print >>file
    
    print >>file, 'del imp, sys, module'
    print >>file
    print >>file, get_module_source(main)

[Sebastian Noack, 2015-05-19 13:25:36]

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
File ensure_dependencies.py (right):

http://codereview.adblockplus.org/4995669794226176/diff/5629499534213120/ensu...
ensure_dependencies.py:197: if self_update and config.get("_self", {}).get("*",
"").lower() not in ("", "0", "false"):
On 2015/05/19 11:52:41, Sebastian Noack wrote:
> The logic checking the "true"-ness of the value is redundant with the check
for
> SKIP_ENSURE_DEPENENDICES. Moving it to a function?

Just realized that we have a ConfigParser here. So you should just use
getboolean() instead.