From 182c8f8689b103334d6747a33dc705d4c130baa1 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Sun, 22 Aug 2021 15:26:12 -0700 Subject: [PATCH 1/5] Env vars like $(FOO) work in paths now --- src/cargo/util/env_vars.rs | 185 ++++++++++++++++++++++++++++++++++++ src/cargo/util/mod.rs | 1 + src/cargo/util/toml/mod.rs | 13 +-- tests/testsuite/main.rs | 1 + tests/testsuite/var_refs.rs | 117 +++++++++++++++++++++++ 5 files changed, 311 insertions(+), 6 deletions(-) create mode 100644 src/cargo/util/env_vars.rs create mode 100644 tests/testsuite/var_refs.rs diff --git a/src/cargo/util/env_vars.rs b/src/cargo/util/env_vars.rs new file mode 100644 index 00000000000..cf90a488e2a --- /dev/null +++ b/src/cargo/util/env_vars.rs @@ -0,0 +1,185 @@ +//! Expands environment variable references in strings. + +use crate::util::CargoResult; +use std::borrow::Cow; + +pub fn expand_env_vars<'a>(s: &'a str) -> CargoResult> { + expand_env_vars_with(s, |n| std::env::var(n).ok()) +} + +pub fn expand_env_vars_with<'a, Q>(s: &'a str, query: Q) -> CargoResult> +where + Q: Fn(&str) -> Option, +{ + // Most strings do not contain environment variable references. + // We optimize for the case where there is no reference, and + // return the same (borrowed) string. + if s.contains('$') { + Ok(Cow::Owned(expand_env_vars_with_slow(s, query)?)) + } else { + Ok(Cow::Borrowed(s)) + } +} + +fn expand_env_vars_with_slow(s: &str, query: Q) -> CargoResult +where + Q: Fn(&str) -> Option, +{ + let mut result = String::with_capacity(s.len() + 50); + let mut rest = s; + while let Some(pos) = rest.find('$') { + let (lo, hi) = rest.split_at(pos); + result.push_str(lo); + let mut ci = hi.chars(); + let c0 = ci.next(); + debug_assert_eq!(c0, Some('$')); // because rest.find() + match ci.next() { + Some('(') => { + // the expected case, which is handled below. + } + Some(c) => { + // We found '$' that was not paired with '('. + // This is not a variable reference. + // Output the $ and continue. + result.push('$'); + result.push(c); + rest = ci.as_str(); + continue; + } + None => { + // We found '$ at the end of the string. + result.push('$'); + break; + } + } + let name_start = ci.as_str(); + let mut name: &str = ""; + let mut default_value: Option<&str> = None; + // Look for ')' or '?' + loop { + let ci_s = ci.as_str(); + let pos = name_start.len() - ci.as_str().len(); + match ci.next() { + None => { + anyhow::bail!("environment variable reference is missing closing parenthesis.") + } + Some(')') => { + match default_value { + Some(d) => default_value = Some(&d[..d.len() - ci_s.len()]), + None => name = &name_start[..name_start.len() - ci_s.len()], + } + rest = ci.as_str(); + break; + } + Some('?') => { + if default_value.is_some() { + anyhow::bail!("environment variable reference has too many '?'"); + } + name = &name_start[..pos]; + default_value = Some(ci.as_str()); + } + Some(_) if default_value.is_some() => { + // consume this, for default value + } + Some(c) if is_legal_env_var_char(c) => { + // continue for next char + } + Some(c) => { + anyhow::bail!("environment variable reference has invalid character {:?}.", c); + } + } + } + + if name.is_empty() { + anyhow::bail!("environment variable reference has invalid empty name"); + } + // We now have the environment variable name, and have parsed the end of the + // name reference. + match (query(name), default_value) { + (Some(value), _) => result.push_str(&value), + (None, Some(value)) => result.push_str(value), + (None, None) => anyhow::bail!(format!( + "environment variable '{}' is not set and has no default value", + name + )), + } + } + result.push_str(rest); + + Ok(result) +} + +fn is_legal_env_var_char(c: char) -> bool { + match c { + 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_' => true, + _ => false, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + macro_rules! assert_str_contains { + ($haystack:expr, $needle:expr) => {{ + let haystack = $haystack; + let needle = $needle; + assert!( + haystack.contains(needle), + "expected {:?} to contain {:?}", + haystack, + needle + ); + }}; + } + + #[test] + fn basic() { + let query = |name: &str| match name { + "FOO" => Some("c:/foo".to_string()), + "BAR" => Some("c:/bar".to_string()), + _ => None, + }; + + let expand = |s| expand_env_vars_with(s, query); + let expand_ok = |s| match expand(s) { + Ok(value) => value, + Err(e) => panic!( + "expected '{}' to expand successfully, but failed: {:?}", + s, e + ), + }; + let expand_err = |s| expand(s).unwrap_err().to_string(); + assert_eq!(expand_ok(""), ""); + assert_eq!(expand_ok("identity"), "identity"); + assert_eq!(expand_ok("$(FOO)/some_package"), "c:/foo/some_package"); + assert_eq!(expand_ok("$FOO/some_package"), "$FOO/some_package"); + + assert_eq!(expand_ok("alpha $(FOO) beta"), "alpha c:/foo beta"); + assert_eq!( + expand_ok("one $(FOO) two $(BAR) three"), + "one c:/foo two c:/bar three" + ); + + assert_eq!(expand_ok("one $(FOO)"), "one c:/foo"); + + // has default, but value is present + assert_eq!(expand_ok("$(FOO?d:/default)"), "c:/foo"); + assert_eq!(expand_ok("$(ZAP?d:/default)"), "d:/default"); + + // error cases + assert_eq!( + expand_err("$(VAR_NOT_SET)"), + "environment variable 'VAR_NOT_SET' is not set and has no default value" + ); + + // invalid name + assert_str_contains!( + expand("$(111)").unwrap_err().to_string(), + "" // "environment variable reference has invalid character." + ); + + expand_err("$("); + expand_err("$(FOO"); + } +} diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 4b8604f92fb..1bb0c6afa07 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -36,6 +36,7 @@ pub mod cpu; mod dependency_queue; pub mod diagnostic_server; pub mod errors; +pub mod env_vars; mod flock; pub mod graph; mod hasher; diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 059c4480377..098f544221a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -252,18 +252,19 @@ impl<'de, P: Deserialize<'de>> de::Deserialize<'de> for TomlDependency

