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

Build wheels on github actions #224

Merged
merged 3 commits into from Sep 7, 2020
Merged

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Apr 1, 2020

Build wheel on github actions using cibuildwheel. Cibuildwheel maintainers will fix most of problems with build system during development.

This PR contains only build wheel. Then it need to be manually uploaded to pypi.org. It is possible to ad such step to this.

Fixes #70

EDIT.
There is no such tools like dealocate or audithweel for windows, so maybe someone can check windows wheel on his machine.

strategy:
fail-fast: false
matrix:
os: [ubuntu-18.04, windows-latest, macos-latest]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be CentOS 6 for manylinux2010 compatibility?

ref: https://www.python.org/dev/peps/pep-0571/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. ubuntu is only host for manylinux docker container (quay.io/pypa/manylinux2010_x86_64 and quay.io/pypa/manylinux2010_i686)

@nbren12
Copy link

nbren12 commented Jul 29, 2020

Hey. I'm just wondering the status of this PR. It seems like @Czaki addressed @jakirkham's question.

@jakirkham
Copy link
Member

This could use some reviews from others 🙂

cc @rabernat @sofroniewn (as we discussed this today) @jni (as this may be of interest)

@Czaki
Copy link
Contributor Author

Czaki commented Jul 29, 2020

I rebase onto current master

@rabernat
Copy link
Contributor

I am supportive of what this PR wants to achieve, but I have no experience building wheels, so I can't be of much help.

@nbren12
Copy link

nbren12 commented Jul 30, 2020

Overall, I think this looks fine, but I have no experience with this wheel building tool. That said, all the related build checks passed though, and I was able to download the artifact.
Screen Shot 2020-07-29 at 5 56 14 PM

What works:

  • I tested that linux wheel works via docker: docker run -ti -v (pwd):/workdir -w /workdir python:3.7 pip install numcodecs-0.6.5.dev12-cp37-cp37m-manylinux1_x86_64.whl

Some issues I noticed:

  • Consider adding some instructions about this process since it does require manual intervention.
  • The only mac OS wheel built numcodecs-0.6.5.dev12-cp37-cp37m-macosx_10_9_x86_64.whl doesn't work on my more modern version of OS 10.

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

As @rabernat pointed out last night during the community, not having numcodecs wheels leads to time-consuming builds. I don't have experience with cibuildwheel, but the action itself seems fine. I'd suggest that we get this merged, tag 0.6.4, gets wheels deployed and then iterate to 0.6.5 etc. as necessary.

My one heads up from our experience with manylinux is that some libraries (like openssl) are statically linked such that a re-release is necessary when upstream releases. This is likely less of an issue for Zarr but I wanted to at least raise the issue.

cc: @manics

@Czaki
Copy link
Contributor Author

Czaki commented Jul 30, 2020

The only mac OS wheel built numcodecs-0.6.5.dev12-cp37-cp37m-macosx_10_9_x86_64.whl doesn't work on my more modern version of OS 10.

