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 musllinux wheel support, Use 'official' llvm build for compilation #13228

Merged
merged 1 commit into from
Jul 31, 2022

Conversation

ben9923
Copy link
Contributor

@ben9923 ben9923 commented Jul 23, 2022

Description

This PR has a couple of goals:

  1. Adding musllinux mypy wheels, for more users to enjoy mypyc's performance improvements.
  2. Eliminating use of an old custom-built clang used in CI (incompatible with newer manylinux images?).
    Upgrades custom clang 6.0.1 to redhat-provided 7.0.1.
  3. Building manylinux2014 wheels instead of EOL manylinux2010. Those users will switch to non-compiled releases.

Note For Merging

As available llvm versions are tied to the OS release, cibuildwheel needs to be updated to v2.3+ (Which defaults to manylinux2014) to keep yum install working.
As for musllinux, cibuildwheel needs to be updated to v2.2+.
This PR should handle both by upgrading to the latest release, but needs to be merged at the same time to not break builds:
mypyc/mypy_mypyc-wheels#40

Related to mypyc/mypy_mypyc-wheels#34

Test Plan

Build & Tests

Invoked mypyc CI matrix for all supported platforms, it appears to fully pass :)
https://github.com/ben9923/mypy_mypyc-wheels/actions/runs/2722183433

Wheel Compatibility

To make sure clang upgrade did not break compatibility with manylinux2014 distros (by linking with too new glibc or something), I tested built wheels with some of them (compatible distros listed here).
All of the following seem to work well: CentOS 7, CentOS 8, Fedora 32, Photon 4.0, Ubuntu 20.04, Debian 11.
For musl, tested: Alpine 3.10, Alpine 3.12, Alpine 3.16.

musllinux Performance

musllinux wheels seem to have a similar performance boost to manylinux.
Invoked mypy on the fastapi repo with universal & platform-specific wheels, and it had a similar ~7.5s vs. ~2s time difference across those two platforms, on my local machine.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you for improving this!

If I could bait you into doing a little more work...
This script was moved to the mypy repo from https://github.com/mypyc/mypy_mypyc-wheels a while back and I think that was the wrong decision, since it's now much harder to validate changes (as you allude to here: mypyc/mypy_mypyc-wheels#40).

How would you feel about moving it back? This would then make reviewing this PR and handling the merges trivial :-)

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Well your fork run is pretty convincing, so I'll just do the double merge and everything should work. We can move the script back across repos at any point in time.
I'll merge this on Monday or Tuesday, in case Jukka wants to do a 0.972 (in which case I'd like to avoid build changes right before he does so)

@github-actions

This comment has been minimized.

@ben9923
Copy link
Contributor Author

ben9923 commented Jul 24, 2022

@hauntsaninja Thanks for reviewing :)

I have a different suggestion - Maybe wheels build workflow should move entirely to this project?
It makes a little more sense to me to have it here, as those wheels are the ones eventually uploaded to PyPI (right?).

Coupling the CI configuration with the project's repo should make it easier to review and adapt to changes.
Having cibuildwheel configuration as part of pyproject.toml (desired end state?) is a good example for this method's advantages.

Problem with that approach is that only 'regular' releases will have an actual GitHub release, but is it really that bad to get development wheels from workflow's build artifacts?

Anyway as you said, it's something to address later 😛

@ben9923 ben9923 force-pushed the ci/build-optimization branch from 0a9aad6 to 97e7ace Compare July 29, 2022 13:46
@ben9923
Copy link
Contributor Author

ben9923 commented Jul 29, 2022

@hauntsaninja Is it still going to be merged soon? 😉 (Edit: Saw the label now haha)

Rebased it to fix conflicts and ran black on it, though I must say some of those code style changes are questionable (to me anyway)... It looks weird.

This change for example in build_wheel.py. It makes code less readable IMO, why all those newlines are needed?

-    env['CIBW_BEFORE_TEST'] = """
+    env[
+        "CIBW_BEFORE_TEST"
+    ] = """
       (
       grep -v lxml {project}/mypy/test-requirements.txt > /tmp/test-requirements.txt
       && cp {project}/mypy/mypy-requirements.txt /tmp/mypy-requirements.txt
       && cp {project}/mypy/build-requirements.txt /tmp/build-requirements.txt
       && pip install -r /tmp/test-requirements.txt
       )
-    """.replace('\n', ' ')
+    """.replace(
+        "\n", " "
+    )

Note there are some new failures when invoking build workflow for Python 3.6, 3.10 and sdist jobs.
Python 3.6 should be removed from CI, code style hook & config should be excluded by MANIFEST, no idea what broke 3.10.

@ben9923 ben9923 force-pushed the ci/build-optimization branch from 97e7ace to 5810f05 Compare July 29, 2022 14:28
@hauntsaninja
Copy link
Collaborator

Thanks for rebasing.

Agreed that some of black's decisions here are questionable. But.... whatever 🤷

Thanks for mentioning. check-manifest should be fixed by #13288
Best guess for Python 3.10 is that there's a difference in error reporting between Python 3.10.0 and Python 3.10.5; I'll look into fixing it

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 30, 2022

Okay, hopefully:
#13289
and
mypyc/mypy_mypyc-wheels#42
gets wheel builds green again

@ben9923
Copy link
Contributor Author

ben9923 commented Jul 30, 2022

Seems like it does, thanks for the effort :)

@hauntsaninja hauntsaninja merged commit ecc653b into python:master Jul 31, 2022
@hauntsaninja
Copy link
Collaborator

Great, merged both your PRs, thank you for this!

hauntsaninja added a commit to hauntsaninja/mypy_mypyc-wheels that referenced this pull request Aug 21, 2022
As of python/mypy#13228 we no longer use the
a custom clang build
hauntsaninja added a commit to mypyc/mypy_mypyc-wheels that referenced this pull request Aug 21, 2022
As of python/mypy#13228 we no longer use the
a custom clang build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants