Allow enabling config-include feature in config

[lschuermann]

What does this PR try to resolve?

Prior to this change, it is not possible to enable the unstable config-include flag from within a config file itself. This is due to the fact that, while cargo does reload configuration files if this flag is set, it does not parse the top-level configuration file's unstable flags before checking whether this flag is present.

This commit forces cargo to load unstable features before this check, and if the flag is present, re-loads configuration files with config-include enabled.

Addresses #7723 (comment).

How should we test and review this PR?

This PR adds a testcase for this behavior to cargo's testsuite. In general, it can be replicated with a config similar to that in the testcase, or as illustrated in #7723 (comment).

Additional information

I'm not sure whether this is the right(TM) fix, or whether the second self.load_unstable_flags_from_config()?; at the bottom of configure() would still be required. I have a feeling it might not be, as reload_rooted_at also calls out to load_unstable_flags_from_config, to collect flags from any newly included configs. If that is not called, no other file was loaded, and thus there should not be a need to re-load unstable flags.

Resolved: #14196 (comment)

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[epage]

r? weihanglo

If thats ok with you as you have a lot more context on this feature.

[weihanglo]

Thanks for the contribution!

Could you split this into two commits that the first one includes the new test, which demonstrates the buggy behavior and passes the CI pipeline. The second commit fixes both the behavior and the test. This pattern is documented here as a tip, which helps use make sure we're not fixing a nonexistent bug. The first commit is also served as an MRE.

[weihanglo]

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.

[lschuermann]

Removed, thanks for the confirmation!

[weihanglo]

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()
        ]
    );
}

[lschuermann]

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.

[weihanglo]

Yeah. That sounds good to me :)

[lschuermann]

@rustbot review

[lschuermann]

Could you split this into two commits that the first one includes the new test, which demonstrates the buggy behavior and passes the CI pipeline. The second commit fixes both the behavior and the test. This pattern is documented here as a tip, which helps use make sure we're not fixing a nonexistent bug. The first commit is also served as an MRE.

I don't know whether there's any infrastructure in place to automate this check (other than pushing every commit individually), but all commits pass the respective tests for me locally:

> git rebase -i --exec 'git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include' HEAD~3
hint: Waiting for your editor to close the file... Waiting for Emacs...
Executing: git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include
 8bfc31f97 test(add): show config-include cannot be enabled in top-level config

running 13 tests
.............
test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 3353 filtered out; finished in 0.31s

Executing: git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include
 45e5cb496 test(add): show config-include correctly merges unstable configs

running 15 tests
...............
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 3353 filtered out; finished in 0.35s

Executing: git log --format="%C(auto) %h %s" -1 && cargo test -q --test testsuite -- config_include
 0e35bea6c Allow enabling `config-include` feature in config

running 15 tests
...............
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 3353 filtered out; finished in 0.33s

Successfully rebased and updated refs/heads/dev/config-include-enable-in-config.

[weihanglo]

Awesome. Let's merge it!

[weihanglo]

I don't know whether there's any infrastructure in place to automate this check (other than pushing every commit individually), but all commits pass the respective tests for me locally:

Not really. CI doesn't enforce that. It's a good idea if we make it an optional CI job, though PR with lots of commits might not like it 😆.

[weihanglo]

@bors r+

[bors]

📌 Commit 0e35bea has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0e35bea with merge c7be9e4...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing c7be9e4 to master...