Skip to content
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

`build_meta.build_sdist` should work if the destination directory already contains a tar.gz #1749

Closed
benoit-pierre opened this issue Apr 22, 2019 · 6 comments

Comments

@benoit-pierre
Copy link
Member

commented Apr 22, 2019

The issue is similar to #1671, see #1745 for how the issue can be resolved.

@pganssle

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

@benoit-pierre Do you have a reproducer? I tried using my original reproducer but using python -m pep517.build --source in place of pip wheel, and it had no problem creating two sdists. Not entirely sure why it wasn't a problem, though.

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

With an updated version? I suspect it works fine when re-creating the exact same source dist.

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Also, I don't know how pep517.build is implemented.

@pganssle

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

With an updated version? I suspect it works fine when re-creating the exact same source dist.

Yeah:

$ ls /tmp/demo_dist_517/dist
foo-0.0.1.tar.gz  foo-0.0.2.tar.gz

Also, I don't know how pep517.build is implemented.

Yeah. I suppose the best reproducer would be an xfail-ing test like the one from #1726 that hits this bug in our minimal PEP 517 build frontend. For now, the only PEP 517 frontends that I know of that build an sdist are tox and pep517.build. If for some reason neither of those hit it that makes this a less important bug to fix, but if it's possible for a valid PEP 517 frontend to hit it, then we should fix it anyway.

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

This fail as expected for the sdist case:

 setuptools/tests/test_build_meta.py | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git i/setuptools/tests/test_build_meta.py w/setuptools/tests/test_build_meta.py
index 7612ebd7..38886268 100644
--- i/setuptools/tests/test_build_meta.py
+++ w/setuptools/tests/test_build_meta.py
@@ -157,9 +157,10 @@ def test_build_wheel(self, build_backend):
 
         assert os.path.isfile(os.path.join(dist_dir, wheel_name))
 
-    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
+    @pytest.mark.parametrize('build_type', ('wheel', 'sdist'))
+    def test_build_with_existing_file_present(self, build_type, tmpdir_cwd):
+        # Building a sdist/wheel should still succeed if there's
+        # already a sdist/wheel in the destination directory.
         files = {
             'setup.py': "from setuptools import setup\nsetup()",
             'VERSION': "0.0.1",
@@ -177,28 +178,30 @@ def test_build_wheel_with_existing_wheel_file_present(self, tmpdir_cwd):
 
         build_files(files)
 
-        dist_dir = os.path.abspath('pip-wheel-preexisting')
+        dist_dir = os.path.abspath('preexisting-' + build_type)
         os.makedirs(dist_dir)
 
-        # make first wheel
         build_backend = self.get_build_backend()
-        wheel_one = build_backend.build_wheel(dist_dir)
+        build_method = getattr(build_backend, 'build_' + build_type)
+
+        # build a first sdist/wheel
+        first_result = build_method(dist_dir)
 
         # change version
         with open("VERSION", "wt") as version_file:
             version_file.write("0.0.2")
 
-        # make *second* wheel
-        wheel_two = self.get_build_backend().build_wheel(dist_dir)
+        # build a *second* sdist/wheel
+        second_result = build_method(dist_dir)
 
-        assert os.path.isfile(os.path.join(dist_dir, wheel_one))
-        assert wheel_one != wheel_two
+        assert os.path.isfile(os.path.join(dist_dir, first_result))
+        assert first_result != second_result
 
-        # 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
+        # and if rebuilding the exact same sdist/wheel?
+        open(os.path.join(dist_dir, second_result), 'w').close()
+        third_result = build_method(dist_dir)
+        assert third_result == second_result
+        assert os.path.getsize(os.path.join(dist_dir, third_result)) > 0
 
     def test_build_sdist(self, build_backend):
         dist_dir = os.path.abspath('pip-sdist')
@webknjaz

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@pganssle it looks like this should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.