launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:no-system-site-packages into launchpad:master.

Commit message:
Stop using --system-site-packages

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/378946

We've been using "virtualenv --system-site-packages" to ensure that Launchpad has access to some system-installed packages that can't or shouldn't be installed as a normal Python dependency for various reasons.  This strategy has always had some problems, but we've been able to deal with them.

However, with recent versions of pip and Python 3, the situation gets worse.  https://github.com/pypa/pip/issues/6264 describes the problem: pip attempts to isolate itself from site-packages when building wheels, leaving only its temporary one; but within a virtualenv created with --system-site-packages it removes the virtualenv's site-packages but not the system one (because virtualenv patches distutils.sysconfig.get_python_lib).  It's not easy to see how pip might reliably and portably escape the virtualenv in order to find the system's site-packages directory.

(On Python 2 we get away with this by luck, because distutils.sysconfig.get_python_lib returns '.../env/lib/python2.7/site-packages' but '.../env/local/lib/python2.7/site-packages' is on sys.path, and pip doesn't realise that they're the same thing due to symlinks; we don't get this stroke of luck on Python 3.)

Installing everything properly in the virtualenv is also difficult.  Several of the ones that remain don't exist as wheels or in another form that pip can install.  We could fake this up (I went so far as to prepare a script to convert the installed version of python-apt into a wheel), but this would incur a significant maintenance burden.

Instead, symlink packages from the system site-packages directory into the virtualenv in "make compile".  It's conceivable that this might confuse pip slightly, but so far it doesn't seem to, and everything else works fine.

This shouldn't be landed until https://portal.admin.canonical.com/C124200 has been resolved and all production deployment targets have the python-tdb package installed.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:no-system-site-packages into launchpad:master.

diff --git a/Makefile b/Makefile
index aacda12..6ca5e50 100644
--- a/Makefile
+++ b/Makefile
@@ -16,15 +16,14 @@ PIP_ENV := LC_ALL=C.UTF-8
 # be reviewed/merged/deployed.
 PIP_NO_INDEX := 1
 PIP_ENV += PIP_NO_INDEX=$(PIP_NO_INDEX)
-# Although --ignore-installed is slower, we need it to avoid confusion with
-# system-installed Python packages.  If we ever manage to remove the need
-# for virtualenv --system-site-packages, then we can remove this too.
-PIP_ENV += PIP_IGNORE_INSTALLED=1
 PIP_ENV += PIP_FIND_LINKS=file://$(WD)/download-cache/dist/
 
 VIRTUALENV := $(PIP_ENV) virtualenv
 PIP := PYTHONPATH= $(PIP_ENV) env/bin/pip --cache-dir=$(WD)/download-cache/
 
+SITE_PACKAGES := \
+	$$(env/bin/python -c 'from distutils.sysconfig import get_python_lib; print(get_python_lib())')
+
 TESTFLAGS=-p $(VERBOSITY)
 TESTOPTS=
 
@@ -246,7 +245,7 @@ $(PY): download-cache constraints.txt setup.py
 	rm -rf env
 	mkdir -p env
 	$(VIRTUALENV) \
-		--python=$(PYTHON) --system-site-packages --never-download \
+		--python=$(PYTHON) --never-download \
 		--extra-search-dir=$(WD)/download-cache/dist/ \
 		env
 	ln -sfn env/bin bin
@@ -265,6 +264,8 @@ compile: $(PY)
 	${SHHH} utilities/relocate-virtualenv env
 	${SHHH} $(MAKE) -C sourcecode build PYTHON=${PYTHON} \
 	    LPCONFIG=${LPCONFIG}
+	$(PYTHON) utilities/link-system-packages.py \
+		"$(SITE_PACKAGES)" system-packages.txt
 	${SHHH} bin/build-twisted-plugin-cache
 	${SHHH} LPCONFIG=${LPCONFIG} ${PY} -t buildmailman.py
 	scripts/update-version-info.sh
@@ -487,13 +488,13 @@ reload-apache: enable-apache-launchpad
 TAGS: compile
 	# emacs tags
 	ctags -R -e --languages=-JavaScript --python-kinds=-i -f $@.new \
-		$(CURDIR)/lib $(CURDIR)/env/lib/$(PYTHON)/site-packages
+		$(CURDIR)/lib "$(SITE_PACKAGES)"
 	mv $@.new $@
 
 tags: compile
 	# vi tags
 	ctags -R --languages=-JavaScript --python-kinds=-i -f $@.new \
-		$(CURDIR)/lib $(CURDIR)/env/lib/$(PYTHON)/site-packages
+		$(CURDIR)/lib "$(SITE_PACKAGES)"
 	mv $@.new $@
 
 PYDOCTOR = pydoctor
diff --git a/doc/pip.txt b/doc/pip.txt
index b570ab8..23c024b 100644
--- a/doc/pip.txt
+++ b/doc/pip.txt
@@ -372,6 +372,5 @@ Possible Future Goals
 =====================
 
 - Use wheels.
-- No longer use system site-packages.
 - No longer use make.
 - Get rid of the sourcecode directory.
diff --git a/setup-requirements.txt b/setup-requirements.txt
index 11ef58a..e7472fc 100644
--- a/setup-requirements.txt
+++ b/setup-requirements.txt
@@ -1,10 +1,3 @@
 pip==20.0.2
 setuptools==44.0.0
 wheel==0.34.2
-
-# Install these early to avoid mismatches with a system-installed
-# python-cffi-backend package.
-# XXX cjwatson 2020-02-06: This would all be much easier if we could remove
-# the need for virtualenv --system-site-packages.
-cffi==1.12.2
-pycparser==2.19
diff --git a/setup.py b/setup.py
index e149110..4a7d190 100644
--- a/setup.py
+++ b/setup.py
@@ -190,8 +190,7 @@ setup(
         'lpjsmin',
         'Markdown',
         'meliae',
-        # Pin version for now to avoid confusion with system site-packages.
-        'mock==1.0.1',
+        'mock',
         'oauth',
         'oops',
         'oops_amqp',
diff --git a/system-packages.txt b/system-packages.txt
new file mode 100644
index 0000000..1737888
--- /dev/null
+++ b/system-packages.txt
@@ -0,0 +1,32 @@
+# System-installed Python packages to link into our virtualenv.  This
+# facility should be reserved for cases where installing them as a normal
+# Python dependency is impossible or unreliable (perhaps due to frequent ABI
+# changes in system libraries they depend on, or frequent security updates
+# managed by the distribution's security team).
+
+# Used by launchpad-buildd.
+apt
+
+# Used by Soyuz and other related code to parse Debian packages and
+# repository index files, and to compare Debian version numbers.
+apt_inst
+apt_pkg
+
+# utilities/js-deps
+convoy
+
+# lp.services.geoip.model
+GeoIP
+
+# lp.testing.html5browser
+gi
+
+# lp.services.fields, lp.services.spriteutils
+PIL
+
+# Dependency of cscvs.
+sqlite
+_sqlite
+
+# Optional dependency of bzr-git and bzr-svn.
+tdb
diff --git a/utilities/link-system-packages.py b/utilities/link-system-packages.py
new file mode 100755
index 0000000..c760f49
--- /dev/null
+++ b/utilities/link-system-packages.py
@@ -0,0 +1,47 @@
+#! /usr/bin/python
+
+# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Link system-installed Python modules into Launchpad's virtualenv."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+from argparse import ArgumentParser
+from distutils.sysconfig import get_python_lib
+import importlib
+import os.path
+import re
+
+
+def link_module(name, virtualenv_libdir):
+    module = importlib.import_module(name)
+    path = module.__file__
+    if os.path.basename(path).startswith("__init__."):
+        path = os.path.dirname(path)
+    system_libdir = get_python_lib(plat_specific=path.endswith(".so"))
+    if os.path.commonprefix([path, system_libdir]) != system_libdir:
+        raise RuntimeError(
+            "%s imported from outside %s (%s)" % (name, system_libdir, path))
+    target_path = os.path.join(
+        virtualenv_libdir, os.path.relpath(path, system_libdir))
+    if os.path.lexists(target_path) and os.path.islink(target_path):
+        os.unlink(target_path)
+    os.symlink(path, target_path)
+
+
+def main():
+    parser = ArgumentParser()
+    parser.add_argument("virtualenv_libdir")
+    parser.add_argument("module_file", type=open)
+    args = parser.parse_args()
+
+    for line in args.module_file:
+        line = re.sub(r"#.*", "", line).strip()
+        if not line:
+            continue
+        link_module(line, args.virtualenv_libdir)
+
+
+if __name__ == "__main__":
+    main()