fix: handle case where output_dir does not already exist on macos & w…

…indows (#1851) * replace `with suppress(FileNotFoundError)` by `.unlink(missing_ok=True)` for macos * also use `.unlink(missing_ok=True)` in pyodide and windows for consistency * remove contextlib imports which are no longer required * Apply suggestions from code review * explicity resolve and create output location * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * use explicit str for move * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update comments based on review feedback * Apply suggestions from code review * Break out functionality to move files to util.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * raise instance of IsADirectoryError with meaningful message * Don't need a comment and a exception message --------- Co-authored-by: Henry Schreiner <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pypa · Jun 7, 2024 · 6c6e0f6 · 6c6e0f6
1 parent 877d3bf
commit 6c6e0f6
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 17 deletions.
diff --git a/cibuildwheel/macos.py b/cibuildwheel/macos.py
@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import contextlib
 import functools
 import os
 import platform
@@ -37,6 +36,7 @@
     get_build_verbosity_extra_flags,
     get_pip_version,
     install_certifi_script,
+    move_file,
     prepare_command,
     read_python_configs,
     shell,
@@ -641,11 +641,13 @@ 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()
-
-                shutil.move(str(repaired_wheel), build_options.output_dir)
-                built_wheels.append(build_options.output_dir / repaired_wheel.name)
+                output_wheel = build_options.output_dir.joinpath(repaired_wheel.name)
+                moved_wheel = move_file(repaired_wheel, output_wheel)
+                if moved_wheel != output_wheel.resolve():
+                    log.warning(
+                        "{repaired_wheel} was moved to {moved_wheel} instead of {output_wheel}"
+                    )
+                built_wheels.append(output_wheel)
 
             # clean up
             shutil.rmtree(identifier_tmp_dir)

diff --git a/cibuildwheel/pyodide.py b/cibuildwheel/pyodide.py
@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import contextlib
 import os
 import shutil
 import sys
@@ -27,6 +26,7 @@
     extract_zip,
     find_compatible_wheel,
     get_pip_version,
+    move_file,
     prepare_command,
     read_python_configs,
     shell,
@@ -395,10 +395,13 @@ 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_wheel = build_options.output_dir.joinpath(repaired_wheel.name)
+                moved_wheel = move_file(repaired_wheel, output_wheel)
+                if moved_wheel != output_wheel.resolve():
+                    log.warning(
+                        "{repaired_wheel} was moved to {moved_wheel} instead of {output_wheel}"
+                    )
+                built_wheels.append(output_wheel)
 
-                shutil.move(str(repaired_wheel), build_options.output_dir)
-                built_wheels.append(build_options.output_dir / repaired_wheel.name)
     finally:
         pass
diff --git a/cibuildwheel/util.py b/cibuildwheel/util.py
@@ -6,6 +6,7 @@
 import os
 import re
 import shlex
+import shutil
 import ssl
 import subprocess
 import sys
@@ -359,6 +360,40 @@ def extract_tar(tar_src: Path, dest: Path) -> None:
         tar_.extractall(dest)
 
 
+def move_file(src_file: Path, dst_file: Path) -> Path:
+    """Moves a file safely while avoiding potential semantic confusion:
+
+    1. `dst_file` must point to the target filename, not a directory
+    2. `dst_file` will be overwritten if it already exists
+    3. any missing parent directories will be created
+
+    Returns the fully resolved Path of the resulting file.
+
+    Raises:
+        NotADirectoryError: If any part of the intermediate path to `dst_file` is an existing file
+        IsADirectoryError: If `dst_file` points directly to an existing directory
+    """
+
+    # Importing here as logger needs various functions from util -> circular imports
+    from .logger import log
+
+    src_file = src_file.resolve()
+    dst_file = dst_file.resolve()
+
+    if dst_file.is_dir():
+        msg = "dst_file must be a valid target filename, not an existing directory."
+        raise IsADirectoryError(msg)
+    dst_file.unlink(missing_ok=True)
+    dst_file.parent.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
+    resulting_file = shutil.move(str(src_file), str(dst_file))
+    resulting_file = Path(resulting_file).resolve()
+    log.notice(f"Moved {src_file} to {resulting_file}")
+    return Path(resulting_file)
+
+
 class DependencyConstraints:
     def __init__(self, base_file_path: Path):
         assert base_file_path.exists()

diff --git a/cibuildwheel/windows.py b/cibuildwheel/windows.py
@@ -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
@@ -34,6 +33,7 @@
     find_compatible_wheel,
     get_build_verbosity_extra_flags,
     get_pip_version,
+    move_file,
     prepare_command,
     read_python_configs,
     shell,
@@ -543,11 +543,13 @@ 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()
-
-                shutil.move(str(repaired_wheel), build_options.output_dir)
-                built_wheels.append(build_options.output_dir / repaired_wheel.name)
+                output_wheel = build_options.output_dir.joinpath(repaired_wheel.name)
+                moved_wheel = move_file(repaired_wheel, output_wheel)
+                if moved_wheel != output_wheel.resolve():
+                    log.warning(
+                        "{repaired_wheel} was moved to {moved_wheel} instead of {output_wheel}"
+                    )
+                built_wheels.append(output_wheel)
 
             # clean up
             # (we ignore errors because occasionally Windows fails to unlink a file and we