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

CI: Add wheel build for macOS #88

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

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 2, 2025

Adds default wheel build: cp313-cp313-macosx_10_13_universal2.whl.
Using build matrix on that platform can be done later if needed.

❯ unzip -l dist.zip
Archive:  dist.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
    52584  01-02-2025 00:55   fuse_python-1.0.9-cp313-cp313-macosx_10_13_universal2.whl
    35120  01-02-2025 00:55   fuse_python-1.0.9.tar.gz
---------                     -------
    87704                     2 files

Artifact download URL: https://github.com/libfuse/python-fuse/actions/runs/12575994383/artifacts/2377153480

Refs

@glensc
Copy link
Contributor Author

glensc commented Jan 5, 2025

@sdelafond any comments on this PR? Ignore the workflow on changes and disabling things, will remove them before making PR ready.

@sdelafond
Copy link
Collaborator

@sdelafond any comments on this PR? Ignore the workflow on changes and disabling things, will remove them before making PR ready.

I typically don't review "draft" PRs, only finalized ones. I had a quick look at it just now since you prodded me, and the ${{ matrix.os }} direction looks globally correct, but I'll probably want to see the whole picture without the linux builds disabled among other things.

@glensc
Copy link
Contributor Author

glensc commented Jan 7, 2025

Ok, since no comments about the contents of the changes, or way of doing things, rebased and squashed commits.

@glensc glensc marked this pull request as ready for review January 7, 2025 17:00
@sdelafond
Copy link
Collaborator

Ok, since no comments about the contents of the changes, or way of doing things

I said earlier "the ${{ matrix.os }} direction looks globally correct, but I'll probably want to see the whole picture without the linux builds disabled among other things".

rebased and squashed commits.

Now that you've cleaned up and finalized your approach, the resulting wheels.yml looks good enough. Two comments:

  • the direct calls to pip|python|brew should ideally be integrated into the Makefile rather than spelt out directly in wheels.yml
  • since most of the wheel-related steps are now gated by an if:$platform, I feel we'd be better off having two cleanly separated build-(linux|macos) jobs even it it means duplicating the actions/upload-artifact steps (or factorizing it somehow)

That can all be improved later.

So with this we have wheels built for MacOS, but not corresponding CI checks in python.yml, leaving the door open for a global CI failure at release time... which of course we can't have :) If you're planning to solve that part yourself, please let me know; if not, I'll put it on my todo list. Once that part completed, we can integrate and merge the whole thing.

@glensc
Copy link
Contributor Author

glensc commented Jan 8, 2025

I agree it's a bit nasty having platform ifs around each step, but wanted to get it out of the door. I'd do the refactoring in a separate PR. I was thinking maybe could add extra step and add if $platform for whole step, needs testing what's possible.

i did it this way right now to have all wheels collected to single artifact download, so it could upload to github release, but since you never wanted the artifacts upload to release, could drop the wheels collecting job, wheels would be still uploaded to artifacts, but from multiple jobs (multiple artifacts).

I'll be checking how to add matrix to regular ci (python.yml), no promises when. but if you beat me to it, ping it here when starting working on it.

@sdelafond
Copy link
Collaborator

I agree it's a bit nasty having platform ifs around each step, but wanted to get it out of the door. I'd do the refactoring in a separate PR.

I understand.

i did it this way right now to have all wheels collected to single artifact download, so it could upload to github release, but since you never wanted the artifacts upload to release

You mean upload to pypi?

could drop the wheels collecting job, wheels would be still uploaded to artifacts, but from multiple jobs (multiple artifacts).

I don't expect it would look different when a new release is created from a tag on GH, but this will be worth testing.

I'll be checking how to add matrix to regular ci (python.yml), no promises when. but if you beat me to it, ping it here when starting working on it.

Will do.

@glensc
Copy link
Contributor Author

glensc commented Jan 15, 2025

You mean upload to pypi?

you said you didn't want github action upload to pypi. so I thought attaching the wheels to github release, so you could download and upload manually. but if you don't use that either, that would simplify some things. i.e drop the wheels collecting under single artitact.

@sdelafond
Copy link
Collaborator

You mean upload to pypi?

you said you didn't want github action upload to pypi. so I thought attaching the wheels to github release, so you could download and upload manually. but if you don't use that either, that would simplify some things. i.e drop the wheels collecting under single artitact.

I don't want to upload to pypi from GH, but I download, check and use the wheels built on GH to create the pypi release.

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