classification
Title: distutils sdist ignores MANIFEST
Type: behavior Stage: resolved
Components: Distutils Versions: Python 3.2, Python 3.3, Python 2.7, 3rd party
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: dmalcolm, eric.araujo, jdennis, jerub, python-dev, tarek
Priority: high Keywords: patch

Created on 2011-02-03 00:05 by jdennis, last changed 2012-02-03 16:30 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
sdist.patch jdennis, 2011-02-03 00:44 Suggested patch review
sdist_new jdennis, 2011-02-03 00:45 result of appying suggested patch
distutils_bug.tar.gz jdennis, 2011-02-04 01:51 reproducer
manifest-respect.patch jerub, 2011-06-04 10:19 python2.7 patch
manifest-respect-3.patch jerub, 2011-06-04 11:22 python3.1+default patch review
manifest-respect-3 jerub, 2011-06-24 13:14 python3.1+default patch review
manifest-respect-3 jerub, 2011-06-24 23:30 Updated python3.1+default patch review
manifest-respect-3 jerub, 2011-06-25 13:54 python3.1 patch with documentation changes review
Messages (23)
msg127777 - (view) Author: John Dennis (jdennis) Date: 2011-02-03 00:05
The behaviour of sdist has changed dramatically in Python 2.7. Some projects prefer to maintain their own manifest file instead of utilizing automatic manifest generation from a template. The defined behaviour of sdist is to check for the presence of both a template and a manifest, if the template is absent but a manifest exists sdist is supposed to read the existing manifest, it no longer does this. Instead it creates a default file list with the catastrophic result of omitting the bulk of a projects files.  

It appears this bug was introduced in r81255 and is discussed in #8688. Unfortunately #8688 contained a number of different issues and was closed addressing only a subset of the problems. Changeset r83996 was introduced to prevent sdist from overwriting a project maintained manifest by testing for a comment at the head of the manifest file. It's not clear to me this was necessary because the write_manifest() should never have been called if the template was absent but a manifest existed.

Even after the application of changeset r83996 one of the fundamental problems in #8688 remained, the manifest is not read. The solution is to check for both the manifest and template (as was formerly done) and if the template is absent but the manifest exists then the manifest should be read.

I have made modifications to get_file_list() to reintroduce the defined behaviour. With the introduction of r83996 it is now legal syntax to have comments in the manifest however read_manifest() was not enhanced to account for the possible presence of comments. I also modified read_manifest() to handle comments.

These suggested fixes are attached as a patch against the current 2.7 maintenance branch. I've also attached a file with the two modified methods because sometimes it's difficult to comprehend a patch.
msg127847 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-04 01:04
This behavior change is actually present in 2.7.1, 3.1.3 and 3.2.  I’m concerned that you’ve found a bug with it (thanks for the report), and that we may have to change behavior again.

First, could you open another report about comment handling in read_manifest?  I overlooked that, and it should be fixed as quickly as possible, although there’s very little change this will make it into 3.2.0 (we’ll have 3.2.1 though).

Second, can you give us a test script (shell or Python) or a set of instructions we can reproduce to observe the behavior you say is defective?  Thanks in advance.
msg127857 - (view) Author: John Dennis (jdennis) Date: 2011-02-04 01:51
$ tar xzf distutils_bug.tar.gz
$ cd distutils_bug
$ ./setup.py sdist

$ ./setup.py sdist
running sdist
running check
warning: sdist: manifest template 'MANIFEST.in' does not exist (using default file list)

not writing to manually maintained manifest file 'MANIFEST'
creating foobar-1.0
making hard links in foobar-1.0...
hard linking README -> foobar-1.0
hard linking setup.py -> foobar-1.0
creating dist
Creating tar archive
removing 'foobar-1.0' (and everything under it)

$ cat MANIFEST 
README
setup.py
my_special_file

