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

Add manylinux to CI wheels build #406

Merged
merged 37 commits into from
May 9, 2020
Merged

Add manylinux to CI wheels build #406

merged 37 commits into from
May 9, 2020

Conversation

bwoodsend
Copy link
Collaborator

Amends PR #405 to include manylinux in the same workflow action.

The legwork is being done by a new script scripts/build-manylinux-wheels.sh which runs inside the docker and does all the building. The results are here. I tried gluing in the original python script you have before but I didn't get anywhere.

I threw in a 32bit manylinux docker image as well - more just out of curiosity to see if it works. It's up to you if you want to keep it.

Currently it does all versions of Python and both dockers (8 builds in total) all in one build so it's a bit slow. If this bothers you I'll split it up a bit.

We'll probably want to rename the files and actions. I've left that so you can choose the names.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks! These are mostly little nits.

.github/workflows/deploy-macos-windows.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-macos-windows.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-macos-windows.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-macos-windows.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-macos-windows.yml Outdated Show resolved Hide resolved
scripts/build-manylinux-wheels.sh Outdated Show resolved Hide resolved
scripts/build-manylinux-wheels.sh Outdated Show resolved Hide resolved
scripts/build-manylinux-wheels.sh Outdated Show resolved Hide resolved
.github/workflows/deploy-macos-windows.yml Outdated Show resolved Hide resolved
scripts/build-manylinux-wheels.sh Outdated Show resolved Hide resolved
bwoodsend and others added 12 commits May 8, 2020 14:05
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@bwoodsend
Copy link
Collaborator Author

bwoodsend commented May 8, 2020

Also try changing the permissions on the file in Git:

chmod 777 scripts/build-manylinux-wheels.sh
git add scripts/build-manylinux-wheels.sh
git commit -m "Update permissions"
git push

Unfortunately that didn't do it. https://github.com/bwoodsend/ultrajson/runs/656760101
It still came out with 644 when calling git ls-files --stage scripts/build-manylinux-wheels.sh. Had to use git update-index --chmod=+x scripts/build-manylinux-wheels.sh.

@bwoodsend
Copy link
Collaborator Author

There we go - think that's all the nits knitted out.

@bwoodsend
Copy link
Collaborator Author

Here's the latest build in case you can't already see it.

@hugovk
Copy link
Member

hugovk commented May 8, 2020

https://pypi.org/project/ujson/2.0.3/#files shows one *-manylinux1_x86_64.whl per Python version.

The artifacts at https://github.com/bwoodsend/ultrajson/runs/656897993?check_suite_focus=true have 3 wheels per Python version:

ujson-2.0.4.dev80-cp35-cp35m-linux_x86_64.manylinux1_i686.whl
ujson-2.0.4.dev80-cp35-cp35m-manylinux1_i686.whl
ujson-2.0.4.dev80-cp35-cp35m-manylinux1_x86_64.whl
ujson-2.0.4.dev80-cp36-cp36m-manylinux1_i686.linux_x86_64.whl
ujson-2.0.4.dev80-cp36-cp36m-manylinux1_i686.whl
ujson-2.0.4.dev80-cp36-cp36m-manylinux1_x86_64.whl
ujson-2.0.4.dev80-cp37-cp37m-manylinux1_i686.linux_x86_64.whl
ujson-2.0.4.dev80-cp37-cp37m-manylinux1_i686.whl
ujson-2.0.4.dev80-cp37-cp37m-manylinux1_x86_64.whl
ujson-2.0.4.dev80-cp38-cp38-linux_x86_64.manylinux1_i686.whl
ujson-2.0.4.dev80-cp38-cp38-manylinux1_i686.whl
ujson-2.0.4.dev80-cp38-cp38-manylinux1_x86_64.whl

Do you know why this is and what the difference is?

@bwoodsend
Copy link
Collaborator Author

OK so the *-manylinux1_x86_64.whl wheels are built in the 64bit docker image manylinux1_x86_64.
The *-manylinux1_i686.whl ones are from the 32bit manylinux1_i686 image.

The *linux_x86_64.manylinux1_i686.whl ones shouldn't be there. That's a bug. It looks like the old 64bit wheels left in the temp-wheels folder are being audited in the 32bit docker. The fix for this would be to clean the temp-wheels folder beforehand which I'll do. I do find strange though that auditwheel was happy to pass 64bit wheels as 32bit. Testing those on 32bit docker with the rather meaty command:

docker run  -e PLAT=manylinux1_i686  -v `pwd`:/io  quay.io/pypa/manylinux1_i686 /bin/bash -c "/opt/python/cp37-cp37m/bin/python -m pip install /io/dist/ujson-2.0.4.dev80-cp37-cp37m-linux_x86_64.manylinux1_i686.whl; /opt/python/cp37-cp37m/bin/python -m pip install pytest; cd /io/ && /opt/python/cp37-cp37m/bin/python -m pytest"

shows that they don't work. They need to go.

@bwoodsend
Copy link
Collaborator Author

for whl in /io/temp-wheels/*.whl; do
rm "$whl"
done
find /io/temp-wheels/ -type f -delete
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, rm -rf /io/temp-wheels before the mkdir. Either is fine.

@hugovk
Copy link
Member

hugovk commented May 9, 2020

Do you know why the macOS/3.8 job was cancelled here?

https://github.com/bwoodsend/ultrajson/runs/657661946?check_suite_focus=true


Let's add this at the top, it means if one job does fail for some reason, it won't auto-cancel the other jobs:

strategy:
fail-fast: false

@hugovk hugovk added changelog: Added For new features release Deploy and release labels May 9, 2020
@bwoodsend
Copy link
Collaborator Author

bwoodsend commented May 9, 2020

Do you know why the macOS/3.8 job was cancelled here?

No I don't. That's weird. I pressed the rerun button. And they all finished this time. Could I have just exceeded some CI minutes quota?

@hugovk
Copy link
Member

hugovk commented May 9, 2020

There should be plenty of quota for open source. Anyway, looking good now, thanks again!

@hugovk hugovk merged commit e56aa43 into ultrajson:master May 9, 2020
@hugovk
Copy link
Member

hugovk commented May 9, 2020

👍 And there they are at TestPyPI https://test.pypi.org/project/ujson/2.0.4.dev96/#files

@bwoodsend
Copy link
Collaborator Author

manylinux has moved on a bit since this. Now an AUDITWHEEL_PLAT environment variable is baked into each docker image and auditwheel checks for this variable automatically so the manual -e PLAT=manylinux1_x86_64 and --plat=$PLAT can go. Would you be interested in a clean up PR?

@hugovk
Copy link
Member

hugovk commented Feb 23, 2021

Sounds good, yes please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features release Deploy and release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants