Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

Use cibuildwheel to produce wheels #42

Closed
wants to merge 16 commits into from
Closed

Use cibuildwheel to produce wheels #42

wants to merge 16 commits into from

Conversation

madig
Copy link
Contributor

@madig madig commented May 26, 2018

Hi there!
i'd like to have wheels of psautohint available so I don't have to install a compiler elsewhere.

For this PR, I use https://github.com/joerick/cibuildwheel to build wheels for 2.7 and 3.6 for Linux, macOS and Windows 32 and 64 bits. The wheels build, but there is no test and deploy step still.

@anthrotype
Copy link
Member

Nice! You should be able to run the tests with CIBW_TEST_COMMAND and CIBW_TEST_REQUIRES

@anthrotype
Copy link
Member

I also used cibuildwheel for uharfbuzz, maybe you can find that useful https://github.com/trufont/uharfbuzz/blob/master/.travis.yml

@madig
Copy link
Contributor Author

madig commented May 26, 2018

Ugh. The test setup is a bit... special.

@madig
Copy link
Contributor Author

madig commented Jul 3, 2018

@khaledhosny @miguelsousa if you're working on this right now, maybe have a look at cibuildwheel :)

@madig
Copy link
Contributor Author

madig commented Jul 4, 2018

Oh boy, you might want to trim your test regime. 30+ Minutes for a full build is hard.

@anthrotype
Copy link
Member

why it takes so long?

.appveyor.yml Outdated
- pip install cibuildwheel==0.9.2 coverage codecov
- cibuildwheel --output-dir wheelhouse
- coverage combine tests
- codecov
Copy link
Member

Choose a reason for hiding this comment

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

@madig I think you can keep calling codecov via tox, as it was before tox -e codecov

.appveyor.yml Outdated
global:
CIBW_SKIP: cp33-* cp34-* cp35-*
CIBW_TEST_REQUIRES: pytest pytest-cov
CIBW_TEST_COMMAND: "cd {project}\\tests && pytest --cov"
Copy link
Member

Choose a reason for hiding this comment

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

maybe you could have tox as the only CIBW_TEST_REQUIRES, and again tox as the only CIBW_TEST_COMMAND. This way the CI and the local developer's test workflow is the same and we avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... will it pick up the built package?

Copy link
Member

Choose a reason for hiding this comment

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

you're right... tox normally will attempt to rebuild from source since it does a python setup.py sdist then installs from there. You want to call tox like we do in compreffor, where we build the wheel inside the multibuild docker (running on CentOS), and then we run the test suite with tox, but on a different docker Ubuntu Trusty image.

https://github.com/googlei18n/compreffor/blob/cde06e744bbd41fbd2f32997ae6c049ab0b3dfa8/config.sh#L33

Copy link
Member

Choose a reason for hiding this comment

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

so you need to find out where cibuildwheel saves the built wheel package, then the CIBW_TEST_COMMAND will be tox --installpkg $wheel

this way tox will not install from source but will install the given wheel inside its own venv, then proceed to run the tests

.coverage Outdated
@@ -0,0 +1 @@
!coverage.py: This is a private format, don't read it directly!{"arcs":{"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\__init__.py":[[-1,1],[1,-1]],"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\autohint.py":[[-31,31],[31,33],[33,34],[34,35],[35,36],[36,37],[37,38],[38,40],[40,42],[42,43],[43,44],[44,47],[47,49],[49,50],[50,51],[51,52],[52,55],[-55,55],[55,56],[56,-55],[55,83],[-83,83],[83,84],[84,-83],[83,87],[-87,87],[87,88],[88,-87],[87,91],[91,119],[119,137],[137,166],[166,186],[186,187],[187,188],[188,189],[189,190],[190,191],[191,192],[192,193],[193,194],[194,195],[195,196],[196,197],[197,198],[198,199],[199,200],[200,201],[201,202],[202,203],[203,204],[204,205],[205,206],[206,207],[207,208],[208,209],[209,210],[210,211],[211,212],[212,213],[213,214],[214,215],[215,216],[216,217],[217,218],[218,221],[221,222],[222,223],[223,224],[224,227],[227,244],[244,245],[245,248],[248,249],[249,250],[250,251],[251,254],[254,264],[264,275],[275,303],[303,355],[355,361],[361,371],[371,378],[378,-31]],"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\psautohint.py":[[-1,1],[1,3],[3,6],[6,14],[14,23],[23,35],[35,-1]],"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\otfFont.py":[[-8,8],[8,10],[10,12],[12,13],[13,14],[14,16],[16,17],[17,19],[19,21],[21,24],[24,29],[29,30],[30,33],[-33,33],[33,34],[34,-33],[33,37],[-37,37],[37,38],[38,-37],[37,41],[41,50],[-50,50],[50,57],[57,69],[69,76],[76,92],[92,110],[110,133],[133,139],[139,147],[147,154],[154,160],[160,166],[166,172],[172,178],[178,192],[192,196],[196,200],[200,-50],[50,204],[204,221],[-221,221],[221,224],[224,231],[231,-221],[221,284],[284,322],[322,323],[323,324],[324,325],[325,326],[326,327],[327,328],[328,329],[329,330],[330,331],[331,332],[332,333],[333,334],[334,335],[335,336],[336,340],[340,697],[697,698],[698,699],[699,702],[702,726],[726,773],[773,787],[787,1016],[-1016,1016],[1016,1017],[1017,1039],[1039,1043],[1043,1050],[1050,1054],[1054,1076],[1076,1090],[1090,1134],[1134,1137],[1137,1141],[1141,1146],[1146,1296],[1296,1300],[1300,-1016],[1016,1337],[1337,1366],[1366,1377],[1377,-8]],"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\fdTools.py":[[-16,16],[16,18],[18,20],[20,21],[21,25],[25,26],[26,27],[27,28],[28,29],[29,30],[30,32],[32,33],[33,34],[34,35],[35,36],[36,37],[37,38],[38,39],[39,40],[40,41],[41,44],[44,45],[45,46],[46,47],[47,48],[48,49],[49,50],[50,51],[51,52],[52,53],[53,54],[54,55],[55,56],[56,57],[57,61],[61,62],[62,63],[63,64],[64,65],[65,66],[66,67],[67,68],[68,69],[69,70],[70,74],[74,75],[75,76],[76,77],[77,78],[78,79],[79,80],[80,81],[81,82],[82,86],[86,87],[87,89],[89,91],[91,93],[93,94],[94,95],[95,96],[96,97],[97,101],[101,102],[102,105],[-105,105],[105,106],[106,-105],[105,109],[-109,109],[109,110],[110,116],[116,130],[130,223],[223,-109],[109,237],[237,409],[409,-16]],"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\ufoFont.py":[[-113,113],[113,115],[115,117],[117,118],[118,119],[119,120],[120,121],[121,123],[123,124],[124,128],[128,214],[214,317],[317,319],[319,320],[320,321],[321,322],[322,325],[325,331],[331,332],[332,333],[333,334],[334,335],[335,336],[336,337],[337,338],[338,340],[340,341],[341,342],[342,343],[343,344],[344,345],[345,346],[346,348],[348,350],[350,351],[351,352],[352,353],[353,354],[354,355],[355,356],[356,357],[357,358],[358,359],[359,360],[360,361],[361,362],[362,363],[363,364],[364,365],[365,366],[366,367],[367,368],[368,370],[370,371],[371,373],[373,374],[374,377],[-377,377],[377,378],[378,-377],[377,381],[-381,381],[381,382],[382,-381],[381,385],[-385,385],[385,386],[386,424],[424,433],[433,441],[441,445],[445,455],[455,462],[462,468],[468,491],[491,506],[506,511],[511,534],[534,556],[556,567],[567,588],[588,595],[595,608],[608,633],[633,701],[701,719],[719,756],[756,761],[761,807],[807,813],[813,843],[843,861],[861,980],[980,985],[985,1019],[1019,1029],[1029,1125],[1125,1133],[1133,1175],[1175,1180],[1180,1186],[1186,1196],[1196,-385],[385,1202],[1202,1215],[1215,1297],[-1297,1297],[1297,1298],[1298,1299],[1299,1301],[1301,1338],[1338,1361],[1361,-1297],[1297,1373],[1373,1593],[1593,1606],[-1606,1606],[1606,1609],[1609,1620],[1620,-1606],[1606,1649],[1649,1670],[1670,1692],[1692,1699],[1699,1722],[1722,1723],[1723,1724],[1724,1725],[1725,1726],[1726,1727],[1727,1728],[1728,1729],[1729,1730],[1730,1731],[1731,1732],[1732,1733],[1733,1734],[1734,1738],[1738,1748],[1748,2138],[2138,2160],[2160,2176],[2176,-113]],"C:\\UsersLocal\\nikolaus.waxweiler\\Development\\psautohint\\python\\psautohint\\__main__.py":[]}}
Copy link
Member

Choose a reason for hiding this comment

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

i think you didn't want to commit this .coverage file. Should be .gitignored

@miguelsousa
Copy link
Member

the tests aren't fast, but it seems that cibuildwheel is building too many wheels. Here's the list I collected from the log,

New WHEEL info tags: cp27-cp27m-manylinux1_x86_64
New WHEEL info tags: cp27-cp27mu-manylinux1_x86_64
New WHEEL info tags: cp36-cp36m-manylinux1_x86_64
New WHEEL info tags: cp37-cp37m-manylinux1_x86_64
New WHEEL info tags: cp27-cp27m-manylinux1_i686
New WHEEL info tags: cp27-cp27mu-manylinux1_i686
New WHEEL info tags: cp36-cp36m-manylinux1_i686
New WHEEL info tags: cp37-cp37m-manylinux1_i686

also remove accidentally added .coverage
@madig
Copy link
Contributor Author

madig commented Jul 4, 2018

