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

Emit warning on env variable case mismatch #9169

Merged
merged 10 commits into from Feb 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ pub struct Config {
target_dir: Option<Filesystem>,
/// Environment variables, separated to assist testing.
env: HashMap<String, String>,
/// Environment variables, converted to uppercase to check for case mismatch
upper_case_env: HashMap<String, String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using a HashSet should be sufficient here, since it doesn't need the values.

Copy link
Author

@ghost ghost Feb 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention behind using a HashMap instead of a HashSet was to use the value for the warning output.

Taking key.as_env_key() will result in the 'correct' key format,
whereas the value in the HashMap will be the actual user defined key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I missed that!

/// Tracks which sources have been updated to avoid multiple updates.
updated_sources: LazyCell<RefCell<HashSet<SourceId>>>,
/// Lock, if held, of the global package cache along with the number of
Expand Down Expand Up @@ -211,6 +213,15 @@ impl Config {
})
.collect();

let upper_case_env = if cfg!(windows) {
HashMap::new()
} else {
env.clone()
.into_iter()
.map(|(k, _)| (k.to_uppercase().replace("-", "_"), k))
.collect()
};

let cache_rustc_info = match env.get("CARGO_CACHE_RUSTC_INFO") {
Some(cache) => cache != "0",
_ => true,
Expand Down Expand Up @@ -244,6 +255,7 @@ impl Config {
creation_time: Instant::now(),
target_dir: None,
env,
upper_case_env,
updated_sources: LazyCell::new(),
package_cache_lock: RefCell::new(None),
http_config: LazyCell::new(),
Expand Down Expand Up @@ -525,7 +537,10 @@ impl Config {
definition,
}))
}
None => Ok(None),
None => {
self.check_environment_key_case_mismatch(key);
Ok(None)
}
}
}

Expand All @@ -545,9 +560,27 @@ impl Config {
return true;
}
}
self.check_environment_key_case_mismatch(key);

false
}

fn check_environment_key_case_mismatch(&self, key: &ConfigKey) {
if cfg!(windows) {
// In the case of windows the check for case mismatch in keys can be skipped
// as windows already converts its environment keys into the desired format.
return;
}

if let Some(env_key) = self.upper_case_env.get(key.as_env_key()) {
let _ = self.shell().warn(format!(
"Environment variables are expected to use uppercase letters and underscores, \
the variable `{}` will be ignored and have no effect",
env_key
));
}
}

/// Get a string config value.
///
/// See `get` for more details.
Expand Down Expand Up @@ -640,7 +673,10 @@ impl Config {
) -> CargoResult<()> {
let env_val = match self.env.get(key.as_env_key()) {
Some(v) => v,
None => return Ok(()),
None => {
self.check_environment_key_case_mismatch(key);
return Ok(());
}
};

let def = Definition::Environment(key.as_env_key().to_string());
Expand Down
25 changes: 25 additions & 0 deletions tests/testsuite/tool_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,31 @@ fn custom_linker_env() {
.run();
}

#[cargo_test]
fn target_in_environment_contains_lower_case() {
let p = project().file("src/main.rs", "fn main() {}").build();

let target_keys = [
"CARGO_TARGET_X86_64_UNKNOWN_LINUX_musl_LINKER",
"CARGO_TARGET_x86_64_unknown_linux_musl_LINKER",
];

for target_key in &target_keys {
let mut execs = p.cargo("build -v --target x86_64-unknown-linux-musl");
execs.env(target_key, "nonexistent-linker").with_status(101);
if cfg!(windows) {
execs.with_stderr_does_not_contain("warning:[..]");
} else {
execs.with_stderr_contains(format!(
"warning: Environment variables are expected to use uppercase letters and underscores, \
the variable `{}` will be ignored and have no effect",
target_key
));
}
execs.run();
}
}

#[cargo_test]
fn cfg_ignored_fields() {
// Test for some ignored fields in [target.'cfg()'] tables.
Expand Down