support config extensions

[onur-ozkan]

Copied from the rustc-dev-guide addition:

When working on different tasks, you might need to switch between different bootstrap >configurations.
Sometimes you may want to keep an old configuration for future use. But saving raw config >values in
random files and manually copying and pasting them can quickly become messy, especially if >you have a
long history of different configurations.

To simplify managing multiple configurations, you can create config extensions.

For example, you can create a simple config file named cross.toml:

[build]
build = "x86_64-unknown-linux-gnu"
host = ["i686-unknown-linux-gnu"]
target = ["i686-unknown-linux-gnu"]


[llvm]
download-ci-llvm = false

[target.x86_64-unknown-linux-gnu]
llvm-config = "/path/to/llvm-19/bin/llvm-config"

Then, include this in your bootstrap.toml:

include = ["cross.toml"]

You can also include extensions within extensions recursively.

Note: In the include field, the overriding logic follows a right-to-left order. For example,
in include = ["a.toml", "b.toml"], extension b.toml overrides a.toml. Also, parent extensions
always overrides the inner ones.

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[TimNN]

Some thoughts (aside from the fact that I like this feature):

1. Consider being explicit about the fact that the additional files must be in the root repository directory, both in the PR description and the docs. edit: I didn't consider the case of bootstrap.toml not being located in the root repository directory.

2. Consider changing that, I'd personally prefer having all custom profiles in a single directory (so they are grouped in the file list, and to make it easier to .gitignore them all).

3. As an alternative, switch from custom profiles to "includes", i.e. referencing files instead of profile names.

   • This would clearly differentiate built-in profiles from custom includes.
   • This would make it easy to use a custom directory for custom profiles, possibly even shared between multiple rust checkouts.
   • We could differentiate profiles and includes by checking for the .toml extension, or by having profile and include be separate fields.
   • Relative includes would need somewhat careful handling (should probably be relative to the current file).
   • Includes could open up the possibility of including multiple files (e.g. one to apply my "general" default configuration, and one to switch between different LLVM configurations). Though this may be better left for a follow-up.

[jieyouxu]

Rerolling review due to bandwidth because I'm still slowly testing #119899.
r? bootstrap

[Kobzol]

r? kobzol

Will take a look tomorrow.

[jieyouxu]

I haven't looked too closely, but based on description

Create some-config1 profile in some-config1.toml.
Create some-config2 profile in some-config2.toml and inherit some-config1.
Create some-config3 profile in some-config3.toml and inherit some-config2.

can this infinite loop? I guess even if it does, just don't do that 😆

[onur-ozkan]

I'm planning to rework on this next week.

@rustbot author

[onur-ozkan]

Consider being explicit about the fact that the additional files must be in the root repository directory, both in the PR description and the docs.

That's not actually true. I keep my config file outside the rust repository and invoke x from there to safely work on different checkouts. Bootstrap reads config files from the current working directory where you call it, so it doesn't have to be the project root.

[TimNN]

Consider being explicit about the fact that the additional files must be in the root repository directory, both in the PR description and the docs.

That's not actually true. I keep my config file outside the rust repository and invoke x from there to safely work on different checkouts. Bootstrap reads config files from the current working directory where you call it, so it doesn't have to be the project root.

Ah, sorry, I didn't consider that case (I didn't know that was even an option).

[rustbot]

This PR modifies bootstrap.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[onur-ozkan]

@rustbot ready

[Kobzol]

Looks simple enough. I think that we should also mark profile as deprecated and just replace it with include = ["src/bootstrap/defaults/<profile>"], and then remove it sometime in the future. Both to avoid having two ways of achieving a similar thing, and also to make the contents of the profiles more discoverable, before it was not at all obvious what does profile = "compiler" mean and from where it is taken.

[onur-ozkan]

Looks simple enough. I think that we should also mark profile as deprecated and just replace it with include = ["src/bootstrap/defaults/<profile>"], and then remove it sometime in the future. Both to avoid having two ways of achieving a similar thing, and also to make the contents of the profiles more discoverable, before it was not at all obvious what does profile = "compiler" mean and from where it is taken.

