pypa / cibuildwheel

🎡 Build Python wheels for all the platforms with minimal configuration.
https://cibuildwheel.pypa.io
Other
1.83k stars 234 forks source link

Ideas for improving the first-run experience #468

Closed robbmcleod closed 2 years ago

robbmcleod commented 3 years ago

Using the minimal example (https://github.com/joerick/cibuildwheel/blob/master/examples/github-minimal.yml) for GitHub Actions the Ubuntu build fails as it appears to be grabbing the system Python 2.7 instead of the specified Python 3.7:

https://github.com/robbmcleod/cpufeature/runs/1461364117

I tried explicitly using python3 instead but that fails on Windows. An I assume pip is still pip2 rather than pip3.

I'm trying to learn how to use this tool so I can build the wheels for numexpr (which I maintain), as that's going to hold up the release of pandas among other things. So any guidance you can provide here would be greatly appreciated.

Czaki commented 3 years ago

You should use CIBW_BUILD to limit python versions for which wheels will be built.

From your log build_selector: BuildSelector('*' - '') means tat cibuildwheel will try build wheel for python 2.7, 3.5, 3.6, 3.7, 3.8 and 3.9

henryiii commented 3 years ago

It requires a recent Python to run - it builds all Python versions (2.7, 3.5-3.9). You need to set CIBW_BUILD or CIBW_SKIP to describe what your project supports.

henryiii commented 3 years ago

Any remaining questions? Can we close?

robbmcleod commented 3 years ago

It's up to you, regarding closing or not. Personally I would have found it helpful if there was a minimal explanation on what cibuildwheel does. The documentation goes straight from minimalist examples to an explanation of the build options. When I read it, I didn't have the order of operations in my head. So some flowchart would be helpful.

The other problem I ran into was the directories defined by {project} and {package} are not the install directory. Instead the package is installed somewhere in /tmp, which makes running pytest potentially an irritation when the package has a c-extension. Setting CIBW_TEST_COMMAND: pytest {package} as suggested by the documentation definitely did not work for me as there was name collision and the c-extensions were not found. I'm unclear on what the practical difference between {project} and {package} is, since they both seem to be source dists.

Czaki commented 3 years ago

Linux wheels are built inside docker images. So it cannot use the same path as in GitHub worker.

Setting CIBW_TEST_COMMAND: pytest {package} as suggested by the documentation definitely did not work for me as there was name collision and the c-extensions were not found. I'm unclear on what the practical difference between {project} and {package} is, since they both seem to be source dists.

It is a problem with your package structure which may cause multiple problems in the future (for example with tox). I suggest moving sources to src directory.

The simple workaround may use --pyargs argument of pytest

joerick commented 3 years ago

The documentation goes straight from minimalist examples to an explanation of the build options. When I read it, I didn't have the order of operations in my head. So some flowchart would be helpful.

This is a good idea, I'll have a think about it

joerick commented 3 years ago

I'm wondering about something like this-

Artboard Copy 2

Czaki commented 3 years ago

Maybe add some boxes with CIBW_BEFORE_BUILD, CIBW_BEFORE_ALL? and some method for mark test as optional?

YannickJadoul commented 3 years ago

This looks pretty cool to me! :-)

Curious: how's it made; how easy would it be to update when we change things? (Not a blocker, but something to consider.)

I agree with @Czaki that adding some indication (dashed lines and annotations at the bottom, for example?) for the customization points would be nice!

robbmcleod commented 3 years ago

Definitely would like the _BEFORE steps for build and test. These are very key steps that the samples do not cover. I would also like to recommend showing use of these environment variables in the samples, but perhaps comment out the lines?

henryiii commented 3 years ago

These are very key steps

@robbmcleod If you use pyproject.toml, the _BEFORE steps are often not needed; I think adding them too prominently is asking for bad behavior. For example, if you have this in your before steps:

pip install setuptools numpy

Then a) users building from the SDist don't get any benefit, and b) NumPy is not pinned, so the wheels will not be loadable from anything but the very latest version of NumPy (if they use the NumPy C api at all). While if you have a pyproject.toml like this:

[build-system]
requires = [
    "setuptools>=42",
    "wheel",
    "numpy==1.13.3; python_version<='3.6'",
    "numpy==1.14.5; python_version=='3.7'",
    "numpy==1.17.3; python_version=='3.8'",
    "numpy==1.19.4; python_version>='3.9'",
]

Now the build is completely reproducible, and works out-of-the-box even if someone can't get a wheel without magic pre-installs (which cannot be specified in a dependency file, like requirements.txt), and can be loaded from any version of NumPy after the minimum (1.13 in my example). (Taken from https://scikit-hep.org/developer/packaging#pep-517518-support-high-priority ). And as an added benefit, pip install . will now break if you are missing any files from your SDist, since it follows exactly the same procedure everywhere.

The before steps are only for non-python setup, which hopefully is kept to a minimum - you want the best possible chance that a build will happen if a wheel is not available, such as on Arch, ClearLinux, etc.