I suppose it's thorough. Which ones for 2.7 do we not need?

@anthrotype
Copy link
Member

No, I don't think it's not too many. Those are the right ones.

@miguelsousa
Copy link
Member

If you say so. Are those installable on Mac? Where are the ones for Windows?

@anthrotype
Copy link
Member

the windows ones are made by Appveyor. The manylinux1 wheels, well... are for Linux

@anthrotype
Copy link
Member

cibuildwheel also takes care of building wheels for macos for various pythons

@madig
Copy link
Contributor Author

madig commented Jul 4, 2018

@miguelsousa see e.g. https://ci.appveyor.com/project/adobe-type-tools/psautohint/build/1.0.215/artifacts

@madig
Copy link
Contributor Author

madig commented Jul 4, 2018

tox is more complicated... will look at it tomorrow.

@anthrotype
Copy link
Member

there's no rush

btw, appveyor say they'll ship python3.7 soon, good to know.
appveyor/ci#2475

@anthrotype
Copy link
Member

anthrotype commented Jul 4, 2018

I guess.. we could ditch the i686 (32-bit only) linux wheels... It's 2018, so it's unlikely anybody is running a 32-bit linux.. at least to build fonts (if they do, I'd say they could compile from source).

@miguelsousa
Copy link
Member

the wheel building should only be triggered on tags, right?

@anthrotype
Copy link
Member

no, at every commit.
The upload only on tags.
Building wheels is not different from building from source. You have to compile it first in order to be able to run the tests.

.travis.yml Outdated
@@ -21,12 +21,13 @@ script:
- cibuildwheel --output-dir wheelhouse
- $PIP install -r requirements.txt
- $PIP install -r dev-requirements.txt
- tox -e codecov
- wheel=`ls ${wheelhouse}/psautohint*.whl | head -n 1`
- tox --installpkg $wheel -e codecov --workdir {project} -c {project}
Copy link
Member

Choose a reason for hiding this comment

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

we don't want to run tox ourselves again, after cibuildwheel. We want the CIBW_TEST_COMMAND to call tox with the --installpkg option...
cibuildwheel builds the wheel, then installs it and runs the given test command. We want tox to take care of configuring the test environment, but we want to make it use a pre-build package (the wheel that we just built with cibuildwheel), instead of rebuilding the module from a sdist (which is its default behavior).
I need to go now, I'll have a look again tomorrow.

@madig
Copy link
Contributor Author

madig commented Jul 4, 2018

Until the test suite is trimmed to not run for 3 minutes, one could make multiple jobs for for every Python version. That might speed things up a bit.

@anthrotype
Copy link
Member

Maybe suggesting to combine tox with cibuildwheel was not a good idea.
They are kind of competing with each other for the control of the CI process.
I suggest you revert this PR to the commit before tox was added, or ditch cibuildwheel and use the alternative, more configurable toolchain, multibuild, that we use for compreffor, brotli-wheels, pyclipper, and others.
I can take over this PR if you wish.

.travis.yml Outdated
@@ -4,7 +4,7 @@ env:
global:
- CIBW_SKIP="cp34-* cp35-*"
- CIBW_TEST_REQUIRES="tox"
- CIBW_TEST_COMMAND="tox --installpkg /tmp/built_wheel/*.whl -e codecov --workdir {project} -c {project}"
- CIBW_TEST_COMMAND="tox --installpkg wheelhouse/*.whl -e codecov --workdir {project} -c {project}"
Copy link
Member

Choose a reason for hiding this comment

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

You don’t want to only run -e codecov tox environment, neither you want to run all the envlist.. but the specific python version that is currently active. I think -e py (or -e py-cov) is the shortcut for the generic python executable. I’m typing from my phone..

.travis.yml Outdated
global:
- CIBW_SKIP="cp34-* cp35-*"
- CIBW_TEST_REQUIRES="tox"
- CIBW_TEST_COMMAND="tox --installpkg wheelhouse/*.whl --workdir {project} -c {project}"
Copy link
Member

Choose a reason for hiding this comment

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

The —installpkg option needs to get the full path to the single wheel. You need to evaluate the glob pattern in a subshell.. it’s getting too complicated. Probably not worth it.

Copy link
Member

Choose a reason for hiding this comment

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

going to try to use /tmp/delocated_wheel/*.whl

@miguelsousa
Copy link
Member

going to leave this alone now. The PR is all yours

@khaledhosny
Copy link
Collaborator

Personally I’m more than happy to only build Linux wheels for python 3.6+, anyone using older versions should either use distro packaged psautohint if available or build from source.

@anthrotype
Copy link
Member

going to work on an alternative PR that uses https://github.com/matthew-brett/multibuild/ to compile the wheels. I'll also try to use pytest-xdist plugin to run the test suite itself in parallel (not just running parallel CI pipeline, but within each, using the multi-core features to speed up running the test suite).

@madig madig closed this Jul 5, 2018
@madig madig deleted the try-cibuildwheel branch July 5, 2018 16:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants