Pin dependencies with poetry, and use this in CI #488

DMRobertson · 2022-01-26T17:57:56Z

Use poetry rather than setuptools to define the package.
Use workflows in backend-meta and the setup-python-poetry action to use poetry in CI.

reivilibre

Seems pretty reasonable otherwise, but it's still a draft so I won't poke further

reivilibre · 2022-02-03T09:43:28Z

.github/workflows/changelog_check.yml

@@ -1,17 +0,0 @@
-name: Changelog


is there a particular reason for coalescing all the pipelines into one? I quite liked the separation because it means there's much less GHA YAML to grok at once when you have to come and mess with it.

Yes and no. Having stuff in one file (e.g. by using a matrix strategy) can help reduce duplication. I'm trying to prevent one file/project having an outdated bit of config where possible. I did deliberately lump the linting into one reusable action so that there's less yaml duplicated between projects.

No hard and fast rules here though.

Oh, and AFAIK it's easiest to make one piece of CI conditional on another piece of CI when they're defined in the same file.

I've also moved the changelog checks into the same yaml file, though that doesn't really interact with poetry.

DMRobertson · 2022-02-08T17:48:50Z

I need to update this with a description of the diff between wheels and sdists before and after poetry.

DMRobertson · 2022-02-08T18:46:55Z

I need to update this with a description of the diff between wheels and sdists before and after poetry.

These are a diff from a489866 to e9e1b7d.

sdist.diff.txt
wheel.diff.txt

In the sdist:

no more MANIFEST.in, matrix_sydent.egg-info
poetry includes matrix_is_test, scripts, scripts-dev, matrix-sydent.service, pyproject.toml,
poetry generates a PKG-INFO and setup.py with more metadata
poetry includes an openmarket change because that branch doesn't have a489866

In the wheel:

poetry does NOT include matrix_is_test
poetry generates a slightly more complete METADATA file
poetry's RECORD file is slightly different: entries are sorted alphabetically, whereas sydent lists files before subdirectories. Additionally, openmarket.py has a different hash, but that's expected because the poetry branch doesn't have a489866
poetry marks itself as the generator of the WHEEL file

So TL;DR there are some slight differences, but I think everything is legitimate.

reivilibre · 2022-02-09T11:04:17Z

no more MANIFEST.in
I was going to reply:
Somewhat paradoxically-sounding, I would think MANIFEST.in should be included in the sdist.
Python's docs say that it is by default: https://packaging.python.org/en/latest/guides/using-manifest-in/#how-files-are-included-in-an-sdist.

But actually I think you're not even using it anymore; instead configuring the sdist through Poetry. That sounds fine then, since pyproject.toml is included. Will review rest..

reivilibre

Looks good, but any chance you can provide updated instructions for contributing (and for developing... I've never touched poetry! :)).

Do the instructions in https://github.com/matrix-org/sydent/blob/main/CONTRIBUTING.md still work? I would guess not from what I've heard.

reivilibre · 2022-02-09T11:09:04Z

.github/workflows/pipeline.yml

+      - run: "scripts-dev/check_newsfragment.sh ${{ github.event.number }}"
+
+  common-python-ci:
+    uses: "matrix-org/backend-meta/.github/workflows/python-poetry-ci.yml@v1"


What are the semantics of @v1 here — is that fixed to a literal git tag, or something smarter?
If so, to update it, do we have to come back here and replace it with @v2?

What are the semantics of @v1 here — is that fixed to a literal git tag, or something smarter?

Literal git tag, branch name or SHA hash.

If so, to update it, do we have to come back here and replace it with @v2?

GHA's advice on versioning is here. (That's for a custom action rather than a reusable workflow, but I think the advice is worth following here too.) TL;DR:

use semver

tag each released version as vMAJOR.MINOR.PATCH.

maintain a major version tag vMAJOR.

if you make a MINOR or PATCH release, move the major version tag to point to the latest release.

if you make a MINOR or PATCH release, move the major version tag to point to the latest release.

Aha, I see — I seem to remember it being bad karma to change a tag but I suppose if it's for an action where this is the guideline, it's fine, I think.

Yeah. I think it needs a --force to actually move the tag.

reivilibre

woots

reivilibre · 2022-02-09T15:45:52Z

.github/workflows/pipeline.yml

+      - run: "scripts-dev/check_newsfragment.sh ${{ github.event.number }}"
+
+  common-python-ci:
+    uses: "matrix-org/backend-meta/.github/workflows/python-poetry-ci.yml@v1"


if you make a MINOR or PATCH release, move the major version tag to point to the latest release.

Aha, I see — I seem to remember it being bad karma to change a tag but I suppose if it's for an action where this is the guideline, it's fine, I think.

reivilibre · 2022-02-09T15:48:15Z

CONTRIBUTING.md

+
+When you're done, you can close your terminal.
+
+### Optional: `direnv`


