Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring unstable flags via config file #8393

Merged
merged 17 commits into from
Jul 10, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ impl<'de> serde::de::Deserialize<'de> for RustdocExternMode {
}
}

#[derive(serde::Deserialize, Debug)]
#[derive(serde::Deserialize, Debug, Default)]
#[serde(default)]
pub struct RustdocExternMap {
registries: HashMap<String, String>,
std: Option<RustdocExternMode>,
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,8 @@ impl Features {
/// and then test for your flag or your value and act accordingly.
///
/// If you have any trouble with this, please let us know!
#[derive(Default, Debug)]
#[derive(Default, Debug, Deserialize)]
#[serde(default, rename_all = "kebab-case")]
pub struct CliUnstable {
pub print_im_a_teapot: bool,
pub unstable_options: bool,
Expand Down
50 changes: 34 additions & 16 deletions src/cargo/util/config/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::util::config::value;
use crate::util::config::{Config, ConfigError, ConfigKey};
use crate::util::config::{ConfigValue as CV, Definition, Value};
use serde::{de, de::IntoDeserializer};
use std::collections::HashSet;
use std::vec;

/// Serde deserializer used to convert config values to a target type using
Expand Down Expand Up @@ -269,37 +270,54 @@ impl<'config> ConfigMapAccess<'config> {

fn new_struct(
de: Deserializer<'config>,
fields: &'static [&'static str],
given_fields: &'static [&'static str],
) -> Result<ConfigMapAccess<'config>, ConfigError> {
let fields: Vec<KeyKind> = fields
.iter()
.map(|field| KeyKind::Normal(field.to_string()))
.collect();
let table = de.config.get_table(&de.key)?;

// Assume that if we're deserializing a struct it exhaustively lists all
// possible fields on this key that we're *supposed* to use, so take
// this opportunity to warn about any keys that aren't recognized as
// fields and warn about them.
if let Some(mut v) = de.config.get_table(&de.key)? {
for (t_key, value) in v.val.drain() {
if fields.iter().any(|k| match k {
KeyKind::Normal(s) => s == &t_key,
KeyKind::CaseSensitive(s) => s == &t_key,
}) {
continue;
}
if let Some(v) = table.as_ref() {
let unused_keys = v
.val
.iter()
.filter(|(k, _v)| !given_fields.iter().any(|gk| gk == k));
for (unused_key, unused_value) in unused_keys {
de.config.shell().warn(format!(
"unused config key `{}.{}` in `{}`",
de.key,
t_key,
value.definition()
unused_key,
unused_value.definition()
))?;
}
}

let mut fields = HashSet::new();

// If the caller is interested in a field which we can provide from
// the environment, get it from there.
for field in given_fields {
let mut field_key = de.key.clone();
field_key.push(field);
for env_key in de.config.env.keys() {
if env_key.starts_with(field_key.as_env_key()) {
fields.insert(KeyKind::Normal(field.to_string()));
}
}
}

// Add everything from the config table we're interested in that we
// haven't already provided via an environment variable
if let Some(v) = table {
for key in v.val.keys() {
fields.insert(KeyKind::Normal(key.clone()));
}
}

Ok(ConfigMapAccess {
de,
fields,
fields: fields.into_iter().collect(),
field_index: 0,
})
}
Expand Down
11 changes: 9 additions & 2 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -742,10 +742,17 @@ impl Config {
.unwrap_or(false);
self.target_dir = cli_target_dir;

// If nightly features are enabled, allow setting Z-flags from config
// using the `unstable` table. Ignore that block otherwise.
if nightly_features_allowed() {
if let Some(val) = self.get::<Option<bool>>("unstable.mtime_on_use")? {
self.unstable_flags.mtime_on_use |= val;
if let Some(unstable_flags) = self.get::<Option<CliUnstable>>("unstable")? {
self.unstable_flags = unstable_flags;
}
// NB. It's not ideal to parse these twice, but doing it again here
// allows the CLI to override config files for both enabling
// and disabling, and doing it up top allows CLI Zflags to
// control config parsing behavior.
self.unstable_flags.parse(unstable_flags)?;
}

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ impl<'de> de::Deserialize<'de> for U32OrBool {
}

#[derive(Deserialize, Serialize, Clone, Debug, Default, Eq, PartialEq)]
#[serde(rename_all = "kebab-case")]
#[serde(default, rename_all = "kebab-case")]
pub struct TomlProfile {
pub opt_level: Option<TomlOptLevel>,
pub lto: Option<StringOrBool>,
Expand Down
10 changes: 10 additions & 0 deletions src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ see a list of flags available.
`-Z unstable-options` is a generic flag for enabling other unstable
command-line flags. Options requiring this will be called out below.

Anything which can be configured with a Z flag can also be set in the cargo
config file (`.cargo/config.toml`) in the `unstable` table. For example:

```toml
[unstable]
mtime-on-use = 'yes'
multitarget = 'yes'
timings = 'yes'
```

Some unstable features will require you to specify the `cargo-features` key in
`Cargo.toml`.

Expand Down
192 changes: 188 additions & 4 deletions tests/testsuite/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -687,10 +687,7 @@ Caused by:
f3: i64,
big: i64,
}
assert_error(
config.get::<S>("S").unwrap_err(),
"missing config key `S.f3`",
);
assert_error(config.get::<S>("S").unwrap_err(), "missing field `f3`");
}

#[cargo_test]
Expand Down Expand Up @@ -1094,6 +1091,78 @@ Caused by:
.is_none());
}

#[cargo_test]
/// Assert that unstable options can be configured with the `unstable` table in
/// cargo config files
fn unstable_table_notation() {
cargo::core::enable_nightly_features();
write_config(
"\
[unstable]
print-im-a-teapot = true
",
);
let config = ConfigBuilder::new().build();
assert_eq!(config.cli_unstable().print_im_a_teapot, true);
}

#[cargo_test]
/// Assert that dotted notation works for configuring unstable options
fn unstable_dotted_notation() {
cargo::core::enable_nightly_features();
write_config(
"\
unstable.print-im-a-teapot = true
",
);
let config = ConfigBuilder::new().build();
assert_eq!(config.cli_unstable().print_im_a_teapot, true);
}

#[cargo_test]
/// Assert that Zflags on the CLI take precedence over those from config
fn unstable_cli_precedence() {
cargo::core::enable_nightly_features();
write_config(
"\
unstable.print-im-a-teapot = true
",
);
let config = ConfigBuilder::new().build();
assert_eq!(config.cli_unstable().print_im_a_teapot, true);

let config = ConfigBuilder::new()
.unstable_flag("print-im-a-teapot=no")
.build();
assert_eq!(config.cli_unstable().print_im_a_teapot, false);
}

#[cargo_test]
/// Assert that atempting to set an unstable flag that doesn't exist via config
/// is ignored on stable
fn unstable_invalid_flag_ignored_on_stable() {
write_config(
"\
unstable.an-invalid-flag = 'yes'
",
);
assert!(ConfigBuilder::new().build_err().is_ok());
}

#[cargo_test]
/// Assert that unstable options can be configured with the `unstable` table in
/// cargo config files
fn unstable_flags_ignored_on_stable() {
write_config(
"\
[unstable]
print-im-a-teapot = true
",
);
let config = ConfigBuilder::new().build();
assert_eq!(config.cli_unstable().print_im_a_teapot, false);
}

#[cargo_test]
fn table_merge_failure() {
// Config::merge fails to merge entries in two tables.
Expand Down Expand Up @@ -1177,6 +1246,27 @@ fn struct_with_opt_inner_struct() {
assert_eq!(f.inner.unwrap().value.unwrap(), 12);
}

#[cargo_test]
fn struct_with_default_inner_struct() {
// Struct with serde defaults.
// Check that can be defined with environment variable.
#[derive(Deserialize, Default)]
#[serde(default)]
struct Inner {
value: i32,
}
#[derive(Deserialize, Default)]
#[serde(default)]
struct Foo {
inner: Inner,
}
let config = ConfigBuilder::new()
.env("CARGO_FOO_INNER_VALUE", "12")
.build();
let f: Foo = config.get("foo").unwrap();
assert_eq!(f.inner.value, 12);
}

#[cargo_test]
fn overlapping_env_config() {
// Issue where one key is a prefix of another.
Expand Down Expand Up @@ -1208,6 +1298,100 @@ fn overlapping_env_config() {
assert_eq!(s.debug, Some(1));
}

#[cargo_test]
fn overlapping_env_with_defaults_errors_out() {
// Issue where one key is a prefix of another.
// This is a limitation of mapping environment variables on to a hierarchy.
// Check that we error out when we hit ambiguity in this way, rather than
// the more-surprising defaulting through.
// If, in the future, we can handle this more correctly, feel free to delete
// this test.
#[derive(Deserialize, Default)]
#[serde(default, rename_all = "kebab-case")]
struct Ambig {
debug: u32,
debug_assertions: bool,
}
let config = ConfigBuilder::new()
.env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
.build();
let err = config.get::<Ambig>("ambig").err().unwrap();
assert!(format!("{}", err).contains("missing config key `ambig.debug`"));

let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "5").build();
let s: Ambig = config.get("ambig").unwrap();
assert_eq!(s.debug_assertions, bool::default());
assert_eq!(s.debug, 5);

let config = ConfigBuilder::new()
.env("CARGO_AMBIG_DEBUG", "1")
.env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
.build();
let s: Ambig = config.get("ambig").unwrap();
assert_eq!(s.debug_assertions, true);
assert_eq!(s.debug, 1);
}

#[cargo_test]
fn struct_with_overlapping_inner_struct_and_defaults() {
// Struct with serde defaults.
// Check that can be defined with environment variable.
#[derive(Deserialize, Default)]
#[serde(default)]
struct Inner {
value: i32,
}

// Containing struct with a prefix of inner
//
// This is a limitation of mapping environment variables on to a hierarchy.
// Check that we error out when we hit ambiguity in this way, rather than
// the more-surprising defaulting through.
// If, in the future, we can handle this more correctly, feel free to delete
// this case.
#[derive(Deserialize, Default)]
#[serde(default)]
struct PrefixContainer {
inn: bool,
inner: Inner,
}
let config = ConfigBuilder::new()
.env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
.build();
let err = config
.get::<PrefixContainer>("prefixcontainer")
.err()
.unwrap();
assert!(format!("{}", err).contains("missing config key `prefixcontainer.inn`"));
let config = ConfigBuilder::new()
.env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
.env("CARGO_PREFIXCONTAINER_INN", "true")
.build();
let f: PrefixContainer = config.get("prefixcontainer").unwrap();
assert_eq!(f.inner.value, 12);
assert_eq!(f.inn, true);

// Containing struct where the inner value's field is a prefix of another
//
// This is a limitation of mapping environment variables on to a hierarchy.
// Check that we error out when we hit ambiguity in this way, rather than
// the more-surprising defaulting through.
// If, in the future, we can handle this more correctly, feel free to delete
// this case.
#[derive(Deserialize, Default)]
#[serde(default)]
struct InversePrefixContainer {
inner_field: bool,
inner: Inner,
}
let config = ConfigBuilder::new()
.env("CARGO_INVERSEPREFIXCONTAINER_INNER_VALUE", "12")
.build();
let f: InversePrefixContainer = config.get("inverseprefixcontainer").unwrap();
assert_eq!(f.inner_field, bool::default());
assert_eq!(f.inner.value, 12);
}

#[cargo_test]
fn string_list_tricky_env() {
// Make sure StringList handles typed env values.
Expand Down