pypa / cibuildwheel

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

Handle case where output_dir does not already exist on macos & windows #1851

Closed MusicalNinjaDad closed 3 months ago

MusicalNinjaDad commented 3 months ago

[Updated 2024-06-04]

In cases where the output_dir does not already exist, the attempt to move the tested wheel to the output_dir actually ends up creating a file named output_dir rather than outputdir/wheelname.whl

This leads to the build process failing on macos and windows if the directory does not exist, or is removed as part of a before-build clean.

This PR updates the handling of the final wheels to:

  1. explicitly resolve the output directory path
  2. explicitly create the output directory if it does not already exist
  3. then move the wheel file

It also cleans up the handling of errors by making use of built in pathlib.Path functionality rather than with contextlib.suppress

It has been tested in Github actions CI against a specific project & commit which provided a consistent, reproducible error described in #1850.

Fixes #1850

Original

Pathlib apparently raises a NotADirectoryError instead of a FileNotFoundError on macos. This causes builds to fail after testing if the output_dir does not already exist.

This PR fixes #1850 by using missing_ok=True introduced with pathlib 3.8 and trusting that Pathlib are able to handle the different errors provided on different OS.

For the sake of consistency it also updates windows and pyodide to follow the same pattern at the same point in the code.

henryiii commented 3 months ago

pyodide as it is not my native system

Pyodide isn't anyone's native system. :)

No worry about running tests locally, that's what CI is for.

MusicalNinjaDad commented 3 months ago

Thanks henryii,

I'm going to put together a quick CI in my fork which tries building the offending repo to check whether this actually works to solve the issue. (Which also hit windows after the workaround worked for macos ...)

henryiii commented 3 months ago

You should be able to just change the cibuildwheel install/use to this PR. Though you might also try rebuilding to see if it a constant failure, or an intermittent one.

"Hit Windows" - does that mean this workaround hit Windows, or without the workaround hit Windows? It's a little troubling if the workaround causes failures on Windows...

MusicalNinjaDad commented 3 months ago

The issue (without workaround) hit windows.

It's consistent - I've reproduced it here: https://github.com/MusicalNinjaDad/cibuildwheel/actions/runs/9361752026/job/25769267931

I only added mkdir wheelhouse to macos.before-all

Looking at it now - it's actually a different NotADirectoryError on the win setup ... I'll take a look at that once I've confirmed this fixes macos

MusicalNinjaDad commented 3 months ago

OK - so my debugging found the issue (and then I saw serhiy-storchaka comment on python/cpython#119993

I've got a fix done, currently final test ongoing. Then I just need to remove the debug logging and push it to this branch ...

henryiii commented 3 months ago

I moved the comments, trailing comments cause this to take 3 lines vs. 2. Also removed the comment about removing, as that's implied IMO anytime you mention a Python version in a comment. I always do something like git grep -E "Python 3\.[89]" to look for these sorts of comments.

MusicalNinjaDad commented 3 months ago

You should be able to just change the cibuildwheel install/use to this PR.

FWIW: I've just done that and the relevant parts of the build have all passed, just waiting on the emulated linux architectures now ...

henryiii commented 3 months ago

Thanks!