Skip to content

Deploy a single py2.py3 wheel per platform #137

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

Merged
merged 2 commits into from
May 14, 2021

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented May 9, 2021

Main goal:

  • Deploy a single py2.py3 wheel per platform: saves build time, space on PyPI & maybe bandwidth.
    • Update convert_to_generic_platform_wheel to convert to a py2.py3 wheel & update jobs not to have python 2.7 jobs
    • Add a test only python 2.7 job not to introduce regressions for python 2
    • Add a check that only 1 wheel per platform is built/uploaded
      • fix: Only rely on auditwheel to add manylinux* platform tags

Other goals:

  • Check the content of the list/* with twine check on every commit
    • This allowed to fix 2 issues:
      1. The long_description_content_type was missing in the metadata
      2. The deployment step on circle-ci would have failed on release

@henryiii
Copy link
Contributor

Ready for rebase. Actually, I'd plan on squash and merge anyway, so up to you if you want to rebase or just unmark as draft. The cp27 jobs need to be removed from the required builds (I can do that), and @jcfr this shouldn't affect your upload, right? It should be much simpler, really, with fewer files with longer names, but if there's any automation here, it might beed to be adjusted?

@mayeut
Copy link
Contributor Author

mayeut commented May 14, 2021

@henryiii, thanks for the heads up.
Now rebased but still marked as draft.

There's still the "tests for python 2 to ensure incompatibilities are not introduced" I'd rather have before this get merged and now the only reason why this is still marked as draft.

With auditwheel 4.0 now in the manylinux images, there's also something weird (2 wheels produced instead of 1) going on with linux aarch64/s390x/ppc64le, I'll probably open another PR add another commit for this.

@mayeut mayeut force-pushed the py2.py3-wheels branch 3 times, most recently from feded1d to 8cac369 Compare May 14, 2021 10:40
@mayeut
Copy link
Contributor Author

mayeut commented May 14, 2021

Amend previous comment:

Given where I'm going with all this, I will add another commit to this PR and hijack a bit the original purpose stated in the title.
I can split this in multiple PRs upon request & will in any case keep the individual commits.
This is just to avoid rebasing every PR each time one is merged (they would conflict for sure).

@mayeut mayeut marked this pull request as ready for review May 14, 2021 12:39
Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Looking through the release process, I don't think there's anything else that needs to be changed. I'll adjust the required checks.

@henryiii
Copy link
Contributor

I might resquash the commits, though, and aim for a rebase and merge. There are two categories of commits, one deploying a single wheel and testing Python 2.7, and another fixing and testing dist stuff (8cac369 and 0ad2b70 if I'm not mistaken)

@mayeut
Copy link
Contributor Author

mayeut commented May 14, 2021

I might resquash the commits, though, and aim for a rebase and merge. There are two categories of commits, one deploying a single wheel and testing Python 2.7, and another fixing and testing dist stuff (8cac369 and 0ad2b70 if I'm not mistaken)

You're exactly right. Those are the 2 categories (I initially thought there would be 3, but rewriting the summary let me understand it's 2).

Please do rewrite history as you see fit. If you want only 2 commits with these 2 categories, you can also let me know and I'd be happy to do it.

@jcfr
Copy link
Contributor

jcfr commented May 14, 2021

Thanks for working on this 🙏 💯

I will post few comments/questions

Copy link
Contributor

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Looks great ! This will not only reduce the number of packages uploaded but also speed up the CI 💯

Few nitpick/questions:

  • Since we already glob the platform, is required to introduce the dist-staging directory ? Adding a comment somewhere in the CI config would be nice.

@mayeut
Copy link
Contributor Author

mayeut commented May 14, 2021

Since we already glob the platform, is required to introduce the dist-staging directory ? Adding a comment somewhere in the CI config would be nice.

It's indeed unnecessary if we leave the rm $whl as it was on circle CI (and add it to Travis CI).

@jcfr
Copy link
Contributor

jcfr commented May 14, 2021

I might resquash the commits, though, and aim for a rebase and merge. There are two categories of commits, one deploying a single wheel and testing Python 2.7, and another fixing and testing dist stuff (8cac369 and 0ad2b70 if I'm not mistaken)

@henryiii Is it something you would like to help with ?

@henryiii
Copy link
Contributor

Is it something you would like to help with ?

Sure, I can do it now.

mayeut added 2 commits May 14, 2021 16:27
Add python 2.7 tests

Check exactly 1 final wheel per build exists in ./dist

Only rely on auditwheel to add manylinux* platform tags

Remove dist-staging
Add `long_description_content_type` to metadata

Fixes twine check warning
@henryiii henryiii enabled auto-merge (rebase) May 14, 2021 20:28
@henryiii henryiii merged commit 70d02bf into scikit-build:master May 14, 2021
@mayeut mayeut deleted the py2.py3-wheels branch May 15, 2021 11:05
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.

3 participants