-
Couldn't load subscription status.
- Fork 274
Publish to PyPI via GitHub CI #893
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
base: main
Are you sure you want to change the base?
Conversation
ff43509 to
9ac1d79
Compare
9ac1d79 to
a3400f1
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.
Thanks for the PR :)
|
Looks like there was an agreement to use Also, building and running |
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
From branch
Upload/download distributions between jobs using respective GitHub actions
@webknjaz Brett proposed using
Are you suggesting I add |
|
When the tag exists first, various parties would treat it as "release happened". But publishing to PyPI may fail and it would require some tag cleanup. Moreover, many release watchers will remember the old commit a being tagged / versioned. To solve this, I tend to treat successful upload to PyPI as the point of no return and only push the tag after that. |
|
As for |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
GitHub's documentation says it's one of the steps:
Perhaps
I figure that is as error-prone as entering it at the command line when running
Ahh, I treat tags the opposite way: to me they are just a named commit, but releases must be made from existing tags. I watch for uploads to PyPI, and I'm completely fine with deleting tags. I'm generally pretty wary of writing Git changes in CI. Honestly, I think it's too limiting to uphold the expectation of "various parties would treat it as "release happened"", as tags are not releases (releases are releases!).
Right, so you're saying I should remove it (at least for CI). I disagree, as it's guaranteed to run before attempting upload to PyPI in this proposal, but it's not necessarily guaranteed in PRs (who may edit/skip CI) or locally. I also think it's unnecessary to build the package on every push to every PR, but it may be inconsequential. To reduce likelihood of |
Yes, it's the default. This is basically for choosing a workflow version from a different branch. Normally, nobody uses it and keeps it as is. But the more important part is being able to define arbitrary inputs.
Sort of. You still can build some input validation into the workflow.
That's not entirely true. GH Releases can auto-create tags — they don't need to be pre-existing. And their drafts don't even need that. The problem here is that you control the repo, but you can't influence how external observers work with it. In general, force-pushing tags is a highly discouraged practice, and they are expected to be more or less immutable. Deleting them usually either means an emergency or a supply chain attack.
Sure, but I'd be highly suspicious when I see a force-pushed tag after doing a
That's what I'm saying — |
|
Failing RTD build due to snowballstem/snowball#229 |
I've made the distribution build and check part of the I don't have familiarity with using the |
Does that run in CI, though? This is my main ask.
I can look into it, yes. I was also thinking that setting the epoch timestamp for reproducibility would be good as well as producing provenance info via SLSA and GH-native attestations — I normally integrate those too. |
No, but it's a necessary step in the (documented) release process. I've added |
|
It's better to catch any problems before the time of release. This is why I always insist on having the metadata validation a part of the regular CI flow. |
That's why I prefer @pradyunsg @henryiii any opinions on doing this? |
|
I like having the release happen via making a GitHub release, sometimes I also have a workflow dispatch option, especially for compiled builds where I can't download it, then do it locally with a temporary token. You lose trusted publishing signing that way, so GitHub Release + workflow dispatch is what I like. I have not reviewed this PR yet, though, that's just a general statement. I'll be done with my 4-in-a-row conferences next week. |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
This reverts commit f250545.
| release: | ||
| types: [published] |
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 the last of my concerns. Having this inverse trigger can lead to weird sync issues.
For example, last week we were releasing pip-tools where this trigger is still used. And we ended up with a Git tag in repo and a GitHub Release existing for like 15-20 hours with no PyPI release, due to some approval challenges. Having a workflow_dispatch can prevent it when the sequence of actions performed is different.
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.
I assume you want the workflow to instead be:
- Push to
main - Publish to PyPI (via manually triggering publish GitHub workflow)
- Create and push Git tag
- Create GitHub release
How do you prevent accidental triggering of publish GitHub workflow? A solution here is to prevent upload except when the wheel's version is of the form "X.Y.Z".
What if there's a problem during tag or release creation, where you have a version on PyPI with no associated tag? A solution here is to yank, but in my opinion this is a much worse outcome than the problems you mentioned, as these can be pinned by downstream users.
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.
How do you prevent accidental triggering of publish GitHub workflow? A solution here is to prevent upload except when the wheel's version is of the form "X.Y.Z".
Having environment: pypi on the publish job allows configuring the GitHub Environment (called pypi) protection rules. I normally add "required reviewers" in there (just adding oneself works). This makes it so when GH reaches the job, it pauses and does not proceed until whoever's listed clicked Approve on GH UI.
I like having an input with the version field in workflow_dispatch: https://github.com/cherrypy/cheroot/blob/ac9b6e5/.github/workflows/ci-cd.yml#L19-L28. And this event is only used for publishing so I don't tend to add checks for it. Though, I do have sanity-checks for the dist names. But yes, a check like you mention could be helpful too, I suppose.
What if there's a problem during tag or release creation, where you have a version on PyPI with no associated tag? A solution here is to yank, but in my opinion this is a much worse outcome than the problems you mentioned, as these can be pinned by downstream users.
To solve this, I separate the GH Release creation into a separate job, so that:
- it's possible to restart that job and it wouldn't attempt re-uploading the same thing to PyPI (which would otherwise crash)
- it's possible to fix whatever problem was there (semi-)manually (this could be wrapped as a Nox command for local invocation as a fallback)
And so with this in mind, in my workflows this doesn't constitute a problem. I'm very intentional about that.
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.
As I said above:
I don't have familiarity with using the
workflow_dispatchGitHub workflows event, and don't feel comfortable switching to that workflow. If you want it, could you please make changes to this PR (or submit a new PR targeting this branch)?
I've never used workflow_dispatch for package release, so I'm not confident enough to write workflow definitions using it.
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.
@EpicWink I can help. Should I send you a patch?
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.
@webknjaz yes please
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.
I've encountered a significant issue with using workflow_dispatch to publish the workflow. In cryptography's publish workflow, the workflow_dispatch must be run from main, but the wheel can be built from any random branch/tag, as there's no hard linking from the ref the publish workflow is run on. In cryptography's case, the publish workflow uses a custom download-artifact action, which makes it harder to follow.
This break in the provenance chain, and I cannot rely on an artefact being generated by a GitHub workflow on a specific Git commit.
@webknjaz is there a way to better link the publish workflow to the build workflow?
Removes the package build and publish from the
releaseNox session, so now it only creates a tag and makes bump/changelog commits.Adds a package build Nox session called
release_build, which optionally checks out a tag (otherwise assumes the tag is already checked out) then builds the distributions.Adds a publish GitHub CI workflow which will be triggered on release, running
release_buildand uploading with the publish action.pypi) and add it topackagingon PyPI. Comments below suggest this environment should be configured to require approvals before running CI workflows.gitis available, but this should be verified.Did you want distribution build to be a separate CI job? That's easy enough to do, but some maintainers I've interacted with prefer the single job. It seems like theI've merged Split build out from release CI job EpicWink/packaging#1 which does this.lintCI workflow already builds and uploads the distributions.Resolves #273