-
Notifications
You must be signed in to change notification settings - Fork 0
Support specifying variants for wheel builds #1
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
Conversation
Accept a `variant-name` config setting that specifies variant metadata for the built wheel. When provided, the variants will be written into wheel metadata, and the variant hash will be appended to the filename. Example usage: python -m build -w -Cvariant-name=x86_64::baseline::v3 -Cvariant-name=blas::impl::mkl
Nice! For For
This looks a bit different from what I used in wheelnext/pep_xxx_wheel_variants#9. I'd use |
So, that would assume that projects using
This part I'm not sure about. We currently don't have a single consistent behavior across different PEP 517 backends, regarding their default flags. In particular, we have two possible scenarios:
If the plugin was implicitly setting flags, we'd effectively be shifting from 1. to 2.
I've used the exact values as end up in project metadata. Note that it needs to include a "provider" (i.e. plugin name), "key" and "value". I went for what seemed like a logical plugin split here: an "x86_64" plugin that includes different features related to x86_64 CPUs (or perhaps a more generic "x86" plugin would be better?), and a "blas" plugin specifically focused on BLAS implementations. |
Yes, correct.
It should definitely be "append" behavior rather than "overwrite" in my opinion. The latter is poor in general, and it gets progressively worse when one has more sources where flags come from (environment variables, CLI args, flags in config files, flags in build scripts). Meson has a sane hierarchy to these, and I think CMake does too? CMake is important here;
Okay, that's reasonable too - let's leave that as is here then. It's just syntax after all, we can decide on it later. |
I've added |
I'm not quite sure what you mean - what is the alternative here that's explicit passing that isn't "don't have any auto-cflags behavior at all"? |
Well, I don't like "auto-cflags" either. If I were to design this, I would make it independent — the frontend takes a list of variant tags, the backend takes specific configuration on how to perform the build. |
Okay, I think Eli and Jonathan both liked it though (e.g., wheelnext/pep_xxx_wheel_variants#9 (comment)). Also Without it, the spelling to build it is also very verbose, like |
I get your point and I don't disagree. However, that won't change the fact that I don't like it, and I have a gut feeling that we may be missing something significant. But well, one way to find out (that is, by implementing it and seeing how it works out). |
Fair enough! I agree there's potential that we are missing something, so let's keep it in mind as we go. |
Now with wheelnext/variantlib#12, we get labeled wheels, e.g.: $ python -m build -nw -Cvariant-name=blas::variant::mkl -Cvariant-name=x86_64::baseline::v3
[…]
Successfully built meson_python-0.18.0.dev0-py3-none-any-60f00cf2+mkl+x86_64_v3.whl
$ python -m build -nw -Cvariant-name=blas::variant::openblas
[…]
Successfully built meson_python-0.18.0.dev0-py3-none-any-b785a480+openblas.whl |
Here's the logic for appending labels: meson-python/mesonpy/__init__.py Lines 363 to 368 in 8c752d5
It tries to use all labels returned by the plugins, but if the filename turns out too long, it removes them one by one, until it is short enough. As a last resort, it outputs a filename with no labels. |
mesonpy/__init__.py
Outdated
for numlabels in range(len(labels), 0, -1): | ||
long_name = "+".join((name, *labels[:numlabels])) | ||
# if labels would give us filename that's longer than 128 | ||
# characters (124 + .whl), strip them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this take into account that we're running this code before auditwheel
is going to make it quite a bit longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it obviously can't predict what will happen once meson-python is done with it. I guess the only way to handle that is to adjust the limit — and I've put a totally arbitrary number here. I suppose we could even make it configurable via config_settings
, so people can adjust when their workflows alter the tags afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. We can always tweak the number later. My gut feel is that 128 is on the generous side, but it doesn't matter too much for now.
Great! The code looks pretty clean and the resulting filenames are nice. I haven't tested locally, but I think this is pretty close to what we want/need. |
Update for current variantlib API. Disable label code for the time being, as the feature has been blocked.
This is now |
Accept a
variant-name
config setting that specifies variant metadata for the built wheel. When provided, the variants will be written into wheel metadata, and the variant hash will be appended to the filename.Example usage: