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

"python setup.py sdist upload --show-response" can fail with "UnboundLocalError: local variable 'result' referenced before assignment" #54576

Closed
jcea opened this issue Nov 8, 2010 · 24 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jcea
Copy link
Member

jcea commented Nov 8, 2010

BPO 10367
Nosy @jcea, @pjeby, @tarekziade, @merwok, @briancurtin, @berkerpeksag
Superseder
  • bpo-12853: global name 'r' is not defined in upload.py
  • Files
  • 10367.diff: Lib/distutils/command/upload.py
  • 10367.updated.diff: Patch the multiple arguments to self.announce
  • test_10367.diff: The unit test
  • 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 2016-01-20.06:27:46.382>
    created_at = <Date 2010-11-08.21:36:51.074>
    labels = ['easy', 'type-bug', 'library']
    title = '"python setup.py sdist upload --show-response" can fail with "UnboundLocalError: local variable \'result\' referenced before assignment"'
    updated_at = <Date 2016-01-20.06:27:46.371>
    user = 'https://github.com/jcea'

    bugs.python.org fields:

    activity = <Date 2016-01-20.06:27:46.371>
    actor = 'berker.peksag'
    assignee = 'eric.araujo'
    closed = True
    closed_date = <Date 2016-01-20.06:27:46.382>
    closer = 'berker.peksag'
    components = ['Distutils', 'Distutils2']
    creation = <Date 2010-11-08.21:36:51.074>
    creator = 'jcea'
    dependencies = []
    files = ['19690', '19913', '19981']
    hgrepos = []
    issue_num = 10367
    keywords = ['patch', 'easy']
    message_count = 24.0
    messages = ['120804', '120809', '120824', '120891', '120893', '121695', '121714', '121793', '123142', '123193', '123262', '123271', '123295', '123297', '123301', '123303', '123305', '123306', '123307', '123314', '123665', '129858', '129869', '258646']
    nosy_count = 9.0
    nosy_names = ['jcea', 'pje', 'tarek', 'eric.araujo', 'brian.curtin', 'Priscila.Manhaes', 'daniel.tavares', 'berker.peksag', 'nilovna']
    pr_nums = []
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = '12853'
    type = 'behavior'
    url = 'https://bugs.python.org/issue10367'
    versions = ['Python 2.7']

    @jcea
    Copy link
    Member Author

    jcea commented Nov 8, 2010

    Uploading a new release of one of my libraries to PYPI I got this error:

    """
    Submitting dist/bsddb3-5.1.1.tar.gz to http://pypi.python.org/pypi
    Upload failed (400): A file named "bsddb3-5.1.1.tar.gz" already exists for  bsddb3-5.1.1. To fix problems with that file you should create a new release.
    Traceback (most recent call last):
      File "setup.py", line 5, in <module>
        import setup2
      File "/home/pybsddb/setup2.py", line 415, in <module>
        'Programming Language :: Python :: 3.2',
      File "/usr/local/lib/python2.7/distutils/core.py", line 152, in setup
        dist.run_commands()
      File "/usr/local/lib/python2.7/distutils/dist.py", line 953, in run_commands
        self.run_command(cmd)
      File "/usr/local/lib/python2.7/distutils/dist.py", line 972, in run_command
        cmd_obj.run()
      File "/usr/local/lib/python2.7/distutils/command/upload.py", line 60, in run
        self.upload_file(command, pyversion, filename)
      File "/usr/local/lib/python2.7/distutils/command/upload.py", line 189, in upload_file
        self.announce('-'*75, result.read(), '-'*75)
    UnboundLocalError: local variable 'result' referenced before assignment
    """

    Checking the code, I see this:

    """
    # send the data
    try:
    result = urlopen(request)
    status = result.getcode()
    reason = result.msg
    except socket.error, e:
    self.announce(str(e), log.ERROR)
    return
    except HTTPError, e:
    status = e.code
    reason = e.msg

            if status == 200:
                self.announce('Server response (%s): %s' % (status, reason),
                              log.INFO)
            else:
                self.announce('Upload failed (%s): %s' % (status, reason),
                              log.ERROR)
            if self.show_response:
                self.announce('-'*75, result.read(), '-'*75)
    """

    Here, if we selected "show_response" *AND* some error happens, "result" will be undefined and the last line will fail.

    This bug was introduced in Python 2.7. It used to work correctly under Python 2.6.

    I didn't check Python 3.x.

    I think this bug is trivial to reproduce: just try to upload any file with no permissions for it, using "--show_response" command line option.

    @jcea jcea added type-crash A hard crash of the interpreter, possibly with a core dump easy labels Nov 8, 2010
    @merwok
    Copy link
    Member

    merwok commented Nov 8, 2010

    Thanks for the report. Can you paste your message and exact Python version on bpo-9199?

    @merwok merwok closed this as completed Nov 8, 2010
    @merwok merwok added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Nov 8, 2010
    @jcea
    Copy link
    Member Author

    jcea commented Nov 9, 2010

    This is not bpo-9199, even covering the same line code.

    In this case the problem is an exception leaving a local variable undefined, crashing later.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Nov 9, 2010

    To better show what the problem is, here's a change that would fix the problem (albeit in an ugly way):

    --- upload.py   2010-07-07 20:16:33.000000000 -0400
    +++ /home/pje/upload.new        2010-11-09 14:30:21.000000000 -0500
    @@ -167,6 +167,9 @@
     
             request = Request(self.repository, data=body,
                               headers=headers)
    +
    +        result = None
    +
             # send the data
             try:
                 result = urlopen(request)
    @@ -186,4 +189,4 @@
                 self.announce('Upload failed (%s): %s' % (status, reason),
                               log.ERROR)
             if self.show_response:
    -            self.announce('-'*75, result.read(), '-'*75)
    +            self.announce('-'*75, result.read() if result is not None else 'ERROR', '-'*75)

    @pjeby pjeby mannequin reopened this Nov 9, 2010
    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Nov 9, 2010

    Btw, a quick review of the 3.x trunk code for this shows that it does *not* have this problem; this is specific to 2.7 and AFAICT *only* 2.7.

    (This problem was introduced in r73436, btw, as of the move to urllib2 vs. httplib.)

    @PriscilaManhaes
    Copy link
    Mannequin

    PriscilaManhaes mannequin commented Nov 20, 2010

    Well, I fixed the problem moving the "if" that instances "result" even though it getting HTTPError into the "try".

    ------------------------------------------------------
    # send the data
    try:
    result = urlopen(request)
    status = result.getcode()
    reason = result.msg
    if self.show_response:
    self.announce('-'*75, result.read(), '-'*75)
    except socket.error, e:
    self.announce(str(e), log.ERROR)
    return
    except HTTPError, e:
    status = e.code
    reason = e.msg

            if status == 200:
                self.announce('Server response (%s): %s' % (status, reason),
                              log.INFO)
            else:
                self.announce('Upload failed (%s): %s' % (status, reason),
                              log.ERROR)

    So now it explains the real error:

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

    writing manifest file 'MANIFEST'
    creating Python-2.7
    creating Python-2.7/Lib
    creating Python-2.7/Tools
    creating Python-2.7/Tools/scripts
    making hard links in Python-2.7...
    hard linking README -> Python-2.7
    '_struct.c' not a regular file -- skipping
    hard linking setup.py -> Python-2.7
    hard linking Lib/smtpd.py -> Python-2.7/Lib
    hard linking Tools/scripts/2to3 -> Python-2.7/Tools/scripts
    hard linking Tools/scripts/idle -> Python-2.7/Tools/scripts
    hard linking Tools/scripts/pydoc -> Python-2.7/Tools/scripts
    Creating tar archive
    removing 'Python-2.7' (and everything under it)
    running upload
    Submitting dist/Python-2.7.tar.gz to http://pypi.python.org/pypi
    Upload failed (401): Unauthorized

    Is this ok?

    @merwok
    Copy link
    Member

    merwok commented Nov 20, 2010

    Thank you for the fix Priscila. Can you submit it as a diff file? (guidelines on http://www.python.org/dev/patches/)

    Tarek: I wonder if we should backport pydoc_server from d2 to test behavior bugs like this one.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Nov 20, 2010

    It looks to me as though this patch reintroduces bpo-9199, as it passes multiple arguments to self.announce() once again. The patch needs to be made against the SVN version of Python, not the released version.

    @danieltavares
    Copy link
    Mannequin

    danieltavares mannequin commented Dec 2, 2010

    Here's an updated patch, which fixes the passing of multiple arguments to self.announce, based on Priscila's fix.

    @merwok
    Copy link
    Member

    merwok commented Dec 3, 2010

    Thanks Daniel. I have good and bad news for this patch.

    The bad news is that I don’t want to accept patches without tests. We need to stop guessing or experimenting with the real PyPI to fix bugs.

    The good news is that we have a mock PyPI server in distutils2. Its use is described at http://distutils2.notmyidea.org/library/distutils2.tests.pypi_server.html and there are examples in the current test suite. Instructions for patching distutils2 are at http://wiki.python.org/moin/Distutils/FixingBugs . When a patch that includes a test is written, I will backport the relevant parts to distutils1.

    @merwok merwok added the stdlib Python modules in the Lib dir label Dec 3, 2010
    @merwok merwok self-assigned this Dec 3, 2010
    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Dec 3, 2010

    Given that this is a pure bugfix to revert a problem in 2.7 -- where no *new* development is being done -- a test isn't actually needed for this patch. There is no point in holding up a distutils1 fix for distutils2's benefit.

    Daniel: thanks for the patch, it looks good to me.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Dec 3, 2010

    Committed Daniel's patch to r86978 in the 2.7 maintenance branch.

    (The 2.x trunk still has this bug, but is permanently closed to new checkins.)

    @briancurtin
    Copy link
    Member

    a test isn't actually needed for this patch.

    This is incorrect.

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Dec 3, 2010

    Philip, Eric is currently assigned to this issue, and was working on a test, obviously.

    It means that commiting a fix without a test without asking him first is is quite rude.

    He and I are maintaining Distutils. Your help is welcome but please do not commit in this area without discussion in particular when the bug is assigned to someone.

    Now if you could provide a test for your change, we would highly appreciate it. Thanks

    @danieltavares
    Copy link
    Mannequin

    danieltavares mannequin commented Dec 3, 2010

    I'll gladly work on the test, unless someone is working on it already. I
    apologize for not sending it originally along with the patch. Since Eric
    only requested the diff, I thought a test wasn't needed.

    Cheers,
    Daniel

    On Fri, Dec 3, 2010 at 1:58 PM, Tarek Ziadé <report@bugs.python.org> wrote:

    Tarek Ziadé <ziade.tarek@gmail.com> added the comment:

    Philip, Eric is currently assigned to this issue, and was working on a
    test, obviously.

    It means that commiting a fix without a test without asking him first is is
    quite rude.

    He and I are maintaining Distutils. Your help is welcome but please do not
    commit in this area without discussion in particular when the bug is
    assigned to someone.

    Now if you could provide a test for your change, we would highly appreciate
    it. Thanks

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue10367\>


    @merwok
    Copy link
    Member

    merwok commented Dec 3, 2010

    Thanks Daniel. The links in my last message should get you started, and any questions are welcome.

    You may be interested in this sprint: http://wiki.montrealpython.org/index.php/Packaging_no.10 I will be on IRC for some hours that night.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Dec 3, 2010

    Whoops, my bad... I misread Eric's earlier message as throwing it back onto *Daniel* to produce a test, not that *he* (Eric) was working on the test.

    IOW, I thought that progress had been stalled a second time on this, and went ahead to pick up the slack. Sorry about that.

    @merwok
    Copy link
    Member

    merwok commented Dec 3, 2010

    You understood right: I asked for a test using the mock PyPI server in distutils2. “I can’t do it” is a valid reply, in which case it would have been picked up by someone else or me.

    You could say that progress has stalled, but there was no urgency, given that the next 2.7 release is not tomorrow. We have more than a hundred bugs on our hands, so waiting a few days for a reply is not a problem :)

    Misunderstandings aside, thanks for your help.

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Dec 3, 2010

    Ugh sorry I thought Eric was working on a test. I misunderstood.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Dec 4, 2010

    The urgency was only that I didn't want the other contributors to this issue to feel as though the bar on their contributions were being raised higher every time they jumped the previous bar.

    IOW, I did it to make them feel like somebody was doing *something*. In hindsight, that was not necessarily the best tactic. ;-)

    (Given the nature of the bugfix and the bugfix-only status of the 2.7 line, I don't think the lack of an automated test is a big deal; the odds that anything other than trivial bugfixes will be applied to this module in the future seem negligible to me. At least, I would *hope* that it is not planned to destabilize this module in 2.7.x any further than it already was by the changes that broke it in the first place.)

    @danieltavares
    Copy link
    Mannequin

    danieltavares mannequin commented Dec 9, 2010

    Here goes the test. Feel free to make any comments, critiques or suggestions, since this is my first take on this code base.

    @danieltavares
    Copy link
    Mannequin

    danieltavares mannequin commented Mar 2, 2011

    Hey Éric,

    Just wanted to check with you about the status of this issue. Any chance this could be reviewed? It's been idle since December and it's still listed as "needs patch".

    Thanks,
    Daniel

    @merwok
    Copy link
    Member

    merwok commented Mar 2, 2011

    Sorry for overlooking this. The test is good, I could trigger the bug with it and then fix it with the patch.

    Would you mind adding the same test for upload_docs? The code was originally copied from upload, so we should test it too.

    Philip: I understand your motives but respectfully disagree with them.

    The urgency was only that I didn't want the other contributors to
    this issue to feel as though the bar on their contributions were
    being raised higher every time they jumped the previous bar.
    I try not to put off people by demanding a test, but asking if they want to write one and providing guidance. I think it’s worked quite well with a lot of our recent contributors from Montréal and other places, but I’m open to critiques about my tone or phrasing, I don’t want to sound like I require a huge amount of work from people who report bugs.

    I did it to make them feel like somebody was doing *something*. In
    hindsight, that was not necessarily the best tactic. ;-)
    Well, I don’t really care about appearances or feelings; the truth of the situation is that there are a lot of easy and hard bugs and a few people to look at them, I don’t think we should mask it. Prior to December, I was reactive in this report.

    (Given the nature of the bugfix and the bugfix-only status of the 2.7
    line, I don't think the lack of an automated test is a big deal; the
    odds that anything other than trivial bugfixes will be applied to
    this module in the future seem negligible to me.
    You know more about distutils than me, but from the short time I’ve worked with this codebase, read bug reports and mailing list archives, I’ve found that it’s too optimistic to change something without an automated test. I have committed a bug in upload.py once :) That’s why I decided to draw a line and stop guessing and hoping things would be right and rely on regression tests. This adds a bit of work for our contributors, but my intention is not to put off people, but to make things more robust. We owe that to our users and to future distutils contributors.

    I do hope this explains why seemingly trivial changes require a test too. I’d also like to thank you for your help in this report.

    @berkerpeksag
    Copy link
    Member

    This has been fixed in bpo-12853. The relevant line is here: https://hg.python.org/cpython/file/2.7/Lib/distutils/command/upload.py#l179

    Closing this one as a duplicate.

    @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
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants