-
-
Notifications
You must be signed in to change notification settings - Fork 121
feat: cargo profiles inheritance #2595
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -37,9 +37,10 @@ use config::{ | |||||
use iddqd::IdOrdMap; | ||||||
use indexmap::IndexMap; | ||||||
use nextest_filtering::{BinaryQuery, EvalContext, Filterset, ParseContext, TestQuery}; | ||||||
use petgraph::{algo::toposort, Directed, Graph}; | ||||||
use serde::Deserialize; | ||||||
use std::{ | ||||||
collections::{BTreeMap, BTreeSet, HashMap, hash_map}, | ||||||
collections::{hash_map, BTreeMap, BTreeSet, HashMap, HashSet}, | ||||||
sync::LazyLock, | ||||||
}; | ||||||
use tracing::warn; | ||||||
|
@@ -792,7 +793,15 @@ impl NextestConfig { | |||||
} | ||||||
|
||||||
fn make_profile(&self, name: &str) -> Result<EarlyProfile<'_>, ProfileNotFound> { | ||||||
// Check for cycles first | ||||||
self.inner.check_inheritance_cycles().unwrap(); | ||||||
|
||||||
let custom_profile = self.inner.get_profile(name)?; | ||||||
let inheritance_chain = if let Some(_) = custom_profile { | ||||||
self.inner.resolve_profile_chain(name)? | ||||||
} else { | ||||||
Vec::new() | ||||||
}; | ||||||
|
||||||
// The profile was found: construct it. | ||||||
let mut store_dir = self.workspace_root.join(&self.inner.store.dir); | ||||||
|
@@ -809,6 +818,7 @@ impl NextestConfig { | |||||
store_dir, | ||||||
default_profile: &self.inner.default_profile, | ||||||
custom_profile, | ||||||
inheritance_chain, | ||||||
test_groups: &self.inner.test_groups, | ||||||
scripts: &self.inner.scripts, | ||||||
compiled_data, | ||||||
|
@@ -874,6 +884,7 @@ pub struct EarlyProfile<'cfg> { | |||||
store_dir: Utf8PathBuf, | ||||||
default_profile: &'cfg DefaultProfileImpl, | ||||||
custom_profile: Option<&'cfg CustomProfileImpl>, | ||||||
inheritance_chain: Vec<&'cfg CustomProfileImpl>, | ||||||
test_groups: &'cfg BTreeMap<CustomTestGroup, TestGroupConfig>, | ||||||
// This is ordered because the scripts are used in the order they're defined. | ||||||
scripts: &'cfg ScriptConfig, | ||||||
|
@@ -924,6 +935,7 @@ impl<'cfg> EarlyProfile<'cfg> { | |||||
store_dir: self.store_dir, | ||||||
default_profile: self.default_profile, | ||||||
custom_profile: self.custom_profile, | ||||||
inheritance_chain: self.inheritance_chain, | ||||||
scripts: self.scripts, | ||||||
test_groups: self.test_groups, | ||||||
compiled_data, | ||||||
|
@@ -941,6 +953,7 @@ pub struct EvaluatableProfile<'cfg> { | |||||
store_dir: Utf8PathBuf, | ||||||
default_profile: &'cfg DefaultProfileImpl, | ||||||
custom_profile: Option<&'cfg CustomProfileImpl>, | ||||||
inheritance_chain: Vec<&'cfg CustomProfileImpl>, // Add this | ||||||
test_groups: &'cfg BTreeMap<CustomTestGroup, TestGroupConfig>, | ||||||
// This is ordered because the scripts are used in the order they're defined. | ||||||
scripts: &'cfg ScriptConfig, | ||||||
|
@@ -984,11 +997,24 @@ impl<'cfg> EvaluatableProfile<'cfg> { | |||||
self.scripts | ||||||
} | ||||||
|
||||||
/// Returns the retry count for this profile. | ||||||
/// Returns the retry count for this profile, considering inheritance | ||||||
pub fn retries(&self) -> RetryPolicy { | ||||||
self.custom_profile | ||||||
.and_then(|profile| profile.retries) | ||||||
.unwrap_or(self.default_profile.retries) | ||||||
// Check custom profile first, then walk up inheritance chain | ||||||
if let Some(profile) = self.custom_profile { | ||||||
if let Some(retries) = profile.retries { | ||||||
return retries; | ||||||
} | ||||||
} | ||||||
|
||||||
// Walk up inheritance chain | ||||||
for parent in &self.inheritance_chain { | ||||||
if let Some(retries) = parent.retries { | ||||||
return retries; | ||||||
} | ||||||
} | ||||||
|
||||||
// Fall back to default | ||||||
self.default_profile.retries | ||||||
Comment on lines
+1002
to
+1017
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good -- you'll want to replicate this logic for all of the other config options here (can you extract this into a common function?) |
||||||
} | ||||||
|
||||||
/// Returns the number of threads to run against for this profile. | ||||||
|
@@ -1124,6 +1150,71 @@ pub(in crate::config) struct NextestConfigImpl { | |||||
} | ||||||
|
||||||
impl NextestConfigImpl { | ||||||
/// Resolves a profile with inheritance chain | ||||||
fn resolve_profile_chain(&self, profile_name: &str) -> Result<Vec<&CustomProfileImpl>, ProfileNotFound> { | ||||||
let mut visited = HashSet::new(); | ||||||
let mut chain = Vec::new(); | ||||||
|
||||||
self.resolve_profile_chain_recursive(profile_name, &mut visited, &mut chain)?; | ||||||
Ok(chain) | ||||||
} | ||||||
|
||||||
fn resolve_profile_chain_recursive<'cfg>( | ||||||
&'cfg self, | ||||||
profile_name: &str, | ||||||
visited: &mut HashSet<String>, | ||||||
chain: &mut Vec<&'cfg CustomProfileImpl>, | ||||||
) -> Result<(), ProfileNotFound> { | ||||||
if visited.contains(profile_name) { | ||||||
return Err(ProfileNotFound::new( | ||||||
profile_name, | ||||||
self.all_profiles().collect::<Vec<_>>(), | ||||||
)); | ||||||
} | ||||||
|
||||||
visited.insert(profile_name.to_string()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need the You might run into variance issues with storing a reference, but I think you can have another lifetime parameter for that. |
||||||
|
||||||
let profile = self.get_profile(profile_name)?; | ||||||
if let Some(profile) = profile { | ||||||
if let Some(parent_name) = &profile.inherit { | ||||||
self.resolve_profile_chain_recursive(parent_name, visited, chain)?; | ||||||
} | ||||||
chain.push(profile); | ||||||
} | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
|
||||||
fn check_inheritance_cycles(&self) -> Result<(), ConfigParseError> { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need some tests, both for inheritance and for cycles. |
||||||
let mut graph = Graph::<String, (), Directed>::new(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, can you store references? |
||||||
let mut node_map = HashMap::new(); | ||||||
|
||||||
for profile in self.all_profiles() { | ||||||
let node = graph.add_node(profile.to_string()); | ||||||
node_map.insert(profile.to_string(), node); | ||||||
} | ||||||
|
||||||
for (profile_name, profile) in &self.other_profiles { | ||||||
if let Some(inherit_name) = &profile.inherit { | ||||||
if let (Some(&from), Some(&to)) = (node_map.get(inherit_name), node_map.get(profile_name)) { | ||||||
graph.add_edge(from, to, ()); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
match toposort(&graph, None) { | ||||||
Ok(_) => Ok(()), | ||||||
Err(cycle) => { | ||||||
let cycle_profile = graph[cycle.node_id()].clone(); | ||||||
Err(ConfigParseError::new( | ||||||
"Inheritance cycle detected in profile configuration", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use lowercase for errors:
Suggested change
|
||||||
None, | ||||||
ConfigParseErrorKind::InheritanceCycle(cycle_profile), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use https://docs.rs/petgraph/latest/petgraph/algo/fn.kosaraju_scc.html to detect all cycles here? Each SCC returned by the algorithm is a cycle -- it would be nice to print out all such cycles here. |
||||||
)) | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
fn get_profile(&self, profile: &str) -> Result<Option<&CustomProfileImpl>, ProfileNotFound> { | ||||||
let custom_profile = match profile { | ||||||
NextestConfig::DEFAULT_PROFILE => None, | ||||||
|
@@ -1337,6 +1428,8 @@ pub(in crate::config) struct CustomProfileImpl { | |||||
junit: JunitImpl, | ||||||
#[serde(default)] | ||||||
archive: Option<ArchiveConfig>, | ||||||
#[serde(default)] | ||||||
inherit: Option<String> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need a trailing comma (did rustfmt not catch this?) |
||||||
} | ||||||
|
||||||
impl CustomProfileImpl { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!