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

Regression: sdists no longer install package data correctly with 35.0.0 #1016

Closed
PiDelport opened this issue Apr 18, 2017 · 8 comments
Closed

Regression: sdists no longer install package data correctly with 35.0.0 #1016

PiDelport opened this issue Apr 18, 2017 · 8 comments

Comments

@PiDelport
Copy link
Contributor

@PiDelport PiDelport commented Apr 18, 2017

Problem description

I have a package that uses setuptools_scm and include_package_data=True to include some Django templates and static files.

With setuptools 34.4.1, installing an sdist will install these package data files without any problems. However, with setuptools 35.0.0, installing the same sdist file will no longer install these package data files.

Note:

  • Only installing sdists seem to be affected by this problem. In particular:
    • Generating sdists seems unaffected: both setuptools 34 and 35 generate sdists with the same (correct) distribution content and SOURCES.txt.
    • Installing sdists previously generated with either setuptools version works correctly with 34.4.1, and fails with 35.0.0.
  • Wheels seem to be unaffected by this, and build and install fine with both setuptools 34 and 35.

Cause?

As far as I can tell, this regression is due to #436 / #1014, which no longer reads the sdist package's existing manifest. When the manifest gets regenerated (?), the files that were previously included by setuptools_scm while building the sdist are no longer included anymore.

@PiDelport
Copy link
Contributor Author

@PiDelport PiDelport commented Apr 18, 2017

Here's a reproducing test script using cartridge-downloads, which uses setuptools_scm and include_package_data=True to include some templates:

PACKAGE=cartridge-downloads  # Change this to test other packages

pip install 'setuptools==34.4.1'
pip install --no-binary "$PACKAGE" "$PACKAGE"
pip show -f "$PACKAGE" >good-info

pip uninstall "$PACKAGE"

pip install 'setuptools==35.0.0'
pip install --no-binary "$PACKAGE" "$PACKAGE"
pip show -f "$PACKAGE" >bad-info

diff -u good-info bad-info

(Note that this uses pip install --no-binary to install the sdist rather than the wheel.)

Diff output:

--- good-info	2017-04-18 16:25:18.364959212 +0200
+++ bad-info	2017-04-18 16:25:32.172929357 +0200
@@ -37,12 +37,6 @@
   cartridge_downloads/models.py
   cartridge_downloads/page_processors.py
   cartridge_downloads/signals.py
-  cartridge_downloads/templates/email/form_response.html
-  cartridge_downloads/templates/email/form_response.txt
-  cartridge_downloads/templates/email/order_receipt.html
-  cartridge_downloads/templates/email/order_receipt.txt
-  cartridge_downloads/templates/shop/complete.html
-  cartridge_downloads/templates/shop/downloads/index.html
   cartridge_downloads/templatetags/__init__.py
   cartridge_downloads/templatetags/__pycache__/__init__.cpython-35.pyc
   cartridge_downloads/templatetags/__pycache__/downloads.cpython-35.pyc

As you can see, setuptools 35.0.0 no longer installs the templates directory as package data.

@PiDelport
Copy link
Contributor Author

@PiDelport PiDelport commented Apr 18, 2017

pyuri is another example of an affected package:

+ diff -u good-info bad-info
--- good-info	2017-04-18 16:36:47.098278222 +0200
+++ bad-info	2017-04-18 16:36:53.994243434 +0200
@@ -19,5 +19,4 @@
   pyuri/__pycache__/uri.cpython-35.pyc
   pyuri/__pycache__/validators.cpython-35.pyc
   pyuri/uri.py
-  pyuri/uri.regex
   pyuri/validators.py
@miccoli
Copy link
Contributor

@miccoli miccoli commented Apr 18, 2017

I'm convinced that my PR #1014 is the culprit, see my comments to #436 .

@jaraco
Copy link
Member

@jaraco jaraco commented Apr 18, 2017

Well, now we know. The quick fix is to revert the change. The better fix would be to also write a test to capture this expectation.

PiDelport added a commit to PiDelport/setuptools that referenced this issue Apr 18, 2017
This test is also a regression test for issue pypa#1016.
@PiDelport
Copy link
Contributor Author

@PiDelport PiDelport commented Apr 18, 2017

Okay, I created an integration test case using pyuri that seems to reliably test for this:

This test case passes on 34, and fails on 35.

If it looks good, I can PR that as a starting point for fixing this issue?

PiDelport added a commit to PiDelport/setuptools that referenced this issue Apr 18, 2017
This test is also a regression test for issue pypa#1016.
@miccoli
Copy link
Contributor

@miccoli miccoli commented Apr 18, 2017

I think that at this point it is better to revert #1014 starting from @pjdelport test case, reopening #436 . Sorry for the regression: when testing #1014 I've completely overlooked the implications beyond sdist creation.

@jaraco
Copy link
Member

@jaraco jaraco commented Apr 18, 2017

I can PR that as a starting point.

I went ahead and pulled in the commit. Thanks for putting it together and presenting it in such an accessible way.

jaraco added a commit that referenced this issue Apr 18, 2017
jaraco added a commit to cherrypy/cherrypy that referenced this issue Apr 18, 2017
This reverts commit edb6f63.

This change was made in reaction to tests failing due to
pypa/setuptools#1016. Better to wait for that issue to be
resolved than to create new duplication in declaration of
package files to later become stale.
@PiDelport
Copy link
Contributor Author

@PiDelport PiDelport commented Apr 19, 2017

Thanks @jaraco!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants