Issue 2443 - Honour SKIP_DEPENDENCY_UPDATES environment variable

ensure_dependencies.py

 #!/usr/bin/env python
 # coding: utf-8
 
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import sys
 import os
 import posixpath
@@ skipping 16 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
 """
 
+SKIP_DEPENDENCY_UPDATES = os.environ.get(
+  "SKIP_DEPENDENCY_UPDATES", ""
+).lower() not in ("", "0", "false")
+
 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):
@@ skipping 43 matching lines @@
     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 + '^{commit}']
     return subprocess.check_output(command, cwd=repo).strip()
 
   def pull(self, repo):
+    # Fetch tracked branches, new tags and the list of available remote branches
     subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo)
+    # Next we need to ensure all remote branches are tracked
+    newly_tracked = False
+    remotes = subprocess.check_output(["git", "branch", "--remotes"], cwd=repo)
+    for match in re.finditer(r"^\s*(origin/(\S+))$", remotes, re.M):
+      remote, local = match.groups()
+      with open(os.devnull, "wb") as devnull:
+        if subprocess.call(["git", "branch", "--track", local, remote],
+                           cwd=repo, stdout=devnull, stderr=devnull) == 0:
+          newly_tracked = True
+    # Finally fetch any newly tracked remote branches
+    if newly_tracked:
+      subprocess.check_call(["git", "fetch", "--quiet", "origin"], 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")
     _ensure_line_exists(exclude_file, module)
 
   def postprocess_url(self, url):
@@ skipping 49 matching lines @@
         if spec:
           result[key] = spec
     return result
   except IOError, e:
     if e.errno != errno.ENOENT:
       raise
     return None
 
 def safe_join(path, subpath):
   # This has been inspired by Flask's safe_join() function
-  forbidden = set([os.sep, os.altsep]) - set([posixpath.sep, None])
+  forbidden = {os.sep, os.altsep} - {posixpath.sep, None}
   if any(sep in subpath for sep in forbidden):
     raise Exception("Illegal directory separator in dependency path %s" % subpath)
 
   normpath = posixpath.normpath(subpath)
   if posixpath.isabs(normpath):
     raise Exception("Dependency path %s cannot be absolute" % subpath)
   if normpath == posixpath.pardir or normpath.startswith(posixpath.pardir + posixpath.sep):
     raise Exception("Dependency path %s has to be inside the repository" % subpath)
   return os.path.join(path, *normpath.split(posixpath.sep))
 
 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
+
+  if SKIP_DEPENDENCY_UPDATES:
+    logging.warning("SKIP_DEPENDENCY_UPDATES environment variable set, "
+                    "%s not cloned", 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)
 
@@ skipping 21 matching lines @@
 
   if type in revisions:
     revision = revisions[type]
   elif "*" in revisions:
     revision = revisions["*"]
   else:
     logging.warning("No revision specified for repository %s (type %s), skipping update" % (target, type))
     return
 
   resolved_revision = repo_types[type].get_revision_id(target, revision)
-  if not resolved_revision:
-    logging.info("Revision %s is unknown, downloading remote changes" % revision)
-    repo_types[type].pull(target)
-    resolved_revision = repo_types[type].get_revision_id(target, revision)
+  current_revision = repo_types[type].get_revision_id(target)
+
+  if resolved_revision != current_revision:
+    if SKIP_DEPENDENCY_UPDATES:
+      logging.warning("SKIP_DEPENDENCY_UPDATES environment variable set, "
+                      "%s not checked out to %s", target, revision)
+      return
+
     if not resolved_revision:
-      raise Exception("Failed to resolve revision %s" % revision)
-
-  current_revision = repo_types[type].get_revision_id(target)
-  if resolved_revision != current_revision:
+      logging.info("Revision %s is unknown, downloading remote changes" % revision)
+      repo_types[type].pull(target)
+      resolved_revision = repo_types[type].get_revision_id(target, revision)
+      if not resolved_revision:
+        raise Exception("Failed to resolve revision %s" % revision)
+
     logging.info("Updating repository %s to revision %s" % (target, resolved_revision))
     repo_types[type].update(target, resolved_revision)
 
 def resolve_deps(repodir, level=0, self_update=True, overrideroots=None, skipdependencies=set()):
   config = read_deps(repodir)
   if config is None:
     if level == 0:
       logging.warning("No dependencies file in directory %s, nothing to do...\n%s" % (repodir, USAGE))
     return
   if level >= 10:
     logging.warning("Too much subrepository nesting, ignoring %s" % repo)
+    return
 
   if overrideroots is not None:
     config["_root"] = overrideroots
 
   for dir, revisions in config.iteritems():
     if dir.startswith("_") or revisions["_source"] in skipdependencies:
       continue
     target = safe_join(repodir, dir)
     ensure_repo(repodir, target, config.get("_root", {}), revisions["_source"])
     update_repo(target, revisions)
@@ skipping 30 matching lines @@
     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)
 
-  if os.environ.get("SKIP_ENSURE_DEPENDENCIES"):

[Sebastian Noack, 2015/05/04 11:33:01]

This will merely check whether the variable is set a non-empty string. Hence
either of the following will still disable dependency updates:

SKIP_ENSURE_DEPENDENCIES=0
SKIP_ENSURE_DEPENDENCIES=false

Maybe we should rather perform following check:

os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", "false")

[kzar, 2015/05/05 13:25:22]

Well I realised that but sure, Done.

-    logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.")

[Sebastian Noack, 2015/05/04 11:33:01]

I think I'd prefer to only show a warning if there is actually a dependency
update that isn't performed because this variable being set, in update_repo():

if not resolved_revision or resolved_revision != current_revision:
  logging.warning('Pending update for repository %s detected! Skipping due to
SKIP_ENSURE_DEPENDENCIES :(')
  return

Otherwise people using this variable will quickly stop caring about this
warning.

[kzar, 2015/05/05 13:25:22]

Disagree with this one, I think that people that set this variable should be
trusted to know what they're doing. IMHO we should always warn them it's enabled
but otherwise assume they really mean what they say. I would not want the script
to check for updates even though I have turned it off.

[Sebastian Noack, 2015/05/05 14:33:24]

There are two situations in which people might use this variable:

1. They are temporarily testing a change in a dependency repo. It's easy to
forget to remove that variable afterwards.
2. They prefer to update dependencies manually, setting this variable globally,
running ensure_dependency.py only once in a while. However, this is a bad
practice, leading to broken and outdated builds and while testing their changes
with potentially outdated dependencies they introduce even more issues.

So no, people who keep that variable don't know what they are doing. However,
regardless for what reason you set this variable, pending updates that are
skipped is a useful information. In particular for the second scenario above,
it's important to remind them when they have outdated dependencies.

But I agree that the name of this variable is slightly misleading then. So maybe
you want to rename it to PRESERVE_DEPENDENCIES or DONT_UPDATE_DEPENDENCIES,
SKIP_DEPENDENCY_UPDATES or UPDATE_DEPENDENCIES (with inverted values).

[Wladimir Palant, 2015/05/05 14:46:37]

While I'm not sure I agree with Sebastian's argumentation above, showing a more
targeted warning would indeed be beneficial - and it is easy enough to
implement. The warning should be moved into the |if resolved_revision !=
current_revision| branch in the update_repo() function. This way various checks
would still run and dependencies would still be checked out if necessary, merely
the update would be replaced by a warning.

SKIP_DEPENDENCY_UPDATES as variable name is fine with me.

[kzar, 2015/05/06 16:23:53]

Felix / Matze - As you two are the main users/proponents of this feature could
you give your thoughts here? (I want to avoid making something that's no longer
useful for you guys!)

[Felix Dahlke, 2015/05/06 16:31:23]

Regarding the warning: Let's definitely have that - this is non-default
behaviour and it needs to be reasonably obvious. However, I'd vote for simply
always uttering a warning when the variable is set. It's simpler, and I think
it's too magical to still do the pull/fetch when this is supposed to be skipped.

Regarding the variable name: Related to whether we should skip the whole thing
or only the hg update/git checkout. I really think we should do the former, thus
I prefer SKIP_ENSURE_DEPENDENCIES.

[Sebastian Noack, 2015/05/06 16:59:23]

Nobody argues about that. We already have consensus on having a warning. ;)
It's not simpler. The complexity is the same. It's merely about were we add the
check for that variable in the code.
It's not magical/unexpected at all, if we call the variable
SKIP_DEPENDENCY_UPDATES rather than SKIP_ENSURE_DEPENDENCIES.

Reasons to not immediately return, but performing all checks, merely leaving the
dependency repositories untouched, from the previous discussion:

* A more targeted warning (raising attention of pending updates)
* No warning if it doesn't make a difference (when there are no pending updates)
* Larger common code path (might be helpful for testing as well)

On 2015/05/06 16:23:53, kzar wrote:
I doubt it will be useless to them if we implement it like I suggested above. In
fact nobody brought up any valid argument against it. And since module owner and
super reviewer already agreed we don't need a 4th or 5th opinion here.

[Felix Dahlke, 2015/05/06 17:14:57]

It'd be clear if you call it SKIP_DEPENDENCY_HG_UPDATE_OR_GIT_CHECKOUT,
"SKIP_DEPENDENCY_UPDATES" is fairly ambiguous IMO, and doesn't really sound like
it's going to go ahead and download potentially huge changes.
True, but as a potential user of this, I wouldn't mind this warning at all.
Something along the lines of: "Warning: SKIP_ENSURE_DEPENDENCIES is set,
skipping ensure_dependencies.py"
We're talking about a way of skipping this script, what do we need a large
common code path for then?
I really don't get the impression you understand the uses cases for this, so why
do you think it's not necessary to listen to the people who will actually use
this? The whole point of this variable is to skip the magic, not to get a
different sort of magic. If you insist on implementing it this way, I suggest we
don't implement it at all and stick to messing with the dependencies file
locally.

[Wladimir Palant, 2015/05/06 17:40:47]

Mostly because these people never manage to provide a valid argument beyond some
hand waving and muttering "magic" ;) Much less describe a use case.

-    sys.exit()
-
   parser = argparse.ArgumentParser(description="Verify dependencies for a set of repositories, by default the repository of this script.")
   parser.add_argument("repos", metavar="repository", type=str, nargs="*", help="Repository path")
   parser.add_argument("-q", "--quiet", action="store_true", help="Suppress informational output")
   args = parser.parse_args()
 
   if args.quiet:
     logging.disable(logging.INFO)
 
   repos = args.repos
   if not len(repos):
     repos = [os.path.dirname(__file__)]
   for repo in repos:
     resolve_deps(repo)