Auto merge of #7456 - alexcrichton:config-only-serde, r=ehuss

Migrate towards exclusively using serde for `Config`

This series of commits was spawned off a thought I had while reading #7253 (comment), although it ended up not really touching on that at all. I was a little unsettled about how unstructured the config accesses are throughout Cargo and we had sort of two systems (one serde which is nice, one which is more manual) for reading config values.

This PR converts everything to run through serde for deserializing values, except for `get_list` which is funky. There's only one usage of that with the `paths` key though and we can probably fix this soon-ish.

In any case, the highlights of this PR are:

* This PR is surprisingly large. I did a lot of movement in `config.rs` to try to make the file smaller and more understandable.
* The `Value` type which retains information about where it was deserialized from is very special, and has special treatment with serde's data model. That's what allows us to use that and serde at the same time.
* The `ConfigRelativePath` and `ConfigKey` structures have been revamped internally, but morally serve the same purposes as before.
* Cargo now has structured `struct` access for a bunch of its configuration (`net`, `http`, `build`, etc). I would ideally like to move toward a world where this is the *only* way to read configuration, or at least everything conventionally runs through those paths.
* Functionally, this PR should have no difference other than tweaks to error messages here and there, and perhaps more strict validation on commands where we validate more configuration on each run than we previously did.
* This isn't a 100% transition for Cargo yet, but I figured it would be a good idea to post this and get some feedback first.
* In the long run I want to remove `get_env`, `get_cv`, and `get_*_priv` from `Config` as internal details. I'd like to move this all to `de.rs` and have it walk down the tree of configuration as we deserialize a value. For now though these all remain in place and that refactoring is left to a future PR.

Author: bors

src/bin/cargo/main.rs

@@ -60,9 +60,7 @@ fn aliased_command(config: &Config, command: &str) -> CargoResult<Option<Vec<Str
                 .collect(),
         ),
         Ok(None) => None,
