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.

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

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jul 7, 2023

As part of dropping support for python 3.7, drop debian buster (python version 3.7) and sub in debian bullseye (python 3.9).

Corresponding Synapse PR's: matrix-org/synapse#15892, matrix-org/synapse#15893

@H-Shay H-Shay requested a review from a team as a code owner July 7, 2023 18:54
# 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

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!

@H-Shay
Copy link
Contributor Author

H-Shay commented Jul 7, 2023

I did a manual run of the build pipeline (on this new branch) to verify that everything builds properly: https://github.com/matrix-org/sytest/actions/runs/5489675592/jobs/10005024592

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!

# 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

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).

@H-Shay H-Shay merged commit 50544bb into develop Jul 10, 2023
@H-Shay H-Shay deleted the shay/drop_buster branch July 10, 2023 16:20
@H-Shay
Copy link
Contributor Author

H-Shay commented Jul 10, 2023

Merged #1358 into develop, thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants