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

distutils sdist ignores MANIFEST #55313

Closed
jdennis mannequin opened this issue Feb 3, 2011 · 23 comments
Closed

distutils sdist ignores MANIFEST #55313

jdennis mannequin opened this issue Feb 3, 2011 · 23 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jdennis
Copy link
Mannequin

jdennis mannequin commented Feb 3, 2011

BPO 11104
Nosy @tarekziade, @merwok, @davidmalcolm
Files
  • sdist.patch: Suggested patch
  • sdist_new: result of appying suggested patch
  • distutils_bug.tar.gz: reproducer
  • manifest-respect.patch: python2.7 patch
  • manifest-respect-3.patch: python3.1+default patch
  • manifest-respect-3: python3.1+default patch
  • manifest-respect-3: Updated python3.1+default patch
  • manifest-respect-3: python3.1 patch with documentation changes
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/merwok'
    closed_at = <Date 2012-02-03.16:30:52.388>
    created_at = <Date 2011-02-03.00:05:43.982>
    labels = ['type-bug', 'library']
    title = 'distutils sdist ignores MANIFEST'
    updated_at = <Date 2012-02-03.16:30:52.387>
    user = 'https://bugs.python.org/jdennis'

    bugs.python.org fields:

    activity = <Date 2012-02-03.16:30:52.387>
    actor = 'eric.araujo'
    assignee = 'eric.araujo'
    closed = True
    closed_date = <Date 2012-02-03.16:30:52.388>
    closer = 'eric.araujo'
    components = ['Distutils']
    creation = <Date 2011-02-03.00:05:43.982>
    creator = 'jdennis'
    dependencies = []
    files = ['20658', '20659', '20672', '22242', '22243', '22437', '22449', '22463']
    hgrepos = []
    issue_num = 11104
    keywords = ['patch']
    message_count = 23.0
    messages = ['127777', '127847', '127857', '127867', '127895', '127937', '137632', '137633', '138891', '138901', '138903', '138941', '139002', '139013', '139065', '139114', '141495', '141499', '141502', '152534', '152535', '152536', '152537']
    nosy_count = 6.0
    nosy_names = ['jerub', 'tarek', 'eric.araujo', 'dmalcolm', 'jdennis', 'python-dev']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue11104'
    versions = ['3rd party', 'Python 2.7', 'Python 3.2', 'Python 3.3']

    @jdennis
    Copy link
    Mannequin Author

    jdennis mannequin commented Feb 3, 2011

    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 bpo-8688. Unfortunately bpo-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 bpo-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.

    @jdennis jdennis mannequin assigned tarekziade Feb 3, 2011
    @jdennis jdennis mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 3, 2011
    @merwok
    Copy link
    Member

    merwok commented Feb 4, 2011

    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.

    @jdennis
    Copy link
    Mannequin Author

    jdennis mannequin commented Feb 4, 2011

    $ 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.

    @merwok
    Copy link
    Member

    merwok commented Feb 4, 2011

    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

    @merwok merwok assigned merwok and unassigned tarekziade Feb 4, 2011
    @jdennis
    Copy link
    Mannequin Author

    jdennis mannequin commented Feb 4, 2011

    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.

    @merwok
    Copy link
    Member

    merwok commented Feb 4, 2011

    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.

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 4, 2011

    I've taken the sdist.patch and wrote some tests for it. The resulting patch is attached as 'manifest-respect.patch'.

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 4, 2011

    This patch is tested against the 3.1 and default branches, the previous patch attached was against the 2.7 branch.

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 24, 2011

    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.

    @merwok
    Copy link
    Member

    merwok commented Jun 24, 2011

    I have outstanding comments and questions on the review page (follow the review link on the right of the 3.x patch).

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 24, 2011

    Oh! I didn't see any notification that there was a review done. Thanks, I'll attend to that.

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 24, 2011

    This patch is an updated patch that fixes the things noted in the review from eric.araujo.

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 24, 2011

    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.

    @merwok
    Copy link
    Member

    merwok commented Jun 25, 2011

    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/*)

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Jun 25, 2011

    É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.

    @merwok
    Copy link
    Member

    merwok commented Jun 25, 2011

    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).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 1, 2011

    New changeset 5d3e22d69d4f by Éric Araujo in branch '3.2':
    Fix regression with distutils MANIFEST handing (bpo-11104, bpo-8688).
    http://hg.python.org/cpython/rev/5d3e22d69d4f

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 1, 2011

    New changeset 21feea7f35e5 by Éric Araujo in branch '2.7':
    Fix regression with distutils MANIFEST handing (bpo-11104, bpo-8688).
    http://hg.python.org/cpython/rev/21feea7f35e5

    @merwok
    Copy link
    Member

    merwok commented Aug 1, 2011

    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.

    @merwok merwok closed this as completed Aug 1, 2011
    @jdennis
    Copy link
    Mannequin Author

    jdennis mannequin commented Feb 3, 2012

    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.

    @jdennis jdennis mannequin reopened this Feb 3, 2012
    @merwok
    Copy link
    Member

    merwok commented Feb 3, 2012

    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.

    @jerub
    Copy link
    Mannequin

    jerub mannequin commented Feb 3, 2012

    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.

    @merwok
    Copy link
    Member

    merwok commented Feb 3, 2012

    Obviously I can’t fix past releases. Sometimes the time machine is not available :)

    @merwok merwok closed this as completed Feb 3, 2012
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant