Fix error with PEP 517 builds when wheel exists (GH #1761)

[shashanksingh28]

Summary of changes

build_meta.build_wheel uses a temporary directory for cases where wheel_directory already has
existing .whl files (so as to not throw unpacking error).

Changes from the patch discussed in the issue:

• pay the cost of creating / cleaning a temp-dir only if necessary
• make it python2.7 compatible

Closes #1671

Pull Request Checklist

• Changes have tests
• News fragment added in changelog.d. See documentation for details

[benoit-pierre]

It would be better to use this for Python 2 compatibility:

 setuptools/py31compat.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git i/setuptools/py31compat.py w/setuptools/py31compat.py
index 1a0705ec..e1da7ee2 100644
--- i/setuptools/py31compat.py
+++ w/setuptools/py31compat.py
@@ -17,9 +17,9 @@ class TemporaryDirectory:
         errors on deletion.
         """
 
-        def __init__(self):
+        def __init__(self, **kwargs):
             self.name = None  # Handle mkdtemp raising an exception
-            self.name = tempfile.mkdtemp()
+            self.name = tempfile.mkdtemp(**kwargs)
 
         def __enter__(self):
             return self.name

So you can use a context manager, ensuring the temporary directory is removed in case of error.

I would also prefer if the code was simplified to always use a temporary directory, as the performance impact should be minimal (since rename can be used).

A more important concern: what if the resulting wheel already exists? The call to os.rename will fail on non-Unix platforms.

Modified test to check for this particular issue:

 setuptools/tests/test_build_meta.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git c/setuptools/tests/test_build_meta.py w/setuptools/tests/test_build_meta.py
index 90400afc..88e29ffe 100644
--- c/setuptools/tests/test_build_meta.py
+++ w/setuptools/tests/test_build_meta.py
@@ -157,7 +157,6 @@ def test_build_wheel(self, build_backend):
 
         assert os.path.isfile(os.path.join(dist_dir, wheel_name))
 
-    @pytest.mark.xfail(reason="Known error, see GH #1671")
     def test_build_wheel_with_existing_wheel_file_present(self, tmpdir_cwd):
         # Building a wheel should still succeed if there's already a wheel
         # in the wheel directory
@@ -195,6 +194,12 @@ def test_build_wheel_with_existing_wheel_file_present(self, tmpdir_cwd):
         assert os.path.isfile(os.path.join(dist_dir, wheel_one))
         assert wheel_one != wheel_two
 
+        # and if rebuilding the same wheel?
+        open(os.path.join(dist_dir, wheel_two), 'w').close()
+        wheel_three = self.get_build_backend().build_wheel(dist_dir)
+        assert wheel_three == wheel_two
+        assert os.path.getsize(os.path.join(dist_dir, wheel_three)) > 0
+
     def test_build_sdist(self, build_backend):
         dist_dir = os.path.abspath('pip-sdist')
         os.makedirs(dist_dir)

And fix in build_meta:

 setuptools/build_meta.py | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git c/setuptools/build_meta.py w/setuptools/build_meta.py
index 47cbcbf6..3472bf8a 100644
--- c/setuptools/build_meta.py
+++ w/setuptools/build_meta.py
@@ -37,6 +37,7 @@
 import distutils
 
 from pkg_resources import parse_requirements
+from setuptools.py31compat import TemporaryDirectory
 
 __all__ = ['get_requires_for_build_sdist',
            'get_requires_for_build_wheel',
@@ -182,14 +183,17 @@ def build_wheel(self, wheel_directory, config_settings=None,
                     metadata_directory=None):
         config_settings = self._fix_config(config_settings)
         wheel_directory = os.path.abspath(wheel_directory)
-        sys.argv = sys.argv[:1] + ['bdist_wheel'] + \
-            config_settings["--global-option"]
-        self.run_setup()
-        if wheel_directory != 'dist':
-            shutil.rmtree(wheel_directory)
-            shutil.copytree('dist', wheel_directory)
-
-        return _file_with_extension(wheel_directory, '.whl')
+        with TemporaryDirectory(dir=wheel_directory) as tmp_dist_dir:
+            sys.argv = sys.argv[:1] + [
+                'bdist_wheel', '--dist-dir', tmp_dist_dir
+            ] + config_settings["--global-option"]
+            self.run_setup()
+            wheel_basename = _file_with_extension(tmp_dist_dir, '.whl')
+            wheel_path = os.path.join(wheel_directory, wheel_basename)
+            if os.path.exists(wheel_path):
+                os.remove(wheel_path)
+            os.rename(os.path.join(tmp_dist_dir, wheel_basename), wheel_path)
+        return wheel_basename
 
     def build_sdist(self, sdist_directory, config_settings=None):
         config_settings = self._fix_config(config_settings)

[pganssle]

@shashanksingh28 I have made a few changes and cleaned up the history a bit, but this looks ready to go to me, assuming @benoit-pierre has no objections.

Just for your future edification, the things I changed:

1. Some of the lines were very long, so I trimmed them to 80 characters. That's not a hard rule of the project, but >100 characters is pretty much out.
2. I reworded the changelog. The changelog is intended to document to end users what has changed between versions. They don't really care about the implementation details like whether a temporary directory is used, they just just care what user-facing behavior was fixed.
3. I prefer to have a nice explanation of the bug in the commit message, and to have atomic commits where the test suite should pass on every commit. I squashed together all the fixup commits.

Also, one last thing to note, it would be better if you worked from a branch rather than working in the master branch of your repository. It makes it harder for maintainers to update your branch, and I've seen it cause many other problems before as well. Once this is merged I recommend either deleting your fork and making a new one or doing a hard reset on your master branch against the upstream master branch, then always making PRs against a feature branch.

Thanks so much for your contribution, it is much appreciated!

@benoit-pierre Looks good? Should we merge?

[benoit-pierre]

LGTM except for some minor tweaks.

[pganssle]

@benoit-pierre Fixed the tweaks, let me know what you think of the new changelog.

Also, for some reason pypy3 is having test failures, not sure if that's related to these changes or not.

[pganssle]

I think this is related to #1730, but it's hard to tell because Travis is timing out. I suspect that either PyPy or something we're doing is preventing some files or processes from being closed on PyPy, and that causes problems with this module in particular, because it opens and a ton of files (possibly hundreds?). We should probably try to actually resolve #1730.

[pganssle]

Thanks @shashanksingh28 and @benoit-pierre and everyone else who contributed to fixing this issue.

[benoit-pierre]

Note that build_sdist should be similarly fixed, not sure about prepare_metadata_for_build_wheel.

[pganssle]

@benoit-pierre Would you mind creating a new issue for build_sdist? That one's probably also a good first issue.

[gaborbernat]

No release since April 22, @pganssle can we do a release from master?

[jaraco]

I'm on it.

[jaraco]

41.1 is going out here.

[benoit-pierre]

@jaraco: the upload to PyPI failed.

[jaraco]

I did notice that. I need to file a ticket for it.

[benoit-pierre]

For which project? I have not tested the PyPI token support yet myself, did not know it existed before I saw your commit.

[jaraco]

For this project. I successfully used the token approach already. I'll describe in the ticket.