Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Pin poetry.core in Docker images #12853

Merged
merged 5 commits into from
May 24, 2022
Merged

Pin poetry.core in Docker images #12853

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad4fbc3
Fix docker images
erikjohnston May 24, 2022
bba8491
Newsfile
erikjohnston May 24, 2022
37e8f50
Update to 1.1.13
erikjohnston May 24, 2022
9284e84
Actually pin what we want to pin, rather than just applying monkey at…
erikjohnston May 24, 2022
4e8b44a
Add back comment
erikjohnston May 24, 2022
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
1 change: 1 addition & 0 deletions changelog.d/12853.docker
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix the docker file after a dependency update.
Copy link
Contributor

@MadLittleMods MadLittleMods May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This changelog entry is not very descriptive and doesn't explain why we made the change (what was broken before).

For reference, these changes are spawning from some discussion in #synapse-dev:matrix.org which discusses the ModuleNotFoundError: No module named 'matrix_common' errors popping up when Complement is running in CI.

As far as I can tell, the dependency was missing in CI because of this reason mentioned in the room discussion:

hmm, the lockfile is changing all deps to from main to dev category :/
Yeah, that seems to be the problem, as export ignores dev deps by default :/

-- @erikjohnston

As the issue description already mentions, the root cause might be because we didn't pin poetry-core before and it was pulling in a recent beta release which introduced the unexpected problem behavior. In any case, pinning to poetry-core==1.1.0a7 makes it work as expected to install all of the deps.

This information should also be in the PR description. I'm not confident in my conclusion here to update it myself.

better PR subject and description please. The current ones will make zero sense in a few months time 😇

-- @richvdh, #12853 (comment)

4 changes: 1 addition & 3 deletions docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ RUN \
apt-get update && apt-get install -y git \
&& rm -rf /var/lib/apt/lists/*

# We install poetry in its own build stage to avoid its dependencies conflicting with
# synapse's dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is still correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

woops

# We use a specific commit from poetry's master branch instead of our usual 1.1.12,
# to incorporate fixes to some bugs in `poetry export`. This commit corresponds to
# https://github.com/python-poetry/poetry/pull/5156 and
Expand All @@ -55,7 +53,7 @@ RUN \
# NB: In poetry 1.2 `poetry export` will be moved into a plugin; we'll need to also
# pip install poetry-plugin-export (https://github.com/python-poetry/poetry-plugin-export).
RUN --mount=type=cache,target=/root/.cache/pip \
pip install --user git+https://github.com/python-poetry/poetry.git@fb13b3a676f476177f7937ffa480ee5cff9a90a5
pip install --user "poetry-core==1.1.0a7" "git+https://github.com/python-poetry/poetry.git@fb13b3a676f476177f7937ffa480ee5cff9a90a5"
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember fighting with this a lot, though I can't exactly remember what this commit did for us. Probably a case of "latest master fixed it" and this was the commit at the time.

I also remember that after I updated some dependency specifications in signedjson that there were suddenly fewer bugs with the export. Given that the debian image doesn't use this commit, I wonder if it might be worth trying to use the same version that the docker image uses.


WORKDIR /synapse

Expand Down