Merge pull request #865 from qwcode/no_reuse_build

deal with pre-existing build dirs

Author: qwcode

pip/commands/install.py

@@ -144,6 +144,12 @@ def __init__(self, *args, **kw):
             default=False,
             help="Include pre-release and development versions. By default, pip only finds stable versions.")
 
+        cmd_opts.add_option(
+            '--no-clean',
+            action='store_true',
+            default=False,
+            help="Don't delete build directories after installs or errors.")
+
         index_opts = cmdoptions.make_option_group(cmdoptions.index_group, self.parser)
 
         self.parser.insert_option_group(0, index_opts)
@@ -230,28 +236,31 @@ def run(self, options, args):
 
             raise InstallationError('--user --editable not supported with setuptools, use distribute')
 
-        if not options.no_download:
-            requirement_set.prepare_files(finder, force_root_egg_info=self.bundle, bundle=self.bundle)
-        else:
-            requirement_set.locate_files()
-
-        if not options.no_install and not self.bundle:
-            requirement_set.install(install_options, global_options, root=options.root_path)
-            installed = ' '.join([req.name for req in
-                                  requirement_set.successfully_installed])
-            if installed:
-                logger.notify('Successfully installed %s' % installed)
-        elif not self.bundle:
-            downloaded = ' '.join([req.name for req in
-                                   requirement_set.successfully_downloaded])
-            if downloaded:
-                logger.notify('Successfully downloaded %s' % downloaded)
-        elif self.bundle:
-            requirement_set.create_bundle(self.bundle_filename)
-            logger.notify('Created bundle in %s' % self.bundle_filename)
-        # Clean up
-        if not options.no_install or options.download_dir:
-            requirement_set.cleanup_files(bundle=self.bundle)
+        try:
+            if not options.no_download:
+                requirement_set.prepare_files(finder, force_root_egg_info=self.bundle, bundle=self.bundle)
+            else:
+                requirement_set.locate_files()
+
+            if not options.no_install and not self.bundle:
+                requirement_set.install(install_options, global_options, root=options.root_path)
+                installed = ' '.join([req.name for req in
+                                      requirement_set.successfully_installed])
+                if installed:
+                    logger.notify('Successfully installed %s' % installed)
+            elif not self.bundle:
+                downloaded = ' '.join([req.name for req in
+                                       requirement_set.successfully_downloaded])
+                if downloaded:
+                    logger.notify('Successfully downloaded %s' % downloaded)
+            elif self.bundle:
+                requirement_set.create_bundle(self.bundle_filename)
+                logger.notify('Created bundle in %s' % self.bundle_filename)
+        finally:
+            # Clean up
+            if (not options.no_clean) and ((not options.no_install) or options.download_dir):
+                requirement_set.cleanup_files(bundle=self.bundle)
+
         if options.target_dir:
             if not os.path.exists(options.target_dir):
                 os.makedirs(options.target_dir)

pip/exceptions.py

@@ -29,3 +29,8 @@ class BadCommand(PipError):
 
 class CommandError(PipError):
     """Raised when there is an error in command-line arguments"""
+
+
+class PreviousBuildDirError(PipError):
+    """Raised when there's a previous conflicting build directory"""
+

pip/req.py

@@ -6,13 +6,14 @@
 import sys
 import shutil
 import tempfile
+import textwrap
 import zipfile
 
 from distutils.util import change_root
 from pip.locations import bin_py, running_under_virtualenv
 from pip.exceptions import (InstallationError, UninstallationError,
                             BestVersionAlreadyInstalled,
-                            DistributionNotFound, CommandError)
+                            DistributionNotFound, PreviousBuildDirError)
 from pip.vcs import vcs
 from pip.log import logger
 from pip.util import (display_path, rmtree, ask, ask_path_exists, backup_dir,
@@ -1045,11 +1046,23 @@ def prepare_files(self, finder, force_root_egg_info=False, bundle=False):
 
                     # NB: This call can result in the creation of a temporary build directory
                     location = req_to_install.build_location(self.build_dir, not self.is_download)
-
-                    ## FIXME: is the existance of the checkout good enough to use it?  I don't think so.
                     unpack = True
                     url = None
-                    if not os.path.exists(os.path.join(location, 'setup.py')):
+
+                    # If a checkout exists, it's unwise to keep going.
+                    # Version inconsistencies are logged later, but do not fail the installation.
+                    if os.path.exists(os.path.join(location, 'setup.py')):
+                        msg = textwrap.dedent("""
+                          pip can't install requirement '%s' due to a pre-existing build directory.
+                           location: %s
+                          This is likely due to a previous installation that failed.
+                          pip is being responsible and not assuming it can delete this.
+                          Please delete it and try again.
+                        """ % (req_to_install, location))
+                        e = PreviousBuildDirError(msg)
+                        logger.fatal(e)
+                        raise e
+                    else:
                         ## FIXME: this won't upgrade when there's an existing package unpacked in `location`
                         if req_to_install.url is None:
                             if not_found:

tests/packages/README.txt

@@ -13,6 +13,10 @@ install). If any earlier step would fail (i.e. egg-info-generation), the
 already-installed version would never be uninstalled, so uninstall-rollback
 would not come into play.
 
+brokenegginfo-0.1.tar.gz
+------------------------
+crafted to fail on egg_info
+
 BrokenEmitsUTF8
 ---------------
 for generating unicode error in py3.x

