Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop old oldstable buster and sub new oldstable bullseye #1358

Merged
merged 1 commit into from
Jul 10, 2023
+15 −19
Merged

Drop old oldstable buster and sub new oldstable bullseye #1358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ jobs:
include:
- base_image: ubuntu:focal
tag: focal
- base_image: debian:buster
tag: buster
- base_image: debian:bullseye
tag: bullseye
- base_image: debian:testing
tag: testing
steps:
Expand Down Expand Up @@ -69,10 +69,10 @@ jobs:
dockerfile: synapse
tags: "matrixdotorg/sytest-synapse:focal"
build_args: "SYTEST_IMAGE_TAG=focal"
- sytest_image_tag: buster
- sytest_image_tag: bullseye
dockerfile: synapse
tags: "matrixdotorg/sytest-synapse:buster"
build_args: "SYTEST_IMAGE_TAG=buster"
tags: "matrixdotorg/sytest-synapse:bullseye"
build_args: "SYTEST_IMAGE_TAG=bullseye"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat surprised that we don't need SYSTEM_PIP_INSTALL_SUFFIX=--break-system-packages here, but if it works then LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it doesn't help that I don't grok Debian's version naming scheme.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption is that it works? The dockerfiles seemed to build but I could be missing something here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC I added the SYSTEM_PIP_INSTALL_SUFFIX stuff to get sytest working under debian testing. I had assumed bullseye was brand-new latest release and so would need this too, but that's not the case: bullseye is nearly two years old.

So yeah, given that the docker images build this is fine. Sorry for the noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all I appreciate the insight!

- sytest_image_tag: testing
dockerfile: synapse
tags: "matrixdotorg/sytest-synapse:testing"
Expand Down
14 changes: 7 additions & 7 deletions docker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ but its dependencies are.
Included currently is:

- `matrixdotorg/sytest` Base container with SyTest dependencies installed
- Tagged by underlying Debian/Ubuntu image: `focal`, `buster` or `testing`
- Tagged by underlying Debian/Ubuntu image: `focal`, `bullseye` or `testing`
- `matrixdotorg/sytest-synapse`: Runs SyTest against Synapse
- Tagged by underlying Debian/Ubunutu image: `focal`, `buster` or `testing`
- Tagged by underlying Debian/Ubunutu image: `focal`, `bullseye` or `testing`

## Target-specific details

Expand All @@ -23,7 +23,7 @@ it is useful to mount a volume there too.
For example:

```
docker run --rm -it -v /path/to/synapse\:/src:ro -v /path/to/where/you/want/logs\:/logs matrixdotorg/sytest-synapse:buster
docker run --rm -it -v /path/to/synapse\:/src:ro -v /path/to/where/you/want/logs\:/logs matrixdotorg/sytest-synapse:focal
```

The following environment variables can be set with `-e` to control the test run:
Expand All @@ -43,7 +43,7 @@ An example of running Synapse in worker mode:

```
docker run --rm -it -e POSTGRES=1 -e WORKERS=1 -v /path/to/synapse\:/src:ro \
-v /path/to/where/you/want/logs\:/logs matrixdotorg/sytest-synapse:buster
-v /path/to/where/you/want/logs\:/logs matrixdotorg/sytest-synapse:focal
```

### Dendrite
Expand All @@ -68,7 +68,7 @@ for example:
docker run --rm -it \
-e SYTEST_BRANCH="my-sytest-branch"
-v /path/to/synapse\:/src:ro -v /path/to/where/you/want/logs\:/logs
matrixdotorg/sytest-synapse:buster
matrixdotorg/sytest-synapse:focal
```

If the branch referred to by `SYTEST_BRANCH` does not exist, `develop` is used
Expand All @@ -80,7 +80,7 @@ the container:

```
docker run --rm -it -v /path/to/synapse\:/src:ro -v /path/to/where/you/want/logs\:/logs \
-v /path/to/code/sytest\:/sytest:ro matrixdotorg/sytest-synapse:buster
-v /path/to/code/sytest\:/sytest:ro matrixdotorg/sytest-synapse:focal
```

## Running a single test file, and other sytest commandline options
Expand All @@ -89,7 +89,7 @@ You can pass arguments to sytest by adding them at the end of the
docker command. For example:

```
docker run --rm -it ... matrixdotorg/sytest-synapse:buster tests/20profile-events.pl
docker run --rm -it ... matrixdotorg/sytest-synapse:focal tests/20profile-events.pl
```

## Building the containers
Expand Down
2 changes: 1 addition & 1 deletion docker/base.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ARG BASE_IMAGE=debian:buster
ARG BASE_IMAGE=debian:bullseye

FROM ${BASE_IMAGE}

Expand Down
2 changes: 1 addition & 1 deletion docker/dendrite.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ARG SYTEST_IMAGE_TAG=buster
ARG SYTEST_IMAGE_TAG=bullseye
FROM matrixdotorg/sytest:${SYTEST_IMAGE_TAG}

ARG GO_VERSION=1.19.1
Expand Down
6 changes: 1 addition & 5 deletions docker/synapse.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ARG SYTEST_IMAGE_TAG=buster
ARG SYTEST_IMAGE_TAG=bullseye

FROM matrixdotorg/sytest:${SYTEST_IMAGE_TAG}

Expand All @@ -21,13 +21,9 @@ RUN mkdir /rust /cargo

RUN curl -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path --default-toolchain stable --profile minimal

# Use the latest version of pip. This pulls in fixes not present in the
# pip version provided by Debian Buster. See
# https://github.com/pypa/setuptools/issues/3457#issuecomment-1190125849
# For now, we need to tell Debian we don't care that we're editing the system python
# installation.
# Some context in https://github.com/pypa/pip/issues/11381#issuecomment-1399263627
RUN ${PYTHON_VERSION} -m pip install -q --upgrade pip ${SYSTEM_PIP_INSTALL_SUFFIX}
Copy link
Contributor Author

@H-Shay H-Shay Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking on dropping this is my reading of the issue mentioned indicates that the version of pip shipped in bullseye (20.3.4-4) should be sufficient as it is greater than 19?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change seems sane to me (even though the sytest docker machinery does not).

I don't follow what is meant by

For now, we need to tell Debian we don't care that we're editing the system python installation. Some context in pypa/pip#11381 (comment)

which is amusing because I wrote the comment. #1334 and #1339 shed some light.

I think it's referring to SYSTEM_PIP_INSTALL_SUFFIX, and I think we probably need to leave this in place for now (unless every distribution is happy with the --break-system-packages option).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, when you refer to "I think we probably need to leave this in place for now", do you mean the SYSTEM_PIP_INSTALL_SUFFIX on line 31/27, or are you referring to the removal on line 30? (My assumption is that you mean the former but just wanted to be sure)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean the SYSTEM_PIP_INSTALL_SUFFIX on line 31/27, or are you referring to the removal on line 30?

Sorry: the former, keeping SYSTEM_PIP_INSTALL_SUFFIX as the diff is currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great thanks!

RUN ${PYTHON_VERSION} -m pip install -q --no-cache-dir poetry==1.3.2 ${SYSTEM_PIP_INSTALL_SUFFIX}

# As part of the Docker build, we attempt to pre-install Synapse's dependencies
Expand Down