classification
Title: "python setup.py sdist upload --show-response" can fail with "UnboundLocalError: local variable 'result' referenced before assignment"
Type: behavior Stage: resolved
Components: Distutils, Distutils2 Versions: Python 2.7
process
Status: closed Resolution: duplicate
Dependencies: Superseder: global name 'r' is not defined in upload.py
View: 12853
Assigned To: eric.araujo Nosy List: Priscila.Manhaes, berker.peksag, brian.curtin, daniel.tavares, eric.araujo, jcea, nilovna, pje, tarek
Priority: normal Keywords: easy, patch

Created on 2010-11-08 21:36 by jcea, last changed 2016-01-20 06:27 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
10367.diff Priscila.Manhaes, 2010-11-20 19:01 Lib/distutils/command/upload.py review
10367.updated.diff daniel.tavares, 2010-12-02 23:12 Patch the multiple arguments to self.announce
test_10367.diff daniel.tavares, 2010-12-09 00:55 The unit test
Messages (24)
msg120804 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2010-11-08 21:36
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.
msg120809 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-08 22:00
Thanks for the report.  Can you paste your message and exact Python version on #9199?
msg120824 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2010-11-09 00:10
This is not issue9199, even covering the same line code.

In this case the problem is an exception leaving a local variable undefined, crashing later.
msg120891 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-11-09 19:31
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)
msg120893 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-11-09 19:36
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.)
msg121695 - (view) Author: Priscila Manhaes (Priscila.Manhaes) Date: 2010-11-20 17:52
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?
msg121714 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-20 18:46
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.
msg121793 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-11-20 22:08
It looks to me as though this patch reintroduces issue9199, 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.
msg123142 - (view) Author: Daniel Tavares (daniel.tavares) Date: 2010-12-02 23:12
Here's an updated patch, which fixes the passing of multiple arguments to self.announce, based on Priscila's fix.
msg123193 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 06:46
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.
msg123262 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-12-03 16:42
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.
msg123271 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-12-03 17:18
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.)
msg123295 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-03 21:49
> a test isn't actually needed for this patch.

This is incorrect.
msg123297 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2010-12-03 21:58
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
msg123301 - (view) Author: Daniel Tavares (daniel.tavares) Date: 2010-12-03 22:37
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>
> _______________________________________
>
msg123303 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 22:46
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.
msg123305 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-12-03 22:57
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.
msg123306 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 23:05
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.
msg123307 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2010-12-03 23:40
Ugh sorry I thought Eric was working on a test. I  misunderstood.
msg123314 - (view) Author: PJ Eby (pje) * (Python committer) Date: 2010-12-04 02:22
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.)
msg123665 - (view) Author: Daniel Tavares (daniel.tavares) Date: 2010-12-09 00:55
Here goes the test. Feel free to make any comments, critiques or suggestions, since this is my first take on this code base.
msg129858 - (view) Author: Daniel Tavares (daniel.tavares) Date: 2011-03-02 07:50
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
msg129869 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-02 10:58
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.
msg258646 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-01-20 06:27
This has been fixed in issue 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.
History
Date User Action Args
2016-01-20 06:27:46berker.peksagsetstatus: open -> closed

superseder: global name 'r' is not defined in upload.py

nosy: + berker.peksag
messages: + msg258646
resolution: duplicate
stage: needs patch -> resolved
2014-03-14 19:21:29nilovnasetnosy: + nilovna
2011-05-04 16:14:41eric.araujolinkissue11970 superseder
2011-03-02 10:58:39eric.araujosetnosy: jcea, pje, tarek, eric.araujo, brian.curtin, Priscila.Manhaes, daniel.tavares
messages: + msg129869
2011-03-02 07:50:16daniel.tavaressetnosy: jcea, pje, tarek, eric.araujo, brian.curtin, Priscila.Manhaes, daniel.tavares
messages: + msg129858
2010-12-12 17:04:49r.david.murraysetpriority: normal
2010-12-09 00:55:22daniel.tavaressetfiles: + test_10367.diff

messages: + msg123665
versions: - 3rd party
2010-12-04 02:22:49pjesetmessages: + msg123314
2010-12-03 23:40:59tareksetmessages: + msg123307
2010-12-03 23:05:00eric.araujosetpriority: high -> (no value)

messages: + msg123306
2010-12-03 22:57:08pjesetmessages: + msg123305
2010-12-03 22:46:53eric.araujosetstage: resolved -> needs patch
messages: + msg123303
versions: + 3rd party
2010-12-03 22:38:33eric.araujosetfiles: - unnamed
2010-12-03 22:37:01daniel.tavaressetfiles: + unnamed

messages: + msg123301
2010-12-03 21:58:55tareksetmessages: + msg123297
2010-12-03 21:49:47brian.curtinsetnosy: + brian.curtin
messages: + msg123295
2010-12-03 17:18:00pjesetstage: needs patch -> resolved
messages: + msg123271
versions: - 3rd party, Python 3.1, Python 3.2
2010-12-03 16:42:37pjesetmessages: + msg123262
2010-12-03 06:46:02eric.araujosetversions: + 3rd party, Python 3.1, Python 3.2
nosy: + tarek

messages: + msg123193

assignee: eric.araujo
components: + Distutils, Distutils2
2010-12-02 23:12:31daniel.tavaressetfiles: + 10367.updated.diff
nosy: + daniel.tavares
messages: + msg123142

2010-11-21 01:23:39eric.araujosetsuperseder: distutils upload command crashes when displaying server response -> (no value)
2010-11-20 22:08:24pjesetmessages: + msg121793
2010-11-20 19:01:24Priscila.Manhaessetfiles: + 10367.diff
keywords: + patch
2010-11-20 18:46:40eric.araujosetmessages: + msg121714
2010-11-20 17:52:56Priscila.Manhaessetnosy: + Priscila.Manhaes
messages: + msg121695
2010-11-09 19:36:35pjesetmessages: + msg120893
2010-11-09 19:31:41pjesetstatus: closed -> open

nosy: + pje
messages: + msg120891

resolution: duplicate -> (no value)
stage: resolved -> needs patch
2010-11-09 00:10:18jceasetmessages: + msg120824
2010-11-08 22:14:42eric.araujolinkissue10368 superseder
2010-11-08 22:00:26eric.araujosetstatus: open -> closed

superseder: distutils upload command crashes when displaying server response
type: crash -> behavior

nosy: + eric.araujo
messages: + msg120809
resolution: duplicate
stage: needs patch -> resolved
2010-11-08 21:36:51jceacreate