From 6c6e0f6ba5d3ff6c01c7cede39034a9cb0e8cf82 Mon Sep 17 00:00:00 2001 From: MusicalNinjaDad <102677655+MusicalNinjaDad@users.noreply.github.com> Date: Fri, 7 Jun 2024 17:37:20 +0200 Subject: [PATCH] fix: handle case where output_dir does not already exist on macos & windows (#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 Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- cibuildwheel/macos.py | 14 ++++++++------ cibuildwheel/pyodide.py | 13 ++++++++----- cibuildwheel/util.py | 35 +++++++++++++++++++++++++++++++++++ cibuildwheel/windows.py | 14 ++++++++------ 4 files changed, 59 insertions(+), 17 deletions(-) diff --git a/cibuildwheel/macos.py b/cibuildwheel/macos.py index 977f7f20..813531b0 100644 --- 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 @@ from .util import ( 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 index 30345f7b..e9e75a5f 100644 --- 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 @@ from .util import ( 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 index 22e2c212..2d127f2b 100644 --- a/cibuildwheel/util.py +++ b/cibuildwheel/util.py @@ -6,6 +6,7 @@ import itertools 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 index 3c0b8779..f5daa28c 100644 --- a/cibuildwheel/windows.py +++ b/cibuildwheel/windows.py @@ -7,7 +7,6 @@ import subprocess 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 @@ from .util import ( 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