Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 15 commits into from
Jun 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
94fced5
replace `with suppress(FileNotFoundError)` by `.unlink(missing_ok=Tru…
MusicalNinjaDad Jun 3, 2024
5f9e4dd
also use `.unlink(missing_ok=True)` in pyodide and windows for consis…
MusicalNinjaDad Jun 3, 2024
cbf5afd
remove contextlib imports which are no longer required
MusicalNinjaDad Jun 3, 2024
872009b
Apply suggestions from code review
henryiii Jun 3, 2024
dfe8737
explicity resolve and create output location
MusicalNinjaDad Jun 4, 2024
9202c83
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 4, 2024
6dd397e
use explicit str for move
MusicalNinjaDad Jun 4, 2024
16b96ca
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 4, 2024
cb804c9
Merge branch 'main' into MusicalNinjaDad/issue1850
MusicalNinjaDad Jun 5, 2024
df77e97
update comments based on review feedback
MusicalNinjaDad Jun 5, 2024
e04bc49
Apply suggestions from code review
henryiii Jun 5, 2024
db4abe2
Break out functionality to move files to util.py
MusicalNinjaDad Jun 6, 2024
8a98f74
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 6, 2024
2f18df5
raise instance of IsADirectoryError with meaningful message
MusicalNinjaDad Jun 6, 2024
03aeff3
Don't need a comment and a exception message
MusicalNinjaDad Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions cibuildwheel/macos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from __future__ import annotations

import contextlib
import functools
import os
import platform
Expand Down Expand Up @@ -641,11 +640,17 @@ def build(options: Options, tmp_path: Path) -> None:

# we're all done here; move it to output (overwrite existing)
if compatible_wheel is None:
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@MusicalNinjaDad MusicalNinjaDad Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@mayeut mayeut Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (?)


# clean up
shutil.rmtree(identifier_tmp_dir)
Expand Down
4 changes: 1 addition & 3 deletions cibuildwheel/pyodide.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from __future__ import annotations

import contextlib
import os
import shutil
import sys
Expand Down Expand Up @@ -395,8 +394,7 @@ def build(options: Options, tmp_path: Path) -> None:

# we're all done here; move it to output (overwrite existing)
if compatible_wheel is None:
with contextlib.suppress(FileNotFoundError):
(build_options.output_dir / repaired_wheel.name).unlink()
build_options.output_dir.joinpath(repaired_wheel.name).unlink(missing_ok=True)

shutil.move(str(repaired_wheel), build_options.output_dir)
built_wheels.append(build_options.output_dir / repaired_wheel.name)
Expand Down
15 changes: 10 additions & 5 deletions cibuildwheel/windows.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import sys
import textwrap
from collections.abc import MutableMapping, Sequence, Set
from contextlib import suppress
from dataclasses import dataclass
from functools import lru_cache
from pathlib import Path
Expand Down Expand Up @@ -543,11 +542,17 @@ def build(options: Options, tmp_path: Path) -> None:

# we're all done here; move it to output (remove if already exists)
if compatible_wheel is None:
with 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)

# clean up
# (we ignore errors because occasionally Windows fails to unlink a file and we
Expand Down