-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add support for building packages that use Poetry #467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all great, I checked out the only-poetry
branch, then ran poetry update
(pinning an older idna for now) and was able to build new wheels and then update the build-requirements.txt file!!
I left some code suggestions inline. The only other comment I have is whether it makes sense to keep the requirements/
folder since it'll now contain just one item, the build-requirements.txt
file. I think we could move it into the root now?
Makes sense, I'll do that in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not giving it the green check yet since it's still a draft and the sd-proxy side needs to be finished, but overall this LGTM :)
7a05b08
to
69ac412
Compare
69ac412
to
3591d52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code overall looks good, just some small things. Running through the test plan now.
I was able to successfully run through the whole test plan on bullseye (bookworm is busted because of the pyyaml wheels issue). I think the main issue is figuring out how we want to handle back-compat of CI pipelines that currently don't activate the venv - either have them activate the venv before this lands or write in extra back-compat in this PR so it gracefully works. Regarding the: -REQUIREMENTS_FILE=requirements/build-requirements.txt
+REQUIREMENTS_FILE=build-requirements.txt change, I'm wondering if you want to just have that be a PR that we coordinate merging at the same time, or write some temporary if exists/else logic in the make file to detect the correct path that can be removed post-poetryification. Other than that, I think we're very close!! |
Thanks @legoktm, I think I've addressed your comments, but let me know if not! I've opened a new draft PR with just the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me - do you want to squash or merge as-is?
Also applies code formatting and cleans up some outdated descriptions/instructions
edd42ab
to
405c735
Compare
Maximally squashed :) |
Awesome :) Let's do it! |
Fixes #459
This PR adds optional support for building packages that use Poetry. It includes some other minor changes:
imp
module in testsI've used
black
code formatting in anticipation of making this part of our CI tooling for this repo. It pulls in @legoktm's work in thewip/poetry
branch.Test plan
General prep
Because you'll be hopping back and forth between
securedrop-proxy
andsecuredrop-builder
, I recommend using two separate terminals during this testing process.You'll need Poetry itself. Install the latest version (1.6.1 as of this writing). It offers several installation methods. I favor the
pipx
method (https://python-poetry.org/docs/#installing-with-pipx), becausepipx
itself is useful to have around and serves a different purpose than Poetry (it lets you safely install CLI tools written in Python). I've tested these changes with Python 3.9 in a Debian 11 (Bullseye) VM.You'll be building debs repeatedly during this process. Every time you successfully build a package, I recommend stashing it away under a descriptive name like
with-poetry.deb
. That way, you can use a tool likediffoscope
later to compare any changes between packages.Verify no regressions for projects using
requirements.txt
securedrop-proxy
atmain
andsecuredrop-builder
atpoetry-support
(this PR). The instructions below assume that they share a parent directory.securedrop-proxy
locally. For example, in thesecuredrop-proxy
checkout:cowsay==6.0
torequirements/requirements.in
make venv && source .venv/bin/activate
)pip-compile --allow-unsafe --generate-hashes requirements/requirements.in --output-file requirements/requirements.txt
securedrop-builder
directory (ideally in a separate terminal), activate its venv (make install-deps && source .venv/bin/activate
).PKG_DIR=../securedrop-proxy/ make build-wheels
sha256sums.txt
contains correct and expected checksum for the newly built wheelsha256sums.txt
:gpg --armor --output securedrop-proxy/sha256sums.txt.asc --detach-sig securedrop-proxy/sha256sums.txt
build-requirements.txt
insecuredrop-proxy
by runningPKG_DIR=../securedrop-proxy/ make requirements
build-requirements.txt
is updated with correct checksum for the new wheelsecuredrop-proxy
package withPKG_PATH=../securedrop-proxy/ make securedrop-proxy
Verify expected behavior for projects using Poetry
securedrop-proxy
atonly-poetry
andsecuredrop-builder
atpoetry-support-with-proxy-changes
securedrop-builder
withgit show
. Note that thebuild-requirements.txt
location is adjusted to be in the root of the repository. This change is necessary to build the repository with the changes in theonly-poetry
branch successfully, where this file has been moved to the root for simplicity..venv
directory insecuredrop-proxy
, undo any changes from previous testing, and ensure your venv is not active.poetry install
to install the dependenciesmake test
securedrop-builder
venv:make install-deps && source .venv/bin/activate
- in future this too will be done via Poetry: Use poetry for bootstrap dependency mgmt #468PKG_PATH=../securedrop-proxy/ make securedrop-proxy
securedrop-proxy
. For example,poetry add cowsay==6.0
.securedrop-builder
, let's try building the new wheel:PKG_DIR=../securedrop-proxy/ make build-wheels
securedrop-proxy/wheels
directorygit diff
)gpg --armor --output securedrop-proxy/sha256sums.txt.asc --detach-sig securedrop- proxy/sha256sums.txt
build-requirements.txt
over insecuredrop-proxy
. Let's do that withPKG_DIR=../securedrop-proxy make requirements
.build-requirements.txt
insecuredrop-proxy
was updated successfully.securedrop-builder
:PKG_PATH=../securedrop-proxy/ make securedrop-proxy
diffoscope
is nice for it if you have a previous .deb to compare with).