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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-02-03 16:30 |
Obviously I can’t fix past releases. Sometimes the time machine is not available :)
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:12 | admin | set | github: 55313 |
2012-02-03 16:30:52 | eric.araujo | set | status: open -> closed resolution: fixed messages:
+ msg152537
|
2012-02-03 16:26:22 | jerub | set | messages:
+ msg152536 |
2012-02-03 16:25:31 | eric.araujo | set | messages:
+ msg152535 |
2012-02-03 16:21:01 | jdennis | set | status: closed -> open resolution: fixed -> (no value) messages:
+ msg152534
|
2011-08-01 12:57:21 | eric.araujo | set | status: open -> closed messages:
+ msg141502
components:
- Distutils2 resolution: fixed stage: commit review -> resolved |
2011-08-01 12:49:57 | python-dev | set | messages:
+ msg141499 |
2011-08-01 12:45:24 | python-dev | set | nosy:
+ python-dev messages:
+ msg141495
|
2011-06-25 20:13:07 | eric.araujo | set | priority: normal -> high
messages:
+ msg139114 |
2011-06-25 13:54:23 | jerub | set | files:
+ manifest-respect-3
messages:
+ msg139065 |
2011-06-25 00:52:55 | eric.araujo | set | stage: commit review messages:
+ msg139013 versions:
+ Python 3.3, - Python 3.1 |
2011-06-24 23:30:57 | jerub | set | files:
+ manifest-respect-3
messages:
+ msg139002 |
2011-06-24 13:14:25 | jerub | set | files:
+ manifest-respect-3
messages:
+ msg138941 |
2011-06-24 10:50:56 | jerub | set | messages:
+ msg138903 |
2011-06-24 10:44:17 | eric.araujo | set | messages:
+ msg138901 |
2011-06-24 08:20:32 | jerub | set | messages:
+ msg138891 |
2011-06-04 11:22:39 | jerub | set | files:
+ manifest-respect-3.patch
messages:
+ msg137633 |
2011-06-04 10:19:24 | jerub | set | files:
+ manifest-respect.patch nosy:
+ jerub messages:
+ msg137632
|
2011-02-07 16:59:39 | jdennis | set | nosy:
+ dmalcolm
|
2011-02-04 20:21:15 | eric.araujo | set | nosy:
tarek, eric.araujo, jdennis messages:
+ msg127937 |
2011-02-04 14:47:01 | jdennis | set | nosy:
tarek, eric.araujo, jdennis messages:
+ msg127895 |
2011-02-04 03:51:48 | eric.araujo | set | assignee: tarek -> eric.araujo messages:
+ msg127867 nosy:
tarek, eric.araujo, jdennis |
2011-02-04 01:51:15 | jdennis | set | files:
+ distutils_bug.tar.gz nosy:
tarek, eric.araujo, jdennis messages:
+ msg127857
|
2011-02-04 01:04:09 | eric.araujo | set | nosy:
tarek, eric.araujo, jdennis messages:
+ msg127847 components:
+ Distutils2 versions:
+ 3rd party, Python 3.1, Python 3.2 |
2011-02-03 00:45:17 | jdennis | set | files:
+ sdist_new nosy:
tarek, eric.araujo, jdennis |
2011-02-03 00:44:40 | jdennis | set | files:
+ sdist.patch nosy:
tarek, eric.araujo, jdennis |
2011-02-03 00:44:03 | jdennis | set | files:
- sdist_new nosy:
tarek, eric.araujo, jdennis |
2011-02-03 00:43:57 | jdennis | set | files:
- sdist.patch nosy:
tarek, eric.araujo, jdennis |
2011-02-03 00:08:20 | jdennis | set | files:
+ sdist_new nosy:
tarek, eric.araujo, jdennis |
2011-02-03 00:06:49 | jdennis | set | files:
+ sdist.patch nosy:
tarek, eric.araujo, jdennis keywords:
+ patch |
2011-02-03 00:05:43 | jdennis | create | |