Merge ~cjwatson/launchpad:virtualenv-20 into launchpad:master

[Colin Watson]

Make build system compatible with virtualenv 20

virtualenv 20 was a complete rewrite of virtualenv. It's mostly compatible with older versions, but there were some places where Launchpad ran into trouble with it:

 * The handling of the `sitecustomize` module is a little different, and we rely on that to load some Launchpad customizations.

 * `env/bin/python3` is now a symlink (indirectly) to `/usr/bin/python3`, rather than being a copy of it, so we can't rely on touching it for timestamp comparison purposes in the `Makefile`. Use `env/instance_name` (which we were already creating for other reasons in `setup.py`) for this instead.

 * We now have to set the `VIRTUAL_ENV` environment variable in `_pythonpath.py` to activate the virtual environment properly.

 * While initially debugging this, I removed an unnecessary `$(PY)` from the invocation of `utilities/create-lp-wadl-and-apidoc.py`, and tidied up the `#!` line of `utilities/js-deps` to match all our other virtualenv-using scripts. These changes turn out not to be strictly required to support virtualenv 20, but I kept them since they seem like reasonable improvements in passing.

[Jürgen Gmach (jugmac00)]

> `env/bin/python3` is now a symlink (indirectly) to `/usr/bin/python3`, rather than being a copy of it

That is possible one reason why virtualenv is a magnitude faster than venv.

> virtualenv 20 was a complete rewrite of virtualenv.

fun fact - Bernát only rewrote virtualenv as he tried to rewrite tox and hit a wall - so he rewrote virtualenv first.

> * It's wise to be a bit more careful about how we execute scripts in a couple more places, although this turns out not to be critical.

This is a bit vague. Could you elaborate where we need to pay attention and why? Otherwise I'd suggest to remove this sentence.

Do I read the commit message correctly that these changes just made the virtualenv usage compatible with virtualenv 20, but still is compatible with the old version?

review: Approve

[Colin Watson (cjwatson)]

You're right that that bullet point was a bit vague. I've fleshed it out.

Indeed, this also remains compatible with the old virtualenv version. I successfully ran the full test suite against this commit.

diff --git a/Makefile b/Makefile
index 69ad953..5d59a25 100644
--- a/Makefile
+++ b/Makefile
@@ -30,6 +30,7 @@ PIP_ENV += PIP_FIND_LINKS="file://$(WD)/wheels/ file://$(WD)/download-cache/dist
 VIRTUALENV := $(PIP_ENV) /usr/bin/virtualenv
 PIP := PYTHONPATH= $(PIP_ENV) env/bin/pip --cache-dir=$(WD)/download-cache/
 
+VENV_INSTANCE_NAME := env/instance_name
 VENV_PYTHON := env/bin/$(PYTHON)
 
 SITE_PACKAGES := \
@@ -77,7 +78,7 @@ API_INDEX = $(APIDOC_DIR)/index.html
 # NB: It's important PIP_BIN only mentions things genuinely produced by pip.
 PIP_BIN = \
     $(PY) \
-    $(VENV_PYTHON) \
+    $(VENV_INSTANCE_NAME) \
     bin/bingtestservice \
     bin/build-twisted-plugin-cache \
     bin/harness \
@@ -131,7 +132,7 @@ hosted_branches: $(PY)
 $(API_INDEX): $(VERSION_INFO) $(PY)
 	$(RM) -r $(APIDOC_DIR) $(APIDOC_DIR).tmp
 	mkdir -p $(APIDOC_DIR).tmp
-	LPCONFIG=$(LPCONFIG) $(PY) ./utilities/create-lp-wadl-and-apidoc.py \
+	LPCONFIG=$(LPCONFIG) ./utilities/create-lp-wadl-and-apidoc.py \
 	    --force "$(APIDOC_TMPDIR)"
 	mv $(APIDOC_TMPDIR) $(APIDOC_DIR)
 
@@ -333,8 +334,9 @@ publish-tarball: build-tarball
 #
 # If we listed every target on the left-hand side, a parallel make would try
 # multiple copies of this rule to build them all.  Instead, we nominally build
-# just $(VENV_PYTHON), and everything else is implicitly updated by that.
-$(VENV_PYTHON): download-cache requirements/combined.txt setup.py
+# just $(VENV_INSTANCE_NAME), and everything else is implicitly updated by
+# that.
+$(VENV_INSTANCE_NAME): download-cache requirements/combined.txt setup.py
 	rm -rf env
 	mkdir -p env
 	$(VIRTUALENV) \
@@ -350,12 +352,12 @@ $(VENV_PYTHON): download-cache requirements/combined.txt setup.py
 		|| { code=$$?; rm -f $@; exit $$code; }
 	touch $@
 
