diff --git a/scripts/build_wheel.py b/scripts/build_wheel.py index 51602d91a99b..e546b2e12ca8 100755 --- a/scripts/build_wheel.py +++ b/scripts/build_wheel.py @@ -21,6 +21,7 @@ import sys import sysconfig import tempfile +import time import warnings from argparse import ArgumentParser, ArgumentTypeError from contextlib import contextmanager @@ -614,7 +615,38 @@ def _tar_pipe_copy(src: Path, dst: Path) -> bool: return producer.returncode == 0 and consumer.returncode == 0 -def sync_tree(src, dst, exclude: Sequence[str] = ()): +# How recently a source file must have been written for its mtime to be +# untrustworthy as a change marker. Inode timestamps come from a coarse clock +# (one timer tick on Linux) and some filesystems store whole seconds, so a file +# rewritten shortly after being copied can still report the mtime the copy +# recorded. A size+mtime comparison would then call it unchanged and leave a +# stale copy behind. Two seconds covers a whole-second-granularity destination +# (which can make an mtime look up to a second older than it is) on top of the +# tick granularity of the source. +_MTIME_RACE_WINDOW = 2.0 + + +def _demote_racy_mtime(dst_file: Path, src_stat: Optional[os.stat_result], + now: float) -> None: + """Break the mtime match for a copy whose source was just written. + + A copy normally records the source's mtime so the next sync can skip it. + That is only sound once the source mtime has aged out of the window above; + before that the source can change again without its mtime moving. Backdating + the copy makes the next sync's comparison mismatch, so the file is re-copied + instead of silently kept stale. It costs one extra copy of files written + right before a sync, and converges: the re-copy records the real mtime. + """ + if src_stat is None or src_stat.st_mtime <= now - _MTIME_RACE_WINDOW: + return + try: + os.utime(dst_file, + (src_stat.st_atime, src_stat.st_mtime - _MTIME_RACE_WINDOW)) + except OSError: + pass + + +def sync_tree(src: Path, dst: Path, exclude: Sequence[str] = ()) -> None: """Mirror the src directory into dst, touching only what changed. Replaces the rmtree+copytree pattern for artifact copy-back: files are @@ -623,17 +655,37 @@ def sync_tree(src, dst, exclude: Sequence[str] = ()): destination (which may be a slow network filesystem). A missing dst is populated via a streamed tar pipeline instead of per-file copies. Symlinks are dereferenced like copytree(symlinks=False); mtimes are - preserved so the next sync can compare against them. exclude lists - fnmatch patterns for entry names to skip. + preserved so the next sync can compare against them, except for sources + written within _MTIME_RACE_WINDOW of the copy, whose mtimes cannot yet + prove the content settled. exclude lists fnmatch patterns for entry names + to skip. """ import fnmatch src = Path(src).resolve() dst = Path(dst) + now = time.time() - def excluded(name): + def excluded(name: str) -> bool: return any(fnmatch.fnmatch(name, pat) for pat in exclude) + def demote_racy_mtimes() -> None: + # A cold populate (tar or copytree) copies source mtimes verbatim, so + # apply the same guard the incremental path applies per file. Walk the + # source rather than the freshly written destination: the source is + # local and warm, and only the few racy entries need a write. + for root, dirs, files in os.walk(src, followlinks=True): + dirs[:] = [d for d in dirs if not excluded(d)] + rel = Path(root).relative_to(src) + for name in files: + if excluded(name): + continue + try: + src_stat = (Path(root) / name).stat() + except OSError: + continue + _demote_racy_mtime(dst / rel / name, src_stat, now) + if dst.is_symlink(): dst.unlink() elif dst.exists() and src == dst.resolve(): @@ -641,11 +693,13 @@ def excluded(name): if not dst.exists(): if not exclude and _tar_pipe_copy(src, dst): + demote_racy_mtimes() return copytree(src, dst, symlinks=False, ignore=shutil.ignore_patterns(*exclude) if exclude else None) + demote_racy_mtimes() return for root, dirs, files in os.walk(src, followlinks=True): @@ -667,11 +721,16 @@ def excluded(name): for name in files: src_file = Path(root) / name dst_file = dst_root / name + src_stat = None try: src_stat = src_file.stat() dst_stat = dst_file.stat() + # Trust the match only once the source mtime has aged past the + # race window; a just-written source can be rewritten again + # without the mtime moving, which would strand a stale copy. if (src_stat.st_size == dst_stat.st_size - and abs(src_stat.st_mtime - dst_stat.st_mtime) < 1e-3): + and abs(src_stat.st_mtime - dst_stat.st_mtime) < 1e-3 + and src_stat.st_mtime <= now - _MTIME_RACE_WINDOW): continue except OSError: pass @@ -679,6 +738,7 @@ def excluded(name): rmtree(dst_file) # copy2: mtime must survive for the next sync's comparison. shutil.copy2(src_file, dst_file) + _demote_racy_mtime(dst_file, src_stat, now) def stage_python_package(project_dir: Path, staging_dir: Path) -> None: diff --git a/tests/unittest/scripts/test_build_wheel_copy_back.py b/tests/unittest/scripts/test_build_wheel_copy_back.py index 8d1449405911..d69999d06628 100644 --- a/tests/unittest/scripts/test_build_wheel_copy_back.py +++ b/tests/unittest/scripts/test_build_wheel_copy_back.py @@ -30,7 +30,9 @@ import os import shutil import stat +import time from pathlib import Path +from typing import Callable import pytest @@ -41,11 +43,34 @@ @pytest.fixture(scope="module") -def sync_tree(): +def build_wheel_module(): spec = importlib.util.spec_from_file_location("build_wheel", SCRIPT_PATH) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) - return module.sync_tree + return module + + +@pytest.fixture +def sync_tree(build_wheel_module): + return build_wheel_module.sync_tree + + +@pytest.fixture(params=["tar", "copytree"]) +def cold_backend(request, build_wheel_module, monkeypatch): + """Force sync_tree's cold populate down each backend in turn. + + A missing destination is filled by _tar_pipe_copy when tar is on PATH and + by copytree otherwise. Both preserve mtimes, so the mtime-collision tests + that consume this fixture must hold on either; without pinning the backend + a run only covers whichever one the environment happens to select. The tar + case skips when tar is absent so it can never silently alias to copytree. + """ + if request.param == "tar": + if shutil.which("tar") is None: + pytest.skip("tar unavailable; cannot exercise the tar cold path") + else: + monkeypatch.setattr(build_wheel_module, "_tar_pipe_copy", lambda src, dst: False) + return request.param def old_copy(src, dst, exclude=()): @@ -183,6 +208,59 @@ def test_warm_noop_is_stable(sync_tree, tmp_path): assert_trees_equal(new_dir, old_dir) +def test_rewrite_without_mtime_change_is_not_skipped( + sync_tree: Callable[..., None], cold_backend: str, tmp_path: Path +) -> None: + """A same-size rewrite that leaves the source mtime untouched must copy. + + Inode timestamps come from a coarse clock, so a file rewritten shortly + after being copied can still report the mtime the copy recorded, and a + size+mtime comparison would call it unchanged. utime pins the mtime to the + copied value, making that collision deterministic instead of a race. + cold_backend runs this once per cold-populate path (tar and copytree). + """ + src = tmp_path / "src" + src.mkdir() + mod = src / "mod.py" + mod.write_bytes(b"def f():\n return 42\n") + new_dir = tmp_path / "new" + sync_tree(src, new_dir) + + copied = mod.stat() + mod.write_bytes(b"def f():\n return 99\n") # same size + os.utime(mod, (copied.st_atime, copied.st_mtime)) + sync_tree(src, new_dir) + + assert (new_dir / "mod.py").read_bytes() == b"def f():\n return 99\n" + + +def test_settled_file_is_not_recopied( + sync_tree: Callable[..., None], cold_backend: str, tmp_path: Path +) -> None: + """A file whose mtime has aged out of the race window is left alone. + + Pins the incremental behaviour itself: the destination is diverged behind + sync_tree's back, so surviving the re-sync proves the file was skipped + rather than rewritten. cold_backend runs this once per cold-populate path + (tar and copytree). + """ + src = tmp_path / "src" + src.mkdir() + aged = src / "aged.bin" + aged.write_bytes(b"aged\n") + old = time.time() - 3600 + os.utime(aged, (old, old)) + new_dir = tmp_path / "new" + sync_tree(src, new_dir) + + dst_file = new_dir / "aged.bin" + dst_file.write_bytes(b"kept\n") # same size as the source + os.utime(dst_file, (old, old)) + sync_tree(src, new_dir) + + assert dst_file.read_bytes() == b"kept\n" + + def test_same_src_dst_is_noop(sync_tree, tmp_path): """sync_tree onto itself must not wipe the tree.