big thanks for this one; my first question was going to be 'bu-bu-but my direnv workflow!!' but you even thought of that :-)

I've found it works pretty well once you have a poetry definition setup.

reivilibre · 2022-02-09T15:51:37Z

scripts-dev/lint.sh

+black .
+flake8
+isort .


How smart are the . parameters to these — will they descend into ignored dirs etc?

I suppose there's no longer such a question as 'will it descend into venvs' because Poetry venvs are somewhere else?

Happy to roll with this, after all.. don't mind me.

I think you can ask poetry to use an existing virtualenv or to create one in a specific location.

How smart are the . parameters to these — will they descend into ignored dirs etc?

black --verbose skips .gitignored directories. To be honest, I haven't tried the rest recently but I think they did sensible things when I was prototyping this a few weeks back.

'will it descend into venvs'

I did have this problem because one prototype of the setup-poetry action was putting the venv in .venv in the checkout. Let me double check that's not still the case...

Yeah, the poetry venv is in path: /home/runner/.cache/pypoetry/virtualenvs (from the CI logs)

DMRobertson · 2022-02-09T16:04:05Z

@reivilibre I've fixed a typo which means that CI actually runs now. Maybe you'd like to have a quick look over the CI output to see if it's to your liking?

reivilibre · 2022-02-09T16:11:13Z

yep, looks like a decent CI output layout! @DMRobertson

David Robertson added 2 commits January 26, 2022 16:44

Ignore .envrc for direnv users

1080184

Replace setup.py with pyproject.toml

0c518e8

DMRobertson force-pushed the dmr/poetry branch 3 times, most recently from ef4c815 to 25cf1e3 Compare January 26, 2022 19:29

This was referenced Feb 2, 2022

GitHub action to setup poetry matrix-org/setup-python-poetry#1

Merged

Lock Dependencies in Synapse matrix-org/synapse#11537

Closed

reivilibre reviewed Feb 3, 2022

View reviewed changes

DMRobertson mentioned this pull request Feb 8, 2022

Reusable workflows for packaging and Poetry-CI matrix-org/backend-meta#1

Merged

David Robertson added 3 commits February 8, 2022 16:26

Tell flake8 to relax when checking stubs

a0e4ca0

Use poetry for CI

f76921d

I've also moved the changelog checks into the same yaml file, though that doesn't really interact with poetry.

changelog

e9e1b7d

DMRobertson force-pushed the dmr/poetry branch from 1e7ff44 to e9e1b7d Compare February 8, 2022 16:28

DMRobertson changed the title ~~Dmr/poetry~~ Pin dependencies with poetry, and use this in CI Feb 8, 2022

DMRobertson marked this pull request as ready for review February 8, 2022 16:46

DMRobertson requested a review from a team as a code owner February 8, 2022 16:46

DMRobertson requested a review from reivilibre February 8, 2022 16:46

reivilibre requested changes Feb 9, 2022

View reviewed changes

David Robertson added 3 commits February 9, 2022 14:34

Use implicit directories in the linter script

65770b4

Poetry instructions

73faf97

Tweak name for slighter nicer GHA UI

47cd9be

DMRobertson requested a review from reivilibre February 9, 2022 14:40

reivilibre approved these changes Feb 9, 2022

View reviewed changes

Fix a missed rename

dfb5d4f

DMRobertson merged commit f42a651 into main Feb 9, 2022

DMRobertson deleted the dmr/poetry branch February 9, 2022 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pin dependencies with poetry, and use this in CI #488

Pin dependencies with poetry, and use this in CI #488

DMRobertson commented Jan 26, 2022 •

edited

Loading

reivilibre left a comment

reivilibre Feb 3, 2022

DMRobertson Feb 3, 2022

DMRobertson Feb 8, 2022

DMRobertson commented Feb 8, 2022

DMRobertson commented Feb 8, 2022

reivilibre commented Feb 9, 2022

reivilibre left a comment

reivilibre Feb 9, 2022

DMRobertson Feb 9, 2022

reivilibre Feb 9, 2022

DMRobertson Feb 9, 2022

reivilibre left a comment

reivilibre Feb 9, 2022

reivilibre Feb 9, 2022

DMRobertson Feb 9, 2022

reivilibre Feb 9, 2022

DMRobertson Feb 9, 2022

DMRobertson Feb 9, 2022

DMRobertson commented Feb 9, 2022

reivilibre commented Feb 9, 2022


		When you're done, you can close your terminal.

		### Optional: `direnv`

Pin dependencies with poetry, and use this in CI #488

Pin dependencies with poetry, and use this in CI #488

Conversation

DMRobertson commented Jan 26, 2022 • edited Loading

reivilibre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMRobertson commented Feb 8, 2022

DMRobertson commented Feb 8, 2022

reivilibre commented Feb 9, 2022

reivilibre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMRobertson commented Feb 9, 2022

reivilibre commented Feb 9, 2022

DMRobertson commented Jan 26, 2022 •

edited

Loading