Czaki commented 3 years ago

@henryiii there are some packages that need to be installed earlier (like scikit-build), because the maintainer of a package which is used in setup.py does not provide a proper sdist file. Sometimes it is better to install a patched version of a package used during the test (from the repository). For example cmake for i686 python.

henryiii commented 3 years ago

That's what pyproject.toml is for. If you are not the maintainer of a package you are building for, then that might be another valid reason for BEFORE_, though.

I would really like to get cmake, ninja, and scikit-build available on all platforms cibuildwheel can run on. But the Universal2 support needs to be done first, both for us and for cmake's python package. CC @jcfr

henryiii commented 3 years ago

Ahh, or, yes, if you have a broken package in your dependencies that is missing a pyproject.toml; but then it is still broken, even with BEFORE, because you can't force the correct environment "before" inside your own pyproject.toml environment, so you are still broken if you need it to build. (Running is fine).

Czaki commented 3 years ago

I would really like to get cmake, ninja, and scikit-build available on all platforms cibuildwheel can run on. But the Universal2 support needs to be done first, both for us and for cmake's python package. CC @jcfr

manylinux images contain really outdated cmake in repositories.

henryiii commented 3 years ago

That's why you use pyproject.toml, you don't have to depend on what's in the manylinux image. You get to control this.

joerick commented 3 years ago

If you use pyproject.toml, the _BEFORE steps are often not needed; I think adding them too prominently is asking for bad behavior.

This is good advice, it would be great to cover this in the documentation, perhaps a note on the BEFORE_BUILD option?

joerick commented 3 years ago

Maybe add some boxes with CIBW_BEFORE_BUILD, CIBW_BEFORE_ALL? and some method for mark test as optional?

I'll have a think about how to include this. It might be hard to figure out where to draw the line on what to include, and also might require updating more often if we add too much detail. Maybe it could show just BEFORE_ALL, BEFORE_BUILD, and TEST_COMMAND?

joerick commented 3 years ago

Another idea on this - we could reverse the order that we build in. Currently we build from oldest to newest, so the first thing you see are errors on the oldest Python still in circulation. Why not build 3.9 first? It's more important that it works on 3.9 than it works on 2.7... and if a user sees all of 3.x succeed and 2.7 fail, they might just SKIP 2.7 and move on with their life :)

YannickJadoul commented 3 years ago

That feels wrong, but might actually not be a bad plan, yes :-) (Already dreading the "ah, everything works" feeling, the getting a 2.7 surprise, but... well yeah :-P )

henryiii commented 3 years ago

Reversing the order of the build seems fine to me, it's pretty arbitrary logically, and if the most common problem is forgetting 2.7, then maybe this is "nicer", though it would waste more CPU time, as you'd do most of the build before it falling on 2.7.

henryiii commented 3 years ago

After @joerick adds a diagram, I thin this can be closed?

joerick commented 3 years ago

I had another go at this diagram- any thoughts on this?

Artboard Copy 11

I'm thinking it could go in the setup page in docs, before we introduce the CI services. We often say to users 'try to get your package to build using pyproject.toml/setup.cfg/setup.py before invoking cibuildwheel' but we never really say that in the docs. So we could say stuff like 'make sure your build works locally using pip wheel .', and 'try to put build requirements in pyproject.toml'.

YannickJadoul commented 3 years ago

Nice work, @joerick! I think this will indeed help people a lot!

A few comments and random thoughts crossing my mind, seeing this:

So, in general, I really like the overview this gives of the whole project! The main thing I'm lacking is that my eyes don't seem to know where to look first, somehow.

Apart from all that, one important question: how was this made? Is this e.g. an SVG that could be changed with Inkscape, or something we could edit with another OS application? I don't think that's a major blocker to get this in (many more benefits to get this in!), but I'm just trying to think whether it would always be your duty to update this.

joerick commented 3 years ago

Thanks for the feedback @YannickJadoul ! Yeah, I like the left-to-right version better, too. It's more natural to have time moving in that direction. Also, this new one is maybe a little too much info? I'll give it another shot, I think.

YannickJadoul commented 3 years ago

Also, this new one is maybe a little too much info?

I'm not too sure about that one. Especially with a bit more contrast and maybe when it's clickable, I don't think this is per se too much, but up to you!

joerick commented 3 years ago

Ok, so here's what I'm thinking....

Option 1... the simple overview. Perhaps suitable for the README.

Artboard Copy 4

Option 2... the detailed clickable version. Only really possible in the docs, due to the interactivity.

Artboard Copy 13

Sorry about the multiple hover states shown above, but you get the gist - hovering/tapping an element shows the info, click/second tap will take you to the docs for that option. Probably built in HTML using CSS grid, which should be quite suited to this sort of thing.

One possibility that I'm also considering is that option 1 is enough, and that option 2 wouldn't be worth the work to code it up... what do you think @YannickJadoul ?