Skip to content

support config extensions #138934

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

Merged
merged 10 commits into from
Apr 20, 2025
Merged

support config extensions #138934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1c1febc
add new config option: `include`
onur-ozkan Apr 1, 2025
89e3bef
document config extensions
onur-ozkan Mar 25, 2025
4e80659
implement cyclic inclusion handling
onur-ozkan Apr 1, 2025
78cb453
document `include` in `bootstrap.example.toml`
onur-ozkan Apr 1, 2025
3f70f19
apply nit notes
onur-ozkan Apr 1, 2025
8e6f50b
fix path and the ordering logic
onur-ozkan Apr 15, 2025
7dfb457
add FIXME note in `TomlConfig::merge`
onur-ozkan Apr 15, 2025
6d52b51
add comment in `TomlConfig::merge` about the merge order
onur-ozkan Apr 15, 2025
8270478
resolve config include FIXME
onur-ozkan Apr 16, 2025
ac7d1be
add coverage on config include logic
onur-ozkan Apr 16, 2025
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
8 changes: 8 additions & 0 deletions bootstrap.example.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@
# Note that this has no default value (x.py uses the defaults in `bootstrap.example.toml`).
#profile = <none>

# Inherits configuration values from different configuration files (a.k.a. config extensions).
# Supports absolute paths, and uses the current directory (where the bootstrap was invoked)
# as the base if the given path is not absolute.
#
# 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.
#include = []

# Keeps track of major changes made to this configuration.
#
# This value also represents ID of the PR that caused major changes. Meaning,
Expand Down
133 changes: 115 additions & 18 deletions src/bootstrap/src/core/config/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use std::cell::{Cell, RefCell};
use std::collections::{BTreeSet, HashMap, HashSet};
use std::fmt::{self, Display};
use std::hash::Hash;
use std::io::IsTerminal;
use std::path::{Path, PathBuf, absolute};
use std::process::Command;
Expand Down Expand Up @@ -701,6 +702,7 @@ pub(crate) struct TomlConfig {
target: Option<HashMap<String, TomlTarget>>,
dist: Option<Dist>,
profile: Option<String>,
include: Option<Vec<PathBuf>>,
}

/// This enum is used for deserializing change IDs from TOML, allowing both numeric values and the string `"ignore"`.
Expand Down Expand Up @@ -747,27 +749,35 @@ enum ReplaceOpt {
}

trait Merge {
fn merge(&mut self, other: Self, replace: ReplaceOpt);
fn merge(
&mut self,
parent_config_path: Option<PathBuf>,
included_extensions: &mut HashSet<PathBuf>,
other: Self,
replace: ReplaceOpt,
);
}

