From 50910dae6800fb11fb5d9a40dc72458c18eca41d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 10 Mar 2022 14:39:40 +0000 Subject: [PATCH 01/14] CI: Fix the olddeps job to run under poetry --- .ci/scripts/test_old_deps.sh | 58 ++++++++++++++++++++++++++++++++---- .github/workflows/tests.yml | 6 ++-- tox.ini | 22 -------------- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index b2859f752261..7600f5332fb0 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -1,6 +1,9 @@ #!/usr/bin/env bash -# this script is run by GitHub Actions in a plain `focal` container; it installs the -# minimal requirements for tox and hands over to the py3-old tox environment. +# this script is run by GitHub Actions in a plain `focal` container; it +# - installs the minimal system requirements, and poetry; +# - patches the project definition file to refer to old versions only; +# - creates a venv with these old versions using poetry; and finally +# - invokes `trial` to run the tests with old deps. # Prevent tzdata from asking for user input export DEBIAN_FRONTEND=noninteractive @@ -9,12 +12,57 @@ set -ex apt-get update apt-get install -y \ - python3 python3-dev python3-pip python3-venv \ - libxml2-dev libxslt-dev xmlsec1 zlib1g-dev tox libjpeg-dev libwebp-dev + python3 python3-dev python3-pip python3-venv pipx \ + libxml2-dev libxslt-dev xmlsec1 zlib1g-dev libjpeg-dev libwebp-dev export LANG="C.UTF-8" # Prevent virtualenv from auto-updating pip to an incompatible version export VIRTUALENV_NO_DOWNLOAD=1 -exec tox -e py3-old +# I'd prefer to use something like this +# https://github.com/python-poetry/poetry/issues/3527 +# https://github.com/pypa/pip/issues/8085 +# rather than this sed script. But that's an Opinion. + +# patch the project definitions in-place +# replace all lower bounds with exact bounds +# but make the pyopenssl 17.0, which can work against an +# OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). +# delete all lines referring to psycopg2 --- so no testing of postgres support +# Omit systemd: we're not logging to journal here. + +sed -i-backup \ + -e "s/[~>]=/==/g" \ + -e "/psycopg2/d" \ + -e 's/pyOpenSSL = "==16.0.0"/pyOpenSSL = "==17.0.0"/' \ + -e '/psycopg2/d' \ + -e '/systemd/d' \ + pyproject.toml + +# Use poetry to do the installation. This ensures that the versions are all mutually +# compatible (as far the package metadata declares, anyway); pip's package resolver +# is more lax. +# +# Rather than `poetry install --no-dev`, we drop all dev dependencies from the +# toml file. This means we don't have to ensure compatibility between old deps and +# dev tools. + +pip install --user toml + +REMOVE_DEV_DEPENDENCIES=" +import toml +with open('pyproject.toml', 'r') as f: + data = toml.loads(f.read()) + +del data['tool']['poetry']['dev-dependencies'] + +with open('pyproject.toml', 'w') as f: + toml.dump(data, f) +" +python3 -c "$REMOVE_DEV_DEPENDENCIES" + +pipx install poetry==1.1.12 +~/.local/bin/poetry lock +~/.local/bin/poetry install -E "all test" +~/.local/bin/poetry run trial -j2 tests diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 5c29867cc89c..a403f076bb73 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -128,18 +128,18 @@ jobs: || true trial-olddeps: + # Note: sqlite only; no postgres if: ${{ !cancelled() && !failure() }} # Allow previous steps to be skipped, but not fail - needs: linting-done +# needs: linting-done runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - name: Test with old deps uses: docker://ubuntu:focal # For old python and sqlite + # Note: focal seems to be using 3.8, but the oldest is 3.7? with: workdir: /github/workspace entrypoint: .ci/scripts/test_old_deps.sh - env: - TRIAL_FLAGS: "--jobs=2" - name: Dump logs # Logs are most useful when the command fails, always include them. if: ${{ always() }} diff --git a/tox.ini b/tox.ini index 69476b5869aa..a96a76bf28be 100644 --- a/tox.ini +++ b/tox.ini @@ -105,29 +105,7 @@ commands = # ) usedevelop=true -# A test suite for the oldest supported versions of Python libraries, to catch -# any uses of APIs not available in them. -[testenv:py3-old] -skip_install = true -usedevelop = false -deps = - Automat == 0.8.0 - lxml - # markupsafe 2.1 introduced a change that breaks Jinja 2.x. Since we depend on - # Jinja >= 2.9, it means this test suite will fail if markupsafe >= 2.1 is installed. - markupsafe < 2.1 - {[base]deps} - -commands = - # Make all greater-thans equals so we test the oldest version of our direct - # dependencies, but make the pyopenssl 17.0, which can work against an - # OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). - /bin/sh -c 'python -m synapse.python_dependencies | sed -e "s/>=/==/g" -e "/psycopg2/d" -e "s/pyopenssl==16.0.0/pyopenssl==17.0.0/" | xargs -d"\n" pip install' - - # Install Synapse itself. This won't update any libraries. - pip install -e ".[test]" - {[testenv]commands} [testenv:benchmark] deps = From a075c2d8c55e49f5ec1dcabf347547c30dd2d36f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 31 Mar 2022 18:21:03 +0100 Subject: [PATCH 02/14] olddeps: echo patch to confirm sed is sensible --- .ci/scripts/test_old_deps.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 7600f5332fb0..735c1d1e7d5e 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -62,6 +62,10 @@ with open('pyproject.toml', 'w') as f: " python3 -c "$REMOVE_DEV_DEPENDENCIES" +echo "::group::Patched pyproject.toml" +diff -u pyproject.toml-backup pyproject.toml +echo "::endgroup::" + pipx install poetry==1.1.12 ~/.local/bin/poetry lock ~/.local/bin/poetry install -E "all test" From cae1cf4f8fe3a1f00b00a566ad1c212e1cdfee80 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 31 Mar 2022 18:21:31 +0100 Subject: [PATCH 03/14] Olddeps: leave a TODO for caret bounds --- .ci/scripts/test_old_deps.sh | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 735c1d1e7d5e..2bd364318b9a 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -25,12 +25,18 @@ export VIRTUALENV_NO_DOWNLOAD=1 # https://github.com/pypa/pip/issues/8085 # rather than this sed script. But that's an Opinion. -# patch the project definitions in-place -# replace all lower bounds with exact bounds -# but make the pyopenssl 17.0, which can work against an -# OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). -# delete all lines referring to psycopg2 --- so no testing of postgres support -# Omit systemd: we're not logging to journal here. +# Patch the project definitions in-place: +# - Replace all lower and tilde bounds with exact bounds +# - Make the pyopenssl 17.0, which can work against an +# OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). +# - Delete all lines referring to psycopg2 --- so no testing of postgres support. +# - Omit systemd: we're not logging to journal here. + +# TODO: also replace caret bounds, see https://python-poetry.org/docs/dependency-specification/#version-constraints +# We don't use these yet, but IIRC they are the default bound used when you `poetry add`. +# The sed expression 's/\^/==/g' ought to do the trick. But it would also change +# `python = "^3.7"` to `python = "==3.7", which would mean we fail because olddeps +# runs on 3.8 (#12343). sed -i-backup \ -e "s/[~>]=/==/g" \ From 544ef606246acff6a77cc83d4bfac070286fc562 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 1 Apr 2022 12:12:53 +0100 Subject: [PATCH 04/14] Olddeps: cat patched pyproject, instead of diffing --- .ci/scripts/test_old_deps.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 2bd364318b9a..afd15378e401 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -69,7 +69,7 @@ with open('pyproject.toml', 'w') as f: python3 -c "$REMOVE_DEV_DEPENDENCIES" echo "::group::Patched pyproject.toml" -diff -u pyproject.toml-backup pyproject.toml +cat pyproject.toml echo "::endgroup::" pipx install poetry==1.1.12 From 507d9e0b8ca4126a0982e0722521b9e24e3dd89c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 12:47:44 +0100 Subject: [PATCH 05/14] Changelog --- changelog.d/12407.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12407.misc diff --git a/changelog.d/12407.misc b/changelog.d/12407.misc new file mode 100644 index 000000000000..896da9b74deb --- /dev/null +++ b/changelog.d/12407.misc @@ -0,0 +1 @@ +Run the olddeps CI job using Poetry. \ No newline at end of file From 35b546cc4dba0c442488d1a4ad05a9c81cbf9769 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 12:49:52 +0100 Subject: [PATCH 06/14] Confirm caret bound problem --- .ci/scripts/test_old_deps.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index afd15378e401..0743099b9b6f 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -32,10 +32,12 @@ export VIRTUALENV_NO_DOWNLOAD=1 # - Delete all lines referring to psycopg2 --- so no testing of postgres support. # - Omit systemd: we're not logging to journal here. -# TODO: also replace caret bounds, see https://python-poetry.org/docs/dependency-specification/#version-constraints -# We don't use these yet, but IIRC they are the default bound used when you `poetry add`. +# TODO: we should also replace caret bounds, see +# https://python-poetry.org/docs/dependency-specification/#version-constraints +# We don't use these yet, but they are the default bound used when you `poetry add` from +# the commandline, rather than editing pyproject.toml directly. # The sed expression 's/\^/==/g' ought to do the trick. But it would also change -# `python = "^3.7"` to `python = "==3.7", which would mean we fail because olddeps +# `python = "^3.7"` to `python = "==3.7"`, which would mean we fail because olddeps # runs on 3.8 (#12343). sed -i-backup \ From fe43772e067076d3cc6d7635f1fdbbdb0c48d3b0 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 13:17:58 +0100 Subject: [PATCH 07/14] Add issue link --- .github/workflows/tests.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a403f076bb73..3abc0ebc792e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -137,6 +137,7 @@ jobs: - name: Test with old deps uses: docker://ubuntu:focal # For old python and sqlite # Note: focal seems to be using 3.8, but the oldest is 3.7? + # See https://github.com/matrix-org/synapse/issues/12343 with: workdir: /github/workspace entrypoint: .ci/scripts/test_old_deps.sh From ae33fc5d808696a9ba285292444bd4814afc6b00 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 13:18:16 +0100 Subject: [PATCH 08/14] Use `--jobs=2` for consistency with other CI --- .ci/scripts/test_old_deps.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 0743099b9b6f..8f21698900b2 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -77,4 +77,4 @@ echo "::endgroup::" pipx install poetry==1.1.12 ~/.local/bin/poetry lock ~/.local/bin/poetry install -E "all test" -~/.local/bin/poetry run trial -j2 tests +~/.local/bin/poetry run trial --jobs=2 tests From 992caed99daf64929af2e6afb86cec566be6bd2a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 13:18:50 +0100 Subject: [PATCH 09/14] Update pyopenssl comment --- .ci/scripts/test_old_deps.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 8f21698900b2..7788081b99e5 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -27,8 +27,8 @@ export VIRTUALENV_NO_DOWNLOAD=1 # Patch the project definitions in-place: # - Replace all lower and tilde bounds with exact bounds -# - Make the pyopenssl 17.0, which can work against an -# OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). +# - Make the pyopenssl 17.0, which is the oldest version that works with +# a `cryptography` compiled against OpenSSL 1.1. # - Delete all lines referring to psycopg2 --- so no testing of postgres support. # - Omit systemd: we're not logging to journal here. From 204e419043f4b8b84b9ef396cd96978807a49bae Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 13:19:11 +0100 Subject: [PATCH 10/14] Rearrange and improve comments --- .ci/scripts/test_old_deps.sh | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 7788081b99e5..6ebd6f1220ea 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -20,11 +20,6 @@ export LANG="C.UTF-8" # Prevent virtualenv from auto-updating pip to an incompatible version export VIRTUALENV_NO_DOWNLOAD=1 -# I'd prefer to use something like this -# https://github.com/python-poetry/poetry/issues/3527 -# https://github.com/pypa/pip/issues/8085 -# rather than this sed script. But that's an Opinion. - # Patch the project definitions in-place: # - Replace all lower and tilde bounds with exact bounds # - Make the pyopenssl 17.0, which is the oldest version that works with @@ -40,6 +35,11 @@ export VIRTUALENV_NO_DOWNLOAD=1 # `python = "^3.7"` to `python = "==3.7"`, which would mean we fail because olddeps # runs on 3.8 (#12343). +# TODO: I'd prefer to use something like this +# https://github.com/python-poetry/poetry/issues/3527 +# https://github.com/pypa/pip/issues/8085 +# rather than this sed script. + sed -i-backup \ -e "s/[~>]=/==/g" \ -e "/psycopg2/d" \ @@ -48,13 +48,14 @@ sed -i-backup \ -e '/systemd/d' \ pyproject.toml -# Use poetry to do the installation. This ensures that the versions are all mutually -# compatible (as far the package metadata declares, anyway); pip's package resolver -# is more lax. +# TODO: once pyproject.toml uses poetry-core as its build-system, we may be able to +# simply `pip install .[all, test]` and run trial directly. (We would have to convince +# ourselves that pip will refuse to install if the olddeps are incompatible with each +# other: folklore contends that pip's resolver is more lax than poetry's.) # -# Rather than `poetry install --no-dev`, we drop all dev dependencies from the -# toml file. This means we don't have to ensure compatibility between old deps and -# dev tools. +# Until then, setuptools is the build system. That means we need to use `poetry` to +# do the installation. `poetry lock` fails because of incompatibilities between dev +# dependencies and old deps. Workaround this by removing dev dependencies entirely. pip install --user toml @@ -70,11 +71,15 @@ with open('pyproject.toml', 'w') as f: " python3 -c "$REMOVE_DEV_DEPENDENCIES" +pipx install poetry==1.1.12 +~/.local/bin/poetry lock + echo "::group::Patched pyproject.toml" cat pyproject.toml echo "::endgroup::" +echo "::group::Lockfile after patch" +cat poetry.lock +echo "::endgroup::" -pipx install poetry==1.1.12 -~/.local/bin/poetry lock ~/.local/bin/poetry install -E "all test" ~/.local/bin/poetry run trial --jobs=2 tests From d8bee664c59a07f033c491b03d90a38d8801fd95 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Apr 2022 13:27:07 +0100 Subject: [PATCH 11/14] Remove debug, so oldeps runs after linting as is the case on `develop` --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 3abc0ebc792e..037c34044dbf 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -130,7 +130,7 @@ jobs: trial-olddeps: # Note: sqlite only; no postgres if: ${{ !cancelled() && !failure() }} # Allow previous steps to be skipped, but not fail -# needs: linting-done + needs: linting-done runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 From 695eb3c5675968c059be3b62a17c987b35c172e6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 31 Mar 2022 14:37:36 +0100 Subject: [PATCH 12/14] Add suggestions missed when cherry-picking from #12337 --- .ci/scripts/test_old_deps.sh | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index 6ebd6f1220ea..b77a62a11d40 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -20,42 +20,38 @@ export LANG="C.UTF-8" # Prevent virtualenv from auto-updating pip to an incompatible version export VIRTUALENV_NO_DOWNLOAD=1 +# TODO: in the future, we could use an implementation of +# https://github.com/python-poetry/poetry/issues/3527 +# https://github.com/pypa/pip/issues/8085 +# to select the lowest possible versions, rather than resorting to this sed script. + # Patch the project definitions in-place: # - Replace all lower and tilde bounds with exact bounds -# - Make the pyopenssl 17.0, which is the oldest version that works with -# a `cryptography` compiled against OpenSSL 1.1. +# - Make the pyopenssl 17.0, which can work against an +# OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). # - Delete all lines referring to psycopg2 --- so no testing of postgres support. # - Omit systemd: we're not logging to journal here. -# TODO: we should also replace caret bounds, see -# https://python-poetry.org/docs/dependency-specification/#version-constraints -# We don't use these yet, but they are the default bound used when you `poetry add` from -# the commandline, rather than editing pyproject.toml directly. +# TODO: also replace caret bounds, see https://python-poetry.org/docs/dependency-specification/#version-constraints +# We don't use these yet, but IIRC they are the default bound used when you `poetry add`. # The sed expression 's/\^/==/g' ought to do the trick. But it would also change -# `python = "^3.7"` to `python = "==3.7"`, which would mean we fail because olddeps +# `python = "^3.7"` to `python = "==3.7", which would mean we fail because olddeps # runs on 3.8 (#12343). -# TODO: I'd prefer to use something like this -# https://github.com/python-poetry/poetry/issues/3527 -# https://github.com/pypa/pip/issues/8085 -# rather than this sed script. - sed -i-backup \ -e "s/[~>]=/==/g" \ -e "/psycopg2/d" \ -e 's/pyOpenSSL = "==16.0.0"/pyOpenSSL = "==17.0.0"/' \ - -e '/psycopg2/d' \ -e '/systemd/d' \ pyproject.toml -# TODO: once pyproject.toml uses poetry-core as its build-system, we may be able to -# simply `pip install .[all, test]` and run trial directly. (We would have to convince -# ourselves that pip will refuse to install if the olddeps are incompatible with each -# other: folklore contends that pip's resolver is more lax than poetry's.) +# Use poetry to do the installation. This ensures that the versions are all mutually +# compatible (as far the package metadata declares, anyway); pip's package resolver +# is more lax. # -# Until then, setuptools is the build system. That means we need to use `poetry` to -# do the installation. `poetry lock` fails because of incompatibilities between dev -# dependencies and old deps. Workaround this by removing dev dependencies entirely. +# Rather than `poetry install --no-dev`, we drop all dev dependencies from the +# toml file. This means we don't have to ensure compatibility between old deps and +# dev tools. pip install --user toml From 160bbd908e39190f7759a099a22266f033f76adf Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Apr 2022 18:11:49 +0100 Subject: [PATCH 13/14] No need for sed to make a backup Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- .ci/scripts/test_old_deps.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index b77a62a11d40..c82025df5c17 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -38,7 +38,7 @@ export VIRTUALENV_NO_DOWNLOAD=1 # `python = "^3.7"` to `python = "==3.7", which would mean we fail because olddeps # runs on 3.8 (#12343). -sed -i-backup \ +sed -i \ -e "s/[~>]=/==/g" \ -e "/psycopg2/d" \ -e 's/pyOpenSSL = "==16.0.0"/pyOpenSSL = "==17.0.0"/' \ From 192d1abb57e447e229ec30e44d74111527d371a2 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 12 Apr 2022 11:22:25 +0100 Subject: [PATCH 14/14] Don't use the old comment about pyopenssl 17 --- .ci/scripts/test_old_deps.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.ci/scripts/test_old_deps.sh b/.ci/scripts/test_old_deps.sh index c82025df5c17..769ca4517edf 100755 --- a/.ci/scripts/test_old_deps.sh +++ b/.ci/scripts/test_old_deps.sh @@ -27,8 +27,8 @@ export VIRTUALENV_NO_DOWNLOAD=1 # Patch the project definitions in-place: # - Replace all lower and tilde bounds with exact bounds -# - Make the pyopenssl 17.0, which can work against an -# OpenSSL 1.1 compiled cryptography (as older ones don't compile on Travis). +# - Make the pyopenssl 17.0, which is the oldest version that works with +# a `cryptography` compiled against OpenSSL 1.1. # - Delete all lines referring to psycopg2 --- so no testing of postgres support. # - Omit systemd: we're not logging to journal here.