-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Added automatic release to pypi on tag. #66
Conversation
@saimn any ideas why it is not running the python tests? |
@cmccully - It must configured on cibuildwheel: https://cibuildwheel.readthedocs.io/en/stable/options/#test-command |
It's can't just be its own separate GitHub workflow file?
…On Fri, Oct 29, 2021 at 5:14 PM Simon Conseil ***@***.***> wrote:
@cmccully <https://github.com/cmccully> - It must configured on
cibuildwheel:
https://cibuildwheel.readthedocs.io/en/stable/options/#test-command
An example here:
https://github.com/musevlt/mpdaf-wheels/blob/master/.github/workflows/build.yml#L17-L18
Since you already used pyproject.toml to configure cibuildwheel the
config should go there.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#66 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABIVYHGVFCN35LERYVMCQIDUJMFFBANCNFSM5G3FO7LA>
.
|
Codecov Report
@@ Coverage Diff @@
## master #66 +/- ##
=======================================
Coverage 93.09% 93.09%
=======================================
Files 7 7
Lines 782 782
Branches 12 12
=======================================
Hits 728 728
Misses 54 54 Continue to review full report at Codecov.
|
@saimn Looks the unit tests workflow was disabled. I re-enabled it. I removed the cron scheduled tests because that appears to be why we got disabled. I'm not sure right now it the cron tests add enough value to keep them going. Otherwise the PR should be ready to go. |
Indeed, Github does that when there is no activity on the repo since some time, but I think once you reenable it it will not be disabled again. However the normal unit test workflow is not run with the wheels, which I think would be good to have. That's the point of cibuildwheel's CIBW_TEST_REQUIRES and CIBW_TEST_COMMAND, once the wheels are build it will install it in a virtualenv and run the tests. |
I feel like for this particular repo the cron tests are a little bit of overkill. We rely on very little outside packages. The packages we do test against (numpy) for example are tested at merge for specific versions. Given that the set of versions we run against is static (i.e. I don't think we would automatically start testing against numpy 1.25 if it was released tomorrow) I'm not sure what periodic runs of the tests buys us. |
pyproject.toml
Outdated
|
||
[tool.cibuildwheel] | ||
# Skip pypy on mac due to numpy/accelerate issues | ||
skip = "pp37-macosx_x86_64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could skip pypy for all platforms. I'm not sure how binary wheels are supported on pypy, and astropy doesn't build pypy wheels.
@cmccully - Tests are now running after each wheel creation though some of them are failing, sometimes because some wheels are missing for dependencies (scipy/astropy for py3.10), and also it seems is a real error on linux 32bits with |
I think I agree with that @saimn. Should we flag that as a known issue? (Though I'm a little surprised there is an issue here). |
I also skipped musllinux (raspberry I think) which is built by default by cibuildwhel since recently, so the jobs are now green and we have wheels for the most common platforms (except sadly cp10 on linux, where for some reason scipy won't install, but it's not specific to astroscrappy and we can fix that later hopefully).
|
It was happening on several i686 builds, so it seems real but 🤷♂️
|
It does seem real, but I have no idea where there would be a difference between 32 and 64 bits in the code. So are we ready to merge this? @saimn |
Yes, I think we are ! 🎉 |
No description provided.