-
-
Notifications
You must be signed in to change notification settings - Fork 134
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>, | ||||||
|
Member
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. nice! |
||||||
| 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
Member
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?) 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. I noticed in the functions they all have the same or similar format of: On top of that we'd have to include inheritance chain logic to go up the chain for any parent Would it be appropriate to encapsulate all of this into a declarative macro? |
||||||
| } | ||||||
|
|
||||||
| /// 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()); | ||||||
|
Member
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> { | ||||||
|
Member
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(); | ||||||
|
Member
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", | ||||||
|
Member
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), | ||||||
|
Member
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> | ||||||
|
Member
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.
Hi @sunshowers, I was taking a look at this part of the code and was wondering if this needed to be resolved as a
Vec<&'cfg CustomProfileImpl>? Earlier we checked forself.inner.check_inheritance_cycles(), which if it passes through that, that would indicate that there are no cycles detected and we can retrieve the root ancestor in the chain, right?Would it be appropriate to store this as an
Option<&'cfg CustomProfileImpl>?Uh oh!
There was an error while loading. Please reload this page.
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.
Oh wait nevermind. I'm assuming within
EvaluatableProfileit is possible to inherit certain configs that are not seen at the end of the chain ofCustomProfileImpl?Like, if I had a profile called "foo" that inherits from a profile called "bar" which inherits from a profile called "baz".
The "bar" profile could have a filled in "slow_timeout" field while "baz" doesn't have a "slow_timeout" field. "foo" would still inherit the "slow_timeout" field from the "baz" profile, is what I'm understanding?