-
-
Notifications
You must be signed in to change notification settings - Fork 636
📦 Build release artifacts in CI workflow #2274
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?
📦 Build release artifacts in CI workflow #2274
Conversation
01d97da to
8eeee7d
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.
Pull Request Overview
This PR integrates tox-dev/workflow's reusable workflow into the CI/CD pipeline to build release artifacts using tox. The build process is migrated from inline GitHub Actions steps to tox environments for better local reproducibility.
- Adds three new tox environments:
cleanup-dists,build-dists, andmetadata-validation - Introduces a
pre-setupjob that computes build variables and artifact names - Refactors the release workflow to call the CI workflow as a reusable workflow
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Adds three new tox environments for cleaning, building, and validating distribution packages |
| .github/workflows/release.yml | Refactored to use the CI workflow as a reusable workflow instead of inline build steps |
| .github/workflows/ci.yml | Added pre-setup job for computing build settings and build job using reusable tox workflow |
| .github/reusables/tox-dev/workflow/reusable-tox/hooks/post-tox-run/action.yml | Hook for verifying and uploading build artifacts after tox runs |
| .github/actions/cache-keys/action.yml | Custom action for calculating dependency file hashes for cache keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ca4e01e to
e47e464
Compare
e47e464 to
0152317
Compare
|
Going forward requires @jezdez to add the respective reusable components to allowlist: jazzband/help#413. |
0152317 to
e257367
Compare
sirosen
left a comment
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.
Fundamentally I'm onboard with this approach, but it's going to take me some extra time to review. I think that's okay, since we're also blocked on an org setting change.
I need to get more familiar with the reusable-tox workflow, and I've only just started (re)reading that work, which I haven't looked at for a couple of months.
I expect to be asking questions here which relate back to that project. Maybe I can turn those questions around into doc contributions upstream. 😄
|
@sirosen yes, the idea is to route generic contributions upstream + maybe experiment with more ideas too. Currently, it must be pinned to a commit hash for this reason alone. I was hoping to get you more involved at some point. I've been using it in probably like 5 projects for a while now and have some ideas of what to change already. |
e257367 to
b5b5d46
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
b5b5d46 to
c2d9e83
Compare
216ae58 to
be7b159
Compare
be7b159 to
8e66b93
Compare
This is a narrowly scoped initial integration of `reusable-tox.yml` into the project's CI/CD infra. It makes use of the following external action and reusable workflow repositories, some are optional: * `codecov/codecov-action` (transitively, via `tox-dev/workflow`) * `irongut/CodeCoverageSummary` (transitively, via `tox-dev/workflow`) * `re-actors/cache-python-deps` « (directly; and also transitively, via `tox-dev/workflow`) * `re-actors/checkout-python-sdist` « (transitively, via `tox-dev/workflow`) * `test-summary/action` (transitively, via `tox-dev/workflow`) * `tox-dev/workflow` « (directly) The marked projects are under my control. This patch plugs the main CI workflow as reusable into the release one. The CI workflow gets two new jobs — pre-setup that computes a number of variables, and build that makes the artifacts and stores them for later consumption by the PyPI upload job. The build job also double-checks that the artifacts are created with expected names. The invocation of `pypa/build` moved to tox so that it's possible to run the same process locally. In follow-up PRs, these dists will also be used in testing.
8e66b93 to
0d7c084
Compare
|
@sirosen this now reflects what I've got in other projects in terms of the dist build job. We'll have to watch out for changes during release since they aren't really testable with tag-as-a-trigger that's currently set up. Obviously, more follow-up changes to come but I wanted something relatively small and digestable first. And iterate later, in small portions. |
sirosen
left a comment
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 reviewed the various pieces here, but I need a bit more time with it before I can approve. I might have another comment or two, but I'm most of the way there on this! (I know it's taken a while.)
| --- | ||
|
|
||
| name: placeholder | ||
| description: placeholder |
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.
Do these appear somewhere in the output? I've never used the composite actions feature, so I'm not super familiar with it. I'd think these need updating, but maybe it doesn't matter?
| f'files-hash-key={files_derived_hash}', | ||
| file=outputs_file, | ||
| ) | ||
| shell: python |
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.
So far, this embedded script, and the other ones like it in ci.yml, are the thing which I find most surprising in this PR.
If I'm reading correctly, the near-equivalent bash would be this?
hash='${{
hashFiles(
'tox.ini',
'pyproject.toml',
'.pre-commit-config.yaml',
'pytest.ini',
'dependencies/**',
'dependencies/*/**',
'setup.cfg'
)
}}'
echo "computed hash is $hash"
echo "files-hash-key=$hash" >> "${GITHUB_OUTPUT}"I have no objection to using Python over bash as the shell for a step, but I was reading it with the expectation that I'd see some Python-specific feature here. It is more verbose in this case and I'm not sure I understand why you made this choice -- am I missing something important that I should note?
If the explanation is merely a matter of preference, then let's mark this resolved, that works for me. But I didn't want to overlook some essential detail here.
| || 30 | ||
| }} | ||
| ... |
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 haven't fully worked this out yet, but I think we should look for a way to make the hook mechanism easier to follow. The "double delegation" pattern (we delegate to reusable-tox, which delegates back to this action) results in some difficult to understand logic here in order to extract values.
i.e. We need fromJSON(inputs.job-dependencies-context).pre-setup.outputs.release-requested, which is a pretty long expression, in order to get an input which we had in-hand in a workflow in this repo (when we did the pre-setup step).
I think I would prefer if it were the responsibility of the ci.yml workflow to specify data to pass to the hooks, and then thread that down into this job. I haven't explored if this is possible, but something like...
# in ci.yml
build:
name: >-
📦 Build dists
needs:
- pre-setup
uses: tox-dev/workflow/.github/workflows/reusable-tox.yml@...
with:
# is it possible to make this something we pass in? (I'm guessing not, but it's an idea)
post-tox-action: .github/workflows/reusables/reusable-tox/post-tox/action.yml
# this is the important part: passing a blob of action inputs
post-tox-action-call-args:
release-requested: ${{ jobs.pre-setup.outputs.release-requested }}
...
# in action.yml , using that data
retention-days: >-
${{
fromJSON(
fromJSON(inputs.call-args).release-requested
)
&& 90
|| 30
}}It's not more powerful than having full access to the needs job context data, but I think the explicit arg-passing style is much easier to follow. I'm not sure how much of this is possible within the constraints of GitHub Actions -- it's been a while since I tinkered with custom actions, but I'm aware that there are some limitations on the system which could make this difficult.
All of the above is more of a comment on reusable-tox than it is about the integration here. I've followed enough to understand that we're getting context data from the original ci.yml passed down into here and we can move forward with this. But it was difficult to follow and I think it may be hard for future contributors to read.
| with Path(environ['GITHUB_OUTPUT']).open( | ||
| mode=FILE_APPEND_MODE, | ||
| ) as outputs_file: | ||
| print('release-requested=true', file=outputs_file) |
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.
The two scripts above seem significantly longer than their bash equivalents. More than in the case of the hashing action, I wonder if these should be bash shell steps, and then the tomllib and setuptools_scm scripts below can set shell: python.
| print( | ||
| f'dist-version-for-filenames={ver.replace("+", "-")}', | ||
| file=outputs_file, | ||
| ) |
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.
Seeing this snippet makes me think of a tool I've been working on which might interest you:
https://mddj.readthedocs.io/en/latest/
The advantage over calling into setuptools_scm directly is that it's generic, so if you wanted a version of this script which you can port across projects, it might look like...
# /// script
# dependencies = ["mddj==0.4.2"]
# ///
from mddj.api import DJ
dj = DJ()
ver = dj.read.version()
... # rest of script followsAnd that will read project.version if it's static, or call into build if it's dynamic.
The project still needs a lot of work; it's just an alpha right now. But I find I frequently want to be able to script around project metadata, so it's an interesting space to try to make a good utility.
| ${{ toJSON(needs) }} | ||
| python-version: 3.12 | ||
| runner-vm-os: ubuntu-latest | ||
| timeout-minutes: 2 |
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.
2 minutes seems a bit tight, if there's any slowdown (e.g., network flakiness). Should we maybe loosen it a little, e.g. to 5 minutes?
| -m twine \ | ||
| check \ | ||
| --strict \ | ||
| dist{/}* |
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 reminds me, I'd like to incorporate check-sdist as well. We can do that in a follow-up.
| [python-cli-options] | ||
| byte-warnings = -b | ||
| byte-errors = -bb | ||
| max-isolation = -E -s -I |
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.
Current docs indicate that -I implies -E and -s. So I think we can cut this down to just -I?
| # some-isolation = -I | ||
| # FIXME: Python 2 shim. Is this equivalent to the above? | ||
| some-isolation = -E -s | ||
| warnings-to-errors = -Werror |
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 have somewhat mixed feelings about this block of config. On the one hand, I can see how it documents the short flags we use. And I didn't know you could have a custom section in tox.ini loaded like this, which is neat!
But I also find that it makes the usage sites harder to read in some sense, since you have to lookup this table to figure out what the command is. (adding a bit of indirection)
The comparison is between
{envpython} \
-I -bb -Werror
and
{envpython} \
{[python-cli-options]byte-errors} \
{[python-cli-options]max-isolation} \
{[python-cli-options]warnings-to-errors}
I'm okay with it, at least to try out, but I'm just noting that I'm not fully convinced as of yet.
This is a narrowly scoped initial integration of
reusable-tox.ymlinto the project's CI/CD infra. It makes use of the following external action and reusable workflow repositories, some are optional:codecov/codecov-action(transitively, viatox-dev/workflow)irongut/CodeCoverageSummary(transitively, viatox-dev/workflow)re-actors/cache-python-deps« (directly; and also transitively, viatox-dev/workflow)re-actors/checkout-python-sdist« (transitively, viatox-dev/workflow)test-summary/action(transitively, viatox-dev/workflow)tox-dev/workflow« (directly)The marked projects are under my control.
This patch plugs the main CI workflow as reusable into the release one. The CI workflow gets two new jobs — pre-setup that computes a number of variables, and build that makes the artifacts and stores them for later consumption by the PyPI upload job. The build job also double-checks that the artifacts are created with expected names.
The invocation of
pypa/buildmoved to tox so that it's possible to run the same process locally.In follow-up PRs, these dists will also be used in testing.
This partially solves #1134.
Contributor checklist
changelog.d/(seechangelog.d/README.mdfor instructions) or the PR text says "no changelog needed".Maintainer checklist
bot:chronographer:skiplabel.