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
Comments
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: """ 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. |
Thanks for the report. Can you paste your message and exact Python version on bpo-9199? |
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. |
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) |
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.) |
Well, I fixed the problem moving the "if" that instances "result" even though it getting HTTPError into the "try". ------------------------------------------------------ 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 writing manifest file 'MANIFEST' Is this ok? |
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. |
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. |
Here's an updated patch, which fixes the passing of multiple arguments to self.announce, based on Priscila's fix. |
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. |
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. |
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.) |
This is incorrect. |
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 |
I'll gladly work on the test, unless someone is working on it already. I Cheers, On Fri, Dec 3, 2010 at 1:58 PM, Tarek Ziadé <report@bugs.python.org> wrote:
|
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. |
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. |
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. |
Ugh sorry I thought Eric was working on a test. I misunderstood. |
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.) |
Here goes the test. Feel free to make any comments, critiques or suggestions, since this is my first take on this code base. |
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, |
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.
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. |
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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: