Issue 2443 - Honour SKIP_DEPENDENCY_UPDATES environment variable

ensure_dependencies.py

Index: ensure_dependencies.py
diff --git a/ensure_dependencies.py b/ensure_dependencies.py
index b6557d051f6661645dea95edfde280ea288f7533..fba358717ec4d2e56bac13ef7d746e5b078c6ea0 100755
--- a/ensure_dependencies.py
+++ b/ensure_dependencies.py
@@ -297,6 +297,10 @@ def _ensure_line_exists(path, pattern):
 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")