tests/packages/brokenegginfo-0.1.tar.gz

@@ -1,24 +1,33 @@
 import os
 import textwrap
 from os.path import abspath, exists, join
-from tests.test_pip import (here, reset_env, run_pip, write_file, mkdir)
+from tests.test_pip import (here, reset_env, run_pip,
+                            write_file, mkdir, path_to_url)
 from tests.local_repos import local_checkout
 from tests.path import Path
 
+find_links = path_to_url(os.path.join(here, 'packages'))
 
-def test_cleanup_after_install_from_pypi():
+def test_cleanup_after_install():
     """
-    Test clean up after installing a package from PyPI.
-
+    Test clean up after installing a package.
     """
     env = reset_env()
-    run_pip('install', 'INITools==0.2', expect_error=True)
-    build = env.scratch_path/"build"
-    src = env.scratch_path/"src"
+    run_pip('install', '--no-index', '--find-links=%s' % find_links, 'simple')
+    build = env.venv_path/"build"
+    src = env.venv_path/"src"
     assert not exists(build), "build/ dir still exists: %s" % build
     assert not exists(src), "unexpected src/ dir exists: %s" % src
     env.assert_no_temp()
 
+def test_no_clean_option_blocks_cleaning():
+    """
+    Test --no-clean option blocks cleaning.
+    """
+    env = reset_env()
+    result = run_pip('install', '--no-clean', '--no-index', '--find-links=%s' % find_links, 'simple')
+    build = env.venv_path/'build'/'simple'
+    assert exists(build), "build/simple should still exist %s" % str(result)
 
 def test_cleanup_after_install_editable_from_hg():
     """
@@ -135,3 +144,25 @@ def test_download_should_not_delete_existing_build_dir():
     assert os.path.exists(env.venv_path/'build'), "build/ should be left if it exists before pip run"
     assert content == 'I am not empty!', "it should not affect build/ and its content"
     assert ['somefile.txt'] == os.listdir(env.venv_path/'build')
+
+def test_cleanup_after_install_exception():
+    """
+    Test clean up after a 'setup.py install' exception.
+    """
+    env = reset_env()
+    #broken==0.2broken fails during install; see packages readme file
+    result = run_pip('install', '-f', find_links, '--no-index', 'broken==0.2broken', expect_error=True)
+    build = env.venv_path/'build'
+    assert not exists(build), "build/ dir still exists: %s" % result.stdout
+    env.assert_no_temp()
+
+def test_cleanup_after_egg_info_exception():
+    """
+    Test clean up after a 'setup.py egg_info' exception.
+    """
+    env = reset_env()
+    #brokenegginfo fails during egg_info; see packages readme file
+    result = run_pip('install', '-f', find_links, '--no-index', 'brokenegginfo==0.1', expect_error=True)
+    build = env.venv_path/'build'
+    assert not exists(build), "build/ dir still exists: %s" % result.stdout
+    env.assert_no_temp()

tests/test_cleanup.py

@@ -0,0 +1,45 @@
+import os
+import tempfile
+import shutil
+
+from pip.exceptions import PreviousBuildDirError
+from pip.index import PackageFinder
+from pip.req import InstallRequirement, RequirementSet
+from tests.test_pip import here, path_to_url, assert_raises_regexp
+
+find_links = path_to_url(os.path.join(here, 'packages'))
+
+class TestRequirementSet(object):
+    """RequirementSet tests"""
+
+    def setup(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def teardown(self):
+        shutil.rmtree(self.tempdir, ignore_errors=True)
+
+    def basic_reqset(self):
+        return RequirementSet(
+            build_dir=os.path.join(self.tempdir, 'build'),
+            src_dir=os.path.join(self.tempdir, 'src'),
+            download_dir=None,
+            download_cache=os.path.join(self.tempdir, 'download_cache'),
+            )
+
+    def test_no_reuse_existing_build_dir(self):
+        """Test prepare_files raise exception with previous build dir"""
+
+        build_dir = os.path.join(self.tempdir, 'build', 'simple')
+        os.makedirs(build_dir)
+        open(os.path.join(build_dir, "setup.py"), 'w')
+        reqset = self.basic_reqset()
+        req = InstallRequirement.from_line('simple')
+        reqset.add_requirement(req)
+        finder = PackageFinder([find_links], [])
+        assert_raises_regexp(
+            PreviousBuildDirError,
+            "pip can't install[\s\S]*%s[\s\S]*%s" % (req, build_dir),
+            reqset.prepare_files,
+            finder
+            )
+

tests/test_req.py

@@ -206,8 +206,8 @@ def test_install_user_in_global_virtualenv_with_conflict_fails(self):
         result2 = run_pip('install', '--user', 'INITools==0.1', expect_error=True)
         resultp = env.run('python', '-c', "import pkg_resources; print(pkg_resources.get_distribution('initools').location)")
         dist_location = resultp.stdout.strip()
-        assert result2.stdout.startswith("Will not install to the user site because it will lack sys.path precedence to %s in %s"
-                                        %('INITools', dist_location)), result2.stdout
+        assert "Will not install to the user site because it will lack sys.path precedence to %s in %s" \
+            % ('INITools', dist_location) in result2.stdout, result2.stdout
 
 
     def test_uninstall_from_usersite(self):