If we do it now, I have to implement the include logic on the python side, which I would rather avoid. I think we should handle the deprecation after the python removal (which is my second item next to stage0 redesign) to keep things simpler. Keeping the profile field for a while should be fine IMO.

[Kobzol]

Sure, that can wait :)

[Kobzol]

Looks good! Left a few nits.

[Kobzol]

This should be resolved relative to the top-level config path. Otherwise it would be inconsistent with how we resolve relative paths in the nested configs. Plus the top-level config might not be in the src directory, you can pass an arbitrary path to the config.

[onur-ozkan]

Oops, I thought config.src was the top-level path of config.toml for a second (confused with config.config), sorry!

[Kobzol]

I tried it with a few complicated-ish situations locally, and I'm afraid that the approach with ignoring duplicates doesn't work 🙈

Consider this:

# configs/root.toml
include = ["nested.toml"]

# configs/nested.toml
include = ["lld-disable.toml"]

[rust]
lld = true

# configs/lld-disable.toml
[rust]
lld = false

When you load configs/root.toml, the lld value will be false instead of true. Because when you load the config, this happens:

1. You load root.toml, and find the nested.toml include
2. You merge nested.toml into yourself with IgnoreDuplicate
3. You load nested.toml and find the lld-disable.toml include
4. You merge lld-disable.toml into yourself with IgnoreDuplicate. This sets lld = false.
5. The implementation of the merge from step 2) continues, where we try to merge lld = true into self.rust. However, since we already have rust.lld = false and we use the IgnoreDuplicate mode, this is skipped, and thus lld = true never overrides lld = false, as it should.

The overriding works for the top-level config, but not for the nested ones.

I think that in order to fix this, it should be enough to reorder code in the merge implementation. Essentially, we have to proceed in reverse order - the most important things should be merged first. So first we merge the actual config, then the includes, and then the profile.

I tried this locally and it seems to work, but I don't know if it's the right solution, because essentially it only works if we use IgnoreDuplicate. If we used Override instead, then the order should be reversed again. So maybe we should decide the order based on the replace mode? 🤷‍♂️

Here is my diff:

diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs
index 9a53ab71567..5357202b52e 100644
--- a/src/bootstrap/src/core/config/config.rs
+++ b/src/bootstrap/src/core/config/config.rs
@@ -686,7 +686,7 @@ pub fn from_triple(triple: &str) -> Self {
 /// This structure uses `Decodable` to automatically decode a TOML configuration
 /// file into this format, and then this is traversed and written into the above
 /// `Config` structure.
-#[derive(Deserialize, Default)]
+#[derive(Deserialize, Debug, Default)]
 #[serde(deny_unknown_fields, rename_all = "kebab-case")]
 pub(crate) struct TomlConfig {
     #[serde(flatten)]
@@ -713,7 +713,7 @@ pub enum ChangeId {
 /// for the "change-id" field to parse it even if other fields are invalid. This ensures
 /// that if deserialization fails due to other fields, we can still provide the changelogs
 /// to allow developers to potentially find the reason for the failure in the logs..
-#[derive(Deserialize, Default)]
+#[derive(Deserialize, Debug, Default)]
 pub(crate) struct ChangeIdWrapper {
     #[serde(alias = "change-id", default, deserialize_with = "deserialize_change_id")]
     pub(crate) inner: Option<ChangeId>,
@@ -773,6 +773,31 @@ fn do_merge<T: Merge>(x: &mut Option<T>, y: Option<T>, replace: ReplaceOpt) {
             }
         }
 
+        // We need to merge in reverse order of priotity - first the config, then includes, then
+        // the profile.
+        do_merge(&mut self.build, build, replace);
+        do_merge(&mut self.install, install, replace);
+        do_merge(&mut self.llvm, llvm, replace);
+        do_merge(&mut self.gcc, gcc, replace);
+        do_merge(&mut self.rust, rust, replace);
+        do_merge(&mut self.dist, dist, replace);
+
+        match (self.target.as_mut(), target) {
+            (_, None) => {}
+            (None, Some(target)) => self.target = Some(target),
+            (Some(original_target), Some(new_target)) => {
+                for (triple, new) in new_target {
+                    if let Some(original) = original_target.get_mut(&triple) {
+                        original.merge(None, &mut Default::default(), new, replace);
+                    } else {
+                        original_target.insert(triple, new);
+                    }
+                }
+            }
+        }
+
+        self.change_id.inner.merge(None, &mut Default::default(), change_id.inner, replace);
+
         let parent_dir = parent_config_path
             .as_ref()
             .and_then(|p| p.parent().map(ToOwned::to_owned))
@@ -808,29 +833,7 @@ fn do_merge<T: Merge>(x: &mut Option<T>, y: Option<T>, replace: ReplaceOpt) {
             included_extensions.remove(&include_path);
         }
 
-        self.change_id.inner.merge(None, &mut Default::default(), change_id.inner, replace);
         self.profile.merge(None, &mut Default::default(), profile, replace);
-
-        do_merge(&mut self.build, build, replace);
-        do_merge(&mut self.install, install, replace);
-        do_merge(&mut self.llvm, llvm, replace);
-        do_merge(&mut self.gcc, gcc, replace);
-        do_merge(&mut self.rust, rust, replace);
-        do_merge(&mut self.dist, dist, replace);
-
-        match (self.target.as_mut(), target) {
-            (_, None) => {}
-            (None, Some(target)) => self.target = Some(target),
-            (Some(original_target), Some(new_target)) => {
-                for (triple, new) in new_target {
-                    if let Some(original) = original_target.get_mut(&triple) {
-                        original.merge(None, &mut Default::default(), new, replace);
-                    } else {
-                        original_target.insert(triple, new);
-                    }
-                }
-            }
-        }
     }
 }
 
@@ -840,6 +843,7 @@ macro_rules! define_config {
         $($field:ident: Option<$field_ty:ty> = $field_key:literal,)*
     }) => {
         $(#[$attr])*
+        #[derive(Debug)]
         struct $name {
             $($field: Option<$field_ty>,)*
         }
@@ -1639,12 +1643,13 @@ pub(crate) fn parse_inner(
         // This must be handled before applying the `profile` since `include`s should always take
         // precedence over `profile`s.
         for include_path in toml.include.clone().unwrap_or_default().iter().rev() {
-            let included_toml = get_toml(include_path).unwrap_or_else(|e| {
+            let include_path = toml_path.parent().unwrap().join(include_path);
+            let included_toml = get_toml(&include_path).unwrap_or_else(|e| {
                 eprintln!("ERROR: Failed to parse '{}': {e}", include_path.display());
                 exit!(2);
             });
             toml.merge(
-                Some(toml_path.join(include_path)),
+                Some(include_path),
                 &mut Default::default(),
                 included_toml,
                 ReplaceOpt::IgnoreDuplicate,
@@ -1721,6 +1726,7 @@ fn get_table(option: &str) -> Result<TomlConfig, toml::de::Error> {
         }
         toml.merge(None, &mut Default::default(), override_toml, ReplaceOpt::Override);
 
+        eprintln!("{toml:#?}");
         config.change_id = toml.change_id.inner;
 
         let Build {

[Kobzol]

Suggested change

	let included_toml = get_toml(include_path).unwrap_or_else(|e| {
	let included_toml = get_toml(toml_path.parent().unwrap().join(include_path)).unwrap_or_else(|e| {

Otherwise the file isn't resolved correctly. (Better do something like let include_path = toml_path.parent().unwrap().join(include_path); at the beginning of the loop).

[onur-ozkan]

I feel like there might be some incorrect cases with your approach. I need to re-check/test everything again.

The implementation becomes flaky with each change. I will add test coverage (next week or so) for each case to ensure everything works properly now and in the future.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.