Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
43 changes: 41 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,30 @@ 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;
}

match self.upper_case_env.get(key.as_env_key()) {
Some(env_key) => {
let _ = self.shell().warn(format!(
"Environment variables require uppercase letters, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the language can be improved here: "Environment variables are expected to use uppercase letters and underscores, the variable {} will be ignored and have no effect".

but the variable: `{}` contains lowercase letters or dashes.",
env_key
));
}
None => {}
}
}

/// Get a string config value.
///
/// See `get` for more details.
Expand Down Expand Up @@ -640,7 +676,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
41 changes: 41 additions & 0 deletions tests/testsuite/tool_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,47 @@ fn custom_linker_env() {
.run();
}

#[cargo_test]
#[cfg(not(windows))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add a test to ensure that windows doesn't emit the warning, and works with a lowercase value?

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 {
p.cargo("build -v --target x86_64-unknown-linux-musl")
.env(target_key, "nonexistent-linker")
.with_status(101)
.with_stderr_contains(format!(
"warning: Environment variables require uppercase letters, \
but the variable: `{}` contains lowercase letters or dashes.",
target_key
))
.run();
}
}

#[cargo_test]
#[cfg(windows)]
fn target_in_environment_contains_lower_case_on_windows() {
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 {
p.cargo("build -v --target x86_64-unknown-linux-musl")
.env(target_key, "nonexistent-linker")
.with_status(0)
.run();
}
}

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