1377 - Blacklist dependencies from SCM tracking

ensure_dependencies.py

 #!/usr/bin/env python
 # coding: utf-8
 
 # This file is part of the Adblock Plus build tools,
 # Copyright (C) 2006-2014 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
@@ skipping 26 matching lines @@
   # File to update this script from (optional)
   _self = buildtools/ensure_dependencies.py
   # Check out elemhidehelper repository into extensions/elemhidehelper directory
   # at tag "1.2".
   extensions/elemhidehelper = elemhidehelper 1.2
   # Check out buildtools repository into buildtools directory at VCS-specific
   # revision IDs.
   buildtools = buildtools hg:016d16f7137b git:f3f8692f82e5
 """
 
-class SCM(object):
-  def _ignore(self, target, file_name):

[Wladimir Palant, 2014/10/07 22:30:14]

That should simply be a helper function _ensure_line_exists, no object
inheritance necessary.

[aalvz, 2014/10/08 13:49:18]

Done.

[Wladimir Palant, 2014/10/08 21:40:30]

And the same file format? That's merely a coincidence that you can reuse the
same code here. This is a helper function with no relation to revision control
systems. Once again, using inheritance here implies similarities that don't
exist in reality.

-    with open(file_name, 'a+') as f:
-      file_content = [l.strip() for l in f.readlines()]
-      if not target in file_content:
-        if len(file_content):
-          f.seek(-1, os.SEEK_CUR)
-          last_character = f.read(1)
-          if not last_character in "\r\n":

[Wladimir Palant, 2014/10/07 22:30:14]

No need to overcomplicate this:

file_content.append(line)
f.seek(0, os.SEEK_SET)
for l in file_content:
  print >>f, l
f.truncate()

Yes, this will overwrite the entire file contents - but the performance
difference shouldn't matter here.

[aalvz, 2014/10/08 13:49:18]

I think the solution you are suggesting and the current one are not that
different and is not more natural in the first solution instead of writing and
overwriting it all? :)

[mathias, 2014/10/08 17:37:55]

While the first point is not really true (though it's not obvious that the outer
`if` block ensures the file having a size of 1 or more), the second being
arguable and the third easy to fix,.. I do see that leaving these caveats for
the Python interpreter to handle is indeed a better choice, especially when
unifying the line-breaks is classified a "desired" side-effect.

Will become adjusted in the next patch set.

-            f.write(os.linesep)
-        f.write(target)
-        f.write(os.linesep)
-
-class Mercurial(SCM):
+class Mercurial():
   def istype(self, repodir):
     return os.path.exists(os.path.join(repodir, ".hg"))
 
   def clone(self, source, target):
     if not source.endswith("/"):
       source += "/"
     subprocess.check_call(["hg", "clone", "--quiet", "--noupdate", source, target])
 
   def get_revision_id(self, repo, rev=None):
     command = ["hg", "id", "--repository", repo, "--id"]
     if rev:
       command.extend(["--rev", rev])
 
     # Ignore stderr output and return code here: if revision lookup failed we
     # should simply return an empty string.
     result = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0]
     return result.strip()
 
   def pull(self, repo):
     subprocess.check_call(["hg", "pull", "--repository", repo, "--quiet"])
 
   def update(self, repo, rev):
     subprocess.check_call(["hg", "update", "--repository", repo, "--quiet", "--check", "--rev", rev])
 
   def ignore(self, target, repo):
 
     if not self.istype(target):
 
       config_path = os.path.join(repo, ".hg", "hgrc")
-      ignore_path = os.path.join(".", ".hg", "dependencies")
+      ignore_path = os.path.abspath(os.path.join(repo, ".hg", "dependencies"))
 
       config = RawConfigParser()
       config.read(config_path)
 
       if not config.has_section("ui"):
         config.add_section("ui")
 
-      if not config.has_option("ui", "ignore.dependencies"):
-        config.set("ui", "ignore.dependencies", ignore_path)
-        with open(config_path, "w") as stream:
-          config.write(stream)
-      else:
-        ignore_path = config.get("ui", "ignore.dependencies")

[Wladimir Palant, 2014/10/07 22:30:14]

Please set that option regardless of whether it is already there - since it has
to be an absolute path, it might be wrong if the repository moved. So you should
always update it in order to have it point to the right location.

[mathias, 2014/10/08 16:32:11]

Agreed, this is necessary due to the issue above, and will be fixed in the next
patch set. Still a bit awkward though..

+      config.set("ui", "ignore.dependencies", ignore_path)
+      with open(config_path, "w") as stream:
+        config.write(stream)
 
       module = os.path.relpath(target, repo)
-      self._ignore(module, ignore_path)
-
-class Git(SCM):
+      _ensure_line_exists(ignore_path, module)
+
+class Git():
   def istype(self, repodir):
     return os.path.exists(os.path.join(repodir, ".git"))
 
   def clone(self, source, target):
     source = source.rstrip("/")
     if not source.endswith(".git"):
       source += ".git"
     subprocess.check_call(["git", "clone", "--quiet", source, target])
 
   def get_revision_id(self, repo, rev="HEAD"):
     command = ["git", "rev-parse", "--revs-only", rev]
     return subprocess.check_output(command, cwd=repo).strip()
 
   def pull(self, repo):
     subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo)
 
   def update(self, repo, rev):
     subprocess.check_call(["git", "checkout", "--quiet", rev], cwd=repo)
 
   def ignore(self, target, repo):
     module = os.path.relpath(target, repo)
     exclude_file = os.path.join(repo, ".git", "info", "exclude")
-    self._ignore(module, exclude_file)
-
-repo_types = {
-  "hg": Mercurial(),
-  "git": Git(),
-}
+    _ensure_line_exists(exclude_file, module)
+
+repo_types = OrderedDict((
+  ("hg", Mercurial()),
+  ("git", Git()),
+))
 
 def parse_spec(path, line):
   if "=" not in line:
     logging.warning("Invalid line in file %s: %s" % (path, line))
     return None, None
 
   key, value = line.split("=", 1)
   key = key.strip()
   items = value.split()
   if not len(items):
@@ skipping 50 matching lines @@
 
 def get_repo_type(repo):
   for name, repotype in repo_types.iteritems():
     if repotype.istype(repo):
       return name
   return None
 
 def ensure_repo(parentrepo, target, roots, sourcename):
   if os.path.exists(target):
     return
+
   parenttype = get_repo_type(parentrepo)
   type = None
   for key in roots:
     if key == parenttype or (key in repo_types and type is None):
       type = key
   if type is None:
     raise Exception("No valid source found to create %s" % target)
 
   url = urlparse.urljoin(roots[type], sourcename)
   logging.info("Cloning repository %s into %s" % (url, target))
@@ skipping 68 matching lines @@
     if sourcedata != targetdata:
       logging.info("Updating %s from %s, don't forget to commit" % (source, target))
       with io.open(target, "wb") as handle:
         handle.write(sourcedata)
       if __name__ == "__main__":
         logging.info("Restarting %s" % target)
         os.execv(sys.executable, [sys.executable, target] + sys.argv[1:])
       else:
         logging.warning("Cannot restart %s automatically, please rerun" % target)
 
+def _ensure_line_exists(path, pattern):
+  with open(path, 'a+') as f:
+    file_content = [l.strip() for l in f.readlines()]
+    if not pattern in file_content:
+      file_content.append(pattern)
+      f.seek(0, os.SEEK_SET)
+      f.truncate()
+      for l in file_content:
+        print >>f, l
+
 if __name__ == "__main__":
   logging.basicConfig(format='%(levelname)s: %(message)s', level=logging.INFO)
   repos = sys.argv[1:]
   if not len(repos):
     repos = [os.getcwd()]
   for repo in repos:
     resolve_deps(repo)