-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat(ci): test that python wheels can be built and installed #546
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #546 +/- ##
=======================================
Coverage 41.14% 41.14%
=======================================
Files 15 15
Lines 1038 1038
=======================================
Hits 427 427
Misses 611 611
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Some platforms fail `pip install` because they don't have maturin available on pypi and maturin is listed as a build dependency. As far as I understand it, configuring the build-system is only needed if we want to let users build the wheel from source when they run `pip install`. Since we provide pre-built wheels I think this isn't that useful.. A side effect of this change is that the maturin action will automatically use the latest version of maturin unless otherwise configured via one of its inputs.
It almost works: https://github.com/radusuciu/git-cliff/actions/runs/8207985679/ The platforms that fail, fail because of the same reason:
Relevant line:
I think this may be because maturin isn't available for all of the same platforms that are listed in the matrix here. In any case, I think we don't actually need maturin to be installed.. I'll need to experiment with this more later, but I think this is a good base. |
This reverts commit a50210d.
Yup, the workflow looks good. Let's see if we need maturin installed. Also, do you think it is a good idea to test it for all the targets or testing it for each platform (Linux/MacOS/Windows) is enough? |
I'm not sure, it does seem like testing each platform is overkill. Maybe we can check out what other rust projects that use maturin are doing? |
Yeah, I agree. Checking out other projects is definitely a good idea. |
I looked around in GitHub a bit and some projects build for each target and some projects just test the platform. I think we can have this check running for each platform as well. |
Whenever you see the line
It means that the wheel wasn't installable on the platform you're testing on, probably because you built for a different platform than the host interpreter running on GitHub actions. As a result, CI is trying to extract the source tarball and build it. You can prevent this fallback with To match the python to test on to the wheel, you might have to use something like https://github.com/uraimo/run-on-arch-action to run your tests. |
Thanks @davidhewitt for that insight! I think we can fix that up. |
Any chance that we get this done before the next release? 😊 @radusuciu |
Description
This adds a CI job that builds and installs python wheels for every platform that wheels are published to PyPI.
Motivation and Context
This should warn against issues such as #534.
How Has This Been Tested?
Please refer to GHA runs of this workflow in my fork: https://github.com/radusuciu/git-cliff/actions/workflows/ci.yml
Types of Changes
Checklist: