Merge pull request #13541 from sbidoul/fix-wheel-cache-concurrency-sbi

Address wheel cache concurrency and performance when temp and cache are not on the same filesystem

Author: sbidoul

news/13540.bugfix.rst

@@ -0,0 +1,5 @@
+Avoid concurrency issues and improve performance when storing wheels into the
+cache of locally built wheels, when the temporary build directory is on a
+different filesystem than the cache directory. To this end, the wheel directory
+passed to the build backend is now a temporary subdirectory inside the cache
+directory.

src/pip/_internal/operations/build/wheel.py

@@ -14,25 +14,25 @@ def build_wheel_pep517(
     name: str,
     backend: BuildBackendHookCaller,
     metadata_directory: str,
-    tempd: str,
+    wheel_directory: str,
 ) -> str | None:
     """Build one InstallRequirement using the PEP 517 build process.
 
     Returns path to wheel if successfully built. Otherwise, returns None.
     """
     assert metadata_directory is not None
     try:
-        logger.debug("Destination directory: %s", tempd)
+        logger.debug("Destination directory: %s", wheel_directory)
 
         runner = runner_with_spinner_message(
             f"Building wheel for {name} (pyproject.toml)"
         )
         with backend.subprocess_runner(runner):
             wheel_name = backend.build_wheel(
-                tempd,
+                wheel_directory=wheel_directory,
                 metadata_directory=metadata_directory,
             )
     except Exception:
         logger.error("Failed building wheel for %s", name)
         return None
-    return os.path.join(tempd, wheel_name)
+    return os.path.join(wheel_directory, wheel_name)

src/pip/_internal/operations/build/wheel_editable.py

@@ -14,23 +14,23 @@ def build_wheel_editable(
     name: str,
     backend: BuildBackendHookCaller,
     metadata_directory: str,
-    tempd: str,
+    wheel_directory: str,
 ) -> str | None:
     """Build one InstallRequirement using the PEP 660 build process.
 
     Returns path to wheel if successfully built. Otherwise, returns None.
     """
     assert metadata_directory is not None
     try:
-        logger.debug("Destination directory: %s", tempd)
+        logger.debug("Destination directory: %s", wheel_directory)
 
         runner = runner_with_spinner_message(
             f"Building editable for {name} (pyproject.toml)"
         )
         with backend.subprocess_runner(runner):
             try:
                 wheel_name = backend.build_editable(
-                    tempd,
+                    wheel_directory=wheel_directory,
                     metadata_directory=metadata_directory,
                 )
             except HookMissing as e:
@@ -44,4 +44,4 @@ def build_wheel_editable(
     except Exception:
         logger.error("Failed building editable for %s", name)
         return None
-    return os.path.join(tempd, wheel_name)
+    return os.path.join(wheel_directory, wheel_name)

src/pip/_internal/operations/build/wheel_legacy.py

@@ -33,7 +33,7 @@ def format_command_result(
 
 def get_legacy_build_wheel_path(
     names: list[str],
-    temp_dir: str,
+    wheel_directory: str,
     name: str,
     command_args: list[str],
     command_output: str,
@@ -55,7 +55,7 @@ def get_legacy_build_wheel_path(
         msg += format_command_result(command_args, command_output)
         logger.warning(msg)
 
-    return os.path.join(temp_dir, names[0])
+    return os.path.join(wheel_directory, names[0])
 
 
 def build_wheel_legacy(
@@ -64,7 +64,7 @@ def build_wheel_legacy(
     source_dir: str,
     global_options: list[str],
     build_options: list[str],
-    tempd: str,
+    wheel_directory: str,
 ) -> str | None:
     """Build one unpacked package using the "legacy" build process.
 
@@ -89,12 +89,12 @@ def build_wheel_legacy(
         setup_py_path,
         global_options=global_options,
         build_options=build_options,
-        destination_dir=tempd,
+        destination_dir=wheel_directory,
     )
 
     spin_message = f"Building wheel for {name} (setup.py)"
     with open_spinner(spin_message) as spinner:
-        logger.debug("Destination directory: %s", tempd)
+        logger.debug("Destination directory: %s", wheel_directory)
 
         try:
             output = call_subprocess(
@@ -108,10 +108,10 @@ def build_wheel_legacy(
             logger.error("Failed building wheel for %s", name)
             return None
 
-        names = os.listdir(tempd)
+        names = os.listdir(wheel_directory)
         wheel_path = get_legacy_build_wheel_path(
             names=names,
-            temp_dir=tempd,
+            wheel_directory=wheel_directory,
             name=name,
             command_args=wheel_args,
             command_output=output,

src/pip/_internal/wheel_builder.py

@@ -5,8 +5,8 @@
 import logging
 import os.path
 import re
-import shutil
 from collections.abc import Iterable
+from tempfile import TemporaryDirectory
 
 from pip._vendor.packaging.utils import canonicalize_name, canonicalize_version
 from pip._vendor.packaging.version import InvalidVersion, Version
@@ -24,7 +24,6 @@
 from pip._internal.utils.misc import ensure_dir, hash_file
 from pip._internal.utils.setuptools_build import make_setuptools_clean_args
 from pip._internal.utils.subprocess import call_subprocess
-from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.utils.urls import path_to_url
 from pip._internal.vcs import vcs
 
@@ -189,7 +188,7 @@ def _build_one_inside_env(
     global_options: list[str],
     editable: bool,
 ) -> str | None:
-    with TempDirectory(kind="wheel") as temp_dir:
+    with TemporaryDirectory(dir=output_dir) as wheel_directory:
         assert req.name
         if req.use_pep517:
             assert req.metadata_directory
@@ -207,14 +206,14 @@ def _build_one_inside_env(
                     name=req.name,
                     backend=req.pep517_backend,
                     metadata_directory=req.metadata_directory,
-                    tempd=temp_dir.path,
+                    wheel_directory=wheel_directory,
                 )
             else:
                 wheel_path = build_wheel_pep517(
                     name=req.name,
                     backend=req.pep517_backend,
                     metadata_directory=req.metadata_directory,
-                    tempd=temp_dir.path,
+                    wheel_directory=wheel_directory,
                 )
         else:
             wheel_path = build_wheel_legacy(
@@ -223,15 +222,19 @@ def _build_one_inside_env(
                 source_dir=req.unpacked_source_directory,
                 global_options=global_options,
                 build_options=build_options,
-                tempd=temp_dir.path,
+                wheel_directory=wheel_directory,
             )
 
         if wheel_path is not None:
             wheel_name = os.path.basename(wheel_path)
             dest_path = os.path.join(output_dir, wheel_name)
             try:
                 wheel_hash, length = hash_file(wheel_path)
-                shutil.move(wheel_path, dest_path)
+                # We can do a replace here because wheel_path is guaranteed to
+                # be in the same filesystem as output_dir. This will perform an
+                # atomic rename, which is necessary to avoid concurrency issues
+                # when populating the cache.
+                os.replace(wheel_path, dest_path)
                 logger.info(
                     "Created wheel for %s: filename=%s size=%d sha256=%s",
                     req.name,

tests/unit/test_wheel.py

@@ -46,7 +46,7 @@ def call_get_legacy_build_wheel_path(
 ) -> str | None:
     wheel_path = get_legacy_build_wheel_path(
         names=names,
-        temp_dir="/tmp/abcd",
+        wheel_directory="/tmp/abcd",
         name="pendulum",
         command_args=["arg1", "arg2"],
         command_output="output line 1\noutput line 2\n",