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

addresses #436: Updating MANIFEST.in does not correctly update the package sdist creates #1014

Merged
merged 1 commit into from Apr 15, 2017

Conversation

Projects
None yet
5 participants
@miccoli
Contributor

miccoli commented Apr 13, 2017

The egg_info command reads an existing .egg-info/SOURCES.txt before writing the new one. Therefore state is preserved, and files once added to SOURCES.txt (e.g. by an include line in MANIFEST.in) will remain forever in SOURCES.txt even if there is no more reason for inclusion.

The problem described in #436 is due to the fact that .egg-info/SOURCES.txt is used for building the list of files in a source distribution (sdist command).

This PR is surgical, in the sense that solves the issue by removing 2 lines of code. However after this modification the setuptools.command.sdist.sdist.read_manifest method is not called anymore within setuptools and its removal could be considered, along with the relevant unit tests.

@miccoli miccoli changed the title from addresses #436 to addresses #436: Updating MANIFEST.in does not correctly update the package sdist creates Apr 13, 2017

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 15, 2017

I can see you mentioned in this ticket that you don't understand why this logic was authored in the first place. I don't know the reason either.

Perhaps it was a premature optimization. Perhaps it was a mistake.

Since there are no tests capturing the use-case that would have protected this functionality and because the functionality appears to only have detrimental effects, I'm inclined to accept the change and see if any adverse effects are reported, at which point we can distill a use-case and write a unit test to capture that expectation.

@jaraco jaraco merged commit 3ac5bbe into pypa:master Apr 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jaraco

This comment has been minimized.

Member

jaraco commented Apr 15, 2017

On further consideration, I think I've imagined a scenario where this functionality might have been expected - if a project were for whatever reason manually populating that file or using a third-party tool or other command to populate that file, then this change will cause those entries to be dropped. For that reason, I think this change is backward-incompatible and I'll release it as such.

jaraco added a commit that referenced this pull request Apr 15, 2017

@miccoli miccoli deleted the miccoli:issue-436 branch Apr 17, 2017

@miccoli

This comment has been minimized.

Contributor

miccoli commented Apr 17, 2017

After reviewing the legacy distutils, I would guess that it was an attempt to maintain compatibility with the distutils's MANIFEST logic:

  • if the manifest file (MANIFEST by default) exists and the first line does not have a comment indicating it is generated from MANIFEST.in, then it is used as is, unaltered

However this makes sense in distutils, where the MANIFEST is either automatically generated or authored, and not in setuptools where .egg-info/SOURCES.txt is always generated.

Thanks for accepting the change, hoping not to break to much existing projects.

@voidlily

This comment has been minimized.

voidlily commented Apr 17, 2017

I found a project that broke with this change: https://bitbucket.org/vangheem/pyzipcode/src

This package installs properly before 35.0.0 and no longer does. Test script:

from pyzipcode import ZipCodeDatabase
z = ZipCodeDatabase()
z[94103]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ubuntu/virtualenvs/venv-2.7.9/lib/python2.7/site-packages/pyzipcode/__init__.py", line 101, in __getitem__
    zip = self.get(str(zip))
  File "/home/ubuntu/virtualenvs/venv-2.7.9/lib/python2.7/site-packages/pyzipcode/__init__.py", line 98, in get
    return format_result(self.conn_manager.query(ZIP_QUERY % zip))
  File "/home/ubuntu/virtualenvs/venv-2.7.9/lib/python2.7/site-packages/pyzipcode/__init__.py", line 32, in query
    cursor.execute(sql)
pysqlite2.dbapi2.OperationalError: no such table: ZipCodes

The file zipcode.db is missing in the installed package in 35.0.0 due to the MANIFEST changes (though the package could also be packaged improperly and lacking a MANIFEST as well, not sure what the exact intention with this change is).

@miccoli

This comment has been minimized.

Contributor

miccoli commented Apr 18, 2017

@voidlily From my tests there is no regression from setuptools==34.4.1: starting from a fresh clone of pyzipcode, the data file pyzipcode/zipcodes.db is not installed:

$ hg clone https://bitbucket.org/vangheem/pyzipcode
$ pip install setuptools==34.4.1
$ pip install ./pyzipcode

To have pyzipcode/zipcodes.db properly included you can try installing setuptools_scm which will automagically include all files under version control in the manifest file (which is .egg-info/SOURCES.txt) and therefore in the installed package.

update

Actually there is a regression when installing from a sdist file: see #1016 , sorry for the wrong assumption of an install form the develop tree.

@pjdelport

This comment has been minimized.

Contributor

pjdelport commented Apr 18, 2017

I think this change may have broken sdist installs with package data: #1016

@haydensun

This comment has been minimized.

haydensun commented Jun 15, 2017

I agree with @miccoli about the behavior of MAINFEST‘s logic. Whenever updating MAINFEST.in, the SOURCES.txt in .egg-info needs to be updated consistently. At present, delete the .egg-info directory before sdist may be a feasible solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment