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

Update conan_requester.py #5162

Merged
merged 3 commits into from May 21, 2019

Conversation

Projects
None yet
4 participants
@trulede
Copy link
Contributor

commented May 17, 2019

Hi, I have a problem where proxy settings are not being taken from the environment (so I have to manually specify them in conan.config). In searching for the solution to that problem I noticed that this code might not work as intended. When the popped variable is True the right part of the "or" will not be executed ... and thus not all proxy settings would be cleared (only the first encountered is removed).

Just as an example to check my thinking:

>>> False or print("hi")
hi
>>> True or print("hi")
True  <=== the right part of the or did not execute.

Do I understand it right? Thanks.

Changelog: Bugfix: Make sure that proxy "http_proxy", "https_proxy", "no_proxy" vars are correctly removed if custom ones are defined in the conan.conf. Also, avoid using urllib.request.getproxies(), they are broken.
Docs: omit

Close #5170

Update conan_requester.py
Hi, I have a problem where proxy settings are _not_ being taken from the environment (so I have to manually specify them). In searching for the solution to that problem I noticed that this code might not work as intended. When the popped variable is True the right part of the "or" will not be executed ... and thus not all proxy settings would be cleared (probably only the first encountered).

>>> False or print("hi")
hi
>>> True or print("hi")
True  <=== the right part of the or did not execute.

Do I understand it right? Thanks.
@CLAassistant

This comment has been minimized.

Copy link

commented May 17, 2019

CLA assistant check
All committers have signed the CLA.

@trulede

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

I have a better understanding of why I have a problem with the Proxy settings. In this code:

def _call_method(self, method, url, **kwargs):
        popped = False
        if self.proxies or self._no_proxy_match:
            old_env = dict(os.environ)
            # Clean the proxies from the environ and use the conan specified proxies
            for var_name in ("http_proxy", "https_proxy", "no_proxy"):
                popped = True if os.environ.pop(var_name, None) else popped
                popped = True if os.environ.pop(var_name.upper(), None) else popped
        try:
            t1 = time.time()

self.proxies will be set; first from the conan.conf file; and then second from a call to urllib.request.getproxies() (conans/client/conf/init.py line 431). The problem with that is that getproxies() returns the proxy settings with truncated names, i.e. http_proxy becomes http, and these apparently do not work.

The comment gives a hint of the problem: Clean the proxies from the environ and use the conan specified proxies. What is happening is there are no conan specified proxies, however the variable self.proxies has been filled by the call to urllib.request.getproxies() which itself returns mangled proxy names which were derived from the environment.

Its possible that this worked for a lot of people because the above code was broken and removed only one proxy string from the environment. It was a matter of chance which one it was, in my case the NO_PROXY was taken first ... and calls to local_host would fail (since the call went the the proxy server ... and that did not work at all).

I will make another change to remove the call to urllib.request.getproxies() as it is confusing the contents of self.proxies.

@trulede

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

Update __init__.py
The call to urllib.request.getproxies() was putting modified system proxy settings into the proxies property (changed name: http_proxy becomes http). Code using that was assuming that these proxy settings came from conan.conf ... and then removed the valid system proxy settings, which resulted in broken proxy handling, since there were no valid settings left.

@lasote lasote added this to the 1.16 milestone May 20, 2019

@lasote lasote self-requested a review May 20, 2019

@lasote lasote self-assigned this May 20, 2019

@lasote

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Hi, could you take a look at the failing test? In general, it looks good and we will try to push it to the next release.

@lasote lasote requested a review from memsharded May 20, 2019

Update __init__.py
Return an empty hash/dict from proxies() rather than None. Unit tests expect a hash, whereas the calling code evaluates (if), therefore an empty hash works in both cases.
@trulede

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Thanks, I just made a small change (returning an empty hash). The tests are OK now.

@lasote

lasote approved these changes May 21, 2019

@lasote lasote assigned memsharded and unassigned lasote May 21, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Thanks very much for contributing this fix. It will be released in next 1.16

@memsharded memsharded merged commit 5e59126 into conan-io:develop May 21, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.