Skip to content

fix(cicd): label build artifacts as dank-mids outputs#1048

Open
BobTheBuidler wants to merge 5 commits intomasterfrom
codex/update-build-mypyc-workflow-for-parallel-builds
Open

fix(cicd): label build artifacts as dank-mids outputs#1048
BobTheBuidler wants to merge 5 commits intomasterfrom
codex/update-build-mypyc-workflow-for-parallel-builds

Conversation

@BobTheBuidler
Copy link
Owner

Motivation

  • The build artifacts produced during the mypyc runs are outputs of the dank-mids library (which vendors aiolimiter), so artifact names and download patterns should reference dank-mids rather than the vendored package.
  • This clarifies ownership of compiled wheels and C/H sources and avoids confusion when publishing or diff-checking generated sources.

Description

  • Renamed artifact upload/download labels and patterns in .github/workflows/build.yaml from aiolimiter-* to dank-mids-* and updated the Linux C/H artifact name to dank-mids-linux-c-headers.
  • Kept artifact paths pointing at the vendored location dank_mids/_vendor/aiolimiter/src/ so the build still collects the vendored extension outputs.
  • No changes were made to the mypyc matrix strategy or gating logic for Linux C/H sources beyond relabeling artifacts.

Testing

  • Ran PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest, which errored during collection due to a missing external executable (ganache-cli), so compiled-extension verification could not be completed.

Codex Task

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 29adb2932c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 30 to 33
include:
- os: ubuntu-latest
artifact_suffix: linux
artifact_glob: "**/*.so"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Specify python-version in matrix include entries

The matrix.include entries only set os plus the artifact fields, but they omit python-version. In GitHub Actions, include data is only merged into a matrix job when it matches an existing combination, so these entries won’t apply to the 15 os+python-version jobs. That leaves matrix.artifact_suffix/matrix.artifact_glob empty in the upload step, producing artifact names like dank-mids--artifacts-3.10 and paths without the glob; the downstream download patterns (dank-mids-macos-artifacts-*, etc.) won’t match those names, so the finalize job will miss compiled binaries. Add python-version to each include row (or use a mapping expression) so every matrix combination gets the suffix/glob.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant