Auto merge of #7649 - ehuss:config2, r=alexcrichton

Config enhancements.

This is a collection of changes to config handling. I intended to split this into separate PRs, but they all built on one another so I decided to do it as one. However, I can still split this up if desired.

High level overview:

- Refactorings, mainly to remove `pub` from `Config::get_table` and use serde API instead.
- Add `--config` CLI option.
- Add config `include` to include other files.

This makes some progress on #5416.
Closes #6699.

This makes a minor user-visible change in regards to `StringList` types. If an array is specified in a config as a list, and also as an env var, they will now be merged. Previously the environment variable overrode the file value. But if it is a string, then it won't join (env var takes precedence). I can probably change this, but I'm not sure if the old behavior is desired, or if it should merge all the time?

**Future plans**
This lays the groundwork for some more changes:
- Work on #7253 (`debug-assertions` and `debug` fails in environment vars). I have some ideas to try.
- Consider removing use of `get_list` for `paths`, and use a `Vec<ConfigRelativePath>`. This will require some non-trivial changes to how `ConfigSeqAccess` works. This is one of the last parts that does not use the serde API.
- Possibly change `[source]` to load config values in a lazy fashion. This will unlock the ability to use environment variables with source definitions (like CARGO_SOURCE_CRATES_IO_REPLACE_WITH).
- Possibly change `[profile]` to load config profiles in a lazy fashion. This will make it easier to use environment variables with profiles, particularly with arbitrarily named profiles.
- Possibly remove the case-sensitive environment variables in `-Zadvanced-env`. I think they are just too awkward, and prone to problems. Instead, drive people towards using `--config` instead of env vars.
- Add support for TOML tables in env vars (like `CARGO_PROFILES={my-profile={opt-level=1}})`). I started implementing it, but then looking at the use cases, it didn't seem as useful as I initially thought. However, it's still an option to try.

**Refactoring overview**

- `[source]` table now uses the serde API.
- `[target]` table now uses the serde API. This is complicated since the 'cfg()' entries are different from the triple entries. The 'cfg()' tables are loaded separately, and are accessed from `Config::target_cfgs`. Otherwise, it just uses `config.get` of the specific target.TRIPLE.
    - Moved the target config stuff into `config/target.rs`.
- Various changes to make this work:
    - Added `PathAndArgs` type which replaces `config.get_path_and_args`.
    - Changed `ConfigKey` to track the key parts as a list (instead of a string). This fixes an issue where quoted keys weren't handled properly (like `[foo.'a.b'.bar]`). This also seems to make a little more sense (it was joining parts into a string only to immediately call `split` on it). Changed various APIs to take a `ConfigKey` object instead of a string to avoid that splitting behavior.
    - `ValueDeserializer` now pre-computes the `Definition` so that it can provide a better error message when a value fails to deserialize.

Overall, there shouldn't be significant user-visible changes. Some error messages have changed and warnings have been added for some ignored keys. `-Zadvanced-env` now works for source and target tables, though I'm not really happy with that feature.

Author: bors

crates/cargo-test-support/src/git.rs

@@ -12,7 +12,7 @@ let git_project = git::new("dep1", |project| {
     project
         .file("Cargo.toml", &basic_manifest("dep1"))
         .file("src/lib.rs", r#"pub fn f() { println!("hi!"); } "#)
-}).unwrap();
+});
 
 // Use the `url()` method to get the file url to the new repository.
 let p = project()

crates/cargo-test-support/src/lib.rs