Note, the MANIFEST should have been read and the resulting tar file should have had my_special_file in it. sdist complains about a missing MANIFEST.in template which it shouldn't, it should use the existing MANIFEST, it also emits a warning about not overwriting a manually maintained MANIFEST which it shouldn't. It should just use the existing MANIFEST.
msg127867 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-04 03:51
Thank you, the bug is clear now.  To fix this regression, the first step is to turn your tarball and instructions into a unit test and then fix the logic in the code.  If you want to do it, there are some process guidelines at http://wiki.python.org/moin/Distutils/FixingBugs
msg127895 - (view) Author: John Dennis (jdennis) Date: 2011-02-04 14:47
No, I don't think I'm going to turn the tarball into a unit test, etc. I didn't break the code but I did report the problem, researched the history, provided a clear explanation, a reproducer and a patch to fix the problem. I think I've done my part as a community member.
msg127937 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-02-04 20:21
I didn’t mean to imply you had to, I just asked if you wanted to.  Reporting the bug is a valuable contribution indeed, I’ll take over for the rest.
msg137632 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-04 10:19
I've taken the sdist.patch and wrote some tests for it. The resulting patch is attached as 'manifest-respect.patch'.
msg137633 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-04 11:22
This patch is tested against the 3.1 and default branches, the previous patch attached was against the 2.7 branch.
msg138891 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-24 08:20
I have 2 patches, with tests, that applies on python2.7 and the python3 series of branches, attached this ticket. I have also got a signed contributor agreement lodged with the PSF.

Can I please have someone either apply my patches or tell me what I need to do in order to change them if they are being rejected.
msg138901 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-24 10:44
I have outstanding comments and questions on the review page (follow the review link on the right of the 3.x patch).
msg138903 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-24 10:50
Oh! I didn't see any notification that there was a review done. Thanks, I'll attend to that.
msg138941 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-24 13:14
This patch is an updated patch that fixes the things noted in the review from eric.araujo.
msg139002 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-24 23:30
Updated the patch to address the 'why not use .strip()' question. I used .rstrip('\r\n') on the basis that filenames may have leading or trailing spaces, and if you need that, you need to be able to specify that in a MANIFEST, but it is perfectly logical to disallow them, so here's a patch that doesn't support them.

It also reduces the line count by 2 because I'm composing the 'comment' and 'blank line' cases.
msg139013 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-25 00:52
Thanks for your work, the code can now be fixed; I’ll have time to commit in a few days.

