Enable downloading behind a proxy by using urllib2 #253

stefano-m · 2015-07-10T16:57:27Z

Behind a proxy, urllib does not work with Python2 due to Python issue
24599.

Instead of using urllib, let's implement urlretrieve with urllib2, which
instead works and leave the Python3 implementation untouched (since that
one works too).

This fixes buildout issue #32.

There's no need to change the try: except:IOError block where urlretrieve is
used (line 195 in download.py) since the exceptions raised by urllib2
are subclasses of IOError and will then be caught.

Behind a proxy, `urllib` does not work with Python2 due to [Python issue 24599](https://bugs.python.org/issue24599). Instead of using `urllib`, let's implement `urlretrieve` with `urllib2`, which instead works and leave the Python3 implementation untouched (since that one works too). This fixes buildout issue #32. There's no need to change the `try: except:IOError` block where `urlretrieve` is used (line 195 in download.py) since the exceptions raised by `urllib2` are subclasses of `IOError` and will then be caught.

stefano-m · 2015-07-10T16:58:08Z

Please note that I have successfully run tox on this change, albeit only on Python 2.7

stefano-m · 2015-07-25T15:53:21Z

Is there any chance that this pull request is going to be considered?

I appreciate any comments and am available to make the required changes if needed.

leorochael · 2015-07-26T01:51:32Z

Looks fine to me.

@jimfulton?

reinout · 2015-08-06T20:03:07Z

Looks mostly OK to me.

There is one thing that worries me a bit: the try/except results in different imported modules being available. URLOpener is being imported on python3, but not anymore on python2, for instance. As long as the tests run, it is probably OK, but...

It looks like either something is being imported (in python3 for instance) that isn't necessary. Or something is missing in python2. (Or I'm overlooking something).

And the case difference between URLOpener and URLopener is very very small and easy to overlook. Perhaps it cannot be avoided, but as one of them is apparently only used in a monkey patch, perhaps a less-similar-looking name (PatchedURLOpener or so) is better?

So: the patch does not look immediately obviously problem-free to me. It does look like it solves the issue. Could you look at the issues I've raised?

Exact same imports in the python3 and python2 part. Perhaps move an import inside one of the monkey-patch thingies?
Name/case confusion. Perhaps just a comment, perhaps a name change.

If you fix something and I don't respond within two days, feel free to remind me to take a look :-)

stefano-m · 2015-08-06T21:40:58Z

@reinout thanks for your comment.

Regarding your first question:

Since the Python issue 24599 only affects Python 2, I have tried to work around that only on the Python 2 side by redefining the urlretrieve function and moving the existing monkey-patching into the Python 3 code block (I also import urllib.request using its original name since there's no need to monkey-patch in Python 2).
The Python 2 block does not need to be monkey-patched any more because it uses a different mechanism, namely urllib2 to emulate the broken urlretrieve. Hence the difference in imports.

I tried to be as conservative as possible in my changes, that's why I have essentially not modified the Python 3 code.

Regarding your second question:

I agree that the class naming is a bit unclear, especially the URLOpener vs the patched URLopener, but I did not want to change anything more that what would've fixed the issue.

It's definitely a good thing to remove the source of confusion, but I feel that it does not belong to this PR, probably it's best done in a separate commit. That said, I'll be happy to change the names in this PR if you think it's appropriate.

Let me know what you think.

reinout · 2015-08-07T00:27:17Z

I'd adjust the name(s) right here in the pull request. I don't have any problems with a few extra cleanup-type lines in a pull request :-)
Regarding different imports for python 2 and python 3: I don't know if you use pyflakes, but it warns for unused and/or missing imports. I love it, as it helps me prevent errors. The current code looks like it could cause problems as it either has too many or too few imports, depending on whether it is run on python 2 or 3. I mean: I should be able to remove the try/except code (just keeping either the python2 or python3 variant) and get a clean result without warnings out of pyflakes. As long as that's the case, I'm fine with it.

pbauer · 2015-08-07T07:11:13Z

See also the rejected pr #33

@reinout

As per @reinout comments in PR #253, let's make it clear that we're assigning request._urlopener to a patched URL opener.

stefano-m · 2015-08-07T15:51:06Z

src/zc/buildout/download.py

-    from urllib import request as urllib # for monkey patch below :(
+    from urllib import request
+
+    class PatchedURLopener(FancyURLopener):


Renamed for clarity as requested by @reinout

stefano-m · 2015-08-07T16:08:02Z

@reinout, I have just pushed a commit to disambiguate the monkey-patched URL opener.

On the other hand...

Regarding different imports for python 2 and python 3: I don't know if you use pyflakes, but it warns for unused and/or missing imports. I love it, as it helps me prevent errors. The current code looks like it could cause problems as it either has too many or too few imports, depending on whether it is run on python 2 or 3. I mean: I should be able to remove the try/except code (just keeping either the python2 or python3 variant) and get a clean result without warnings out of pyflakes. As long as that's the case, I'm fine with it.

I use flake8 that in turn uses (also) pyflakes. I am not getting any warning about unused imports on both Python 2 and Python 3. To double check I have also run pyflakes directly on the download.py module but I didn't get any warning about imports. Am I missing something?

