Skip to content

Should cargo config get print warning if proxy config found in git/libcurl ? #12572

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

Open
Carreau opened this issue Aug 27, 2023 · 3 comments
Open
Labels
A-configuration Area: cargo config files and env vars C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-medium Experience: Medium S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. Z-cargo-config Nightly: cargo config subcommand

Comments

@Carreau
Copy link

Carreau commented Aug 27, 2023

Problem

In #12566 it was slightly challenging to diagnose the issue as cargo +nightly -Zunstable-options config get http.proxy would say error: config value http.proxy is not set while proxy value was read from git configuration.

Proposed Solution

cargo config get already have logic and maybe_env utils function to print things like:

The following environment variables may affect the loaded values

pub fn http_proxy_exists(http: &CargoHttpConfig, config: &Config) -> bool also exists and has a more reliable logic as to wether a proxy is set.

In pub fn get I suggest to peak at the requested key and if it is either http or http.proxy do something similar at maybe_env, and print a warning like:

http.proxy may be affected by your git configuration or one of "http_proxy", "HTTP_PROXY", "https_proxy", "HTTPS_PROXY" env variable.

Notes

We can even likely modify proxy.rs, with crate-private functions to tell us where the proxy is set and have --show-origin works as well.

@Carreau Carreau added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Aug 27, 2023
@weihanglo
Copy link
Member

weihanglo commented Aug 27, 2023

Thanks for the proposal. It is indeed a great idea to have a custom message for each key. Since cargo config get is still unstable, I lean towards not adding ad-hoc message at this moment.

#12087 is a relevant issue that a config value is intertwined with other environment variables. I wonder if we can revise the way we document environment variable for all config values. Like, only show the CARGO_* environment variable that is part of Cargo's configuration system. For config values that may be affected by other environments, just document the precedence explicitly and clearly.


Let's focus on documentation first!

triage:

@rustbot label -S-triage +S-documenting-cargo-itself +A-configuration +S-needs-design +E-medium

@weihanglo weihanglo added A-documenting-cargo-itself Area: Cargo's documentation A-configuration Area: cargo config files and env vars S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. E-medium Experience: Medium and removed S-triage Status: This issue is waiting on initial triage. labels Aug 27, 2023
@Carreau
Copy link
Author

Carreau commented Aug 28, 2023

For config values that may be affected by other environments, just document the precedence explicitly and clearly.

I think this is already clearly documented:

Sets an HTTP and HTTPS proxy to use. The format is in [libcurl format] as in
`[protocol://]host[:port]`. If not set, Cargo will also check the `http.proxy`
setting in your global git configuration. If none of those are set, the
`HTTPS_PROXY` or `https_proxy` environment variables set the proxy for HTTPS
requests, and `http_proxy` sets it for HTTP requests.

With one caveat, it says global configuration while I believe it also looks at local/.git/config

The precedence of CARGO_* is also quite explicit:

[...] the environment variable `CARGO_FOO_BAR` [...] 

Environment variables will take precedence over TOML configuration files. [...]

Since cargo config get is still unstable, I lean towards not adding ad-hoc message at this moment.

... Or do you mean "documenting in the output of cargo config get ?

@weihanglo
Copy link
Member

I am thinking of making changes to configuration.md in general. But you're right, this issue be scoped in proxies itself. For general doc enhancements I should have opened a new issue.

Okay. I'll let this remain open for ad-hoc warning message.

@rustbot label -A-documenting-cargo-itself +Z-cargo-config

@rustbot rustbot added Z-cargo-config Nightly: cargo config subcommand and removed A-documenting-cargo-itself Area: Cargo's documentation labels Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` E-medium Experience: Medium S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. Z-cargo-config Nightly: cargo config subcommand
Projects
None yet
Development

No branches or pull requests

3 participants