Skip to content

Commit

Permalink
fix: Fix custom IDs and implicit deps. (#1228)
Browse files Browse the repository at this point in the history
* Keep track of renamed IDs.

* Update changelog.

* Try a diff approach.

* Update tests.

* Fix tests.

* Try bash again.
  • Loading branch information
milesj authored Dec 13, 2023
1 parent d03be4d commit 82f00f1
Show file tree
Hide file tree
Showing 13 changed files with 169 additions and 102 deletions.
4 changes: 2 additions & 2 deletions .cargo/nextest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ failure-output = "immediate-final"
retries = 1
test-threads = 2

# [profile.default]
# test-threads = 8
[profile.default]
test-threads = 8
1 change: 1 addition & 0 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ jobs:
- uses: mozilla-actions/[email protected]
- name: Checking coverage status
id: coverage
shell: bash
run: echo "coverage=$WITH_COVERAGE" >> $GITHUB_OUTPUT
env:
WITH_COVERAGE:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,22 @@ source: crates/cli/tests/project_graph_test.rs
expression: assert.output()
---
digraph {
0 [ label="explicitAndImplicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
1 [ label="nodeNameScope" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="nodeNameOnly" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
3 [ label="node" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
0 [ label="nodeNameScope" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
1 [ label="nodeNameOnly" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="node" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
3 [ label="explicitAndImplicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
4 [ label="explicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
5 [ label="noLang" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
6 [ label="implicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
0 -> 1 [ label="production" arrowhead=none]
0 -> 2 [ label="peer" arrowhead=none]
3 -> 2 [ label="production" arrowhead=box, arrowtail=box]
3 -> 1 [ label="production" arrowhead=box, arrowtail=box]
0 -> 3 [ label="development" arrowhead=none]
4 -> 1 [ label="production" arrowhead=box, arrowtail=box]
4 -> 3 [ label="development" arrowhead=box, arrowtail=box]
6 -> 1 [ label="development" arrowhead=box, arrowtail=box]
6 -> 3 [ label="production" arrowhead=box, arrowtail=box]
2 -> 1 [ label="production" arrowhead=box, arrowtail=box]
2 -> 0 [ label="production" arrowhead=box, arrowtail=box]
3 -> 0 [ label="production" arrowhead=box, arrowtail=box]
3 -> 1 [ label="peer" arrowhead=box, arrowtail=box]
3 -> 2 [ label="development" arrowhead=box, arrowtail=box]
4 -> 0 [ label="production" arrowhead=box, arrowtail=box]
4 -> 2 [ label="development" arrowhead=box, arrowtail=box]
6 -> 0 [ label="development" arrowhead=box, arrowtail=box]
6 -> 2 [ label="production" arrowhead=box, arrowtail=box]
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ digraph {
1 [ label="emptyConfig" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="platforms" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
3 [ label="bar" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
4 [ label="basic" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
5 [ label="noConfig" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
4 [ label="noConfig" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
5 [ label="basic" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
6 [ label="advanced" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
7 [ label="foo" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
8 [ label="baz" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
4 -> 5 [ label="production" arrowhead=box, arrowtail=box]
7 -> 3 [ label="production" arrowhead=box, arrowtail=box]
7 -> 8 [ label="production" arrowhead=box, arrowtail=box]
7 [ label="baz" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
8 [ label="foo" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
5 -> 4 [ label="production" arrowhead=box, arrowtail=box]
8 -> 3 [ label="production" arrowhead=box, arrowtail=box]
8 -> 7 [ label="production" arrowhead=box, arrowtail=box]
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ source: crates/cli/tests/project_graph_test.rs
expression: assert.output()
---
digraph {
0 [ label="foo" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
1 [ label="bar" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="baz" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
0 -> 1 [ label="production" arrowhead=none]
0 -> 2 [ label="production" arrowhead=none]
0 [ label="bar" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
1 [ label="baz" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="foo" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 -> 0 [ label="production" arrowhead=box, arrowtail=box]
2 -> 1 [ label="production" arrowhead=box, arrowtail=box]
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@ source: crates/node/platform/tests/project_aliases_test.rs
expression: graph.to_dot()
---
digraph {
0 [ label="explicitAndImplicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
1 [ label="nodeNameScope" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="nodeNameOnly" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
3 [ label="node" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
0 [ label="nodeNameScope" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
1 [ label="nodeNameOnly" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
2 [ label="node" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
3 [ label="explicitAndImplicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
4 [ label="explicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
5 [ label="noLang" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
6 [ label="implicit" style=filled, shape=oval, fillcolor=gray, fontcolor=black]
0 -> 1 [ label="production" arrowhead=none]
0 -> 2 [ label="peer" arrowhead=none]
3 -> 2 [ label="production" arrowhead=box, arrowtail=box]
3 -> 1 [ label="production" arrowhead=box, arrowtail=box]
0 -> 3 [ label="development" arrowhead=none]
4 -> 1 [ label="production" arrowhead=box, arrowtail=box]
4 -> 3 [ label="development" arrowhead=box, arrowtail=box]
6 -> 1 [ label="development" arrowhead=box, arrowtail=box]
6 -> 3 [ label="production" arrowhead=box, arrowtail=box]
2 -> 1 [ label="production" arrowhead=box, arrowtail=box]
2 -> 0 [ label="production" arrowhead=box, arrowtail=box]
3 -> 0 [ label="production" arrowhead=box, arrowtail=box]
3 -> 1 [ label="peer" arrowhead=box, arrowtail=box]
3 -> 2 [ label="development" arrowhead=box, arrowtail=box]
4 -> 0 [ label="production" arrowhead=box, arrowtail=box]
4 -> 2 [ label="development" arrowhead=box, arrowtail=box]
6 -> 0 [ label="development" arrowhead=box, arrowtail=box]
6 -> 2 [ label="production" arrowhead=box, arrowtail=box]
}

11 changes: 10 additions & 1 deletion nextgen/project-graph/src/project_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub struct ProjectGraphCache<'graph> {
pub struct ProjectNode {
pub alias: Option<String>,
pub index: NodeIndex,
pub original_id: Option<Id>,
pub source: WorkspaceRelativePathBuf,
}

Expand Down Expand Up @@ -377,7 +378,15 @@ impl ProjectGraph {
} else {
self.nodes
.iter()
.find(|(_, node)| node.alias.as_ref().is_some_and(|a| a == alias_or_id))
.find(|(_, node)| {
node.alias
.as_ref()
.is_some_and(|alias| alias == alias_or_id)
|| node
.original_id
.as_ref()
.is_some_and(|id| id == alias_or_id)
})
.map(|(id, _)| id.as_str())
.unwrap_or(alias_or_id)
})
Expand Down
112 changes: 89 additions & 23 deletions nextgen/project-graph/src/project_graph_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,13 @@ pub struct ProjectGraphBuilder<'app> {
/// Nodes (projects) inserted into the graph.
nodes: FxHashMap<Id, NodeIndex>,

/// Projects that have explicitly renamed themselves.
/// Maps original ID to renamed ID.
renamed_ids: FxHashMap<Id, Id>,

/// The root project ID.
#[serde(skip)]
root_project_id: Option<Id>,
root_id: Option<Id>,

/// Mapping of project IDs to file system sources,
/// derived from the `workspace.projects` setting.
Expand All @@ -81,7 +85,8 @@ impl<'app> ProjectGraphBuilder<'app> {
aliases: FxHashMap::default(),
graph: DiGraph::new(),
nodes: FxHashMap::default(),
root_project_id: None,
renamed_ids: FxHashMap::default(),
root_id: None,
sources: FxHashMap::default(),
};

Expand Down Expand Up @@ -181,6 +186,12 @@ impl<'app> ProjectGraphBuilder<'app> {
});
}

for (original_id, id) in self.renamed_ids {
nodes.entry(id).and_modify(|node| {
node.original_id = Some(original_id);
});
}

let mut graph = ProjectGraph::new(self.graph, nodes, context.workspace_root);

graph.working_dir = context.working_dir.to_owned();
Expand Down Expand Up @@ -215,7 +226,7 @@ impl<'app> ProjectGraphBuilder<'app> {
&mut self,
project_locator: &str,
cycle: &mut FxHashSet<Id>,
) -> miette::Result<NodeIndex> {
) -> miette::Result<(Id, NodeIndex)> {
let id = self.resolve_id(project_locator);

// Already loaded, exit early with existing index
Expand All @@ -225,7 +236,7 @@ impl<'app> ProjectGraphBuilder<'app> {
"Project already exists in the project graph, skipping load",
);

return Ok(*index);
return Ok((id, *index));
}

// Check that the project ID is configured
Expand All @@ -239,44 +250,92 @@ impl<'app> ProjectGraphBuilder<'app> {
};

// Create the project
let project = self.build_project(id, source).await?;
let dependencies = project
.dependencies
.values()
.map(|v| v.to_owned())
.collect::<Vec<_>>(); // How to avoid cloning???

// Add to the graph
let mut project = self.build_project(id, source).await?;
let id = project.id.clone();
let index = self.graph.add_node(project);

cycle.insert(id.clone());
self.nodes.insert(id.clone(), index);

// Create dependent projects
for dep_config in dependencies {
if cycle.contains(&dep_config.id) {
let mut edges = vec![];

for dep_config in project.dependencies.values_mut() {
let loaded_dep_id = if cycle.contains(&dep_config.id) {
debug!(
id = id.as_str(),
dependency_id = dep_config.id.as_str(),
"Encountered a dependency cycle (from project); will disconnect nodes to avoid recursion",
);

continue;

// Don't link the root project to any project, but still load it
} else if matches!(dep_config.scope, DependencyScope::Root) {
self.internal_load(&dep_config.id, cycle).await?;
self.internal_load(&dep_config.id, cycle).await?.0

// Otherwise link projects
} else {
let dep_index = self.internal_load(&dep_config.id, cycle).await?;
let dep = self.internal_load(&dep_config.id, cycle).await?;
edges.push((dep.1, dep_config.scope));
dep.0
};

self.graph.add_edge(index, dep_index, dep_config.scope);
if loaded_dep_id != dep_config.id {
dep_config.id = loaded_dep_id;
}
}

// Add to the graph
let index = self.graph.add_node(project);

self.nodes.insert(id.clone(), index);

for edge in edges {
self.graph.add_edge(index, edge.0, edge.1);
}

cycle.clear();

Ok(index)
Ok((id, index))

// // Create the project
// let project = self.build_project(id, source).await?;
// let dependencies = project
// .dependencies
// .values()
// .map(|v| v.to_owned())
// .collect::<Vec<_>>(); // How to avoid cloning???

// // Add to the graph
// let id = project.id.clone();
// let index = self.graph.add_node(project);

// cycle.insert(id.clone());
// self.nodes.insert(id.clone(), index);

// // Create dependent projects
// for dep_config in dependencies {
// if cycle.contains(&dep_config.id) {
// debug!(
// id = id.as_str(),
// dependency_id = dep_config.id.as_str(),
// "Encountered a dependency cycle (from project); will disconnect nodes to avoid recursion",
// );

// // Don't link the root project to any project, but still load it
// } else if matches!(dep_config.scope, DependencyScope::Root) {
// self.internal_load(&dep_config.id, cycle).await?;

// // Otherwise link projects
// } else {
// let dep_index = self.internal_load(&dep_config.id, cycle).await?;

// self.graph.add_edge(index, dep_index, dep_config.scope);
// }
// }

// cycle.clear();

// Ok(index)
}

/// Create and build the project with the provided ID and source.
Expand All @@ -303,7 +362,7 @@ impl<'app> ProjectGraphBuilder<'app> {
.workspace_config
.experiments
.interweaved_task_inheritance,
root_project_id: self.root_project_id.as_ref(),
root_project_id: self.root_id.as_ref(),
toolchain_config: context.toolchain_config,
workspace_root: context.workspace_root,
},
Expand Down Expand Up @@ -350,6 +409,8 @@ impl<'app> ProjectGraphBuilder<'app> {
if let Some(alias) = project.alias.as_ref() {
self.aliases.insert(alias.to_owned(), project.id.clone());
}

self.renamed_ids.insert(id, project.id.clone());
}

Ok(project)
Expand Down Expand Up @@ -483,7 +544,7 @@ impl<'app> ProjectGraphBuilder<'app> {
.await?;

// Find the root project
self.root_project_id = sources.iter().find_map(|(id, source)| {
self.root_id = sources.iter().find_map(|(id, source)| {
if source.as_str().is_empty() || source.as_str() == "." {
Some(id.to_owned())
} else {
Expand All @@ -502,9 +563,14 @@ impl<'app> ProjectGraphBuilder<'app> {
}

fn resolve_id(&self, project_locator: &str) -> Id {
match self.aliases.get(project_locator) {
let id = match self.aliases.get(project_locator) {
Some(project_id) => project_id.to_owned(),
None => Id::raw(project_locator),
};

match self.renamed_ids.get(&id) {
Some(new_id) => new_id.to_owned(),
None => id,
}
}
}
Loading

0 comments on commit 82f00f1

Please sign in to comment.