Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
41 changes: 39 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,16 @@ impl Config {
})
.collect();

let mut upper_case_env: HashMap<String, String> = HashMap::new();

if !cfg!(windows) {
upper_case_env = env
.clone()
.into_iter()
.map(|(k, _)| (k.to_uppercase().replace("-", "_"), k))
.collect();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a minor stylistic choice, but I think it is a little clearer to initialize only once, something like this:

        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 +256,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 +538,10 @@ impl Config {
definition,
}))
}
None => Ok(None),
None => {
self.check_environment_key_case_mismatch(key);
Ok(None)
}
}
}

Expand All @@ -545,9 +561,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) {
return;
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 it might be good to have a comment here explaining why this is ignored on windows (for those who may not be familiar that windows automatically ASCII upper-cases keys).

(Hopefully nobody uses lower-case non-ASCII characters 😄.)

}
match self.upper_case_env.get(key.as_env_key()) {
Some(env_key) => {
let _ = self.shell().warn(format!(
"Variables in environment require uppercase,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a backslash to continue on the next line.

Suggested change
"Variables in environment require uppercase,
"Variables in environment require uppercase, \

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a stylistic suggestion:

Suggested change
"Variables in environment require uppercase,
"Environment variables require uppercase,

but given variable: {}, contains lowercase or dash.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically Cargo's error message style uses backticks to wrap a literal value.

Suggested change
but given variable: {}, contains lowercase or dash.",
but the variable `{}` contains lowercase letters or dash.",

env_key
));
}
None => {}
}
}

/// Get a string config value.
///
/// See `get` for more details.
Expand Down Expand Up @@ -640,7 +674,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
23 changes: 23 additions & 0 deletions tests/testsuite/tool_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,29 @@ 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: Variables in environment require uppercase,
but given variable: {}, contains lowercase or dash.",
target_key
))
.run();
}
}

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