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.

Sign up for GitHub

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

Jump to bottom

Allow enabling config-include feature in config #14196

Merged
merged 3 commits into from
Jul 6, 2024
Merged

Allow enabling config-include feature in config #14196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bfc31f
test(add): show config-include cannot be enabled in top-level config
lschuermann Jul 6, 2024
45e5cb4
test(add): show config-include correctly merges unstable configs
lschuermann Jul 6, 2024
0e35bea
Allow enabling `config-include` feature in config
lschuermann Jul 6, 2024
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
7 changes: 5 additions & 2 deletions src/cargo/util/context/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,11 @@ impl GlobalContext {
self.cli_config = Some(cli_config.iter().map(|s| s.to_string()).collect());
self.merge_cli_args()?;
}

// Load the unstable flags from config file here first, as the config
// file itself may enable inclusion of other configs. In that case, we
// want to re-load configs with includes enabled:
self.load_unstable_flags_from_config()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct, and it should be safe to remove the other call of load_unstable_flags_from_config at the bottom of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, thanks for the confirmation!

if self.unstable_flags.config_include {
// If the config was already loaded (like when fetching the
// `[alias]` table), it was loaded with includes disabled because
Expand Down Expand Up @@ -1091,8 +1096,6 @@ impl GlobalContext {
let cli_target_dir = target_dir.as_ref().map(|dir| Filesystem::new(dir.clone()));
self.target_dir = cli_target_dir;

self.load_unstable_flags_from_config()?;

Ok(())
}

Expand Down
165 changes: 165 additions & 0 deletions tests/testsuite/config_include.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,171 @@ fn simple() {
assert_eq!(gctx.get::<i32>("key3").unwrap(), 4);
}

#[cargo_test]
fn enable_in_unstable_config() {
// config-include enabled in the unstable config table:
write_config_at(
".cargo/config.toml",
"
include = 'other.toml'
key1 = 1
key2 = 2

[unstable]
config-include = true
",
);
write_config_at(
".cargo/other.toml",
"
key2 = 3
key3 = 4
",
);
let gctx = GlobalContextBuilder::new()
.nightly_features_allowed(true)
.build();
assert_eq!(gctx.get::<i32>("key1").unwrap(), 1);
assert_eq!(gctx.get::<i32>("key2").unwrap(), 2);
assert_eq!(gctx.get::<i32>("key3").unwrap(), 4);
}

Copy link
Member

@weihanglo weihanglo Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add another test to ensure that

  • A mix of hierarchy and include works correctly.
  • Fields under the [unstable] table are merged correctly.
#[cargo_test]
fn mix_of_hierarchy_and_include() {
    write_config_at(
        "foo/.cargo/config.toml",
        "
        include = 'other.toml'
        key1 = 1

        # also make sure unstable flags merge in the correct order
        [unstable]
        features = ['1']
        ",
    );
    write_config_at(
        "foo/.cargo/other.toml",
        "
        key1 = 2
        key2 = 2

        [unstable]
        features = ['2']
        ",
    );
    write_config_at(
        ".cargo/config.toml",
        "
        include = 'other.toml'
        key1 = 3
        key2 = 3
        key3 = 3

        [unstable]
        features = ['3']
        ",
    );
    write_config_at(
        ".cargo/other.toml",
        "
        key1 = 4
        key2 = 4
        key3 = 4
        key4 = 4

        [unstable]
        features = ['4']
        ",
    );
    let gctx = GlobalContextBuilder::new()
        .unstable_flag("config-include")
        .cwd("foo")
        .nightly_features_allowed(true)
        .build();
    assert_eq!(gctx.get::<i32>("key1").unwrap(), 1);
    assert_eq!(gctx.get::<i32>("key2").unwrap(), 2);
    assert_eq!(gctx.get::<i32>("key3").unwrap(), 3);
    assert_eq!(gctx.get::<i32>("key4").unwrap(), 4);
    assert_eq!(
        gctx.get::<Vec<String>>("unstable.features").unwrap(),
        vec![
            "4".to_string(),
            "3".to_string(),
            "2".to_string(),
            "1".to_string()
        ]
    );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this detailed example!

If I'm not mistaken, this example is not currently present in the testsuite and seems useful on its own, even when just enabling config include as an unstable CLI flag. Given the tricky code around re-loading the config in case of feature changes, would it make sense to include your test-case both on its own verbatim, and with config-include feature? That would make sure that cargo shows identical behavior for both methods of enabling this feature.

In that case I'd add this test case twice, one verbatim as you provided and one as a _with_enable_in_unstable_config variant. I'll push this in a second, let me know whether this makes sense to you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. That sounds good to me :)

#[cargo_test]
fn mix_of_hierarchy_and_include() {
write_config_at(
"foo/.cargo/config.toml",
"
include = 'other.toml'
key1 = 1

# also make sure unstable flags merge in the correct order
[unstable]
features = ['1']
",
);
write_config_at(
"foo/.cargo/other.toml",
"
key1 = 2
key2 = 2

[unstable]
features = ['2']
",
);
write_config_at(
".cargo/config.toml",
"
include = 'other.toml'
key1 = 3
key2 = 3
key3 = 3

[unstable]
features = ['3']
",
);
write_config_at(
".cargo/other.toml",
"
key1 = 4
key2 = 4
key3 = 4
key4 = 4

[unstable]
features = ['4']
",
);
let gctx = GlobalContextBuilder::new()
.unstable_flag("config-include")
.cwd("foo")
.nightly_features_allowed(true)
.build();
assert_eq!(gctx.get::<i32>("key1").unwrap(), 1);
assert_eq!(gctx.get::<i32>("key2").unwrap(), 2);
assert_eq!(gctx.get::<i32>("key3").unwrap(), 3);
assert_eq!(gctx.get::<i32>("key4").unwrap(), 4);
assert_eq!(
gctx.get::<Vec<String>>("unstable.features").unwrap(),
vec![
"4".to_string(),
"3".to_string(),
"2".to_string(),
"1".to_string()
]
);
}

#[cargo_test]
fn mix_of_hierarchy_and_include_with_enable_in_unstable_config() {
// `mix_of_hierarchy_and_include`, but with the config-include
// feature itself enabled in the unstable config table:
write_config_at(
"foo/.cargo/config.toml",
"
include = 'other.toml'
key1 = 1

# also make sure unstable flags merge in the correct order
[unstable]
features = ['1']
config-include = true
",
);
write_config_at(
"foo/.cargo/other.toml",
"
key1 = 2
key2 = 2

[unstable]
features = ['2']
",
);
write_config_at(
".cargo/config.toml",
"
include = 'other.toml'
key1 = 3
key2 = 3
key3 = 3

[unstable]
features = ['3']
",
);
write_config_at(
".cargo/other.toml",
"
key1 = 4
key2 = 4
key3 = 4
key4 = 4

[unstable]
features = ['4']
",
);
let gctx = GlobalContextBuilder::new()
.cwd("foo")
.nightly_features_allowed(true)
.build();
assert_eq!(gctx.get::<i32>("key1").unwrap(), 1);
assert_eq!(gctx.get::<i32>("key2").unwrap(), 2);
assert_eq!(gctx.get::<i32>("key3").unwrap(), 3);
assert_eq!(gctx.get::<i32>("key4").unwrap(), 4);
assert_eq!(
gctx.get::<Vec<String>>("unstable.features").unwrap(),
vec![
"4".to_string(),
"3".to_string(),
"2".to_string(),
"1".to_string()
]
);
}

#[cargo_test]
fn works_with_cli() {
write_config_at(
Expand Down