@nbren12 Could you write more? I try this wheel on MacOS 10.14.16 and all test passed (Some xfail, but I'm not sure what is proper state). I also fix configuration and currently all test runs.

I use cibuildwheel in my projects (and I'm contributor to this tool). And nice thing is that It painless allow to deliver most recent wheels. For example your appveyor build still doe not provide wheel for python 3.8. cibuildwheel is ready for python 3.9 and when its reach stable ABI then next version of cibuildwheel enable 3.9 build and this wheels will be build by default.

@joshmoore Could you provide link to example wheel. For me it looks like wrong result of auditwheel libraries collection and may be need to report to auditwheel maintainers. It looks like you does not use delocate on your macos wheel build, so some dependecies may be missed, need to install separately.

@jakirkham I do not know why AppVeyor build fail.

@manics
Copy link

manics commented Jul 30, 2020

@Czaki the problem occurs when openssl is statically linked into a linux wheel https://github.com/ome/zeroc-ice-py-manylinux/releases/tag/0.3.0

It will usually load, but you occasionally run into several downstream problems such as ciphers or certificates not being found. In addition you also take on the responsibility of having to rebuild the wheel when openssl is updated. For this reason we only ever used these manylinux wheels for CI testing of our other packages, it's not used in production.

@Czaki
Copy link
Contributor Author

Czaki commented Jul 30, 2020

@manics So the problem is static linking ssl library. And it could create problem in many cases, ex. when ssl is upgraded in system.

@nbren12
Copy link

nbren12 commented Jul 30, 2020

@Czaki Forget I said it doesn't work. I was trying to pip install the py3.6 wheel from python 3.7 🤦 . It works perfectly when I pip install the correct wheel.

@Czaki
Copy link
Contributor Author

Czaki commented Jul 30, 2020

@jakirkham If I good check appveyor and OSX CI runs are to build wheel. So they could be removed?

@manics If You use MACOSX_DEPLOYMENT_TARGET=10.9 environment variable in build of your package you could build wheel against older version of macos system (In such situation there cannot be any dependency from brew etc)

@hammer
Copy link

hammer commented Aug 31, 2020

We've hit the lack of wheels for numcodecs in our downstream project https://github.com/pystatgen/sgkit/issues/134. I have no idea if this PR solves #70, but I just thought I'd register our interest!

@joshmoore
Copy link
Member

Can someone retry the appveyor builds? (2 of 3)

@Czaki
Copy link
Contributor Author

Czaki commented Aug 31, 2020

I do not know why this PR stuck. There are any questions which I should response?

From my point of view appveyor is obsolete because it is for building wheel which is covered by github actions from this PR, but @joshmoore I could force repush last commit.

@nbren12
Copy link

nbren12 commented Sep 1, 2020

I agree with @Czaki. It would be great to see this merged to avoid having to rebuild numcodecs repeatedly. This PR didn't edit the appveyor configs so it's not clear why that is failing.

@Czaki
Copy link
Contributor Author

Czaki commented Sep 1, 2020

I agree with @Czaki. It would be great to see this merged to avoid having to rebuild numcodecs repeatedly. This PR didn't edit the appveyor configs so it's not clear why that is failing.

As I look to log it fail because of network problem.

@joshmoore
Copy link
Member

joshmoore commented Sep 1, 2020

I don't have access to the appveyor build so I can't restart, but I can ignore that failure and merge. The only caveat I can think of is that once the wheels are present, users on a broken platform (if there is one) will need to use pip install --no-binary until a subsequent PR can fix the issue. Alternatively, we could delete the wheels and revert this PR.

My vote would be to move forward with this to start gathering feedback. Since this potentially breaks users space, I'd say this falls under [https://github.com/zarr-developers/governance/blob/master/GOVERNANCE.md#decision-making-process]("Changes to the APIs"). However, it's also been open for sometime so shall we say:

👉 Objections to merging this by the end of the week.

If there are no thumbs down or negative comments below by the end of business Friday (CEST), I'll merge (unless someone beats me to it). I'd appreciate some extra eyes watching for community issues over the weekend and next week.

Obviously, if someone has an addition that will improve the OS support, feel free to speak up.

P.S. I note that on my 10.15.6 mac with a fresh Python 3.7 conda, the wheel does install for me.

@Czaki
Copy link
Contributor Author

Czaki commented Sep 1, 2020

The only caveat I can think of is that once the wheels are present, users on a broken platform (for example newer OS X)

What type of error you expect? If I good understand macos do not have problem to work with code compiled against older os (backward compatibility). So why You expect some problems?

@joshmoore
Copy link
Member

Hi @Czaki. That was the only one issue I saw mentioned above, but testing it myself I didn't see anything. i.e. I don't know of any issues but this is asking if anyone else does. ;)

@Czaki
Copy link
Contributor Author

Czaki commented Sep 2, 2020

@joshmoore But this mentioned problem comes from try install python 3.6 wheel to python 3.7

@Czaki Forget I said it doesn't work. I was trying to pip install the py3.6 wheel from python 3.7 facepalm . It works perfectly when I pip install the correct wheel.

@joshmoore
Copy link
Member

After updating with the latest upstream commits, all checks are now green. Since weren't any objections following my post last week, now merging.

🎉 Thanks, @Czaki et al. Keep your 🤞 .

@joshmoore joshmoore merged commit aa46cfe into zarr-developers:master Sep 7, 2020
@joshmoore joshmoore mentioned this pull request Sep 7, 2020
9 tasks
@jakirkham
Copy link
Member

Do you know if it is possible to build wheels for PyPy? Is there infrastructure that we would be able to leverage for that?

@hammer
Copy link

hammer commented Sep 11, 2020

cibuildwheel can do PyPy too https://cibuildwheel.readthedocs.io/en/stable/#what-does-it-do

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.

Building wheels
7 participants