ENH: determine wheel tags by Python interpreter introspection & tests and CI improvements #202
Conversation
dnicolodi
force-pushed
the
tags
branch
21 times, most recently
from
5fbd7d0 to
1d81d62
Compare
|
The code turns out nice and simple. The most complex part is a workaround for some macOS SDK bug. It would be nice if more of this string fiddling would be implemented in the standard library, but it does not seem as fragile as I was fearing. @FFY00 It would be nice if I went ahead and replaced the use of The tests failures on macOS are due to the fact that a version of |
dnicolodi
force-pushed
the
tags
branch
6 times, most recently
from
2296a72 to
a1ed04c
Compare
|
@FFY00 I think this is now ready. After some serious debugging effort the CI should be happy now. Please take a look. I would appreciate if we could cut a release after merging this, unless there are other important things pending. I would like to go back to release wheels for Python 3.7, which is the thing that originally prompted this work. |
The extension modules filename suffixes do not contain enough information to correctly determine the wheel tags. Instead introspect the Python interpreter to derive the wheel tags. This is the same approach used by other PEP517 backends, most notably wheel. The wheel contents only to determine whether the wheel contains python ABI dependent modules or other platform dependent code. The packaging module is the reference wheel tags derivation implementation and it is used (or vendored) by most python packages dealing with wheels. However, the API provided by packaging is cumbersome to use for our purposes and, with the goal of merging this code into Meson in the future, it is good to avoid an additional dependency. Therefore, the tags derivation code is reimplemented. Tests are added to verify that the tags produced by meson-python agree with the ones produced by packaging to ensure that the two implementations will not diverge. Fixes mesonbuild#142, fixes mesonbuild#189, fixes mesonbuild#190.
Looking for the string 'any' in the wheel filenames triggers when the platform tag contains the string 'manylinux'. The ABI and platform tags are anyhow verified a few lines above.
Also skip the tests on Cygwin.
Remove (wrong!) cross platform setup for test run only on Linux.
GitPython does not bring any particularly useful abstraction, is one more dependency, and makes debugging failures in the test setup harder.
Cygwin was installed but the regular Python interpreter was actually used to run the test. Fix this. Also merge Cygwin tests setup definition with the one for other environments. The Python distributed by Cygwin is broken and requires some peculiar maneuvers to make it suitable to create virtual environments. Address that in the CI configuration.
There was a problem hiding this comment.
I think this looks good. Huge thanks for working on this @dnicolodi! 🎉
I'm happy to finish off #175 as soon as this goes if if you'd like that too - that's quite important for building on non-wheel-supported platforms. |
|
Great work, by the way! :) |
|
We have a couple things ready that are blocked by fixes on the PR, after we get this in we can merger all the blocked stuff and make a release. |
|
The system ninja work would be very nice to have indeed. |
|
This "passes" but the pytest -> test name change messes up the require checks. Personally, I prefer having a "pass" check that depends on all the other checks, and then only require that (one per workflow, unfortunately if you have multiple workflows, but still much simpler than maintaining one per job, and if you change the checks all the old merged PRs don't change to orange dots). |
|
Yeah. Sorry, I didn't realize that the rename would have caused this issue. However, also the readthedocs job is stuck, so this is not the only thing preventing this from being merged automatically. Now that all the CI jobs are fixed, can't we simply require that all jobs succeed? I'm not sure why the status of old PRs should change changing the definition of the checks for future PRs. It seems a GitHub bug to me. Edit: I don't have the rights to check this for the meson-python project, but on another project of mines I don't find the option to allow the merge only when all CI jobs pass anymore. I was sure there was something to this effect. All I find now is the possibility to manually select individual "checks". I would be an useful feature if it would at least accept a glob pattern or something, but having to manually select and click through the list really sucks. I'll have a look at how to define a "collect" job that depends on all the tests in the "tests.yml" workflow. This indeed should make the manual selection suck a bit less. Thanks for the hint @henryiii |
|
I am gonna merge as-is and temporarily disable the required checks in the branch protection rules. I really like @henryiii's proposal, so we could work on that and add the required checks again. |
The X got lost in the big refactoring in mesonbuild#202.
The X got lost in the big refactoring in #202.
The X got lost in the big refactoring in #202. Signed-off-by: Filipe Laíns <[email protected]>
Looking at the wheel tags generation code in
packagingand inwheelit looks like it is full of cruft to support old interpreter versions. I tried to see how much effort it takes to have the most minimal compliant implementation. This for now only tests that my implementation returns the same values as thepackaging-based one. Pushing it here just to have the tests run in CI.