@@ -1123,45 +1123,10 @@ impl Execs {
     }
 
     fn normalize_matcher(&self, matcher: &str) -> String {
-        // Let's not deal with / vs \ (windows...)
-        let matcher = matcher.replace("\\\\", "/").replace("\\", "/");
-
-        // Weirdness for paths on Windows extends beyond `/` vs `\` apparently.
-        // Namely paths like `c:\` and `C:\` are equivalent and that can cause
-        // issues. The return value of `env::current_dir()` may return a
-        // lowercase drive name, but we round-trip a lot of values through `Url`
-        // which will auto-uppercase the drive name. To just ignore this
-        // distinction we try to canonicalize as much as possible, taking all
-        // forms of a path and canonicalizing them to one.
-        let replace_path = |s: &str, path: &Path, with: &str| {
-            let path_through_url = Url::from_file_path(path).unwrap().to_file_path().unwrap();
-            let path1 = path.display().to_string().replace("\\", "/");
-            let path2 = path_through_url.display().to_string().replace("\\", "/");
-            s.replace(&path1, with)
-                .replace(&path2, with)
-                .replace(with, &path1)
-        };
-
-        // Do the template replacements on the expected string.
-        let matcher = match &self.process_builder {
-            None => matcher,
-            Some(p) => match p.get_cwd() {
-                None => matcher,
-                Some(cwd) => replace_path(&matcher, cwd, "[CWD]"),
-            },
-        };
-
-        // Similar to cwd above, perform similar treatment to the root path
-        // which in theory all of our paths should otherwise get rooted at.
-        let root = paths::root();
-        let matcher = replace_path(&matcher, &root, "[ROOT]");
-
-        // Let's not deal with \r\n vs \n on windows...
-        let matcher = matcher.replace("\r", "");
-
-        // It's easier to read tabs in outputs if they don't show up as literal
-        // hidden characters
-        matcher.replace("\t", "<tab>")
+        normalize_matcher(
+            matcher,
+            self.process_builder.as_ref().and_then(|p| p.get_cwd()),
+        )
     }
 
     fn match_std(
@@ -1429,6 +1394,52 @@ pub fn lines_match(expected: &str, mut actual: &str) -> bool {
     actual.is_empty() || expected.ends_with("[..]")
 }
 
+/// Variant of `lines_match` that applies normalization to the strings.
+pub fn normalized_lines_match(expected: &str, actual: &str, cwd: Option<&Path>) -> bool {
+    let expected = normalize_matcher(expected, cwd);
+    let actual = normalize_matcher(actual, cwd);
+    lines_match(&expected, &actual)
+}
+
+fn normalize_matcher(matcher: &str, cwd: Option<&Path>) -> String {
+    // Let's not deal with / vs \ (windows...)
+    let matcher = matcher.replace("\\\\", "/").replace("\\", "/");
+
+    // Weirdness for paths on Windows extends beyond `/` vs `\` apparently.
+    // Namely paths like `c:\` and `C:\` are equivalent and that can cause
+    // issues. The return value of `env::current_dir()` may return a
+    // lowercase drive name, but we round-trip a lot of values through `Url`
+    // which will auto-uppercase the drive name. To just ignore this
+    // distinction we try to canonicalize as much as possible, taking all
+    // forms of a path and canonicalizing them to one.
+    let replace_path = |s: &str, path: &Path, with: &str| {
+        let path_through_url = Url::from_file_path(path).unwrap().to_file_path().unwrap();
+        let path1 = path.display().to_string().replace("\\", "/");
+        let path2 = path_through_url.display().to_string().replace("\\", "/");
+        s.replace(&path1, with)
+            .replace(&path2, with)
+            .replace(with, &path1)
+    };
+
+    // Do the template replacements on the expected string.
+    let matcher = match cwd {
+        None => matcher,
+        Some(p) => replace_path(&matcher, p, "[CWD]"),
+    };
+
+    // Similar to cwd above, perform similar treatment to the root path
+    // which in theory all of our paths should otherwise get rooted at.
+    let root = paths::root();
+    let matcher = replace_path(&matcher, &root, "[ROOT]");
+
+    // Let's not deal with \r\n vs \n on windows...
+    let matcher = matcher.replace("\r", "");
+
+    // It's easier to read tabs in outputs if they don't show up as literal
+    // hidden characters
+    matcher.replace("\t", "<tab>")
+}
+
 #[test]
 fn lines_match_works() {
     assert!(lines_match("a b", "a b"));
@@ -1835,3 +1846,12 @@ pub fn symlink_supported() -> bool {
 pub fn symlink_supported() -> bool {
     true
 }
+
+/// The error message for ENOENT.
+///
+/// It's generally not good to match against OS error messages, but I think
+/// this one is relatively stable.
+#[cfg(windows)]
+pub const NO_SUCH_FILE_ERR_MSG: &str = "The system cannot find the file specified. (os error 2)";
+#[cfg(not(windows))]
+pub const NO_SUCH_FILE_ERR_MSG: &str = "No such file or directory (os error 2)";

crates/resolver-tests/tests/resolve.rs

@@ -62,12 +62,13 @@ proptest! {
             .configure(
                 1,
                 None,
-                &None,
+                None,
                 false,
                 false,
                 false,
                 &None,
                 &["minimal-versions".to_string()],
+                &[],
             )
             .unwrap();
 
@@ -565,12 +566,13 @@ fn test_resolving_minimum_version_with_transitive_deps() {
         .configure(
             1,
             None,
-            &None,
+            None,
             false,
             false,
             false,
             &None,
             &["minimal-versions".to_string()],
+            &[],
         )
         .unwrap();
 

src/bin/cargo/cli.rs

@@ -10,6 +10,9 @@ use super::list_commands;
 use crate::command_prelude::*;
 
 pub fn main(config: &mut Config) -> CliResult {
+    // CAUTION: Be careful with using `config` until it is configured below.
+    // In general, try to avoid loading config values unless necessary (like
+    // the [alias] table).
     let args = match cli().get_matches_safe() {
         Ok(args) => args,
         Err(e) => {
@@ -90,8 +93,18 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'"
     }
 
     let args = expand_aliases(config, args)?;
+    let (cmd, subcommand_args) = match args.subcommand() {
+        (cmd, Some(args)) => (cmd, args),
+        _ => {
+            // No subcommand provided.
+            cli().print_help()?;
+            return Ok(());
+        }
+    };
+    config_configure(config, &args, subcommand_args)?;
+    super::init_git_transports(&config);
 
-    execute_subcommand(config, &args)
+    execute_subcommand(config, cmd, subcommand_args)
 }
 
 pub fn get_version_string(is_verbose: bool) -> String {
@@ -147,34 +160,39 @@ fn expand_aliases(
     Ok(args)
 }
 
-fn execute_subcommand(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
-    let (cmd, subcommand_args) = match args.subcommand() {
-        (cmd, Some(args)) => (cmd, args),
-        _ => {
-            cli().print_help()?;
-            return Ok(());
-        }
-    };
-
+fn config_configure(
+    config: &mut Config,
+    args: &ArgMatches<'_>,
+    subcommand_args: &ArgMatches<'_>,
+) -> CliResult {
     let arg_target_dir = &subcommand_args.value_of_path("target-dir", config);
-
+    let config_args: Vec<&str> = args.values_of("config").unwrap_or_default().collect();
+    let quiet = if args.is_present("quiet") || subcommand_args.is_present("quiet") {
+        Some(true)
+    } else {
+        None
+    };
     config.configure(
         args.occurrences_of("verbose") as u32,
-        if args.is_present("quiet") || subcommand_args.is_present("quiet") {
-            Some(true)
-        } else {
-            None
-        },
-        &args.value_of("color").map(|s| s.to_string()),
+        quiet,
+        args.value_of("color"),
         args.is_present("frozen"),
         args.is_present("locked"),
         args.is_present("offline"),
         arg_target_dir,
         &args
             .values_of_lossy("unstable-features")
             .unwrap_or_default(),
+        &config_args,
     )?;
+    Ok(())
+}
 
+fn execute_subcommand(
+    config: &mut Config,
+    cmd: &str,
+    subcommand_args: &ArgMatches<'_>,
+) -> CliResult {
     if let Some(exec) = commands::builtin_exec(cmd) {
         return exec(config, subcommand_args);
     }
@@ -241,6 +259,11 @@ See 'cargo help <command>' for more information on a specific command.\n",
         .arg(opt("frozen", "Require Cargo.lock and cache are up to date").global(true))
         .arg(opt("locked", "Require Cargo.lock is up to date").global(true))
         .arg(opt("offline", "Run without accessing the network").global(true))
+        .arg(
+            multi_opt("config", "KEY=VALUE", "Override a configuration value")
+                .global(true)
+                .hidden(true),
+        )
         .arg(
             Arg::with_name("unstable-features")
                 .help("Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details")

src/bin/cargo/main.rs

@@ -35,7 +35,6 @@ fn main() {
     let result = match cargo::ops::fix_maybe_exec_rustc() {
         Ok(true) => Ok(()),
         Ok(false) => {
-            init_git_transports(&config);
             let _token = cargo::util::job::setup();
             cli::main(&mut config)
         }