Merge pull request #8283 from uranusjr/pre-existing-build-directory-fix

Pre-existing build directory fix

Author: pfmoore

src/pip/_internal/operations/prepare.py

@@ -379,6 +379,7 @@ def _download_should_save(self):
     def prepare_linked_requirement(
         self,
         req,  # type: InstallRequirement
+        parallel_builds=False,  # type: bool
     ):
         # type: (...) -> AbstractDistribution
         """Prepare a requirement that would be obtained from req.link
@@ -415,7 +416,11 @@ def prepare_linked_requirement(
         with indent_log():
             # Since source_dir is only set for editable requirements.
             assert req.source_dir is None
-            req.ensure_has_source_dir(self.build_dir, autodelete_unpacked)
+            req.ensure_has_source_dir(
+                self.build_dir,
+                autodelete=autodelete_unpacked,
+                parallel_builds=parallel_builds,
+            )
             # If a checkout exists, it's unwise to keep going.  version
             # inconsistencies are logged later, but do not fail the
             # installation.

src/pip/_internal/req/req_install.py

@@ -7,6 +7,7 @@
 import os
 import shutil
 import sys
+import uuid
 import zipfile
 
 from pip._vendor import pkg_resources, six
@@ -339,8 +340,8 @@ def from_path(self):
                 s += '->' + comes_from
         return s
 
-    def ensure_build_location(self, build_dir, autodelete):
-        # type: (str, bool) -> str
+    def ensure_build_location(self, build_dir, autodelete, parallel_builds):
+        # type: (str, bool, bool) -> str
         assert build_dir is not None
         if self._temp_build_dir is not None:
             assert self._temp_build_dir.path
@@ -354,16 +355,19 @@ def ensure_build_location(self, build_dir, autodelete):
             )
 
             return self._temp_build_dir.path
-        if self.editable:
-            name = self.name.lower()
-        else:
-            name = self.name
+
+        # When parallel builds are enabled, add a UUID to the build directory
+        # name so multiple builds do not interfere with each other.
+        dir_name = canonicalize_name(self.name)
+        if parallel_builds:
+            dir_name = "{}_{}".format(dir_name, uuid.uuid4().hex)
+
         # FIXME: Is there a better place to create the build_dir? (hg and bzr
         # need this)
         if not os.path.exists(build_dir):
             logger.debug('Creating directory %s', build_dir)
             os.makedirs(build_dir)
-        actual_build_dir = os.path.join(build_dir, name)
+        actual_build_dir = os.path.join(build_dir, dir_name)
         # `None` indicates that we respect the globally-configured deletion
         # settings, which is what we actually want when auto-deleting.
         delete_arg = None if autodelete else False
@@ -588,8 +592,13 @@ def assert_source_matches_version(self):
             )
 
     # For both source distributions and editables
-    def ensure_has_source_dir(self, parent_dir, autodelete=False):
-        # type: (str, bool) -> None
+    def ensure_has_source_dir(
+        self,
+        parent_dir,
+        autodelete=False,
+        parallel_builds=False,
+    ):
+        # type: (str, bool, bool) -> None
         """Ensure that a source_dir is set.
 
         This will create a temporary build dir if the name of the requirement
