Emit warning on env variable case mismatch #9169

ghost · 2021-02-13T11:01:53Z

When running a command like cargo --target TRIPPLE cargo expects to find the environment variable CARGO_TARGET_[TRIPPLE]_* with uppercase and underscores. This check emits a warning if the checked environment variable has a mismatching case and/or contains dashes rather than underscores. The warning contains the given env variable as well as an explanation for the cause of the warning.

The check is skipped on windows as environment variables are treated as case insensitive on the platform.

Fixes #8285

rust-highfive · 2021-02-13T11:01:55Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ehuss · 2021-02-13T18:47:20Z

r? @ehuss

ehuss · 2021-02-13T18:50:58Z

src/cargo/util/config/mod.rs

@@ -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>,


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

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.

Oh, sorry, I missed that!

ehuss · 2021-02-13T19:00:07Z

src/cargo/util/config/mod.rs

+        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();
+        }


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() };

ehuss · 2021-02-13T19:01:40Z

src/cargo/util/config/mod.rs

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


This needs a backslash to continue on the next line.

Suggested change

"Variables in environment require uppercase,

"Variables in environment require uppercase, \

ehuss · 2021-02-13T19:02:54Z

src/cargo/util/config/mod.rs

+        if cfg!(windows) {
+            return;


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 😄.)

ehuss · 2021-02-13T19:19:56Z

src/cargo/util/config/mod.rs

+            Some(env_key) => {
+                let _ = self.shell().warn(format!(
+                    "Variables in environment require uppercase,
+                        but given variable: {}, contains lowercase or dash.",


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.",

ehuss · 2021-02-13T19:20:37Z

src/cargo/util/config/mod.rs

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


Just a stylistic suggestion:

Suggested change

"Variables in environment require uppercase,

"Environment variables require uppercase,

ehuss · 2021-02-13T19:21:40Z

tests/testsuite/tool_paths.rs

@@ -353,6 +353,29 @@ fn custom_linker_env() {
        .run();
 }

+#[cargo_test]
+#[cfg(not(windows))]


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

lukaslueg · 2021-02-13T23:15:44Z

src/cargo/util/config/mod.rs

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


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".

ehuss · 2021-02-14T19:35:16Z

Thanks! I pushed a small change to combine the two tests into one.

@bors r+

bors · 2021-02-14T19:35:17Z

📌 Commit 0564466 has been approved by ehuss

bors · 2021-02-14T19:35:25Z

⌛ Testing commit 0564466 with merge af564b2...

bors · 2021-02-14T20:17:47Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing af564b2 to master...

Update cargo 8 commits in ab64d1393b5b77c66b6534ef5023a1b89ee7bf64..bf5a5d5e5d3ae842a63bfce6d070dfd438cf6070 2021-02-10 00:19:10 +0000 to 2021-02-18 15:49:14 +0000 - Propagate `lto=off` harder (rust-lang/cargo#9182) - refactor: make deref intentions more straightforward (rust-lang/cargo#9183) - Update link for no_std attribute. (rust-lang/cargo#9174) - Remove mention of --message-format taking multiple values (rust-lang/cargo#9173) - Emit warning on env variable case mismatch (rust-lang/cargo#9169) - Implement Rustdoc versioning checks (rust-lang/cargo#8640) - Bump to 0.53.0, update changelog (rust-lang/cargo#9168) - Prevent testsuite from loading config out of sandbox. (rust-lang/cargo#9164)

Nick Flueckiger added 2 commits February 13, 2021 11:46

Implement uppercase env map for key case mismatch

11c82a8

Exclude test on windows

a7ae635

rust-highfive assigned Eh2406 Feb 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2021

Nick Flueckiger added 2 commits February 13, 2021 12:14

Lint

dd4b95f

Correct target

6ea8abc

rust-highfive assigned ehuss and unassigned Eh2406 Feb 13, 2021

ehuss reviewed Feb 13, 2021

View reviewed changes

Nick Flueckiger added 2 commits February 13, 2021 21:17

Adapt implementation with reviewer suggestions and requested changes

6af3194

Update windows check

c863102

lukaslueg reviewed Feb 13, 2021

View reviewed changes

Nick Flueckiger and others added 4 commits February 14, 2021 00:20

Fix windows test case

86c6e42

Update wording and use if let

d3aed35

Re-add backticks

4234077

Combine target env tests.

0564466

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2021

bors merged commit af564b2 into rust-lang:master Feb 14, 2021

ehuss mentioned this pull request Feb 18, 2021

Update cargo rust-lang/rust#82275

Merged

ehuss added this to the 1.52.0 milestone Feb 6, 2022

ehuss mentioned this pull request Mar 16, 2023

Handle case mismatches when looking up env vars in the Config snapshot #11824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit warning on env variable case mismatch #9169

Emit warning on env variable case mismatch #9169

ghost commented Feb 13, 2021

rust-highfive commented Feb 13, 2021

ehuss commented Feb 13, 2021

ehuss Feb 13, 2021

ghost Feb 13, 2021 •

edited by ghost

Loading

ehuss Feb 13, 2021

ehuss Feb 13, 2021

ehuss Feb 13, 2021

ehuss Feb 13, 2021

ehuss Feb 13, 2021

ehuss Feb 13, 2021

ehuss Feb 13, 2021

lukaslueg Feb 13, 2021

ehuss commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

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

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

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

Emit warning on env variable case mismatch #9169

Emit warning on env variable case mismatch #9169

Conversation

ghost commented Feb 13, 2021

rust-highfive commented Feb 13, 2021

ehuss commented Feb 13, 2021

Choose a reason for hiding this comment

ghost Feb 13, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

ghost Feb 13, 2021 •

edited by ghost

Loading