-$(subst $(VENV_PYTHON),,$(PIP_BIN)): $(VENV_PYTHON)
+$(subst $(VENV_INSTANCE_NAME),,$(PIP_BIN)): $(VENV_INSTANCE_NAME)
 
 # Explicitly update version-info.py rather than declaring $(VERSION_INFO) as
 # a prerequisite, to make sure it's up to date when doing deployments.
 .PHONY: compile
-compile: $(VENV_PYTHON)
+compile: $(VENV_INSTANCE_NAME)
 	${SHHH} utilities/relocate-virtualenv env
 	$(PYTHON) utilities/link-system-packages.py \
 		"$(SITE_PACKAGES)" system-packages.txt
diff --git a/_pythonpath.py b/_pythonpath.py
index a09cb74..1773f89 100644
--- a/_pythonpath.py
+++ b/_pythonpath.py
@@ -25,16 +25,14 @@ python_version = '%s.%s' % sys.version_info[:2]
 stdlib_dir = os.path.join(env, 'lib', 'python%s' % python_version)
 
 if ('site' in sys.modules and
-    not sys.modules['site'].__file__.startswith(
-        os.path.join(stdlib_dir, 'site.py'))):
-    # We have the wrong site.py, so our paths are not set up correctly.
-    # We blow up, with a hopefully helpful error message.
+        'lp_sitecustomize' not in sys.modules):
+    # Site initialization has been run but lp_sitecustomize was not loaded,
+    # so something is set up incorrectly.  We blow up, with a hopefully
+    # helpful error message.
     raise RuntimeError(
-        'The wrong site.py is imported (%r imported, %r expected). '
-        'Scripts should usually be '
+        'Python was invoked incorrectly.  Scripts should usually be '
         "started with Launchpad's bin/py, or with a Python invoked with "
-        'the -S flag.' % (
-        sys.modules['site'].__file__, os.path.join(stdlib_dir, 'site.py')))
+        'the -S flag.')
 
 # Ensure that the virtualenv's standard library directory is in sys.path;
 # activate_this will not put it there.
@@ -53,6 +51,7 @@ if not sys.executable.startswith(top + os.sep) or 'site' not in sys.modules:
     sys.prefix = env
     os.environ['PATH'] = (
         os.path.join(env, 'bin') + os.pathsep + os.environ.get('PATH', ''))
+    os.environ['VIRTUAL_ENV'] = env
     site_packages = os.path.join(
         env, 'lib', 'python%s' % python_version, 'site-packages')
     import site
diff --git a/setup.py b/setup.py
index 88f502c..e26320d 100644
--- a/setup.py
+++ b/setup.py
@@ -98,10 +98,18 @@ class lp_develop(develop):
                 """)
             self.write_script("py", py_header + py_script_text)
 
+            # Install site customizations for this virtualenv.  In principle
+            # we just want to install sitecustomize and have site load it,
+            # but this doesn't work with virtualenv 20.x
+            # (https://github.com/pypa/virtualenv/issues/1703).  Note that
+            # depending on the resolution of
+            # https://bugs.python.org/issue33944 we may need to change this
+            # again in future.
             env_top = os.path.join(os.path.dirname(__file__), "env")
-            stdlib_dir = get_python_lib(standard_lib=True, prefix=env_top)
+            site_packages_dir = get_python_lib(prefix=env_top)
             orig_sitecustomize = self._get_orig_sitecustomize()
-            sitecustomize_path = os.path.join(stdlib_dir, "sitecustomize.py")
+            sitecustomize_path = os.path.join(
+                site_packages_dir, "_sitecustomize.py")
             with open(sitecustomize_path, "w") as sitecustomize_file:
                 sitecustomize_file.write(dedent("""\
                     import os
@@ -114,6 +122,12 @@ class lp_develop(develop):
                     """))
                 if orig_sitecustomize:
                     sitecustomize_file.write(orig_sitecustomize)
+            # Awkward naming; this needs to come lexicographically after any
+            # other .pth files.
+            sitecustomize_pth_path = os.path.join(
+                site_packages_dir, "zzz_run_venv_sitecustomize.pth")
+            with open(sitecustomize_pth_path, "w") as sitecustomize_pth_file:
+                sitecustomize_pth_file.write("import _sitecustomize\n")
 
             # Write out the build-time value of LPCONFIG so that it can be
             # used by scripts as the default instance name.
diff --git a/utilities/js-deps b/utilities/js-deps
index e8059f1..30d9b36 100755
--- a/utilities/js-deps
+++ b/utilities/js-deps
@@ -1,7 +1,8 @@
-#!bin/py
+#! /usr/bin/python3 -S
 
 import _pythonpath  # noqa: F401
+
 from convoy.meta import main
 
-main()
 
+main()