As far as I can see, the imports seem to be in the right blocks.

urllib is only used by Python 3
urllib2 is only used by Python 2
urlparse is used by both Python 2 and Python 3, although it must be imported from different modules

These are the warnings that I get from flake8 on download.py:

Line  Col  Level   ID        Message
   61   1 warning  E302   expected 2 blank lines, found 1
  204  31 warning  E128   continuation line under-indented for visual indent

Would you mind posting the errors you get from pyflakes? That would be really helpful.

reinout · 2015-08-07T19:03:22Z

I didn't get any pyflakes errors as I only looked at your diff visually :-) I was only worrying about the different conditional imports being used later on by others that don't realize they're in a try/except. But on second thought we don't need to worry about that. The test runner will catch that. And there's no need to complicate the code further.

Looks good, I'll merge it.

…x_32 Enable downloading behind a proxy by using urllib2

reinout · 2015-08-07T19:06:01Z

I've added a changelog entry (directly in master). See 21344c7

Can you check if that's the right text?

reinout · 2015-08-07T19:37:33Z

I've added a test fix on master (29a13cb, completely unrelated to your pull request) that hopefully gets the winbot tests running again. Those are scheduled in 1.5 hours.

I'm waiting for the results from that and I think I'll just make a release. Nothing mayor has changed, but waiting for a bigger change isn't needed, right?

stefano-m · 2015-08-07T21:07:33Z

@reinout, thanks for the merge. This will help me pushing the use of buildout where I work!

Regarding releasing, I think it's OK even for a smallish bug fix given that the project is quite stable and mature.

Also, I've added a comment on 21344c7 with a possible better wording.

reinout · 2015-08-07T23:23:40Z

Hm, the windows issue is almost fixed. I'll open a separate issue for that.

I've changed the changelog a bit. Mentioning the python 2 issue is a good idea.

I've released 2.4.1. Can you check to make sure it works for you?

stefano-m · 2015-08-08T20:02:58Z

@reinout, thanks for that. I've just realized that we should've also said that this closes issue #32 (and that issue should be closed accordingly). Of course, it's not worth re-relasing just because of that. But it would be nice to have CHANGES.rst mention the issue number since it's already being done.

I will update those files in my next PR. If not, feel free to remind me before merging!

Also, I will confirm the fix in the released version ASAP.

reinout · 2015-08-08T22:38:41Z

I've closed #32 :-) I tend to forget those things sometimes.

stefano-m · 2015-08-10T09:00:22Z

@reinout, I have downloaded the latest release and I can confirm that this PR fixes #32

Thanks for merging it!

Disambiguate patched URL opener

9de262f

As per @reinout comments in PR #253, let's make it clear that we're assigning request._urlopener to a patched URL opener.

stefano-m reviewed Aug 7, 2015
View reviewed changes

reinout added a commit that referenced this pull request Aug 7, 2015

Merge pull request #253 from stefano-m/allow_download_behind_proxy_fi…

f425816

…x_32 Enable downloading behind a proxy by using urllib2

reinout merged commit f425816 into buildout:master Aug 7, 2015

reinout added a commit that referenced this pull request Aug 7, 2015

Added changelog entry for #253

21344c7

stefano-m mentioned this pull request Aug 8, 2015

zc.buildout fails behind a cntlm proxy #32

Closed

stefano-m deleted the allow_download_behind_proxy_fix_32 branch August 10, 2015 09:00

reinout mentioned this pull request Sep 7, 2015

Disabled patching of FancyURLopener in the download.py if used Python>=3.3. #263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable downloading behind a proxy by using urllib2 #253

Enable downloading behind a proxy by using urllib2 #253

stefano-m commented Jul 10, 2015

stefano-m commented Jul 10, 2015

stefano-m commented Jul 25, 2015

leorochael commented Jul 26, 2015

reinout commented Aug 6, 2015

stefano-m commented Aug 6, 2015

reinout commented Aug 7, 2015

pbauer commented Aug 7, 2015

stefano-m Aug 7, 2015

stefano-m commented Aug 7, 2015

reinout commented Aug 7, 2015

reinout commented Aug 7, 2015

reinout commented Aug 7, 2015

stefano-m commented Aug 7, 2015

reinout commented Aug 7, 2015

stefano-m commented Aug 8, 2015

reinout commented Aug 8, 2015

stefano-m commented Aug 10, 2015

Enable downloading behind a proxy by using urllib2 #253

Enable downloading behind a proxy by using urllib2 #253

Conversation

stefano-m commented Jul 10, 2015

stefano-m commented Jul 10, 2015

stefano-m commented Jul 25, 2015

leorochael commented Jul 26, 2015

reinout commented Aug 6, 2015

stefano-m commented Aug 6, 2015

reinout commented Aug 7, 2015

pbauer commented Aug 7, 2015

stefano-m Aug 7, 2015

Choose a reason for hiding this comment

stefano-m commented Aug 7, 2015

reinout commented Aug 7, 2015

reinout commented Aug 7, 2015

reinout commented Aug 7, 2015

stefano-m commented Aug 7, 2015

reinout commented Aug 7, 2015

stefano-m commented Aug 8, 2015

reinout commented Aug 8, 2015

stefano-m commented Aug 10, 2015