Explicitly configure noproxy support in Curl
#6115
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @ehuss |
src/cargo/ops/registry.rs
Outdated
| @@ -388,6 +388,13 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< | |||
| } else { | |||
| handle.useragent(&version().to_string())?; | |||
| } | |||
|
|
|||
| let noproxy = env::var("NO_PROXY") | |||
There was a problem hiding this comment.
Should this be NO_PROXY or no_proxy?
There was a problem hiding this comment.
Oh this is just respecting curl's existing convention with the env var NO_PROXY. Although did you mean the local variable should be called no_proxy (aka add an underscore?)
There was a problem hiding this comment.
The environment variables in #6114 used no_proxy rather than NO_PROXY. IIRC curl has a couple of other mysteriously lower case variables as well?
There was a problem hiding this comment.
Sure we can add support for both, curl does indeed support both
Apparently if we don't do it then our configuration otherwise means it won't automatically get picked up! Closes rust-lang#6114
| @@ -34,6 +34,8 @@ system: | |||
| will otherwise be used. | |||
| * `CARGO_CACHE_RUSTC_INFO` — If this is set to 0 then Cargo will not try to cache | |||
| compiler version information. | |||
| * `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` - these variables are all ready by | |||
There was a problem hiding this comment.
ready → read
trafic → traffic
Also, it's a bit redundant to say "read by Cargo" since all of these vars are read by cargo. I would split NO_PROXY into a separate entry, and explain its format a little.
| @@ -388,6 +388,14 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult< | |||
| } else { | |||
| handle.useragent(&version().to_string())?; | |||
| } | |||
|
|
|||
| let noproxy = env::var("NO_PROXY").ok() | |||
There was a problem hiding this comment.
I'm not sure I understand the original motivation for this. It looks like libcurl reads these environment variables automatically (https://github.com/curl/curl/blob/835a2fe694214341b601b2439082a30a24cc9da9/lib/url.c#L2707-L2723) and has for a while (though it looks like its been changed a bit over the years). Testing on my system, stable cargo reads them just fine. I have libcurl 7.54 which isn't exactly new.
What exactly is the issue here? I'm concerned without understanding why it doesn't work for the original reporter that this might not fix it for them. Maybe they have an old version of libcurl? Or a misconfiguration?
There was a problem hiding this comment.
I am the original reporter, and I have tried to get the variable to work for me, but it just doesn't. Neither on my mac, or in a CI environment with CentOS 7. Here is the version of libraries that Cargo is using:
λ otool -L ~/.cargo/bin/cargo
/Users/nate/.cargo/bin/cargo:
/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 57740.60.18)
/usr/lib/libcurl.4.dylib (compatibility version 7.0.0, current version 9.0.0)
/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.8)
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1349.8.0)
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.60.2)
/usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 1.0.0)
There was a problem hiding this comment.
You know what, I may have been using lowercased no_proxy. Let me try tomorrow with NO_PROXY
There was a problem hiding this comment.
@natemara any luck with tests to see how it went?
There was a problem hiding this comment.
@alexcrichton here are some logging traces that I captured. There is one trace in a completely clean environment (with env -i), and one with proxy environment variables set, and NO_PROXY, and no_proxy set to git.corp.com. The one with proxy variables fails, and the one without proxy variables succeeds to download this dependency. I also used mitmproxy to see what was going on with the requests - it looks like the requests are not proxied when NO_PROXY is set, but the package still fails to download. Is there some different logic that activates if any of the proxy variables are set?
https://gist.github.com/natemara/817bb64a08f8403305cf0563768ca51c
There was a problem hiding this comment.
Adding .git should fix it, but that is chopped off at https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/git/source.rs#L102.
There was a problem hiding this comment.
And yes, I believe our git server (gitlab) requires the .git at the end.
There was a problem hiding this comment.
If I comment out the block that I linked above, it works perfectly.
There was a problem hiding this comment.
@natemara do you have other dependencies on the same crate elsewhere? That canonicalization is purely used for hashing and comparison, it's not actually used for fetching dependencies. Instead what's written down exactly in Cargo.toml is used to fetch dependencies. If you have one dependency on /logger and one on /logger.git (for example) then they'll has to the same bucket and resolve to the same dependency (and sort of nondeterministically pick one of the dependencies to fetch).
I don't really understand how commenting out the logic would fix anything because we don't use the "canonical URL" to actually fetch anything. Do you know of any other dependencies active throughout the system on this /logger.git url?
There was a problem hiding this comment.
My mistake - when I got it to work I had the proxy settings disabled. I think I've made a small example of my problem that can be distributed so that you can see the behavior for yourself. If you download this project https://gitlab.com/natemara/cargo-proxy-consumer, then run the run script in the root dir, you should be able to download the dependency just fine. If you uncomment lines 10-15 and run it again, the download should fail with 404s.
Were you able to reproduce this?
Apparently if we don't do it then our configuration otherwise means it
won't automatically get picked up!
Closes #6114