Can you check if the documentation still accurately describes the behavior?  (search for MANIFEST in Doc/distutils/*)
msg139065 - (view) Author: Stephen Thorne (jerub) * Date: 2011-06-25 13:54
Éric mentioned that i should check that this behaviour matches the documentation. I have gone and looked for all instances of MANIFEST in the documentation and found one place which was inconsistent. I've added the doc patch to the patch. Please review this new version.
msg139114 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-25 20:13
Thanks for your work.  I will make a final test and review and commit it in the coming days.  I’ll adapt the versionchanged text, since I won’t commit this in 3.1 (in security mode now, not bugfix mode).
msg141495 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-01 12:45
New changeset 5d3e22d69d4f by Éric Araujo in branch '3.2':
Fix regression with distutils MANIFEST handing (#11104, #8688).
http://hg.python.org/cpython/rev/5d3e22d69d4f
msg141499 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-01 12:49
New changeset 21feea7f35e5 by Éric Araujo in branch '2.7':
Fix regression with distutils MANIFEST handing (#11104, #8688).
http://hg.python.org/cpython/rev/21feea7f35e5
msg141502 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-01 12:57
I did some more work on the patch and committed.  Thanks again to both of you.

I also thought again about this remark:

> Changeset r83996 was introduced to prevent sdist from overwriting a
> project maintained manifest by testing for a comment at the head of
> the manifest file. It's not clear to me this was necessary because the
> write_manifest() should never have been called if the template was
> absent but a manifest existed.

I also wonder if that was necessary; I did not fully understand the bug at the time.  Unfortunately, now that we have a handful of releases out there with this change, we have to support the magic comment.  For distutils2 however (a.k.a. “packaging” module in 3.3), I’ll open another bug report and if the same bugs apply (MANIFEST handling is a bit different there), I’ll try to fix the bug without using a magic comment.
msg152534 - (view) Author: John Dennis (jdennis) Date: 2012-02-03 16:21
The changesets are not in the release27-maint branch. sdist still does not generate a correct archive for release, this is a very serious regression.
msg152535 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-03 16:25
> The changesets are not in the release27-maint branch.
Where did you look?  This looks like a Subversion branch name, but now we’re using Mercurial.  If you look a few messages up, you’ll see that a changeset was committed to the 2.7 branch and will be included in 2.7.3.
msg152536 - (view) Author: Stephen Thorne (jerub) * Date: 2012-02-03 16:26
Yep - 2.7.2 was released 11th June 2011, the fix was committed Aug 1st 2011. So it won't be in the current 2.7 release.
msg152537 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-03 16:30
Obviously I can’t fix past releases.  Sometimes the time machine is not available :)
History
Date User Action Args
2012-02-03 16:30:52eric.araujosetstatus: open -> closed
resolution: fixed
messages: + msg152537
2012-02-03 16:26:22jerubsetmessages: + msg152536
2012-02-03 16:25:31eric.araujosetmessages: + msg152535
2012-02-03 16:21:01jdennissetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg152534
2011-08-01 12:57:21eric.araujosetstatus: open -> closed
messages: + msg141502

components: - Distutils2
resolution: fixed
stage: commit review -> resolved
2011-08-01 12:49:57python-devsetmessages: + msg141499
2011-08-01 12:45:24python-devsetnosy: + python-dev
messages: + msg141495
2011-06-25 20:13:07eric.araujosetpriority: normal -> high

messages: + msg139114
2011-06-25 13:54:23jerubsetfiles: + manifest-respect-3

messages: + msg139065
2011-06-25 00:52:55eric.araujosetstage: commit review
messages: + msg139013
versions: + Python 3.3, - Python 3.1
2011-06-24 23:30:57jerubsetfiles: + manifest-respect-3

messages: + msg139002
2011-06-24 13:14:25jerubsetfiles: + manifest-respect-3

messages: + msg138941
2011-06-24 10:50:56jerubsetmessages: + msg138903
2011-06-24 10:44:17eric.araujosetmessages: + msg138901
2011-06-24 08:20:32jerubsetmessages: + msg138891
2011-06-04 11:22:39jerubsetfiles: + manifest-respect-3.patch

messages: + msg137633
2011-06-04 10:19:24jerubsetfiles: + manifest-respect.patch
nosy: + jerub
messages: + msg137632

2011-02-07 16:59:39jdennissetnosy: + dmalcolm
2011-02-04 20:21:15eric.araujosetnosy: tarek, eric.araujo, jdennis
messages: + msg127937
2011-02-04 14:47:01jdennissetnosy: tarek, eric.araujo, jdennis
messages: + msg127895
2011-02-04 03:51:48eric.araujosetassignee: tarek -> eric.araujo
messages: + msg127867
nosy: tarek, eric.araujo, jdennis
2011-02-04 01:51:15jdennissetfiles: + distutils_bug.tar.gz
nosy: tarek, eric.araujo, jdennis
messages: + msg127857
2011-02-04 01:04:09eric.araujosetnosy: tarek, eric.araujo, jdennis
messages: + msg127847
components: + Distutils2
versions: + 3rd party, Python 3.1, Python 3.2
2011-02-03 00:45:17jdennissetfiles: + sdist_new
nosy: tarek, eric.araujo, jdennis
2011-02-03 00:44:40jdennissetfiles: + sdist.patch
nosy: tarek, eric.araujo, jdennis
2011-02-03 00:44:03jdennissetfiles: - sdist_new
nosy: tarek, eric.araujo, jdennis
2011-02-03 00:43:57jdennissetfiles: - sdist.patch
nosy: tarek, eric.araujo, jdennis
2011-02-03 00:08:20jdennissetfiles: + sdist_new
nosy: tarek, eric.araujo, jdennis
2011-02-03 00:06:49jdennissetfiles: + sdist.patch
nosy: tarek, eric.araujo, jdennis
keywords: + patch
2011-02-03 00:05:43jdenniscreate