impl Merge for TomlConfig {
fn merge(
&mut self,
TomlConfig { build, install, llvm, gcc, rust, dist, target, profile, change_id }: Self,
parent_config_path: Option<PathBuf>,
included_extensions: &mut HashSet<PathBuf>,
TomlConfig { build, install, llvm, gcc, rust, dist, target, profile, change_id, include }: Self,
replace: ReplaceOpt,
) {
fn do_merge<T: Merge>(x: &mut Option<T>, y: Option<T>, replace: ReplaceOpt) {
if let Some(new) = y {
if let Some(original) = x {
original.merge(new, replace);
original.merge(None, &mut Default::default(), new, replace);
} else {
*x = Some(new);
}
}
}

self.change_id.inner.merge(change_id.inner, replace);
self.profile.merge(profile, replace);
self.change_id.inner.merge(None, &mut Default::default(), change_id.inner, replace);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should put an assert here that replace is IgnoreDuplicates? Because if it isn't, the priority logic of this function becomes wrong again.

Also please add a comment that explains why we use this ordering; we need to first merge our own data when combined with IgnoreDuplicates, and only then apply the includes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should put an assert here that replace is IgnoreDuplicates? Because if it isn't, the priority logic of this function becomes wrong again.

But isn’t that relevant to the caller? I think that coverage is better handled by a unit test instead. There might not be any extra config in include in which case this function ends up working the same as it does on the current upstream master.

Also please add a comment that explains why we use this ordering; we need to first merge our own data when combined with IgnoreDuplicates, and only then apply the includes.

Sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

But isn’t that relevant to the caller? I think that coverage is better handled by a unit test instead. There might not be any extra config in include in which case this function ends up working the same as it does on the current upstream master.

The function has an invariant that it keeps a certain preference order for profiles/includes/other parameters. This invariant only holds when IgnoreDuplicates is passed; if someone passes something else, the function breaks. So in that case we should add an assert.

Arguably, the merge function shouldn't be used for includes and profiles, precisely because it only works with a single replace mode. The fact that you had to pass the original config path to the function kind of hints that the function is being abused and it is quite hacky :) But I don't want to block this PR further, let's keep it like it is, and just add an assert to make sure that it won't silently change behavior in the future without us knowing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd really prefer to resolve this FIXME and handle that case properly with a test coverage rather than adding that assertion into the implementation details. I will try to allocate some time to do it by the end of today or early tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

A test would be nice, but that doesn't change the fact that if the function is called with a different argument, it will silently break its behavior. Assertions are useful exactly for these kinds of situations.

self.profile.merge(None, &mut Default::default(), profile, replace);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused by this at first, but then I realized that profile merging is "lazy"; in includes, only the profile value is overwritten, but the final profile is only applied in the root config, not for the intermediate included configs. I guess that in most cases this won't be an issue, but it deserves at least a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it, could you clarify how does this cause an override? It should only use the root profile and ignore the rest if there are any.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I thought would happen is that profile replacement is "eager", in other words if you have something like this:

# root.toml
profile = "bar"
include = ["a.toml"]

# a.toml
profile = "foo"

include = ["b.toml"]

# b.toml
profile = "baz"

Then b.toml will be "filled" with data from baz, then a.toml will be "filled" with data from foo etc. But what happens is that only the profile key is overwritten, and the profile itself is only expanded in the root. That's probably fine though, I couldn't think of a situation where that is not what we want.

One weird thing that I just realized related to this is that the profile key now has higher precedence in the parent than the profile key in the child. So in the example above, the bar profile will be actually applied, even though I would expect that baz will be applied.


do_merge(&mut self.build, build, replace);
do_merge(&mut self.install, install, replace);
Expand All @@ -782,13 +792,50 @@ impl Merge for TomlConfig {
(Some(original_target), Some(new_target)) => {
for (triple, new) in new_target {
if let Some(original) = original_target.get_mut(&triple) {
original.merge(new, replace);
original.merge(None, &mut Default::default(), new, replace);
} else {
original_target.insert(triple, new);
}
}
}
}

let parent_dir = parent_config_path
.as_ref()
.and_then(|p| p.parent().map(ToOwned::to_owned))
.unwrap_or_default();

// `include` handled later since we ignore duplicates using `ReplaceOpt::IgnoreDuplicate` to
// keep the upper-level configuration to take precedence.
for include_path in include.clone().unwrap_or_default().iter().rev() {
let include_path = parent_dir.join(include_path);
let include_path = include_path.canonicalize().unwrap_or_else(|e| {
eprintln!("ERROR: Failed to canonicalize '{}' path: {e}", include_path.display());
exit!(2);
});

let included_toml = Config::get_toml_inner(&include_path).unwrap_or_else(|e| {
eprintln!("ERROR: Failed to parse '{}': {e}", include_path.display());
exit!(2);
});

assert!(
included_extensions.insert(include_path.clone()),
"Cyclic inclusion detected: '{}' is being included again before its previous inclusion was fully processed.",
include_path.display()
);

self.merge(
Some(include_path.clone()),
included_extensions,
included_toml,
// Ensures that parent configuration always takes precedence
// over child configurations.
ReplaceOpt::IgnoreDuplicate,
);

included_extensions.remove(&include_path);
}
}
}

Expand All @@ -803,7 +850,13 @@ macro_rules! define_config {
}