-        Err(_) => config
-            .get_list(&alias_name)?
-            .map(|record| record.val.iter().map(|s| s.0.to_string()).collect()),
+        Err(_) => config.get::<Option<Vec<String>>>(&alias_name)?,
     };
     let result = user_alias.or_else(|| match command {
         "b" => Some(vec!["build".to_string()]),

src/cargo/core/compiler/build_config.rs

@@ -62,15 +62,16 @@ impl BuildConfig {
         requested_target: &Option<String>,
         mode: CompileMode,
     ) -> CargoResult<BuildConfig> {
+        let cfg = config.build_config()?;
         let requested_kind = match requested_target {
             Some(s) => CompileKind::Target(CompileTarget::new(s)?),
-            None => match config.get_string("build.target")? {
-                Some(cfg) => {
-                    let value = if cfg.val.ends_with(".json") {
-                        let path = cfg.definition.root(config).join(&cfg.val);
+            None => match &cfg.target {
+                Some(val) => {
+                    let value = if val.raw_value().ends_with(".json") {
+                        let path = val.clone().resolve_path(config);
                         path.to_str().expect("must be utf-8 in toml").to_string()
                     } else {
-                        cfg.val
+                        val.raw_value().to_string()
                     };
                     CompileKind::Target(CompileTarget::new(&value)?)
                 }
@@ -88,8 +89,7 @@ impl BuildConfig {
                  its environment, ignoring the `-j` parameter",
             )?;
         }
-        let cfg_jobs: Option<u32> = config.get("build.jobs")?;
-        let jobs = jobs.or(cfg_jobs).unwrap_or(::num_cpus::get() as u32);
+        let jobs = jobs.or(cfg.jobs).unwrap_or(::num_cpus::get() as u32);
 
         Ok(BuildConfig {
             requested_kind,

src/cargo/core/compiler/build_context/target_info.rs

@@ -6,6 +6,7 @@ use std::str::{self, FromStr};
 
 use crate::core::compiler::CompileKind;
 use crate::core::TargetKind;
+use crate::util::config::StringList;
 use crate::util::{CargoResult, CargoResultExt, Config, ProcessBuilder, Rustc};
 use cargo_platform::{Cfg, CfgExpr};
 
@@ -427,9 +428,8 @@ fn env_args(
         CompileKind::Target(target) => target.short_name(),
     };
     let key = format!("target.{}.{}", target, name);
-    if let Some(args) = config.get_list_or_split_string(&key)? {
-        let args = args.val.into_iter();
-        rustflags.extend(args);
+    if let Some(args) = config.get::<Option<StringList>>(&key)? {
+        rustflags.extend(args.as_slice().iter().cloned());
     }
     // ...including target.'cfg(...)'.rustflags
     if let Some(target_cfg) = target_cfg {
@@ -450,9 +450,8 @@ fn env_args(
 
             for n in cfgs {
                 let key = format!("target.{}.{}", n, name);
-                if let Some(args) = config.get_list_or_split_string(&key)? {
-                    let args = args.val.into_iter();
-                    rustflags.extend(args);
+                if let Some(args) = config.get::<Option<StringList>>(&key)? {
+                    rustflags.extend(args.as_slice().iter().cloned());
                 }
             }
         }
@@ -463,10 +462,14 @@ fn env_args(
     }
 
     // Then the `build.rustflags` value.
-    let key = format!("build.{}", name);
-    if let Some(args) = config.get_list_or_split_string(&key)? {
-        let args = args.val.into_iter();
-        return Ok(args.collect());
+    let build = config.build_config()?;
+    let list = if name == "rustflags" {
+        &build.rustflags
+    } else {
+        &build.rustdocflags
+    };
+    if let Some(list) = list {
+        return Ok(list.as_slice().to_vec());
     }
 
     Ok(Vec::new())

src/cargo/core/compiler/context/mod.rs

@@ -99,10 +99,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             }
         };
 
-        let pipelining = bcx
-            .config
-            .get::<Option<bool>>("build.pipelining")?
-            .unwrap_or(true);
+        let pipelining = bcx.config.build_config()?.pipelining.unwrap_or(true);
 
         Ok(Self {
             bcx,

src/cargo/core/compiler/output_depinfo.rs

@@ -81,10 +81,10 @@ pub fn output_depinfo<'a, 'b>(cx: &mut Context<'a, 'b>, unit: &Unit<'a>) -> Carg
     let mut visited = HashSet::new();
     let success = add_deps_for_unit(&mut deps, cx, unit, &mut visited).is_ok();
     let basedir_string;
-    let basedir = match bcx.config.get_path("build.dep-info-basedir")? {
+    let basedir = match bcx.config.build_config()?.dep_info_basedir.clone() {
         Some(value) => {
             basedir_string = value
-                .val
+                .resolve_path(bcx.config)
                 .as_os_str()
                 .to_str()
                 .ok_or_else(|| internal("build.dep-info-basedir path not utf-8"))?

src/cargo/core/package.rs

@@ -376,9 +376,7 @@ impl<'cfg> PackageSet<'cfg> {
         // that it's buggy, and we've empirically seen that it's buggy with HTTP
         // proxies.
         let mut multi = Multi::new();
-        let multiplexing = config
-            .get::<Option<bool>>("http.multiplexing")?
-            .unwrap_or(true);
+        let multiplexing = config.http_config()?.multiplexing.unwrap_or(true);
         multi
             .pipelining(false, multiplexing)
             .chain_err(|| "failed to enable multiplexing/pipelining in curl")?;

src/cargo/core/profiles.rs

@@ -38,7 +38,7 @@ impl Profiles {
 
         let incremental = match env::var_os("CARGO_INCREMENTAL") {
             Some(v) => Some(v == "1"),
-            None => config.get::<Option<bool>>("build.incremental")?,
+            None => config.build_config()?.incremental,
         };
 
         if !features.is_enabled(Feature::named_profiles()) {

src/cargo/ops/cargo_new.rs

@@ -1,17 +1,18 @@
+use crate::core::{compiler, Workspace};
+use crate::util::errors::{self, CargoResult, CargoResultExt};
+use crate::util::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo};
+use crate::util::{paths, validate_package_name, Config};
+use git2::Config as GitConfig;
+use git2::Repository as GitRepository;
+use serde::de;
+use serde::Deserialize;
 use std::collections::BTreeMap;
 use std::env;
 use std::fmt;
 use std::fs;
 use std::io::{BufRead, BufReader, ErrorKind};
 use std::path::{Path, PathBuf};
-
-use git2::Config as GitConfig;
-use git2::Repository as GitRepository;
-
-use crate::core::{compiler, Workspace};
-use crate::util::errors::{self, CargoResult, CargoResultExt};
-use crate::util::{existing_vcs_repo, internal, FossilRepo, GitRepo, HgRepo, PijulRepo};
-use crate::util::{paths, validate_package_name, Config};
+use std::str::FromStr;
 
 use toml;
 
@@ -24,6 +25,31 @@ pub enum VersionControl {
     NoVcs,
 }
 
+impl FromStr for VersionControl {
+    type Err = failure::Error;
+
+    fn from_str(s: &str) -> Result<Self, failure::Error> {
+        match s {
+            "git" => Ok(VersionControl::Git),
+            "hg" => Ok(VersionControl::Hg),
+            "pijul" => Ok(VersionControl::Pijul),
+            "fossil" => Ok(VersionControl::Fossil),
+            "none" => Ok(VersionControl::NoVcs),
+            other => failure::bail!("unknown vcs specification: `{}`", other),
+        }
+    }
+}
+
+impl<'de> de::Deserialize<'de> for VersionControl {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        FromStr::from_str(&s).map_err(de::Error::custom)
+    }
+}
+
 #[derive(Debug)]
 pub struct NewOptions {
     pub version_control: Option<VersionControl>,
@@ -102,9 +128,11 @@ impl NewOptions {
     }
 }
 
+#[derive(Deserialize)]
 struct CargoNewConfig {
     name: Option<String>,
     email: Option<String>,
+    #[serde(rename = "vcs")]
     version_control: Option<VersionControl>,
 }
 
@@ -543,7 +571,7 @@ fn init_vcs(path: &Path, vcs: VersionControl, config: &Config) -> CargoResult<()
 fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> {
     let path = opts.path;
     let name = opts.name;
-    let cfg = global_config(config)?;
+    let cfg = config.get::<CargoNewConfig>("cargo-new")?;
 
     // Using the push method with two arguments ensures that the entries for
     // both `ignore` and `hgignore` are in sync.
@@ -752,30 +780,3 @@ fn discover_author() -> CargoResult<(String, Option<String>)> {
 
     Ok((name, email))
 }
-
-fn global_config(config: &Config) -> CargoResult<CargoNewConfig> {
-    let name = config.get_string("cargo-new.name")?.map(|s| s.val);
-    let email = config.get_string("cargo-new.email")?.map(|s| s.val);
-    let vcs = config.get_string("cargo-new.vcs")?;
-
-    let vcs = match vcs.as_ref().map(|p| (&p.val[..], &p.definition)) {
-        Some(("git", _)) => Some(VersionControl::Git),
-        Some(("hg", _)) => Some(VersionControl::Hg),
-        Some(("pijul", _)) => Some(VersionControl::Pijul),
-        Some(("none", _)) => Some(VersionControl::NoVcs),
-        Some((s, p)) => {
-            return Err(internal(format!(
-                "invalid configuration for key \
-                 `cargo-new.vcs`, unknown vcs `{}` \
-                 (found in {})",
-                s, p
-            )));
-        }
-        None => None,
-    };
-    Ok(CargoNewConfig {
-        name,
-        email,
-        version_control: vcs,
-    })
-}

src/cargo/ops/registry.rs

@@ -408,34 +408,26 @@ pub fn http_handle_and_timeout(config: &Config) -> CargoResult<(Easy, HttpTimeou
 }
 
 pub fn needs_custom_http_transport(config: &Config) -> CargoResult<bool> {
-    let proxy_exists = http_proxy_exists(config)?;
-    let timeout = HttpTimeout::new(config)?.is_non_default();
-    let cainfo = config.get_path("http.cainfo")?;
-    let check_revoke = config.get_bool("http.check-revoke")?;
-    let user_agent = config.get_string("http.user-agent")?;
-    let ssl_version = config.get::<Option<SslVersionConfig>>("http.ssl-version")?;
-
-    Ok(proxy_exists
-        || timeout
-        || cainfo.is_some()
-        || check_revoke.is_some()
-        || user_agent.is_some()
-        || ssl_version.is_some())
+    Ok(http_proxy_exists(config)?
+        || *config.http_config()? != Default::default()
+        || env::var_os("HTTP_TIMEOUT").is_some())
 }
 
 /// Configure a libcurl http handle with the defaults options for Cargo
 pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<HttpTimeout> {
+    let http = config.http_config()?;
     if let Some(proxy) = http_proxy(config)? {
         handle.proxy(&proxy)?;
     }
-    if let Some(cainfo) = config.get_path("http.cainfo")? {
-        handle.cainfo(&cainfo.val)?;
+    if let Some(cainfo) = &http.cainfo {
+        let cainfo = cainfo.resolve_path(config);
+        handle.cainfo(&cainfo)?;
     }
-    if let Some(check) = config.get_bool("http.check-revoke")? {
-        handle.ssl_options(SslOpt::new().no_revoke(!check.val))?;
+    if let Some(check) = http.check_revoke {
+        handle.ssl_options(SslOpt::new().no_revoke(!check))?;
     }
-    if let Some(user_agent) = config.get_string("http.user-agent")? {
-        handle.useragent(&user_agent.val)?;
+    if let Some(user_agent) = &http.user_agent {
+        handle.useragent(user_agent)?;
     } else {
         handle.useragent(&version().to_string())?;
     }
@@ -456,23 +448,25 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<
         };
         Ok(version)
     }
-    if let Some(ssl_version) = config.get::<Option<SslVersionConfig>>("http.ssl-version")? {
+    if let Some(ssl_version) = &http.ssl_version {
         match ssl_version {
             SslVersionConfig::Single(s) => {
                 let version = to_ssl_version(s.as_str())?;
                 handle.ssl_version(version)?;
             }
             SslVersionConfig::Range(SslVersionConfigRange { min, max }) => {
-                let min_version =
-                    min.map_or(Ok(SslVersion::Default), |s| to_ssl_version(s.as_str()))?;
-                let max_version =
-                    max.map_or(Ok(SslVersion::Default), |s| to_ssl_version(s.as_str()))?;
+                let min_version = min
+                    .as_ref()
+                    .map_or(Ok(SslVersion::Default), |s| to_ssl_version(s))?;
+                let max_version = max
+                    .as_ref()
+                    .map_or(Ok(SslVersion::Default), |s| to_ssl_version(s))?;
                 handle.ssl_min_max_version(min_version, max_version)?;
             }
         }
     }
 
-    if let Some(true) = config.get::<Option<bool>>("http.debug")? {
+    if let Some(true) = http.debug {
         handle.verbose(true)?;
         handle.debug_function(|kind, data| {
             let (prefix, level) = match kind {
@@ -513,11 +507,10 @@ pub struct HttpTimeout {
 
 impl HttpTimeout {
     pub fn new(config: &Config) -> CargoResult<HttpTimeout> {
-        let low_speed_limit = config
-            .get::<Option<u32>>("http.low-speed-limit")?
-            .unwrap_or(10);
+        let config = config.http_config()?;
+        let low_speed_limit = config.low_speed_limit.unwrap_or(10);
         let seconds = config
-            .get::<Option<u64>>("http.timeout")?
+            .timeout
             .or_else(|| env::var("HTTP_TIMEOUT").ok().and_then(|s| s.parse().ok()))
             .unwrap_or(30);
         Ok(HttpTimeout {
@@ -526,10 +519,6 @@ impl HttpTimeout {
         })
     }
 
-    fn is_non_default(&self) -> bool {
-        self.dur != Duration::new(30, 0) || self.low_speed_limit != 10
-    }
-
     pub fn configure(&self, handle: &mut Easy) -> CargoResult<()> {
         // The timeout option for libcurl by default times out the entire
         // transfer, but we probably don't want this. Instead we only set
@@ -548,8 +537,9 @@ impl HttpTimeout {
 /// Favor cargo's `http.proxy`, then git's `http.proxy`. Proxies specified
 /// via environment variables are picked up by libcurl.
 fn http_proxy(config: &Config) -> CargoResult<Option<String>> {
-    if let Some(s) = config.get_string("http.proxy")? {
-        return Ok(Some(s.val));
+    let http = config.http_config()?;
+    if let Some(s) = &http.proxy {
+        return Ok(Some(s.clone()));
     }
     if let Ok(cfg) = git2::Config::open_default() {
         if let Ok(s) = cfg.get_str("http.proxy") {

src/cargo/sources/git/utils.rs

@@ -707,10 +707,8 @@ pub fn fetch(
     // repositories instead of `libgit2`-the-library. This should make more
     // flavors of authentication possible while also still giving us all the
     // speed and portability of using `libgit2`.
-    if let Some(val) = config.get_bool("net.git-fetch-with-cli")? {
-        if val.val {
-            return fetch_with_cli(repo, url, refspec, config);
-        }
+    if let Some(true) = config.net_config()?.git_fetch_with_cli {
+        return fetch_with_cli(repo, url, refspec, config);
     }
 
     debug!("doing a fetch for {}", url);