Skip to content

Fix packaging checks and simplify CI #1302

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

janbrasna
Copy link

Due to a bug in twine<6.0.1 not reading the correct metadata using wildcards thus failing the tox -e packaging steps, I've moved the pin from 5.1.x to 6.1.x to include some additional compatibility with packaging dependencies.

This also bumps outdated actions/* and generally simplifies and speeds up the workflow.

Inlining the h2spec step also allows for getting rid of its install script and exposing the access token.

NB: To currently pass ruff/mypy checks, I had to fix runtime-cast-value (TC006) by quoting the type expressions in h2/stream occurrences of cast()

Logs: actions/runs/14669502174

- name: Test with tox
run: |
tox --parallel 0
run: mkdir bin && curl -sSL "https://github.com/summerwind/h2spec/releases/latest/download/h2spec_linux_amd64.tar.gz" | tar -xz --directory ./bin && ./bin/h2spec --version
Copy link
Member

Choose a reason for hiding this comment

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

Does this always work? The original intention of calling the GitHub API using a token was to get around rate-limiting.

Copy link
Author

Choose a reason for hiding this comment

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

Basically this is not using any API endpoint or raw file access, but the convenience public link for releases — that unless flagged by their security is generally unlimited. (They have a ~5k hits/hr limit however I think they don't limit access from their own infra to stuff like packages, containers or releases?) — I'll check the frequency but we've been using this in rust workflows (instead of waiting for cargo builds) for many years, can't remember it ever failing over a rate limit in GHA.

Copy link
Author

Choose a reason for hiding this comment

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

@Kriechi (Just to make it more traceable: the CI never had used the public release URL, it only ever used the API, unauthenticated: a.) GitHub@9df8f94, b.) Travis@0cb4090… so I don't believe the non-APIv3 way was ever used.)

@Kriechi
Copy link
Member

Kriechi commented Apr 27, 2025

Not sure about the ruff/mypy typing related changes in this PR. The last commit to the master branch tested cleanly. Could you please shed some light on why you proposed this part? If relevant, please split it out into a dedicated PR for discussion.

@janbrasna
Copy link
Author

@Kriechi Same here, but also packaging tests passed back then — and now they'd fail [e.g.], probably based on some downstream version bumps in dependencies introducing bugs. I'd much rather avoid the cast quoting myself, but the checks won't pass otherwise [e.g.]. Would you like me to add TC006 to ignored rules instead for now?

@janbrasna
Copy link
Author

(NB: On last master CI run Feb 27, "ruff>=0.8.0,<1" collected ruff==0.9.8, today it resolves to ruff==0.11.7 👀 … The twine version wouldn't change, however on Feb 27 freeze shows packaging==24.2, and today it runs with packaging==25.0 so that might be a metadata change that hit the preexisting twine bug, not exhibited in earlier runs.)

@janbrasna
Copy link
Author

So, another option here might be, instead of adding the rule to ignorelist, to constrain ruff a bit more, and find a version that's comfortable for now, e.g. "ruff>=0.8.0,<0.10.0" — however that means freezing the rules at that point in time few months back and not getting any new cool toys. Fine with either way, please advise on direction to tweak this further, happy to follow whatever sounds more comfortable for everyone…

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
@Kriechi
Copy link
Member

Kriechi commented Apr 27, 2025

I prefer going with a reasonably "latest" version of ruff et al. So let's keep the version pin as is.
Please split out the TC006-related changes into a separate PR.

Not sure I understand the twine changes here yet - are they related to the ruff dependency? If not, please also split them out - unless thats the only thing remaining in this PR here.

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

Successfully merging this pull request may close these issues.

2 participants