@@ -601,7 +610,9 @@ def ensure_has_source_dir(self, parent_dir, autodelete=False):
         """
         if self.source_dir is None:
             self.source_dir = self.ensure_build_location(
-                parent_dir, autodelete
+                parent_dir,
+                autodelete=autodelete,
+                parallel_builds=parallel_builds,
             )
 
     # For editable installations

src/pip/_internal/resolution/resolvelib/candidates.py

@@ -255,7 +255,9 @@ def __init__(
 
     def _prepare_abstract_distribution(self):
         # type: () -> AbstractDistribution
-        return self._factory.preparer.prepare_linked_requirement(self._ireq)
+        return self._factory.preparer.prepare_linked_requirement(
+            self._ireq, parallel_builds=True,
+        )
 
 
 class EditableCandidate(_InstallRequirementBackedCandidate):

tests/functional/test_install_cleanup.py

@@ -20,7 +20,11 @@ def test_no_clean_option_blocks_cleaning_after_install(script, data):
 
 
 @pytest.mark.network
-def test_cleanup_prevented_upon_build_dir_exception(script, data):
+def test_cleanup_prevented_upon_build_dir_exception(
+    script,
+    data,
+    use_new_resolver,
+):
     """
     Test no cleanup occurs after a PreviousBuildDirError
     """
@@ -31,12 +35,14 @@ def test_cleanup_prevented_upon_build_dir_exception(script, data):
     result = script.pip(
         'install', '-f', data.find_links, '--no-index', 'simple',
         '--build', build,
-        expect_error=True, expect_temp=True,
+        expect_error=(not use_new_resolver),
+        expect_temp=(not use_new_resolver),
     )
 
-    assert result.returncode == PREVIOUS_BUILD_DIR_ERROR, str(result)
-    assert "pip can't proceed" in result.stderr, str(result)
-    assert exists(build_simple), str(result)
+    if not use_new_resolver:
+        assert result.returncode == PREVIOUS_BUILD_DIR_ERROR, str(result)
+        assert "pip can't proceed" in result.stderr, str(result)
+        assert exists(build_simple), str(result)
 
 
 @pytest.mark.network

tests/functional/test_new_resolver.py

@@ -863,7 +863,6 @@ def test_new_resolver_extra_merge_in_package(
         assert_installed(script, pkg="1.0.0", dep="1.0.0", depdev="1.0.0")
 
 
-@pytest.mark.xfail(reason="pre-existing build directory")
 def test_new_resolver_build_directory_error_zazo_19(script):
     """https://github.com/pradyunsg/zazo/issues/19#issuecomment-631615674
 

tests/functional/test_wheel.py

@@ -191,7 +191,11 @@ def test_pip_wheel_fail(script, data):
     assert result.returncode != 0
 
 
-def test_no_clean_option_blocks_cleaning_after_wheel(script, data):
+def test_no_clean_option_blocks_cleaning_after_wheel(
+    script,
+    data,
+    use_new_resolver,
+):
     """
     Test --no-clean option blocks cleaning after wheel build
     """
@@ -202,9 +206,11 @@ def test_no_clean_option_blocks_cleaning_after_wheel(script, data):
         'simple',
         expect_temp=True,
     )
-    build = build / 'simple'
-    assert exists(build), \
-        "build/simple should still exist {result}".format(**locals())
+
+    if not use_new_resolver:
+        build = build / 'simple'
+        message = "build/simple should still exist {}".format(result)
+        assert exists(build), message
 
 
 def test_pip_wheel_source_deps(script, data):
@@ -224,7 +230,11 @@ def test_pip_wheel_source_deps(script, data):
     assert "Successfully built source" in result.stdout, result.stdout
 
 
-def test_pip_wheel_fail_cause_of_previous_build_dir(script, data):
+def test_pip_wheel_fail_cause_of_previous_build_dir(
+    script,
+    data,
+    use_new_resolver,
+):
     """
     Test when 'pip wheel' tries to install a package that has a previous build
     directory
@@ -240,11 +250,14 @@ def test_pip_wheel_fail_cause_of_previous_build_dir(script, data):
         'wheel', '--no-index',
         '--find-links={data.find_links}'.format(**locals()),
         '--build', script.venv_path / 'build',
-        'simple==3.0', expect_error=True, expect_temp=True,
+        'simple==3.0',
+        expect_error=(not use_new_resolver),
+        expect_temp=(not use_new_resolver),
     )
 
     # Then I see that the error code is the right one
-    assert result.returncode == PREVIOUS_BUILD_DIR_ERROR, result
+    if not use_new_resolver:
+        assert result.returncode == PREVIOUS_BUILD_DIR_ERROR, result
 
 
 def test_wheel_package_with_latin1_setup(script, data):

tests/unit/test_req_install.py

@@ -21,7 +21,7 @@ def test_tmp_build_directory(self):
         requirement = InstallRequirement(None, None)
         tmp_dir = tempfile.mkdtemp('-build', 'pip-')
         tmp_build_dir = requirement.ensure_build_location(
-            tmp_dir, autodelete=False
+            tmp_dir, autodelete=False, parallel_builds=False,
         )
         assert (
             os.path.dirname(tmp_build_dir) ==