Skip to content
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

Add publish and release workflows #47

Merged
merged 35 commits into from
Nov 21, 2024
Merged

Add publish and release workflows #47

merged 35 commits into from
Nov 21, 2024

Conversation

flying-sheep
Copy link
Collaborator

@flying-sheep flying-sheep commented Nov 18, 2024

Fixes #46

Based on https://github.com/pydantic/pydantic-core/blob/6472887b3ad3e865e494b46efb66a71e87ceb1cf/.github/workflows/ci.yml#L399-L500

Release flow

Trigger a build

  1. Trigger build by adding Full Build cause CI to do a full build to a PR (as seen here), pushing a tag to a repo (any tag will do it), or pushing to main
  2. Wheel builds for all supported architectures happen
  3. Some checks happen on the collected wheels

Create an actual release on PyPI and GitHub

  1. push a tag matching semver (e.g. v1.2.3 or v1.2.4a1)
  2. as above
  3. as above
  4. the wheels are uploaded to new PyPI release
  5. a pretty spartan GitHub release is made

Alternatives:

Why not release-plz

The way release-plz works is as follows:

  • it expects to be triggered by a push event to the main branch
    • if the last pushed commit is a merged Release PR (see below), it executes the command release which creates a GitHub release and a crates.io release
    • if the last pushed commit is something else, it executes the command release-pr which creates or updates a Release PR that bumps the versions and appends to a changelog

