-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix wheel cache concurrency when temp and cache are not on the same filesystem #13541
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
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. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,18 @@ 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 rename here because wheel_path is guaranteed to be | ||
# in the same filesystem as output_dir. An atomic is rename | ||
# to avoid concurrency issues when populating the cache. | ||
os.rename(wheel_path, dest_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why switch from While the temporary path probably supports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the cache filesystem does not support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. For reference this is the PR that changed it from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking it should be |
||
logger.info( | ||
"Created wheel for %s: filename=%s size=%d sha256=%s", | ||
req.name, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like it to be just a bit more clear that a temporary directory is created inside the cache directory, e.g.