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

Add support for building packages that use Poetry #467

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Sep 20, 2023

Fixes #459

This PR adds optional support for building packages that use Poetry. It includes some other minor changes:

  • comment and documentation updates to avoid "PyPI mirror" terminology in favor of simpler "local wheels" terminology (and some corrections to update us with recent changes in this repo, such as Reorganize how wheels are stored, stop special-casing bootstrap #382)
  • avoid use of deprecated imp module in tests
  • consolidate utility functions for easier maintenance; add tests including for previously uncovered code

I'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 the wip/poetry branch.

Test plan

General prep

Because you'll be hopping back and forth between securedrop-proxy and securedrop-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), because pipx 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 like diffoscope later to compare any changes between packages.

Verify no regressions for projects using requirements.txt

  1. Ensure you have local checkouts of securedrop-proxy at main and securedrop-builder at poetry-support (this PR). The instructions below assume that they share a parent directory.
  2. Add a dependency of your choosing to securedrop-proxy locally. For example, in the securedrop-proxy checkout:
  • Add cowsay==6.0 to requirements/requirements.in
  • Activate your venv (make venv && source .venv/bin/activate)
  • Run pip-compile --allow-unsafe --generate-hashes requirements/requirements.in --output-file requirements/requirements.txt
  1. In the securedrop-builder directory (ideally in a separate terminal), activate its venv (make install-deps && source .venv/bin/activate).
  2. Build the new wheel using PKG_DIR=../securedrop-proxy/ make build-wheels
  • Confirm no build errors
  • Verify that generated sha256sums.txt contains correct and expected checksum for the newly built wheel
  1. Sign the new sha256sums.txt: gpg --armor --output securedrop-proxy/sha256sums.txt.asc --detach-sig securedrop-proxy/sha256sums.txt
  2. Update the build-requirements.txt in securedrop-proxy by running PKG_DIR=../securedrop-proxy/ make requirements
  • Confirm build-requirements.txt is updated with correct checksum for the new wheel
  1. Build a new securedrop-proxy package with PKG_PATH=../securedrop-proxy/ make securedrop-proxy
  • Confirm that package is built successfully

Verify expected behavior for projects using Poetry

  1. Ensure you have local checkouts of securedrop-proxy at only-poetry and securedrop-builder at poetry-support-with-proxy-changes
  2. Examine the additional commit (relative to this PR) in securedrop-builder with git show. Note that the build-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 the only-poetry branch successfully, where this file has been moved to the root for simplicity.
  3. Delete any stale .venv directory in securedrop-proxy, undo any changes from previous testing, and ensure your venv is not active.
  4. Run poetry install to install the dependencies
  5. Run make test
  • Observe that tests are passing
  1. Activate the 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 #468
  2. Run PKG_PATH=../securedrop-proxy/ make securedrop-proxy
  • Observe that you can build the package
  1. Let's add a new dependency over in securedrop-proxy. For example, poetry add cowsay==6.0.
  2. Back in securedrop-builder, let's try building the new wheel: PKG_DIR=../securedrop-proxy/ make build-wheels
  • Observe that the build completes successfully and new wheels exist in the securedrop-proxy/wheels directory
  • Observe that sha256sums has been updated with the new wheel checksums (git diff)
  1. Sign the new shasums: gpg --armor --output securedrop-proxy/sha256sums.txt.asc --detach-sig securedrop- proxy/sha256sums.txt
  2. This wheel won't be used yet until we update build-requirements.txt over in securedrop-proxy. Let's do that with PKG_DIR=../securedrop-proxy make requirements.
  • Observe that build-requirements.txt in securedrop-proxy was updated successfully.
  1. Try building a package with the new wheel back in securedrop-builder: PKG_PATH=../securedrop-proxy/ make securedrop-proxy
  • Examine that the package does include the dependency you added (diffoscope is nice for it if you have a previous .deb to compare with).

Copy link
Member

@legoktm legoktm left a 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?

scripts/build-sync-wheels Outdated Show resolved Hide resolved
scripts/utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@eloquence
Copy link
Member Author

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 only-poetry branch and then update here as well.

Copy link
Member

@legoktm legoktm left a 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 :)

@eloquence eloquence marked this pull request as ready for review October 4, 2023 23:40
Copy link
Member

@legoktm legoktm left a 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.

scripts/update-requirements Outdated Show resolved Hide resolved
scripts/utils.py Outdated Show resolved Hide resolved
scripts/utils.py Outdated Show resolved Hide resolved
scripts/verify-hashes Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
scripts/utils.py Outdated Show resolved Hide resolved
scripts/verify-hashes Outdated Show resolved Hide resolved
@legoktm
Copy link
Member

legoktm commented Oct 20, 2023

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!!

@eloquence
Copy link
Member Author

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 build-requirements.txt change, so we can merge this together with the securedrop-proxy changes after this main compat PR lands.

Copy link
Member

@legoktm legoktm left a 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
@eloquence
Copy link
Member Author

Maximally squashed :)

@legoktm
Copy link
Member

legoktm commented Oct 25, 2023

Awesome :) Let's do it!

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

Successfully merging this pull request may close these issues.

Support SDW components that manage dependencies using poetry
2 participants