Handle case where output_dir does not already exist on macos & windows #1851
Conversation
Pyodide isn't anyone's native system. :) No worry about running tests locally, that's what CI is for. |
|
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 ...) |
|
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... |
|
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 Looking at it now - it's actually a different NotADirectoryError on the win setup ... |
|
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 ... |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
MusicalNinjaDad
changed the title
|
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 |
cibuildwheel/macos.py
Outdated
| with contextlib.suppress(FileNotFoundError): | ||
| (build_options.output_dir / repaired_wheel.name).unlink() | ||
| output_dir = build_options.output_dir.resolve() | ||
| output_wheel = output_dir.joinpath(repaired_wheel.name).resolve() | ||
| output_wheel.unlink(missing_ok=True) | ||
|
|
||
| shutil.move(str(repaired_wheel), build_options.output_dir) | ||
| built_wheels.append(build_options.output_dir / repaired_wheel.name) | ||
| # shutil.move() will rename the file to what we were expecting to be the parent directory if we don't ensure it exists | ||
| output_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # using shutil.move() as Path.rename() is not guaranteed to work across filesystem boundaries | ||
| # explicit str() needed for Python 3.8 | ||
| shutil.move(str(repaired_wheel), str(output_wheel)) | ||
| built_wheels.append(output_wheel) |
There was a problem hiding this comment.
IMHO, we should add a move function in util.py which takes care of unlinking, creating the parent directory and actually moving the wheel. It could be reused on all 3 platforms (workaround might be needed for local builds on macOS with pyodide for exemple).
There was a problem hiding this comment.
Hi mayeut,
That's a good idea.
Also a good catch: I missed updating pyodide as I wasn't building for it on my test project.
I'd be happy to put the change together later today.
What's the issue that you would see with local macos-pyodide builds?
There was a problem hiding this comment.
I'd imagine the util function has a signature along the lines of: movewheel(wheel: Path) -> Path, directly accesses build_options as this is global and returns output_wheel, leaving built_wheels.append at the calling site for clarity.
Does that make sense?
There was a problem hiding this comment.
The function signature should be something like move(src: Path, dst: Path) -> None
So similar to shutil.move but taking care of unlinking dst and creating the parent directory of dst.
There was a problem hiding this comment.
How about this? (See latest push). I'm happy with the shape of the function and it follows shutil.move closely (which also returns the destination path).
I'm slightly less happy with the import inside the function but felt that logging the move was valuable enough to pay that price.
I've tested against macos & windows. I couldn't test pyodide as the test project I'm using is rust-based and I haven't got pyodide/emscripten working for that yet ...
There was a problem hiding this comment.
What sort of issues are you hitting? It should Just Work™ in most cases, so there's a chance it's hitting a Pyodide or cibuildwheel bug.
There was a problem hiding this comment.
We can probably split utils up later to avoid the circular import, it's pretty big. Not something to worry about for this PR.
Why is the warning at every usage? Would it make sense to warn inside the function if it doesn't copy where it's supposed to?
There was a problem hiding this comment.
What sort of issues are you hitting? It should Just Work™ in most cases, so there's a chance it's hitting a Pyodide or cibuildwheel bug.
rust seemed to be targetting the wrong target on the one time I tried "just switching it on" as part of testing this PR. Looking at the code and the implementation PR on cibuildwheel side, I get the impression that I "just" need to add a few config entries into pyproject.toml, find the best way to build in pipeline, check whether ADO artefact repos can accept the wheels, and set up some kind of test environment to play with the result...
It looks interesting enough that I'm likely to try playing with it in my sandbox project soon
There was a problem hiding this comment.
Why is the warning at every usage? Would it make sense to warn inside the function if it doesn't copy where it's supposed to?
I'd consider it to be in the responsibility of the main script to double check that a called util did what was expected, that also covers the risk of later changes to utils etc.
Adding the warning to util.move_file feels like a no-op to me ... the only line that would effectively be checked is whether shutil.move did what was expected (?)
FWIW: I've just done that and the relevant parts of the build have all passed, just waiting on the emulated linux architectures now ... |
|
Thanks! |
[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_dirrather thanoutputdir/wheelname.whlThis 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:
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
NotADirectoryErrorinstead of aFileNotFoundErroron macos. This causes builds to fail after testing if the output_dir does not already exist.This PR fixes #1850 by using
missing_ok=Trueintroduced 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.