unfortunately a GitHub-only mode (release-plz/release-plz#1144) is not yet implemented, so unless we want to publish to crates.io, we need to implement the release command ourselves. Might be easy or not.

TODOs

  • make sure the stub generator is built and check for the stubs in the wheels
  • add check if wheels are importable and so on
  • add check if tag matches version on release or change the flow
  • research if we should change some parameters, e.g. musllinux_1_{1->2} or manylinux_{2_8->1_7} or so
  • test the publish step (temporarily modify it to dry-run the PyPI publish step, then …)
    • create a semver tag to see if the GitHub release is created (we might we need to add some permissions or set GITHUB_TOKEN to make it work)
    • create a non-semver tab to see if the publish step is skipped
  • use PEP 440 instead of semver parsing

TODO build errors:

@flying-sheep flying-sheep marked this pull request as draft November 18, 2024 12:09
@flying-sheep flying-sheep added the Full Build cause CI to do a full build label Nov 18, 2024
@flying-sheep
Copy link
Collaborator Author

@LDeakin I’m not sure if installing OpenSSL is the right move. It might result in symlinking to a version that isn’t guaranteed to be there.

It’s unfortunate but I don’t think there’s a version of OpenSSL that can be readily installed in all Linux distributions, is there?

@LDeakin
Copy link
Collaborator

LDeakin commented Nov 18, 2024

Yea not sure, I just went with sfackler/rust-openssl#2036 (comment), which seemed popular

@LDeakin
Copy link
Collaborator

LDeakin commented Nov 18, 2024

I'm thinking of giving up on i686, also do we need non-musl x86 x86_64 as well?

warning: zstd-sys@2.0.13+zstd.1.5.6: Compiler family detection failed due to error: ToolNotFound: Failed to find tool. Is `i686-linux-gnu-gcc` installed?
@flying-sheep
Copy link
Collaborator Author

flying-sheep commented Nov 18, 2024

Yea not sure, I just went with sfackler/rust-openssl#2036 (comment), which seemed popular

Doesn‘t mean that it actually produces a usable result. I think there are two possible ways to do this:

  1. link against a stable ABI. If OpenSSL has that and we can check if we‘re linking against it (e.g. openssl.so.1 and not say openssl.1.3.6), we’re good
  2. statically link it. I’m pretty sure that’s what’s happening on the other platforms, that’s why they didn’t make problems. But it does have the disadvantage that we’re in charge of delivering security fixes – which amounts to frequent releases.

I’d much prefer 1., but I don’t know if it’s possible, i.e. if OpenSSL offers a stable ABI.

I'm thinking of giving up on i686, also do we need non-musl x86 x86_64 as well?

Ah yeah! I think I need to go over this and check if there are any other platforms we’re missing because I only realized halfway through that I copied the wrong include section

@LDeakin
Copy link
Collaborator

LDeakin commented Nov 18, 2024

OpenSSL issue fixed properly with d4e8bd7

I'll take my hands off this for a bit if you want to have a crack at the remaining platforms.

@flying-sheep
Copy link
Collaborator Author

flying-sheep commented Nov 18, 2024

I’m good, I’m enjoying my evening. I just saw you working on this and wanted to share the tidbits I knew but hadn’t written down yet. I enabled linux x86_64 rn though haha!

I copied it from here and missed the entries they used for PGO builds. We also need to check that the wheels that each job makes is correct, e.g. it’s a bit suspect to me that only that windows i686 build has python-architecture: x86.

This workflow also needs quite some more parts like making a GH release and so on, and tying other things together.

Finally, we should merge #38 first, I really think there shouldn’t be two different rust crates called “zarrs” in the same build, and we should make sure the stubs end up in the wheels.

Ignored files are excluded by maturin when packaging
Copy link
Collaborator

@LDeakin LDeakin left a comment

Choose a reason for hiding this comment

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

Sorry for all the CI spam tonight @flying-sheep.

The x86_64 wheels work on windows/linux 🚀.
Adding _internal.pyi to VC with 48ff131 was necessary to get it in the wheels.

Any other blocking TODOs?
@ilan-gold, is PyPi trusted publishing setup?

@flying-sheep
Copy link
Collaborator Author

The release PR job isn’t functional yet: I don’t think release-plz works without releasing to crates.

And it would try to release to PyPI for every tag, that needs to be fixed as well.

@ilan-gold
Copy link
Owner

ilan-gold commented Nov 18, 2024

@LDeakin no I needed the file name of the release-triggering yml first - I didn't want to just guess at it because I wasn't clear on the consequences of a "wrong guess." But once this merges, I can do that.

@flying-sheep flying-sheep marked this pull request as ready for review November 19, 2024 09:59
@flying-sheep
Copy link
Collaborator Author

flying-sheep commented Nov 19, 2024

@LDeakin adding the stub to VCS makes things harder in other ways: now pre-commit wants to format it, whereas build.rs regenerates the unformatted one.

And where do we want to run ruff from? pre-commit run --files=python/zarrs/_internal.pyi ruff-format? ruff format python/zarrs/_internal.pyi? Neither is in the PATH for my VS Code right now …

The only thing I can think of would be to

  • commit it
  • format it
  • check during the tests that regenerating and formatting it doesn’t create a diff

Copy link
Owner

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

What is up with the submodule?

.github/workflows/cd.yml Show resolved Hide resolved
@flying-sheep
Copy link
Collaborator Author

What is up with the submodule?

no clue, it always pops up out of nowhere!

@LDeakin
Copy link
Collaborator

LDeakin commented Nov 20, 2024

add check if tag matches version on release or change the flow

@flying-sheep, is 64cf651 roughly what you had in mind?

test the publish step (temporarily modify it to dry-run the PyPI publish step, then …)

Shall we try publish to test.pypi.org with 83fbb0c?

@flying-sheep
Copy link
Collaborator Author

OK let me specify what I want to do:

  1. identify if we have a version tag (v and then a PEP 440 compliant version)
  2. if no (i.e. no tag or a tag that’s no version), just build, but then exit (successfully) without publishing
  3. if yes, get the version from both tag and package metadata, then
    1. check if the tag version is properly normalized or fail (e.g. note how this gets changed: str(packaging.version.Version("1.2.0.alpha1")) == '1.2.0a1')
    2. check if the tag version matches the metadata version or fail
  4. publish

@flying-sheep flying-sheep merged commit 81817b4 into main Nov 21, 2024
17 checks passed
@flying-sheep flying-sheep deleted the pa/release-publish branch November 21, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Full Build cause CI to do a full build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish to PyPI
3 participants