Issue 10367: "python setup.py sdist upload --show-response" can fail with "UnboundLocalError: local variable 'result' referenced before assignment" (original) (raw)
Created on 2010-11-08 21:36 by jcea, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (24)
Author: Jesús Cea Avión (jcea) * 
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 import setup2 File "/home/pybsddb/setup2.py", line 415, in '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.
Author: Éric Araujo (eric.araujo) *
Date: 2010-11-08 22:00
Thanks for the report. Can you paste your message and exact Python version on #9199?
Author: Jesús Cea Avión (jcea) *
Date: 2010-11-09 00:10
This is not , even covering the same line code.
In this case the problem is an exception leaving a local variable undefined, crashing later.
Author: PJ Eby (pje) *
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)
Author: PJ Eby (pje) *
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.)
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?
Author: Éric Araujo (eric.araujo) *
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.
Author: PJ Eby (pje) *
Date: 2010-11-20 22:08
It looks to me as though this patch reintroduces , 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.
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.
Author: Éric Araujo (eric.araujo) *
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.
Author: PJ Eby (pje) *
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.
Author: PJ Eby (pje) *
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.)
Author: Brian Curtin (brian.curtin) *
Date: 2010-12-03 21:49
a test isn't actually needed for this patch.
This is incorrect.
Author: Tarek Ziadé (tarek) *
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
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>
Author: Éric Araujo (eric.araujo) *
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.
Author: PJ Eby (pje) *
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.
Author: Éric Araujo (eric.araujo) *
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.
Author: Tarek Ziadé (tarek) *
Date: 2010-12-03 23:40
Ugh sorry I thought Eric was working on a test. I misunderstood.
Author: PJ Eby (pje) *
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.)
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.
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
Author: Éric Araujo (eric.araujo) *
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.
Author: Berker Peksag (berker.peksag) *
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
2022-04-11 14:57:08
admin
set
github: 54576
2016-01-20 06:27:46
berker.peksag
set
status: open -> closed
superseder: global name 'r' is not defined in upload.py
nosy: + berker.peksag
messages: +
resolution: duplicate
stage: needs patch -> resolved
2014-03-14 19:21:29
nilovna
set
nosy: + nilovna
2011-05-04 16:14:41
eric.araujo
link
2011-03-02 10:58:39
eric.araujo
set
nosy:jcea, pje, tarek, eric.araujo, brian.curtin, Priscila.Manhaes, daniel.tavares
messages: +
2011-03-02 07:50:16
daniel.tavares
set
nosy:jcea, pje, tarek, eric.araujo, brian.curtin, Priscila.Manhaes, daniel.tavares
messages: +
2010-12-12 17:04:49
r.david.murray
set
priority: normal
2010-12-09 00:55:22
daniel.tavares
set
files: + test_10367.diff
messages: +
versions: - 3rd party
2010-12-04 02:22:49
pje
set
messages: +
2010-12-03 23:40:59
tarek
set
messages: +
2010-12-03 23:05:00
eric.araujo
set
priority: high -> (no value)
messages: +
2010-12-03 22:57:08
pje
set
messages: +
2010-12-03 22:46:53
eric.araujo
set
stage: resolved -> needs patch
messages: +
versions: + 3rd party
2010-12-03 22:38:33
eric.araujo
set
files: - unnamed
2010-12-03 22:37:01
daniel.tavares
set
files: + unnamed
messages: +
2010-12-03 21:58:55
tarek
set
messages: +
2010-12-03 21:49:47
brian.curtin
set
nosy: + brian.curtin
messages: +
2010-12-03 17🔞00
pje
set
stage: needs patch -> resolved
messages: +
versions: - 3rd party, Python 3.1, Python 3.2
2010-12-03 16:42:37
pje
set
messages: +
2010-12-03 06:46:02
eric.araujo
set
versions: + 3rd party, Python 3.1, Python 3.2
nosy: + tarek
messages: +
assignee: eric.araujo
components: + Distutils, Distutils2
2010-12-02 23:12:31
daniel.tavares
set
files: + 10367.updated.diff
nosy: + daniel.tavares
messages: +
2010-11-21 01:23:39
eric.araujo
set
superseder: distutils upload command crashes when displaying server response -> (no value)
2010-11-20 22:08:24
pje
set
messages: +
2010-11-20 19:01:24
Priscila.Manhaes
set
files: + 10367.diff
keywords: + patch
2010-11-20 18:46:40
eric.araujo
set
messages: +
2010-11-20 17:52:56
Priscila.Manhaes
set
nosy: + Priscila.Manhaes
messages: +
2010-11-09 19:36:35
pje
set
messages: +
2010-11-09 19:31:41
pje
set
status: closed -> open
nosy: + pje
messages: +
resolution: duplicate -> (no value)
stage: resolved -> needs patch
2010-11-09 00:10:18
jcea
set
messages: +
2010-11-08 22:14:42
eric.araujo
link
2010-11-08 22:00:26
eric.araujo
set
status: open -> closed
superseder: distutils upload command crashes when displaying server response
type: crash -> behavior
nosy: + eric.araujo
messages: +
resolution: duplicate
stage: needs patch -> resolved
2010-11-08 21:36:51
jcea
create