impl Merge for $name {
fn merge(&mut self, other: Self, replace: ReplaceOpt) {
fn merge(
&mut self,
_parent_config_path: Option<PathBuf>,
_included_extensions: &mut HashSet<PathBuf>,
other: Self,
replace: ReplaceOpt
) {
$(
match replace {
ReplaceOpt::IgnoreDuplicate => {
Expand Down Expand Up @@ -903,7 +956,13 @@ macro_rules! define_config {
}

impl<T> Merge for Option<T> {
fn merge(&mut self, other: Self, replace: ReplaceOpt) {
fn merge(
&mut self,
_parent_config_path: Option<PathBuf>,
_included_extensions: &mut HashSet<PathBuf>,
other: Self,
replace: ReplaceOpt,
) {
match replace {
ReplaceOpt::IgnoreDuplicate => {
if self.is_none() {
Expand Down Expand Up @@ -1363,13 +1422,15 @@ impl Config {
Self::get_toml(&builder_config_path)
}

#[cfg(test)]
pub(crate) fn get_toml(_: &Path) -> Result<TomlConfig, toml::de::Error> {
Ok(TomlConfig::default())
pub(crate) fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
#[cfg(test)]
return Ok(TomlConfig::default());

#[cfg(not(test))]
Self::get_toml_inner(file)
}

#[cfg(not(test))]
pub(crate) fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
fn get_toml_inner(file: &Path) -> Result<TomlConfig, toml::de::Error> {
let contents =
t!(fs::read_to_string(file), format!("config file {} not found", file.display()));
// Deserialize to Value and then TomlConfig to prevent the Deserialize impl of
Expand Down Expand Up @@ -1548,7 +1609,8 @@ impl Config {
// but not if `bootstrap.toml` hasn't been created.
let mut toml = if !using_default_path || toml_path.exists() {
config.config = Some(if cfg!(not(test)) {
toml_path.canonicalize().unwrap()
toml_path = toml_path.canonicalize().unwrap();
toml_path.clone()
} else {
toml_path.clone()
});
Expand Down Expand Up @@ -1576,6 +1638,26 @@ impl Config {
toml.profile = Some("dist".into());
}

// Reverse the list to ensure the last added config extension remains the most dominant.
// For example, given ["a.toml", "b.toml"], "b.toml" should take precedence over "a.toml".
//
// 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 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(include_path),
&mut Default::default(),
included_toml,
ReplaceOpt::IgnoreDuplicate,
);
}

if let Some(include) = &toml.profile {
// Allows creating alias for profile names, allowing
// profiles to be renamed while maintaining back compatibility
Expand All @@ -1597,7 +1679,12 @@ impl Config {
);
exit!(2);
});
toml.merge(included_toml, ReplaceOpt::IgnoreDuplicate);
toml.merge(
Some(include_path),
&mut Default::default(),
included_toml,
ReplaceOpt::IgnoreDuplicate,
);
}

let mut override_toml = TomlConfig::default();
Expand All @@ -1608,7 +1695,12 @@ impl Config {

let mut err = match get_table(option) {
Ok(v) => {
override_toml.merge(v, ReplaceOpt::ErrorOnDuplicate);
override_toml.merge(
None,
&mut Default::default(),
v,
ReplaceOpt::ErrorOnDuplicate,
);
continue;
}
Err(e) => e,
Expand All @@ -1619,7 +1711,12 @@ impl Config {
if !value.contains('"') {
match get_table(&format!(r#"{key}="{value}""#)) {
Ok(v) => {
override_toml.merge(v, ReplaceOpt::ErrorOnDuplicate);
override_toml.merge(
None,
&mut Default::default(),
v,
ReplaceOpt::ErrorOnDuplicate,
);
continue;
}
Err(e) => err = e,
Expand All @@ -1629,7 +1726,7 @@ impl Config {
eprintln!("failed to parse override `{option}`: `{err}");
exit!(2)
}
toml.merge(override_toml, ReplaceOpt::Override);
toml.merge(None, &mut Default::default(), override_toml, ReplaceOpt::Override);

config.change_id = toml.change_id.inner;

Expand Down
Loading
Loading