{ } pub trait ResolveToPath { - fn resolve(&self, config: &Config) -> PathBuf; + fn resolve(&self, config: &Config) -> CargoResult; } impl ResolveToPath for String { - fn resolve(&self, _: &Config) -> PathBuf { - self.into() + fn resolve(&self, _: &Config) -> CargoResult { + let expanded_path = crate::util::env_vars::expand_env_vars(self)?; + Ok(expanded_path.to_string().into()) } } impl ResolveToPath for ConfigRelativePath { - fn resolve(&self, c: &Config) -> PathBuf { - self.resolve_path(c) + fn resolve(&self, c: &Config) -> CargoResult { + Ok(self.resolve_path(c)) } } @@ -1887,7 +1888,7 @@ impl DetailedTomlDependency

{ SourceId::for_git(&loc, reference)? } (None, Some(path), _, _) => { - let path = path.resolve(cx.config); + let path = path.resolve(cx.config)?; cx.nested_paths.push(path.clone()); // If the source ID for the package we're parsing is a path // source, then we normalize the path here to get rid of diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 8c30bf929a8..710ed6ba680 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -123,6 +123,7 @@ mod tree; mod tree_graph_features; mod unit_graph; mod update; +mod var_refs; mod vendor; mod verify_project; mod version; diff --git a/tests/testsuite/var_refs.rs b/tests/testsuite/var_refs.rs new file mode 100644 index 00000000000..32ded169f64 --- /dev/null +++ b/tests/testsuite/var_refs.rs @@ -0,0 +1,117 @@ +//! Tests for variable references, e.g. { path = "$(FOO)/bar/foo" } +#![allow(unused_imports)] + +use cargo_test_support::registry::Package; +use cargo_test_support::{basic_lib_manifest, basic_manifest, git, project, sleep_ms}; +use std::env; +use std::fs; + +#[cargo_test] +fn var_refs() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["utils/zoo", "bar"] + "#, + ) + .file( + "utils/zoo/Cargo.toml", + r#" + [package] + name = "zoo" + version = "1.0.0" + edition = "2018" + authors = [] + + [lib] + "#, + ) + .file( + "utils/zoo/src/lib.rs", + r#" + pub fn hello() { println!("Hello, world!"); } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "1.0.0" + edition = "2018" + authors = [] + + [dependencies] + zoo = { path = "$(UTILS_ROOT)/zoo" } + "#, + ) + .file( + "bar/src/main.rs", + r#" + fn main() { + zoo::hello(); + } + "#, + ) + .build(); + + p.cargo("build") + .cwd("bar") + .env("UTILS_ROOT", "../utils") + .run(); + assert!(p.bin("bar").is_file()); +} + +#[cfg(todo)] // error propagation is not working correctly. +#[cargo_test] +fn var_refs_var_not_set() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + edition = "2018" + authors = [] + + [dependencies] + bar = { path = "$(BAD_VAR)/bar" } + "#, + ) + .file("src/lib.rs", r#""#) + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr_contains("environment variable 'BAD_VAR' is not set") + .run(); +} + +#[cfg(todo)] // error propagation is not working correctly. +#[cargo_test] +fn var_refs_bad_syntax() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + edition = "2018" + authors = [] + + [dependencies] + bar = { path = "$(BAD_VAR" } + "#, + ) + .file("src/lib.rs", r#""#) + .build(); + + p.cargo("build") + .with_status(101) + .with_stderr_contains("variable reference '$(FOO)' is missing closing parenthesis") + .run(); +} From a109c5fbf9ec1fc322d7fa19f61328cefffb0dcb Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Mon, 30 Aug 2021 11:53:39 -0700 Subject: [PATCH 2/5] Improve var refs, testing --- src/cargo/util/env_vars.rs | 286 ++++++++++++++++++++++-------------- tests/testsuite/var_refs.rs | 74 +++++++++- 2 files changed, 244 insertions(+), 116 deletions(-) diff --git a/src/cargo/util/env_vars.rs b/src/cargo/util/env_vars.rs index cf90a488e2a..aed1acc85f8 100644 --- a/src/cargo/util/env_vars.rs +++ b/src/cargo/util/env_vars.rs @@ -3,89 +3,116 @@ use crate::util::CargoResult; use std::borrow::Cow; +/// Expands a string, replacing references to environment variables with their +/// values. +/// +/// This function uses [expand_env_vars_with] to expand references to the +/// environment variables of the current process. pub fn expand_env_vars<'a>(s: &'a str) -> CargoResult> { - expand_env_vars_with(s, |n| std::env::var(n).ok()) + expand_vars_with(s, |n| std::env::var(n).ok()) } -pub fn expand_env_vars_with<'a, Q>(s: &'a str, query: Q) -> CargoResult> +/// Expands a string, replacing references to variables with values provided by +/// the caller. +/// +/// This function looks for references to variables, similar to environment +/// variable references in command-line shells or Makefiles, and replaces the +/// references with values. The caller provides a `query` function which gives +/// the values of the variables. +/// +/// The syntax used for variable references is `${name}` or ${name?default}` if +/// a default value is provided. The curly braces are always required; +/// `$FOO` will not be interpreted as a variable reference (and will be copied +/// to the output). +/// +/// If a variable is referenced, then it must have a value (`query` must return +/// `Some) or the variable reference must provide a default value (using the +/// `...?default` syntax). If `query` returns `None` and the variable reference +/// does not provide a default value, then the expansion of the entire string +/// will fail and the function will return `Err`. +/// +/// Most strings processed by Cargo will not contain variable references. +/// Hence, this function uses `Cow` for its return type; it will return +/// its input string as `Cow::Borrowed` if no variable references were found. +pub fn expand_vars_with<'a, Q>(s: &'a str, query: Q) -> CargoResult> where Q: Fn(&str) -> Option, { - // Most strings do not contain environment variable references. - // We optimize for the case where there is no reference, and - // return the same (borrowed) string. - if s.contains('$') { - Ok(Cow::Owned(expand_env_vars_with_slow(s, query)?)) + let mut rest: &str; + let mut result: String; + if let Some(pos) = s.find('$') { + result = String::with_capacity(s.len() + 50); + result.push_str(&s[..pos]); + rest = &s[pos..]; } else { - Ok(Cow::Borrowed(s)) - } -} + // Most strings do not contain environment variable references. + // We optimize for the case where there is no reference, and + // return the same (borrowed) string. + return Ok(Cow::Borrowed(s)); + }; -fn expand_env_vars_with_slow(s: &str, query: Q) -> CargoResult -where - Q: Fn(&str) -> Option, -{ - let mut result = String::with_capacity(s.len() + 50); - let mut rest = s; while let Some(pos) = rest.find('$') { - let (lo, hi) = rest.split_at(pos); - result.push_str(lo); - let mut ci = hi.chars(); - let c0 = ci.next(); + result.push_str(&rest[..pos]); + rest = &rest[pos..]; + let mut chars = rest.chars(); + let c0 = chars.next(); debug_assert_eq!(c0, Some('$')); // because rest.find() - match ci.next() { - Some('(') => { + match chars.next() { + Some('{') => { // the expected case, which is handled below. } Some(c) => { - // We found '$' that was not paired with '('. + // We found '$' that was not paired with '{'. // This is not a variable reference. // Output the $ and continue. result.push('$'); result.push(c); - rest = ci.as_str(); + rest = chars.as_str(); continue; } None => { - // We found '$ at the end of the string. + // We found '$' at the end of the string. result.push('$'); break; } } - let name_start = ci.as_str(); - let mut name: &str = ""; - let mut default_value: Option<&str> = None; - // Look for ')' or '?' + let name_start = chars.as_str(); + let name: &str; + let default_value: Option<&str>; + // Look for '}' or '?' loop { - let ci_s = ci.as_str(); - let pos = name_start.len() - ci.as_str().len(); - match ci.next() { + let pos = name_start.len() - chars.as_str().len(); + match chars.next() { None => { - anyhow::bail!("environment variable reference is missing closing parenthesis.") + anyhow::bail!("environment variable reference is missing closing brace.") } - Some(')') => { - match default_value { - Some(d) => default_value = Some(&d[..d.len() - ci_s.len()]), - None => name = &name_start[..name_start.len() - ci_s.len()], - } - rest = ci.as_str(); + Some('}') => { + name = &name_start[..pos]; + default_value = None; break; } Some('?') => { - if default_value.is_some() { - anyhow::bail!("environment variable reference has too many '?'"); - } name = &name_start[..pos]; - default_value = Some(ci.as_str()); - } - Some(_) if default_value.is_some() => { - // consume this, for default value - } - Some(c) if is_legal_env_var_char(c) => { - // continue for next char + let default_value_start = chars.as_str(); + loop { + let pos = chars.as_str(); + if let Some(c) = chars.next() { + if c == '}' { + default_value = Some( + &default_value_start[..default_value_start.len() - pos.len()], + ); + break; + } + } else { + anyhow::bail!( + "environment variable reference is missing closing brace." + ); + } + } + break; } - Some(c) => { - anyhow::bail!("environment variable reference has invalid character {:?}.", c); + Some(_) => { + // consume this character (as part of var name) } } } @@ -103,83 +130,126 @@ where name )), } + rest = chars.as_str(); } result.push_str(rest); - - Ok(result) -} - -fn is_legal_env_var_char(c: char) -> bool { - match c { - 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_' => true, - _ => false, - } + Ok(Cow::Owned(result)) } #[cfg(test)] mod tests { use super::*; - macro_rules! assert_str_contains { - ($haystack:expr, $needle:expr) => {{ - let haystack = $haystack; - let needle = $needle; - assert!( - haystack.contains(needle), - "expected {:?} to contain {:?}", - haystack, - needle - ); - }}; - } - #[test] fn basic() { let query = |name: &str| match name { - "FOO" => Some("c:/foo".to_string()), - "BAR" => Some("c:/bar".to_string()), + "FOO" => Some("/foo".to_string()), + "BAR" => Some("/bar".to_string()), + "FOO(ZAP)" => Some("/foo/zap".to_string()), + "WINKING_FACE" => Some("\u{1F609}".to_string()), + "\u{1F916}" => Some("ROBOT FACE".to_string()), _ => None, }; - let expand = |s| expand_env_vars_with(s, query); - let expand_ok = |s| match expand(s) { - Ok(value) => value, - Err(e) => panic!( - "expected '{}' to expand successfully, but failed: {:?}", - s, e - ), - }; - let expand_err = |s| expand(s).unwrap_err().to_string(); - assert_eq!(expand_ok(""), ""); - assert_eq!(expand_ok("identity"), "identity"); - assert_eq!(expand_ok("$(FOO)/some_package"), "c:/foo/some_package"); - assert_eq!(expand_ok("$FOO/some_package"), "$FOO/some_package"); - - assert_eq!(expand_ok("alpha $(FOO) beta"), "alpha c:/foo beta"); - assert_eq!( - expand_ok("one $(FOO) two $(BAR) three"), - "one c:/foo two c:/bar three" - ); + let expand = |s| expand_vars_with(s, query); - assert_eq!(expand_ok("one $(FOO)"), "one c:/foo"); + macro_rules! case { + ($input:expr, $expected_output:expr) => {{ + let input = $input; + let expected_output = $expected_output; + match expand(input) { + Ok(output) => { + assert_eq!(output, expected_output, "input = {:?}", input); + } + Err(e) => { + panic!( + "Expected string {:?} to expand successfully, but it failed: {:?}", + input, e + ); + } + } + }}; + } + + macro_rules! err_case { + ($input:expr, $expected_error:expr) => {{ + let input = $input; + let expected_error = $expected_error; + match expand(input) { + Ok(output) => { + panic!("Expected expansion of string {:?} to fail, but it succeeded with value {:?}", input, output); + } + Err(e) => { + let message = e.to_string(); + assert_eq!(message, expected_error, "input = {:?}", input); + } + } + }} + } + + // things without references should not change. + case!("", ""); + case!("identity", "identity"); + + // we require ${...} (braces), so we ignore $FOO. + case!("$FOO/some_package", "$FOO/some_package"); + + // make sure variable references at the beginning, middle, and end + // of a string all work correctly. + case!("${FOO}", "/foo"); + case!("${FOO} one", "/foo one"); + case!("one ${FOO}", "one /foo"); + case!("one ${FOO} two", "one /foo two"); + case!("one ${FOO} two ${BAR} three", "one /foo two /bar three"); + + // variable names can contain most characters, except for '}' or '?' + // (Windows sets "ProgramFiles(x86)", for example.) + case!("${FOO(ZAP)}", "/foo/zap"); - // has default, but value is present - assert_eq!(expand_ok("$(FOO?d:/default)"), "c:/foo"); - assert_eq!(expand_ok("$(ZAP?d:/default)"), "d:/default"); + // variable is set, and has a default (which goes unused) + case!("${FOO?/default}", "/foo"); - // error cases - assert_eq!( - expand_err("$(VAR_NOT_SET)"), + // variable is not set, but does have default + case!("${VAR_NOT_SET?/default}", "/default"); + + // variable is not set and has no default + err_case!( + "${VAR_NOT_SET}", "environment variable 'VAR_NOT_SET' is not set and has no default value" ); - // invalid name - assert_str_contains!( - expand("$(111)").unwrap_err().to_string(), - "" // "environment variable reference has invalid character." - ); + // environment variables with unicode values are ok + case!("${WINKING_FACE}", "\u{1F609}"); + + // strings with unicode in them are ok + case!("\u{1F609}${FOO}", "\u{1F609}/foo"); + + // environment variable names with unicode in them are ok + case!("${\u{1F916}}", "ROBOT FACE"); + + // default values with unicode in them are ok + case!("${VAR_NOT_SET?\u{1F916}}", "\u{1F916}"); - expand_err("$("); - expand_err("$(FOO"); + // invalid names + err_case!( + "${}", + "environment variable reference has invalid empty name" + ); + err_case!( + "${?default}", + "environment variable reference has invalid empty name" + ); + err_case!( + "${", + "environment variable reference is missing closing brace." + ); + err_case!( + "${FOO", + "environment variable reference is missing closing brace." + ); + err_case!( + "${FOO?default", + "environment variable reference is missing closing brace." + ); } } diff --git a/tests/testsuite/var_refs.rs b/tests/testsuite/var_refs.rs index 32ded169f64..432c7b3985f 100644 --- a/tests/testsuite/var_refs.rs +++ b/tests/testsuite/var_refs.rs @@ -1,4 +1,4 @@ -//! Tests for variable references, e.g. { path = "$(FOO)/bar/foo" } +//! Tests for variable references, e.g. { path = "${FOO}/bar/foo" } #![allow(unused_imports)] use cargo_test_support::registry::Package; @@ -7,7 +7,7 @@ use std::env; use std::fs; #[cargo_test] -fn var_refs() { +fn basic() { let p = project() .file( "Cargo.toml", @@ -44,7 +44,7 @@ fn var_refs() { authors = [] [dependencies] - zoo = { path = "$(UTILS_ROOT)/zoo" } + zoo = { path = "${UTILS_ROOT}/zoo" } "#, ) .file( @@ -64,9 +64,67 @@ fn var_refs() { assert!(p.bin("bar").is_file()); } +#[cargo_test] +fn basic_with_default() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["utils/zoo", "bar"] + "#, + ) + .file( + "utils/zoo/Cargo.toml", + r#" + [package] + name = "zoo" + version = "1.0.0" + edition = "2018" + authors = [] + + [lib] + "#, + ) + .file( + "utils/zoo/src/lib.rs", + r#" + pub fn hello() { println!("Hello, world!"); } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "1.0.0" + edition = "2018" + authors = [] + + [dependencies] + zoo = { path = "${UTILS_ROOT?../utils}/zoo" } + "#, + ) + .file( + "bar/src/main.rs", + r#" + fn main() { + zoo::hello(); + } + "#, + ) + .build(); + + // Note: UTILS_ROOT is not set in the environment. + p.cargo("build") + .cwd("bar") + .run(); + assert!(p.bin("bar").is_file()); +} + #[cfg(todo)] // error propagation is not working correctly. #[cargo_test] -fn var_refs_var_not_set() { +fn var_not_set() { let p = project() .file( "Cargo.toml", @@ -78,7 +136,7 @@ fn var_refs_var_not_set() { authors = [] [dependencies] - bar = { path = "$(BAD_VAR)/bar" } + bar = { path = "${BAD_VAR}/bar" } "#, ) .file("src/lib.rs", r#""#) @@ -92,7 +150,7 @@ fn var_refs_var_not_set() { #[cfg(todo)] // error propagation is not working correctly. #[cargo_test] -fn var_refs_bad_syntax() { +fn bad_syntax() { let p = project() .file( "Cargo.toml", @@ -104,7 +162,7 @@ fn var_refs_bad_syntax() { authors = [] [dependencies] - bar = { path = "$(BAD_VAR" } + bar = { path = "${BAD_VAR" } "#, ) .file("src/lib.rs", r#""#) @@ -112,6 +170,6 @@ fn var_refs_bad_syntax() { p.cargo("build") .with_status(101) - .with_stderr_contains("variable reference '$(FOO)' is missing closing parenthesis") + .with_stderr_contains("environment variable reference is missing closing brace.") .run(); } From 4f96de6d13d2359f56c1a43261b2a94fa79a0fac Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Mon, 30 Aug 2021 12:40:48 -0700 Subject: [PATCH 3/5] rustfmt --- src/cargo/util/env_vars.rs | 6 +++--- src/cargo/util/mod.rs | 2 +- tests/testsuite/var_refs.rs | 6 ++---- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/env_vars.rs b/src/cargo/util/env_vars.rs index aed1acc85f8..b60788bcb51 100644 --- a/src/cargo/util/env_vars.rs +++ b/src/cargo/util/env_vars.rs @@ -20,13 +20,13 @@ pub fn expand_env_vars<'a>(s: &'a str) -> CargoResult> { /// references with values. The caller provides a `query` function which gives /// the values of the variables. /// -/// The syntax used for variable references is `${name}` or ${name?default}` if -/// a default value is provided. The curly braces are always required; +/// The syntax used for variable references is `${name}` or `${name?default}` if +/// a default value is provided. The curly braces are always required; /// `$FOO` will not be interpreted as a variable reference (and will be copied /// to the output). /// /// If a variable is referenced, then it must have a value (`query` must return -/// `Some) or the variable reference must provide a default value (using the +/// `Some`) or the variable reference must provide a default value (using the /// `...?default` syntax). If `query` returns `None` and the variable reference /// does not provide a default value, then the expansion of the entire string /// will fail and the function will return `Err`. diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 1bb0c6afa07..b23ff89c848 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -35,8 +35,8 @@ mod counter; pub mod cpu; mod dependency_queue; pub mod diagnostic_server; -pub mod errors; pub mod env_vars; +pub mod errors; mod flock; pub mod graph; mod hasher; diff --git a/tests/testsuite/var_refs.rs b/tests/testsuite/var_refs.rs index 432c7b3985f..0274ca0f4d5 100644 --- a/tests/testsuite/var_refs.rs +++ b/tests/testsuite/var_refs.rs @@ -115,10 +115,8 @@ fn basic_with_default() { ) .build(); - // Note: UTILS_ROOT is not set in the environment. - p.cargo("build") - .cwd("bar") - .run(); + // Note: UTILS_ROOT is not set in the environment. + p.cargo("build").cwd("bar").run(); assert!(p.bin("bar").is_file()); } From 1baae565e4f5aba8cbbb63e4ca304a4aba1272d8 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 31 Aug 2021 12:05:49 -0700 Subject: [PATCH 4/5] Make expand-env-vars unstable feature --- src/cargo/core/features.rs | 4 +++ src/cargo/util/env_vars.rs | 13 ++----- src/cargo/util/toml/mod.rs | 53 +++++++++++++++++++++++---- src/doc/src/reference/unstable.md | 23 ++++++++++++ tests/testsuite/var_refs.rs | 60 +++++++++++++++++++++++++------ 5 files changed, 125 insertions(+), 28 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index a1a3e0797e8..34cdc898ae7 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -408,6 +408,8 @@ features! { // Allow specifying different binary name apart from the crate name (unstable, different_binary_name, "", "reference/unstable.html#different-binary-name"), + + (unstable, expand_env_vars, "", "reference/unstable.html#expand-env-vars"), } pub struct Feature { @@ -656,6 +658,7 @@ unstable_cli_options!( unstable_options: bool = ("Allow the usage of unstable options"), weak_dep_features: bool = ("Allow `dep_name?/feature` feature syntax"), skip_rustdoc_fingerprint: bool = (HIDDEN), + expand_env_vars: bool = ("Expand environment variable references like ${FOO} in certain contexts, such as `path = \"...\"` in dependencies."), ); const STABILIZED_COMPILE_PROGRESS: &str = "The progress bar is now always \ @@ -878,6 +881,7 @@ impl CliUnstable { "extra-link-arg" => stabilized_warn(k, "1.56", STABILIZED_EXTRA_LINK_ARG), "configurable-env" => stabilized_warn(k, "1.56", STABILIZED_CONFIGURABLE_ENV), "future-incompat-report" => self.future_incompat_report = parse_empty(k, v)?, + "expand-env-vars" => self.expand_env_vars = parse_empty(k, v)?, _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/util/env_vars.rs b/src/cargo/util/env_vars.rs index b60788bcb51..5dcdd422ffb 100644 --- a/src/cargo/util/env_vars.rs +++ b/src/cargo/util/env_vars.rs @@ -3,15 +3,6 @@ use crate::util::CargoResult; use std::borrow::Cow; -/// Expands a string, replacing references to environment variables with their -/// values. -/// -/// This function uses [expand_env_vars_with] to expand references to the -/// environment variables of the current process. -pub fn expand_env_vars<'a>(s: &'a str) -> CargoResult> { - expand_vars_with(s, |n| std::env::var(n).ok()) -} - /// Expands a string, replacing references to variables with values provided by /// the caller. /// @@ -36,7 +27,7 @@ pub fn expand_env_vars<'a>(s: &'a str) -> CargoResult> { /// its input string as `Cow::Borrowed` if no variable references were found. pub fn expand_vars_with<'a, Q>(s: &'a str, query: Q) -> CargoResult> where - Q: Fn(&str) -> Option, + Q: Fn(&str) -> CargoResult>, { let mut rest: &str; let mut result: String; @@ -122,7 +113,7 @@ where } // We now have the environment variable name, and have parsed the end of the // name reference. - match (query(name), default_value) { + match (query(name)?, default_value) { (Some(value), _) => result.push_str(&value), (None, Some(value)) => result.push_str(value), (None, None) => anyhow::bail!(format!( diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 098f544221a..ca640380191 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt; use std::marker::PhantomData; @@ -251,19 +252,59 @@ impl<'de, P: Deserialize<'de>> de::Deserialize<'de> for TomlDependency

{ } } +/// Expands any environment variables found in `string`. +/// +/// Expanding environment variables is currently an unstable feature. +/// If the feature is not enabled, then we do not expand variable +/// references, so "${FOO}" remains exactly "${FOO}" in the output. +/// +/// This feature may be in one of _three_ modes: +/// * Completely disabled. This is the default (stable) behavior. In this mode, +/// variable references in `string` are ignored (treated like normal text). +/// * `-Z expand_env_vars` was specified on the command-line, but the current +/// manifest does not specify `cargo-features = ["expand_env_vars"]. In this +/// mode, we do look for variable references. If we find _any_ variable +/// reference, then we report an error. This mode is intended only to allow us +/// to do Crater runs, so that we can find any projects that this new +/// feature would break, since they contain strings that match our `${FOO}` syntax. +/// * The current manifest contains `cargo-features = ["expand_env_vars"]`. +/// In this mode, we look for variable references and process them. +/// This is the mode that will eventually be stabilized and become the default. +fn expand_env_var<'a>( + string: &'a str, + config: &Config, + features: &Features, +) -> CargoResult> { + if config.cli_unstable().expand_env_vars || features.is_enabled(Feature::expand_env_vars()) { + let expanded_path = crate::util::env_vars::expand_vars_with(string, |name| { + if features.is_enabled(Feature::expand_env_vars()) { + Ok(std::env::var(name).ok()) + } else { + // This manifest contains a string which looks like a variable + // reference, but the manifest has not enabled the feature. + // Report an error. + anyhow::bail!("this manifest uses environment variable references (e.g. ${FOO}) but has not specified `cargo-features = [\"expand-env-vars\"]`."); + } + })?; + Ok(expanded_path) + } else { + Ok(Cow::Borrowed(string)) + } +} + pub trait ResolveToPath { - fn resolve(&self, config: &Config) -> CargoResult; + fn resolve(&self, config: &Config, features: &Features) -> CargoResult; } impl ResolveToPath for String { - fn resolve(&self, _: &Config) -> CargoResult { - let expanded_path = crate::util::env_vars::expand_env_vars(self)?; - Ok(expanded_path.to_string().into()) + fn resolve(&self, config: &Config, features: &Features) -> CargoResult { + let expanded = expand_env_var(self, config, features)?; + Ok(expanded.to_string().into()) } } impl ResolveToPath for ConfigRelativePath { - fn resolve(&self, c: &Config) -> CargoResult { + fn resolve(&self, c: &Config, _features: &Features) -> CargoResult { Ok(self.resolve_path(c)) } } @@ -1888,7 +1929,7 @@ impl DetailedTomlDependency

{ SourceId::for_git(&loc, reference)? } (None, Some(path), _, _) => { - let path = path.resolve(cx.config)?; + let path = path.resolve(cx.config, cx.features)?; cx.nested_paths.push(path.clone()); // If the source ID for the package we're parsing is a path // source, then we normalize the path here to get rid of diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 35c877b7e2c..b705bbe6b83 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -1328,6 +1328,29 @@ filename = "007bar" path = "src/main.rs" ``` +### expand-env-vars + +The `expand-env-vars` feature allows certain fields within manifests to refer +to environment variables, using `${FOO}` syntax. Currently, the only field that +may refer to environment variables is the `path` field of a dependency. + +For example: + +```toml +cargo-features = ["expand-env-vars"] + +[dependencies] +foo = { path = "${MY_PROJECT_ROOT}/some/long/path/utils" } +``` + +For organizations that manage large code bases, using relative walk-up paths +(e.g. `../../../../long/path/utils`) is not practical; some organizations +forbid using walk-up paths. The `expand-env-vars` feature allows Cargo +to work in such systems. + +The curly braces are required; `$FOO` will not be interpreted as a variable +reference. + ## Stabilized and removed features ### Compile progress diff --git a/tests/testsuite/var_refs.rs b/tests/testsuite/var_refs.rs index 0274ca0f4d5..550b17f0c96 100644 --- a/tests/testsuite/var_refs.rs +++ b/tests/testsuite/var_refs.rs @@ -1,13 +1,9 @@ //! Tests for variable references, e.g. { path = "${FOO}/bar/foo" } -#![allow(unused_imports)] -use cargo_test_support::registry::Package; -use cargo_test_support::{basic_lib_manifest, basic_manifest, git, project, sleep_ms}; -use std::env; -use std::fs; +use cargo_test_support::project; #[cargo_test] -fn basic() { +fn simple() { let p = project() .file( "Cargo.toml", @@ -37,6 +33,8 @@ fn basic() { .file( "bar/Cargo.toml", r#" + cargo-features = ["expand-env-vars"] + [package] name = "bar" version = "1.0.0" @@ -58,6 +56,7 @@ fn basic() { .build(); p.cargo("build") + .masquerade_as_nightly_cargo() .cwd("bar") .env("UTILS_ROOT", "../utils") .run(); @@ -95,6 +94,8 @@ fn basic_with_default() { .file( "bar/Cargo.toml", r#" + cargo-features = ["expand-env-vars"] + [package] name = "bar" version = "1.0.0" @@ -116,17 +117,51 @@ fn basic_with_default() { .build(); // Note: UTILS_ROOT is not set in the environment. - p.cargo("build").cwd("bar").run(); + p.cargo("build") + .masquerade_as_nightly_cargo() + .arg("-Zunstable-options") + .arg("-Zexpand-env-vars") + .cwd("bar") + .run(); assert!(p.bin("bar").is_file()); } -#[cfg(todo)] // error propagation is not working correctly. +#[cargo_test] +fn missing_features() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + edition = "2018" + + [lib] + + [dependencies] + utils = { path = "${UTILS_ROOT}/utils" } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build") + .masquerade_as_nightly_cargo() + .arg("-Zexpand-env-vars") + .with_status(101) + .with_stderr_contains("[..]this manifest uses environment variable references [..] but has not specified `cargo-features = [\"expand-env-vars\"]`.[..]") + .run(); +} + #[cargo_test] fn var_not_set() { let p = project() .file( "Cargo.toml", r#" + cargo-features = ["expand-env-vars"] + [package] name = "foo" version = "1.0.0" @@ -141,18 +176,20 @@ fn var_not_set() { .build(); p.cargo("build") + .masquerade_as_nightly_cargo() .with_status(101) - .with_stderr_contains("environment variable 'BAD_VAR' is not set") + .with_stderr_contains("[..]environment variable 'BAD_VAR' is not set[..]") .run(); } -#[cfg(todo)] // error propagation is not working correctly. #[cargo_test] fn bad_syntax() { let p = project() .file( "Cargo.toml", r#" + cargo-features = ["expand-env-vars"] + [package] name = "foo" version = "1.0.0" @@ -167,7 +204,8 @@ fn bad_syntax() { .build(); p.cargo("build") + .masquerade_as_nightly_cargo() .with_status(101) - .with_stderr_contains("environment variable reference is missing closing brace.") + .with_stderr_contains("[..]environment variable reference is missing closing brace.[..]") .run(); } From 941eb69be04b8ba6928ba1238c3ac8c1197cc957 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 31 Aug 2021 12:16:45 -0700 Subject: [PATCH 5/5] fix env_vars unit test --- src/cargo/util/env_vars.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/env_vars.rs b/src/cargo/util/env_vars.rs index 5dcdd422ffb..51a209b04a3 100644 --- a/src/cargo/util/env_vars.rs +++ b/src/cargo/util/env_vars.rs @@ -133,13 +133,18 @@ mod tests { #[test] fn basic() { - let query = |name: &str| match name { - "FOO" => Some("/foo".to_string()), - "BAR" => Some("/bar".to_string()), - "FOO(ZAP)" => Some("/foo/zap".to_string()), - "WINKING_FACE" => Some("\u{1F609}".to_string()), - "\u{1F916}" => Some("ROBOT FACE".to_string()), - _ => None, + let query = |name: &str| { + Ok(Some( + match name { + "FOO" => "/foo", + "BAR" => "/bar", + "FOO(ZAP)" => "/foo/zap", + "WINKING_FACE" => "\u{1F609}", + "\u{1F916}" => "ROBOT FACE", + _ => return Ok(None), + } + .to_string(), + )) }; let expand = |s| expand_vars_with(s, query);