From f985bf07b91008b86d7de5b8af9129e16ab9085a Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Tue, 14 Jun 2022 20:26:09 +0300 Subject: [PATCH 01/31] BuildPlan generation from lock file does not break when there is a dependency removal from manifest file. --- forc-pkg/src/pkg.rs | 66 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index f237d92e423..338b3cec263 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -163,6 +163,13 @@ pub struct BuildPlan { graph: Graph, path_map: PathMap, compilation_order: Vec, + /// The differences between graph described by lock_file and the graph that actually constructed. + /// + /// If the BuildPlan is constructed from a manifest file, this will be empty. + /// + /// If the BuildPlan is constructed from a lock file there might be some differences between the graph described by the lock file and the one that actually created. + /// The differences between them are the path dependencies that are removed from the manifest file after the lock file is created. + removed_deps: Vec, } /// Parameters to pass through to the `sway_core::BuildConfig` during compilation. @@ -218,7 +225,7 @@ pub type DependencyName = String; pub struct PkgDiff { pub added: Vec<(DependencyName, Pkg)>, - pub removed: Vec<(DependencyName, Pkg)>, + pub removed: Vec, } impl BuildPlan { @@ -231,6 +238,7 @@ impl BuildPlan { graph, path_map, compilation_order, + removed_deps: vec![], }) } @@ -271,18 +279,23 @@ impl BuildPlan { graph, path_map, compilation_order, + removed_deps: vec![], }) } /// Attempt to load the build plan from the `Lock`. pub fn from_lock(proj_path: &Path, lock: &Lock, sway_git_tag: &str) -> Result { - let graph = lock.to_graph()?; + let mut graph = lock.to_graph()?; + let tmp_compilation_order = compilation_order(&graph)?; + let (path_map, removed_deps) = + graph_to_path_map(proj_path, &mut graph, &tmp_compilation_order, sway_git_tag)?; + // Since path_map removes removed dependencies from the graph compilation order may change let compilation_order = compilation_order(&graph)?; - let path_map = graph_to_path_map(proj_path, &graph, &compilation_order, sway_git_tag)?; Ok(Self { graph, path_map, compilation_order, + removed_deps, }) } @@ -296,7 +309,7 @@ impl BuildPlan { /// Ensure that the build plan is valid for the given manifest. pub fn validate(&self, manifest: &Manifest, sway_git_tag: &str) -> Result { let mut added = vec![]; - let mut removed = vec![]; + let mut removed = self.removed_deps.clone(); // Retrieve project's graph node. let proj_node = *self .compilation_order @@ -345,11 +358,12 @@ impl BuildPlan { .into_iter() .map(|pkg| (pkg.0.clone(), pkg.1.clone())) .collect(); - removed = plan_dep_pkgs + let mut temp_removed = plan_dep_pkgs .difference(&manifest_dep_pkgs) .into_iter() - .map(|pkg| (pkg.0.clone(), pkg.1.clone())) + .map(|pkg| (pkg.0.clone())) .collect(); + removed.append(&mut temp_removed); } // Ensure the pkg names of all nodes match their associated manifests. @@ -366,7 +380,13 @@ impl BuildPlan { ); } } - Ok(PkgDiff { added, removed }) + println!("{:?}", added); + println!("{:?}", removed); + + Ok(PkgDiff { + added, + removed, + }) } /// View the build plan's compilation graph. @@ -392,7 +412,7 @@ fn remove_deps( graph: &mut Graph, path_map: &PathMap, proj_node: NodeIx, - to_remove: &[(DependencyName, Pkg)], + to_remove: &[DependencyName], ) { use petgraph::visit::Bfs; @@ -406,7 +426,7 @@ fn remove_deps( .is_none() || to_remove .iter() - .any(|removed_dep| removed_dep.1 == graph[node].unpinned(path_map)) + .any(|removed_dep| *removed_dep == graph[node].unpinned(path_map).name) { graph.remove_node(node); } @@ -754,11 +774,12 @@ pub fn compilation_order(graph: &Graph) -> Result> { /// containing the path to the local source for every node in the graph. pub fn graph_to_path_map( proj_manifest_dir: &Path, - graph: &Graph, + graph: &mut Graph, compilation_order: &[NodeIx], sway_git_tag: &str, -) -> Result { +) -> Result<(PathMap, Vec)> { let mut path_map = PathMap::new(); + let mut removed_deps = vec![]; // We resolve all paths in reverse compilation order. // That is, we follow paths starting from the project root. @@ -778,6 +799,27 @@ pub fn graph_to_path_map( // Resolve all following dependencies, knowing their parents' paths will already be resolved. for dep_node in path_resolve_order { let dep = &graph[dep_node]; + // If the dep is a path dependency check if it is removed. + if let SourcePinned::Path(_) = &dep.source { + // Retrieve the parent node to construct the relative path. + let (parent_node, _) = graph + .edges_directed(dep_node, Direction::Incoming) + .next() + .map(|edge| (edge.source(), edge.weight().clone())) + .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; + let parent = &graph[parent_node]; + + // Construct the path relative to the parent's path. + let parent_path = &path_map[&parent.id()]; + let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; + + // Check if parent's manifest file includes this dependency. If not this is a removed dependency and we should continue with the following dep and remove the removed dependency from the graph. + if parent_manifest.dep(&dep.name).is_none() { + removed_deps.push(dep.name.clone()); + graph.remove_node(dep_node); + continue; + } + } let dep_path = match &dep.source { SourcePinned::Root => bail!("more than one root package detected in graph"), SourcePinned::Git(git) => { @@ -843,7 +885,7 @@ pub fn graph_to_path_map( path_map.insert(dep.id(), dep_path.canonicalize()?); } - Ok(path_map) + Ok((path_map, removed_deps)) } /// Given a `graph`, the node index of a path dependency within that `graph`, and the supposed From b4219a7c2c6185d0ba94e155a5392480c4ed27c3 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 16 Jun 2022 21:10:01 +0300 Subject: [PATCH 02/31] While removing the deleted nodes handle orphaned childs --- forc-pkg/src/pkg.rs | 36 +++++++++++++++++++++++------------- forc/src/ops/forc_build.rs | 1 + 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 338b3cec263..e6a75fbcd75 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -163,10 +163,10 @@ pub struct BuildPlan { graph: Graph, path_map: PathMap, compilation_order: Vec, - /// The differences between graph described by lock_file and the graph that actually constructed. - /// + /// The differences between graph described by lock_file and the graph that actually constructed. + /// /// If the BuildPlan is constructed from a manifest file, this will be empty. - /// + /// /// If the BuildPlan is constructed from a lock file there might be some differences between the graph described by the lock file and the one that actually created. /// The differences between them are the path dependencies that are removed from the manifest file after the lock file is created. removed_deps: Vec, @@ -380,13 +380,7 @@ impl BuildPlan { ); } } - println!("{:?}", added); - println!("{:?}", removed); - - Ok(PkgDiff { - added, - removed, - }) + Ok(PkgDiff { added, removed }) } /// View the build plan's compilation graph. @@ -784,7 +778,6 @@ pub fn graph_to_path_map( // We resolve all paths in reverse compilation order. // That is, we follow paths starting from the project root. let mut path_resolve_order = compilation_order.iter().cloned().rev(); - // Add the project's package to the map. let proj_node = path_resolve_order .next() @@ -807,15 +800,32 @@ pub fn graph_to_path_map( .next() .map(|edge| (edge.source(), edge.weight().clone())) .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; - let parent = &graph[parent_node]; + + let parent = &graph[parent_node].clone(); // Construct the path relative to the parent's path. let parent_path = &path_map[&parent.id()]; let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; - // Check if parent's manifest file includes this dependency. If not this is a removed dependency and we should continue with the following dep and remove the removed dependency from the graph. + // Check if parent's manifest file includes this dependency. + // If not this is a removed dependency and we should continue with the following dep and remove the removed dependency from the graph. if parent_manifest.dep(&dep.name).is_none() { removed_deps.push(dep.name.clone()); + // We should be connecting the parent of the removed dependency to each child of removed node. + // Consider the following scenerio: a -> b -> c + // If you remove b, the edges between b -> c will be unvalidated so while processing c, you cannot find the parent. + // Instead you need to fetch all children of b and add an edge between those children and a before removing. + + // Get children of the node to delete + let child_nodes: Vec = graph + .edges_directed(dep_node, Direction::Outgoing) + .map(|edges| (edges.target())) + .collect(); + // Add an edge between the node that is going to be removed's child nodes and parent of the same node. + for child in child_nodes { + graph.add_edge(parent_node, child, parent.name.clone()); + } + // Remove the deleted node graph.remove_node(dep_node); continue; } diff --git a/forc/src/ops/forc_build.rs b/forc/src/ops/forc_build.rs index 06e4ff18cc4..92360bfcda9 100644 --- a/forc/src/ops/forc_build.rs +++ b/forc/src/ops/forc_build.rs @@ -91,6 +91,7 @@ pub fn build(command: BuildCommand) -> Result { let mut new_lock_cause = None; let mut plan = plan_result.or_else(|e| -> Result { if locked { + println!("{}", e); bail!( "The lock file {} needs to be updated but --locked was passed to prevent this.", lock_path.to_string_lossy() From 9495057008985ada19c15b75bfec529b7810ed51 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 16 Jun 2022 21:15:43 +0300 Subject: [PATCH 03/31] removed debug println --- forc/src/ops/forc_build.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/forc/src/ops/forc_build.rs b/forc/src/ops/forc_build.rs index 92360bfcda9..06e4ff18cc4 100644 --- a/forc/src/ops/forc_build.rs +++ b/forc/src/ops/forc_build.rs @@ -91,7 +91,6 @@ pub fn build(command: BuildCommand) -> Result { let mut new_lock_cause = None; let mut plan = plan_result.or_else(|e| -> Result { if locked { - println!("{}", e); bail!( "The lock file {} needs to be updated but --locked was passed to prevent this.", lock_path.to_string_lossy() From e97c684ee75e42778e9980dd1230b8b2f48c717e Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Fri, 17 Jun 2022 19:10:45 +0300 Subject: [PATCH 04/31] removed_deps removed from BuildPlan --- forc-pkg/src/pkg.rs | 136 +++++++++++++++++++++++++------------ forc/src/ops/forc_build.rs | 11 ++- 2 files changed, 102 insertions(+), 45 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index e6a75fbcd75..3d24fc653b5 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -5,7 +5,7 @@ use crate::{ use anyhow::{anyhow, bail, Context, Error, Result}; use forc_util::{ find_file_name, git_checkouts_directory, kebab_to_snake_case, print_on_failure, - print_on_success, print_on_success_library, println_yellow_err, + print_on_success, print_on_success_library, println_red, println_yellow_err, }; use fuels_types::JsonABI; use petgraph::{ @@ -163,13 +163,6 @@ pub struct BuildPlan { graph: Graph, path_map: PathMap, compilation_order: Vec, - /// The differences between graph described by lock_file and the graph that actually constructed. - /// - /// If the BuildPlan is constructed from a manifest file, this will be empty. - /// - /// If the BuildPlan is constructed from a lock file there might be some differences between the graph described by the lock file and the one that actually created. - /// The differences between them are the path dependencies that are removed from the manifest file after the lock file is created. - removed_deps: Vec, } /// Parameters to pass through to the `sway_core::BuildConfig` during compilation. @@ -238,7 +231,6 @@ impl BuildPlan { graph, path_map, compilation_order, - removed_deps: vec![], }) } @@ -279,37 +271,54 @@ impl BuildPlan { graph, path_map, compilation_order, - removed_deps: vec![], }) } /// Attempt to load the build plan from the `Lock`. - pub fn from_lock(proj_path: &Path, lock: &Lock, sway_git_tag: &str) -> Result { + pub fn from_lock( + proj_path: &Path, + lock: &Lock, + sway_git_tag: &str, + ) -> Result<(Self, Vec)> { let mut graph = lock.to_graph()?; let tmp_compilation_order = compilation_order(&graph)?; let (path_map, removed_deps) = - graph_to_path_map(proj_path, &mut graph, &tmp_compilation_order, sway_git_tag)?; + graph_to_path_map(proj_path, &graph, &tmp_compilation_order, sway_git_tag)?; + let removed_deps_vec = removed_deps + .clone() + .into_iter() + .map(|dep| graph[dep].name.clone()) + .collect(); + // Remove the unresolveable path dependencies from the graph + apply_diff_after_lock(removed_deps, &mut graph, &path_map, sway_git_tag)?; // Since path_map removes removed dependencies from the graph compilation order may change let compilation_order = compilation_order(&graph)?; - Ok(Self { - graph, - path_map, - compilation_order, - removed_deps, - }) + Ok(( + Self { + graph, + path_map, + compilation_order, + }, + removed_deps_vec, + )) } /// Attempt to load the build plan from the `Forc.lock` file. - pub fn from_lock_file(lock_path: &Path, sway_git_tag: &str) -> Result { + pub fn from_lock_file(lock_path: &Path, sway_git_tag: &str) -> Result<(Self, Vec)> { let proj_path = lock_path.parent().unwrap(); let lock = Lock::from_path(lock_path)?; Self::from_lock(proj_path, &lock, sway_git_tag) } /// Ensure that the build plan is valid for the given manifest. - pub fn validate(&self, manifest: &Manifest, sway_git_tag: &str) -> Result { + pub fn validate( + &self, + removed: Vec, + manifest: &Manifest, + sway_git_tag: &str, + ) -> Result { let mut added = vec![]; - let mut removed = self.removed_deps.clone(); + let mut removed = removed; // Retrieve project's graph node. let proj_node = *self .compilation_order @@ -764,16 +773,75 @@ pub fn compilation_order(graph: &Graph) -> Result> { }) } +/// Applies the differences between the graph that is described by the lock file and the graph that +/// is actually possible to build. +/// +/// +/// There might be some differences between the graph that is described by the lock file and +/// the graph that can actually possible to build. This is happening in the case of a dependency +/// (described by path) removal after the lock file is generated. Since dependency is removed +/// graph_to_path_map cannot find the required path for the dependency entry in the lock file. +/// This requires us to remove those removed nodes from the graph so that we can have a BuildPlan +/// generated from lock file. After the BuildPlan is generated it can be used to apply diffs by +/// apply_pkg_diff(). +fn apply_diff_after_lock( + deps_to_remove: HashSet, + graph: &mut Graph, + path_map: &PathMap, + sway_git_tag: &str, +) -> Result<()> { + for dep in deps_to_remove { + let dep_name = &graph[dep].name.clone(); + + let (parent_node, _) = graph + .edges_directed(dep, Direction::Incoming) + .next() + .map(|edge| (edge.source(), edge.weight().clone())) + .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; + + let parent = &graph[parent_node].clone(); + // Construct the path relative to the parent's path. + let parent_path = &path_map[&parent.id()]; + let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; + + // Check if parent's manifest file includes this dependency. + // If not this is a removed dependency and we should remove the removed dependency from the graph. + if parent_manifest.dep(dep_name).is_none() { + // We should be connecting the parent of the removed dependency to each child of removed node. + // Consider the following scenerio: a -> b -> c + // If you remove b, the edges between b -> c will be unvalidated so while processing c, you cannot find the parent. + // Instead you need to fetch all children of b and add an edge between those children and a before removing. + + // Get children of the node to delete + let child_nodes: Vec = graph + .edges_directed(dep, Direction::Outgoing) + .map(|edges| (edges.target())) + .collect(); + // Add an edge between the node that is going to be removed's child nodes and parent of the same node. + for child in child_nodes { + // Check if there is already an edge if there is not add it + if !graph.edges(parent_node).any(|edge| edge.target() == child) { + graph.add_edge(parent_node, child, graph[child].name.clone()); + } + } + println_red(&format!(" Removing {}", dep_name)); + // Remove the deleted node + graph.remove_node(dep); + } + } + Ok(()) +} + /// Given graph of pinned dependencies and the directory for the root node, produce a path map /// containing the path to the local source for every node in the graph. pub fn graph_to_path_map( proj_manifest_dir: &Path, - graph: &mut Graph, + graph: &Graph, compilation_order: &[NodeIx], sway_git_tag: &str, -) -> Result<(PathMap, Vec)> { +) -> Result<(PathMap, HashSet)> { let mut path_map = PathMap::new(); - let mut removed_deps = vec![]; + let mut removed_deps = HashSet::new(); // We resolve all paths in reverse compilation order. // That is, we follow paths starting from the project root. @@ -808,25 +876,9 @@ pub fn graph_to_path_map( let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; // Check if parent's manifest file includes this dependency. - // If not this is a removed dependency and we should continue with the following dep and remove the removed dependency from the graph. + // If not this is a removed dependency and we should continue with the following dep if parent_manifest.dep(&dep.name).is_none() { - removed_deps.push(dep.name.clone()); - // We should be connecting the parent of the removed dependency to each child of removed node. - // Consider the following scenerio: a -> b -> c - // If you remove b, the edges between b -> c will be unvalidated so while processing c, you cannot find the parent. - // Instead you need to fetch all children of b and add an edge between those children and a before removing. - - // Get children of the node to delete - let child_nodes: Vec = graph - .edges_directed(dep_node, Direction::Outgoing) - .map(|edges| (edges.target())) - .collect(); - // Add an edge between the node that is going to be removed's child nodes and parent of the same node. - for child in child_nodes { - graph.add_edge(parent_node, child, parent.name.clone()); - } - // Remove the deleted node - graph.remove_node(dep_node); + removed_deps.insert(dep_node); continue; } } diff --git a/forc/src/ops/forc_build.rs b/forc/src/ops/forc_build.rs index 06e4ff18cc4..039d5f4e4c8 100644 --- a/forc/src/ops/forc_build.rs +++ b/forc/src/ops/forc_build.rs @@ -77,7 +77,13 @@ pub fn build(command: BuildCommand) -> Result { let lock_path = lock_path(manifest.dir()); - let plan_result = pkg::BuildPlan::from_lock_file(&lock_path, SWAY_GIT_TAG); + let from_lock = pkg::BuildPlan::from_lock_file(&lock_path, SWAY_GIT_TAG); + let removed_deps = from_lock + .as_ref() + .map(|from_lock| from_lock.1.clone()) + .unwrap_or_default(); + + let plan_result = from_lock.map(|from_lock| from_lock.0); // Retrieve the old lock file state so we can produce a diff. let old_lock = plan_result @@ -85,7 +91,6 @@ pub fn build(command: BuildCommand) -> Result { .ok() .map(|plan| Lock::from_graph(plan.graph())) .unwrap_or_default(); - // Check if there are any errors coming from the BuildPlan generation from the lock file // If there are errors we will need to create the BuildPlan from scratch, i.e fetch & pin everything let mut new_lock_cause = None; @@ -108,7 +113,7 @@ pub fn build(command: BuildCommand) -> Result { // If there are no issues with the BuildPlan generated from the lock file // Check and apply the diff. if new_lock_cause.is_none() { - let diff = plan.validate(&manifest, SWAY_GIT_TAG)?; + let diff = plan.validate(removed_deps, &manifest, SWAY_GIT_TAG)?; if !diff.added.is_empty() || !diff.removed.is_empty() { new_lock_cause = Some(anyhow!("lock file did not match manifest `diff`")); plan = plan.apply_pkg_diff(diff, SWAY_GIT_TAG, offline)?; From 38af42ef5e64ab6ba5757a989d7152f07d186f60 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Fri, 17 Jun 2022 21:23:35 +0300 Subject: [PATCH 05/31] While checking from parent's manifest check for package name as well --- forc-pkg/src/pkg.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 3d24fc653b5..2fca89d5a54 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -870,14 +870,19 @@ pub fn graph_to_path_map( .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; let parent = &graph[parent_node].clone(); - // Construct the path relative to the parent's path. let parent_path = &path_map[&parent.id()]; let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; // Check if parent's manifest file includes this dependency. // If not this is a removed dependency and we should continue with the following dep - if parent_manifest.dep(&dep.name).is_none() { + // Note that we need to check from package name as well since dependency name and package name is not the same thing + if parent_manifest.dep(&dep.name).is_none() + && !parent_manifest + .deps() + .filter_map(|dep| dep.1.package()) + .any(|package| package == &dep.name) + { removed_deps.insert(dep_node); continue; } From e469399f7ed052173006b928294022ff28620d42 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Fri, 17 Jun 2022 21:25:06 +0300 Subject: [PATCH 06/31] removed unnecessary reference --- forc-pkg/src/pkg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 2fca89d5a54..6d5e951c069 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -881,7 +881,7 @@ pub fn graph_to_path_map( && !parent_manifest .deps() .filter_map(|dep| dep.1.package()) - .any(|package| package == &dep.name) + .any(|package| package == dep.name) { removed_deps.insert(dep_node); continue; From 4be588085d67c79e2d16252ad38640e6d5392fd9 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 20 Jun 2022 12:21:09 +0300 Subject: [PATCH 07/31] split validate into generate_diff and validate --- forc-pkg/src/pkg.rs | 30 +++++++++++++++++++----------- forc/src/ops/forc_build.rs | 4 ++-- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 6d5e951c069..cba0212f385 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -237,10 +237,11 @@ impl BuildPlan { /// Create a new build plan from an existing one. Needs the difference with the existing plan with the lock. pub fn apply_pkg_diff( &self, - pkg_diff: PkgDiff, + pkg_diff: &PkgDiff, sway_git_tag: &str, offline_mode: bool, ) -> Result { + self.validate(sway_git_tag)?; let mut graph = self.graph.clone(); let mut path_map = self.path_map.clone(); @@ -249,7 +250,7 @@ impl BuildPlan { .last() .ok_or_else(|| anyhow!("Invalid Graph"))?; let PkgDiff { added, removed } = pkg_diff; - remove_deps(&mut graph, &path_map, proj_node, &removed); + remove_deps(&mut graph, &path_map, proj_node, removed); let mut visited_map: HashMap = graph .node_references() @@ -261,12 +262,13 @@ impl BuildPlan { &mut graph, &mut path_map, &self.compilation_order, - &added, + added, sway_git_tag, offline_mode, &mut visited_map, )?; let compilation_order = compilation_order(&graph)?; + self.validate(sway_git_tag)?; Ok(Self { graph, path_map, @@ -279,7 +281,7 @@ impl BuildPlan { proj_path: &Path, lock: &Lock, sway_git_tag: &str, - ) -> Result<(Self, Vec)> { + ) -> Result<(Self, Vec)> { let mut graph = lock.to_graph()?; let tmp_compilation_order = compilation_order(&graph)?; let (path_map, removed_deps) = @@ -304,18 +306,19 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Forc.lock` file. - pub fn from_lock_file(lock_path: &Path, sway_git_tag: &str) -> Result<(Self, Vec)> { + pub fn from_lock_file( + lock_path: &Path, + sway_git_tag: &str, + ) -> Result<(Self, Vec)> { let proj_path = lock_path.parent().unwrap(); let lock = Lock::from_path(lock_path)?; Self::from_lock(proj_path, &lock, sway_git_tag) } - /// Ensure that the build plan is valid for the given manifest. - pub fn validate( + pub fn generate_diff( &self, - removed: Vec, + removed: Vec, manifest: &Manifest, - sway_git_tag: &str, ) -> Result { let mut added = vec![]; let mut removed = removed; @@ -347,7 +350,7 @@ impl BuildPlan { if det.version.is_some() { println_yellow_err(&format!( " WARNING! Dependency \"{}\" specifies the unused `version` field: \ - consider using `branch` or `tag` instead", + consider using `branch` or `tag` instead", dep_name )); } @@ -375,6 +378,11 @@ impl BuildPlan { removed.append(&mut temp_removed); } + Ok(PkgDiff { added, removed }) + } + + /// Ensure that the build plan is valid for the given manifest. + fn validate(&self, sway_git_tag: &str) -> Result<()> { // Ensure the pkg names of all nodes match their associated manifests. for node in self.graph.node_indices() { let pkg = &self.graph[node]; @@ -389,7 +397,7 @@ impl BuildPlan { ); } } - Ok(PkgDiff { added, removed }) + Ok(()) } /// View the build plan's compilation graph. diff --git a/forc/src/ops/forc_build.rs b/forc/src/ops/forc_build.rs index 039d5f4e4c8..b46212ca933 100644 --- a/forc/src/ops/forc_build.rs +++ b/forc/src/ops/forc_build.rs @@ -113,10 +113,10 @@ pub fn build(command: BuildCommand) -> Result { // If there are no issues with the BuildPlan generated from the lock file // Check and apply the diff. if new_lock_cause.is_none() { - let diff = plan.validate(removed_deps, &manifest, SWAY_GIT_TAG)?; + let diff = plan.generate_diff(removed_deps, &manifest)?; if !diff.added.is_empty() || !diff.removed.is_empty() { new_lock_cause = Some(anyhow!("lock file did not match manifest `diff`")); - plan = plan.apply_pkg_diff(diff, SWAY_GIT_TAG, offline)?; + plan = plan.apply_pkg_diff(&diff, SWAY_GIT_TAG, offline)?; } } From aa90853fa005084f9b21eb0829e75123c1317eaf Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 20 Jun 2022 14:28:43 +0300 Subject: [PATCH 08/31] apply_pkg_diff visits nodes to delete in reverse BFS order --- forc-pkg/src/pkg.rs | 54 +++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index cba0212f385..0a784878e69 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -286,14 +286,26 @@ impl BuildPlan { let tmp_compilation_order = compilation_order(&graph)?; let (path_map, removed_deps) = graph_to_path_map(proj_path, &graph, &tmp_compilation_order, sway_git_tag)?; - let removed_deps_vec = removed_deps + let removed_deps_vec: Vec = removed_deps .clone() .into_iter() .map(|dep| graph[dep].name.clone()) .collect(); + // Get the project node from compilation order. + let project_node = *tmp_compilation_order + .last() + .ok_or_else(|| anyhow!("can't get the project node"))?; + // Remove the unresolveable path dependencies from the graph - apply_diff_after_lock(removed_deps, &mut graph, &path_map, sway_git_tag)?; - // Since path_map removes removed dependencies from the graph compilation order may change + apply_diff_after_lock( + removed_deps, + &mut graph, + project_node, + &path_map, + sway_git_tag, + )?; + + // Since apply_diff_after_lock removes removed dependencies from the graph compilation order may change let compilation_order = compilation_order(&graph)?; Ok(( Self { @@ -306,6 +318,8 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Forc.lock` file. + /// Return the best effor BuildPlan and the packages removed + /// from project's manifest file after the lock file is generated. pub fn from_lock_file( lock_path: &Path, sway_git_tag: &str, @@ -795,14 +809,21 @@ pub fn compilation_order(graph: &Graph) -> Result> { fn apply_diff_after_lock( deps_to_remove: HashSet, graph: &mut Graph, + project_node: NodeIx, path_map: &PathMap, sway_git_tag: &str, ) -> Result<()> { - for dep in deps_to_remove { - let dep_name = &graph[dep].name.clone(); + use petgraph::visit::{Bfs, Walker}; + // Visit all the nodes after their parent is visited. + let deps: Vec = Bfs::new(&*graph, project_node).iter(&*graph).collect(); + + // Reverse the order so that we are sure we will get to a child node before it's parent. + // This is needed because if we remove parent of a node before itself we cannot find and check parent's manifest. + for dep in deps.iter().rev().filter(|dep| deps_to_remove.contains(dep)) { + let dep_name = &graph[*dep].name.clone(); let (parent_node, _) = graph - .edges_directed(dep, Direction::Incoming) + .edges_directed(*dep, Direction::Incoming) .next() .map(|edge| (edge.source(), edge.weight().clone())) .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; @@ -814,27 +835,12 @@ fn apply_diff_after_lock( // Check if parent's manifest file includes this dependency. // If not this is a removed dependency and we should remove the removed dependency from the graph. + + // After #1836 is merged, we will also need to check if it is inside patches section of the manifest. if parent_manifest.dep(dep_name).is_none() { - // We should be connecting the parent of the removed dependency to each child of removed node. - // Consider the following scenerio: a -> b -> c - // If you remove b, the edges between b -> c will be unvalidated so while processing c, you cannot find the parent. - // Instead you need to fetch all children of b and add an edge between those children and a before removing. - - // Get children of the node to delete - let child_nodes: Vec = graph - .edges_directed(dep, Direction::Outgoing) - .map(|edges| (edges.target())) - .collect(); - // Add an edge between the node that is going to be removed's child nodes and parent of the same node. - for child in child_nodes { - // Check if there is already an edge if there is not add it - if !graph.edges(parent_node).any(|edge| edge.target() == child) { - graph.add_edge(parent_node, child, graph[child].name.clone()); - } - } println_red(&format!(" Removing {}", dep_name)); // Remove the deleted node - graph.remove_node(dep); + graph.remove_node(*dep); } } Ok(()) From 316a871134cbaf81022055f15810207f578e017c Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 20 Jun 2022 16:36:29 +0300 Subject: [PATCH 09/31] docs added for generate_diff --- forc-pkg/src/pkg.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 0a784878e69..13030f67213 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -277,6 +277,10 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Lock`. + /// Returns the best effor BuildPlan and the packages removed + /// from project's manifest file after the lock file is generated. + /// + /// The returned removed dependencies are already removed from the dependency graph. pub fn from_lock( proj_path: &Path, lock: &Lock, @@ -318,8 +322,10 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Forc.lock` file. - /// Return the best effor BuildPlan and the packages removed + /// Returns the best effor BuildPlan and the packages removed /// from project's manifest file after the lock file is generated. + /// + /// The returned removed dependencies are already removed from the dependency graph. pub fn from_lock_file( lock_path: &Path, sway_git_tag: &str, @@ -329,6 +335,17 @@ impl BuildPlan { Self::from_lock(proj_path, &lock, sway_git_tag) } + /// Attempt to find the difference between the graph constructed from the lock file + /// and the graph that is described by the manifest file. The difference is used to + /// update BuildPlan. + /// + /// The `removed` is the dependencies that needed to be removed by the BuildPlan::from_lock + /// in order to construct the BuildPlan from the lock file. The removal is needed once there + /// is a dependency (provided as a path dependency) removed from the manifest file after the + /// lock file is generated. + /// + /// generate_diff combines those removed packages with other types of dependencies (ex: git dependencies) + /// and returnes the complete set of differences between the lock file and manifest file. pub fn generate_diff( &self, removed: Vec, From 38cc164d07a4e8a03db31d0a19d61e28cb7af775 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 23 Jun 2022 12:26:15 +0300 Subject: [PATCH 10/31] removed unnecessary logic in apply_diff_after_lock --- forc-pkg/src/pkg.rs | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index daadce19a63..943aff79cf0 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -886,35 +886,11 @@ fn apply_diff_after_lock( path_map: &PathMap, sway_git_tag: &str, ) -> Result<()> { - use petgraph::visit::{Bfs, Walker}; - // Visit all the nodes after their parent is visited. - let deps: Vec = Bfs::new(&*graph, project_node).iter(&*graph).collect(); - - // Reverse the order so that we are sure we will get to a child node before it's parent. - // This is needed because if we remove parent of a node before itself we cannot find and check parent's manifest. - for dep in deps.iter().rev().filter(|dep| deps_to_remove.contains(dep)) { + for dep in deps_to_remove { let dep_name = &graph[*dep].name.clone(); - - let (parent_node, _) = graph - .edges_directed(*dep, Direction::Incoming) - .next() - .map(|edge| (edge.source(), edge.weight().clone())) - .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; - - let parent = &graph[parent_node].clone(); - // Construct the path relative to the parent's path. - let parent_path = &path_map[&parent.id()]; - let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; - - // Check if parent's manifest file includes this dependency. - // If not this is a removed dependency and we should remove the removed dependency from the graph. - - // After #1836 is merged, we will also need to check if it is inside patches section of the manifest. - if parent_manifest.dep(dep_name).is_none() { - println_red(&format!(" Removing {}", dep_name)); - // Remove the deleted node - graph.remove_node(*dep); - } + println_red(&format!(" Removing {}", dep_name)); + // Remove the deleted node + graph.remove_node(*dep); } Ok(()) } From 344ceaa4a605ba4a4103431b32e375b58a6082c4 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 23 Jun 2022 12:45:17 +0300 Subject: [PATCH 11/31] doc updates --- forc-pkg/src/pkg.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 943aff79cf0..3ccb5d74193 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -383,7 +383,7 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Forc.lock` file. - /// Returns the best effor BuildPlan and the packages removed + /// Returns the best effort BuildPlan and the packages removed /// from project's manifest file after the lock file is generated. /// /// The returned removed dependencies are already removed from the dependency graph. @@ -399,14 +399,6 @@ impl BuildPlan { /// Attempt to find the difference between the graph constructed from the lock file /// and the graph that is described by the manifest file. The difference is used to /// update BuildPlan. - /// - /// The `removed` is the dependencies that needed to be removed by the BuildPlan::from_lock - /// in order to construct the BuildPlan from the lock file. The removal is needed once there - /// is a dependency (provided as a path dependency) removed from the manifest file after the - /// lock file is generated. - /// - /// generate_diff combines those removed packages with other types of dependencies (ex: git dependencies) - /// and returnes the complete set of differences between the lock file and manifest file. pub fn generate_diff(&self, manifest: &Manifest) -> Result { let mut added = vec![]; let mut removed = vec![]; @@ -871,6 +863,7 @@ pub fn compilation_order(graph: &Graph) -> Result> { /// Applies the differences between the graph that is described by the lock file and the graph that /// is actually possible to build. /// +/// `deps_to_remove` : Indices of path dependencies that are removed from manifest file after the lock file is generated. /// /// There might be some differences between the graph that is described by the lock file and /// the graph that can actually possible to build. This is happening in the case of a dependency From cb255e281c0704ff97c37fce05d98377f6c34fef Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 23 Jun 2022 12:47:40 +0300 Subject: [PATCH 12/31] clippy && extra dereferencing removed --- forc-pkg/src/pkg.rs | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 3ccb5d74193..c5f23bfd519 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -356,19 +356,9 @@ impl BuildPlan { .into_iter() .map(|dep| graph[dep].name.clone()) .collect(); - // Get the project node from compilation order. - let project_node = *tmp_compilation_order - .last() - .ok_or_else(|| anyhow!("can't get the project node"))?; // Remove the unresolveable path dependencies from the graph - apply_diff_after_lock( - removed_deps, - &mut graph, - project_node, - &path_map, - sway_git_tag, - )?; + apply_diff_after_lock(removed_deps, &mut graph)?; // Since apply_diff_after_lock removes removed dependencies from the graph compilation order may change let compilation_order = compilation_order(&graph)?; @@ -872,18 +862,12 @@ pub fn compilation_order(graph: &Graph) -> Result> { /// This requires us to remove those removed nodes from the graph so that we can have a BuildPlan /// generated from lock file. After the BuildPlan is generated it can be used to apply diffs by /// apply_pkg_diff(). -fn apply_diff_after_lock( - deps_to_remove: HashSet, - graph: &mut Graph, - project_node: NodeIx, - path_map: &PathMap, - sway_git_tag: &str, -) -> Result<()> { +fn apply_diff_after_lock(deps_to_remove: HashSet, graph: &mut Graph) -> Result<()> { for dep in deps_to_remove { - let dep_name = &graph[*dep].name.clone(); + let dep_name = &graph[dep].name.clone(); println_red(&format!(" Removing {}", dep_name)); // Remove the deleted node - graph.remove_node(*dep); + graph.remove_node(dep); } Ok(()) } From cb6a2e00fb995d121fee36119f0e437bf229e0d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kaya=20G=C3=B6kalp?= Date: Thu, 23 Jun 2022 12:49:21 +0300 Subject: [PATCH 13/31] Apply suggestions from code review effor -> effort Co-authored-by: mitchmindtree --- forc-pkg/src/pkg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index c5f23bfd519..7e6eb10d793 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -338,7 +338,7 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Lock`. - /// Returns the best effor BuildPlan and the packages removed + /// Returns the best effort BuildPlan and the packages removed /// from project's manifest file after the lock file is generated. /// /// The returned removed dependencies are already removed from the dependency graph. From bb40017d86a73ed762d12cd2513dfd9d4e4b392c Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 27 Jun 2022 10:38:15 +0300 Subject: [PATCH 14/31] duplicate code removed --- forc-pkg/src/pkg.rs | 39 ++++++++++++--------------------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 7e6eb10d793..b3c62952938 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -900,33 +900,6 @@ pub fn graph_to_path_map( // Resolve all following dependencies, knowing their parents' paths will already be resolved. for dep_node in path_resolve_order { let dep = &graph[dep_node]; - // If the dep is a path dependency check if it is removed. - if let SourcePinned::Path(_) = &dep.source { - // Retrieve the parent node to construct the relative path. - let (parent_node, _) = graph - .edges_directed(dep_node, Direction::Incoming) - .next() - .map(|edge| (edge.source(), edge.weight().clone())) - .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; - - let parent = &graph[parent_node].clone(); - // Construct the path relative to the parent's path. - let parent_path = &path_map[&parent.id()]; - let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; - - // Check if parent's manifest file includes this dependency. - // If not this is a removed dependency and we should continue with the following dep - // Note that we need to check from package name as well since dependency name and package name is not the same thing - if parent_manifest.dep(&dep.name).is_none() - && !parent_manifest - .deps() - .filter_map(|dep| dep.1.package()) - .any(|package| package == dep.name) - { - removed_deps.insert(dep_node); - continue; - } - } let dep_path = match &dep.source { SourcePinned::Root => bail!("more than one root package detected in graph"), SourcePinned::Git(git) => { @@ -959,6 +932,18 @@ pub fn graph_to_path_map( // Construct the path relative to the parent's path. let parent_path = &path_map[&parent.id()]; let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; + // Check if parent's manifest file includes this dependency. + // If not this is a removed dependency and we should continue with the following dep + // Note that we need to check from package name as well since dependency name and package name is not the same thing + if parent_manifest.dep(&dep.name).is_none() + && !parent_manifest + .deps() + .filter_map(|dep| dep.1.package()) + .any(|package| package == dep.name) + { + removed_deps.insert(dep_node); + continue; + } let detailed = parent_manifest .dependencies .as_ref() From 19050468415e3642844c95b05e73ae90eb3aaf7e Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 27 Jun 2022 11:30:58 +0300 Subject: [PATCH 15/31] change visibility of apply_pkg_diff and from_lock to private --- forc-pkg/src/pkg.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index b3c62952938..4df04a70649 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -296,7 +296,7 @@ impl BuildPlan { } /// Create a new build plan from an existing one. Needs the difference with the existing plan with the lock. - pub fn apply_pkg_diff( + fn apply_pkg_diff( &self, pkg_diff: &PkgDiff, sway_git_tag: &str, @@ -338,11 +338,11 @@ impl BuildPlan { } /// Attempt to load the build plan from the `Lock`. - /// Returns the best effort BuildPlan and the packages removed + /// Returns the best effort BuildPlan and the packages removed /// from project's manifest file after the lock file is generated. /// /// The returned removed dependencies are already removed from the dependency graph. - pub fn from_lock( + fn from_lock( proj_path: &Path, lock: &Lock, sway_git_tag: &str, From 5d466ec8dc7c479e72266231051bb49948532b36 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 27 Jun 2022 11:39:18 +0300 Subject: [PATCH 16/31] doc updated --- forc-pkg/src/pkg.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 4df04a70649..6940f50f174 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -853,7 +853,7 @@ pub fn compilation_order(graph: &Graph) -> Result> { /// Applies the differences between the graph that is described by the lock file and the graph that /// is actually possible to build. /// -/// `deps_to_remove` : Indices of path dependencies that are removed from manifest file after the lock file is generated. +/// `deps_to_remove` : Indices of path dependencies that are removed from their parent's manifest file after the lock file is generated. /// /// There might be some differences between the graph that is described by the lock file and /// the graph that can actually possible to build. This is happening in the case of a dependency @@ -874,6 +874,9 @@ fn apply_diff_after_lock(deps_to_remove: HashSet, graph: &mut Graph) -> /// Given graph of pinned dependencies and the directory for the root node, produce a path map /// containing the path to the local source for every node in the graph. +/// +/// Returns the generated path map and the nodes that should be removed from the graph (the nodes +/// that are removed from their parent's manifest file). pub fn graph_to_path_map( proj_manifest_dir: &Path, graph: &Graph, From 4ac21d430ad657b9b531b7f110c9806bb5d84c37 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 27 Jun 2022 12:00:04 +0300 Subject: [PATCH 17/31] patch checks are added --- forc-pkg/src/manifest.rs | 7 +++++++ forc-pkg/src/pkg.rs | 20 ++++++++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index cbb6b9c7b14..dc406c193f4 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -334,6 +334,13 @@ impl Manifest { .and_then(|deps| deps.get(dep_name)) } + /// Retrieve the listed patches for the given name. + pub fn patch(&self, patch_name: &str) -> Option<&BTreeMap> { + self.patch + .as_ref() + .and_then(|patches| patches.get(patch_name)) + } + /// Finds and returns the name of the dependency associated with a package of the specified /// name if there is one. /// diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 8a42691be61..536b6c69c8b 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -874,6 +874,19 @@ fn apply_diff_after_lock(deps_to_remove: HashSet, graph: &mut Graph) -> } Ok(()) } +/// Search dependencies and patches for the details for a pinned dependency +fn check_manifest_for_pinned(manifest: &ManifestFile, pinned_dep: &Pinned) -> bool { + manifest.dep(&pinned_dep.name).is_none() + && !manifest + .deps() + .filter_map(|dep| dep.1.package()) + .any(|package| package == pinned_dep.name) + && !manifest.patch(&pinned_dep.name).is_none() + && !manifest + .patches() + .find_map(|patches| patches.1.get(&pinned_dep.name)) + .is_none() +} /// Given graph of pinned dependencies and the directory for the root node, produce a path map /// containing the path to the local source for every node in the graph. @@ -941,12 +954,7 @@ pub fn graph_to_path_map( // Check if parent's manifest file includes this dependency. // If not this is a removed dependency and we should continue with the following dep // Note that we need to check from package name as well since dependency name and package name is not the same thing - if parent_manifest.dep(&dep.name).is_none() - && !parent_manifest - .deps() - .filter_map(|dep| dep.1.package()) - .any(|package| package == dep.name) - { + if check_manifest_for_pinned(&parent_manifest, dep) { removed_deps.insert(dep_node); continue; } From 057afeba14f47d3b282f07f636c8526a4b7afe85 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 27 Jun 2022 12:26:53 +0300 Subject: [PATCH 18/31] check for patches corrected --- forc-pkg/src/pkg.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 536b6c69c8b..6aaae992663 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -881,8 +881,8 @@ fn check_manifest_for_pinned(manifest: &ManifestFile, pinned_dep: &Pinned) -> bo .deps() .filter_map(|dep| dep.1.package()) .any(|package| package == pinned_dep.name) - && !manifest.patch(&pinned_dep.name).is_none() - && !manifest + && manifest.patch(&pinned_dep.name).is_none() + && manifest .patches() .find_map(|patches| patches.1.get(&pinned_dep.name)) .is_none() From f11442b25f9cc66bd5b24cbda1d6fda5362a2fea Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Wed, 29 Jun 2022 10:52:25 +0300 Subject: [PATCH 19/31] lsp uses load_from_manifest, from_lock_file is private --- forc-pkg/src/pkg.rs | 5 +---- sway-lsp/src/core/document.rs | 7 ++++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 6aaae992663..70e64ba4a27 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -379,10 +379,7 @@ impl BuildPlan { /// from project's manifest file after the lock file is generated. /// /// The returned removed dependencies are already removed from the dependency graph. - pub fn from_lock_file( - lock_path: &Path, - sway_git_tag: &str, - ) -> Result<(Self, Vec)> { + fn from_lock_file(lock_path: &Path, sway_git_tag: &str) -> Result<(Self, Vec)> { let proj_path = lock_path.parent().unwrap(); let lock = Lock::from_path(lock_path)?; Self::from_lock(proj_path, &lock, sway_git_tag) diff --git a/sway-lsp/src/core/document.rs b/sway-lsp/src/core/document.rs index cf5f74ed3b9..9ae9bc8490b 100644 --- a/sway-lsp/src/core/document.rs +++ b/sway-lsp/src/core/document.rs @@ -160,9 +160,10 @@ impl TextDocument { let silent_mode = true; let manifest = pkg::ManifestFile::from_dir(&manifest_dir, forc::utils::SWAY_GIT_TAG).unwrap(); - let lock_path = forc_util::lock_path(manifest.dir()); - let plan = pkg::BuildPlan::from_lock_file(&lock_path, forc::utils::SWAY_GIT_TAG).unwrap(); - let res = pkg::check(&plan.0, silent_mode, forc::utils::SWAY_GIT_TAG).unwrap(); + let plan = + pkg::BuildPlan::load_from_manifest(&manifest, false, false, forc::utils::SWAY_GIT_TAG) + .unwrap(); + let res = pkg::check(&plan, silent_mode, forc::utils::SWAY_GIT_TAG).unwrap(); match res { CompileAstResult::Failure { .. } => None, From 2bc0d94f48623573a1dc0e51cf5760ebac8ef383 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Wed, 29 Jun 2022 11:08:52 +0300 Subject: [PATCH 20/31] add use::collections::HashSet --- forc-pkg/src/pkg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 4770e665bcb..a385e1f41a0 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -17,7 +17,7 @@ use petgraph::{ }; use serde::{Deserialize, Serialize}; use std::{ - collections::{hash_map, BTreeSet, HashMap}, + collections::{hash_map, BTreeSet, HashMap, HashSet}, fmt, fs, hash::{Hash, Hasher}, path::{Path, PathBuf}, From d3a10ae4c634bf4f726e71d1306a0a556e14f039 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Mon, 4 Jul 2022 14:16:09 +1000 Subject: [PATCH 21/31] WIP Improve `BuildPlan` graph reconstruction --- forc-pkg/src/manifest.rs | 31 +- forc-pkg/src/pkg.rs | 975 +++++++++++++++++++-------------------- 2 files changed, 510 insertions(+), 496 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index 9224602aecf..a88fc7c0c8e 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -107,6 +107,7 @@ impl ManifestFile { /// implicitly. In this case, the `sway_git_tag` is used to specify the pinned commit at which /// we fetch `std`. pub fn from_file(path: PathBuf, sway_git_tag: &str) -> Result { + let path = path.canonicalize()?; let manifest = Manifest::from_file(&path, sway_git_tag)?; Ok(Self { manifest, path }) } @@ -144,11 +145,15 @@ impl ManifestFile { } /// The path to the `Forc.toml` from which this manifest was loaded. + /// + /// This will always be a canonical path. pub fn path(&self) -> &Path { &self.path } /// The path to the directory containing the `Forc.toml` from which this manfiest was loaded. + /// + /// This will always be a canonical path. pub fn dir(&self) -> &Path { self.path() .parent() @@ -157,6 +162,8 @@ impl ManifestFile { /// Given the directory in which the file associated with this `Manifest` resides, produce the /// path to the entry file as specified in the manifest. + /// + /// This will always be a canonical path. pub fn entry_path(&self) -> PathBuf { self.dir() .join(constants::SRC_DIR) @@ -200,6 +207,20 @@ impl ManifestFile { .as_ref() .and_then(|profiles| profiles.get(profile_name)) } + + /// Given the name of a dependency, if that dependency is `path` dependency, returns the full + /// canonical `Path` to the dependency. + pub fn dep_path(&self, dep_name: &str) -> Option { + let dir = self.dir(); + let details = self.dep_detailed(dep_name)?; + details.path.as_ref().and_then(|path_str| { + let path = Path::new(path_str); + match path.is_absolute() { + true => Some(path.to_owned()), + false => dir.join(path).canonicalize().ok(), + } + }) + } } impl Manifest { @@ -333,8 +354,16 @@ impl Manifest { .and_then(|deps| deps.get(dep_name)) } + /// Retrieve a reference to the dependency with the given name. + pub fn dep_detailed(&self, dep_name: &str) -> Option<&DependencyDetails> { + self.dep(dep_name).and_then(|dep| match dep { + Dependency::Simple(_) => None, + Dependency::Detailed(detailed) => Some(detailed) + }) + } + /// Retrieve the listed patches for the given name. - pub fn patch(&self, patch_name: &str) -> Option<&BTreeMap> { + pub fn patch(&self, patch_name: &str) -> Option<&PatchMap> { self.patch .as_ref() .and_then(|patches| patches.get(patch_name)) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index a385e1f41a0..41e85e7ef8d 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -6,15 +6,11 @@ use crate::{ use anyhow::{anyhow, bail, Context, Error, Result}; use forc_util::{ find_file_name, git_checkouts_directory, kebab_to_snake_case, print_on_failure, - print_on_success, print_on_success_library, println_red, println_yellow_err, + print_on_success, print_on_success_library, }; use fuel_tx::StorageSlot; use fuels_types::JsonABI; -use petgraph::{ - self, - visit::{EdgeRef, IntoNodeReferences}, - Directed, Direction, -}; +use petgraph::{self, visit::EdgeRef, Directed, Direction}; use serde::{Deserialize, Serialize}; use std::{ collections::{hash_map, BTreeSet, HashMap, HashSet}, @@ -78,7 +74,7 @@ pub struct Pinned { #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] pub enum Source { /// Used to refer to the root project. - Root, + Root(PathBuf), /// A git repo with a `Forc.toml` manifest at its root. Git(SourceGit), /// A path to a directory with a `Forc.toml` manifest at its root. @@ -210,16 +206,12 @@ pub struct SourcePinnedParseError; /// ``` pub type DependencyName = String; -pub struct PkgDiff { - pub added: Vec<(DependencyName, Pkg)>, - pub removed: Vec<(DependencyName, Pkg)>, -} - impl BuildPlan { /// Create a new build plan for the project by fetching and pinning dependenies. pub fn new(manifest: &ManifestFile, sway_git_tag: &str, offline: bool) -> Result { - let path = manifest.dir().to_path_buf(); - let (graph, path_map) = fetch_deps(path, manifest, sway_git_tag, offline)?; + let mut graph = Graph::default(); + let mut path_map = PathMap::default(); + fetch_graph(manifest, offline, sway_git_tag, &mut graph, &mut path_map)?; let compilation_order = compilation_order(&graph)?; Ok(Self { graph, @@ -228,58 +220,73 @@ impl BuildPlan { }) } - /// Create a new build plan taking into account the state of both the Manifest and the existing lock file if there is one. + /// Create a new build plan taking into account the state of both the Manifest and the existing + /// lock file if there is one. /// - /// This will first attempt to load a build plan from the lock file and validate the resulting graph using the current state of the Manifest. + /// This will first attempt to load a build plan from the lock file and validate the resulting + /// graph using the current state of the Manifest. /// - /// This includes checking if the [dependencies] or [patch] tables have changed and checking the validity of the local path dependencies. - /// If any changes are detected, the graph is updated and any new packages that require fetching are fetched. + /// This includes checking if the [dependencies] or [patch] tables have changed and checking + /// the validity of the local path dependencies. If any changes are detected, the graph is + /// updated and any new packages that require fetching are fetched. /// - /// The resulting build plan should always be in a valid state that is ready for building or checking. + /// The resulting build plan should always be in a valid state that is ready for building or + /// checking. pub fn load_from_manifest( manifest: &ManifestFile, locked: bool, offline: bool, sway_git_tag: &str, ) -> Result { - let lock_path = forc_util::lock_path(manifest.dir()); - let from_lock = BuildPlan::from_lock_file(&lock_path, sway_git_tag); - let removed_deps = from_lock - .as_ref() - .map(|from_lock| from_lock.1.clone()) - .unwrap_or_default(); - - let plan_result = from_lock.map(|from_lock| from_lock.0); - // Retrieve the old lock file state so we can produce a diff. - let old_lock = plan_result - .as_ref() - .ok() - .map(|plan| Lock::from_graph(plan.graph())) - .unwrap_or_default(); - - // Check if there are any errors coming from the BuildPlan generation from the lock file - // If there are errors we will need to create the BuildPlan from scratch, i.e fetch & pin everything + // Keep track of the cause for the new lock file if it turns out we need one. let mut new_lock_cause = None; - let mut plan = plan_result.or_else(|e| -> Result { + + // First, attempt to load the lock. + let lock_path = forc_util::lock_path(manifest.dir()); + let lock = Lock::from_path(&lock_path).unwrap_or_else(|e| { new_lock_cause = if e.to_string().contains("No such file or directory") { Some(anyhow!("lock file did not exist")) } else { Some(e) }; - let plan = BuildPlan::new(manifest, sway_git_tag, offline)?; - Ok(plan) - })?; - - // If there are no issues with the BuildPlan generated from the lock file - // Check and apply the diff. - if new_lock_cause.is_none() { - let diff = plan.generate_diff(manifest)?; - if !diff.added.is_empty() || !diff.removed.is_empty() || !removed_deps.is_empty() { - new_lock_cause = Some(anyhow!("lock file did not match manifest `diff`")); - plan = plan.apply_pkg_diff(&diff, sway_git_tag, offline)?; - } + Lock::default() + }); + + // Next, construct the package graph from the lock. + let mut graph = lock.to_graph().unwrap_or_else(|e| { + new_lock_cause = Some(anyhow!("Invalid lock: {}", e)); + Graph::default() + }); + + // Since the lock file was last created there are many ways in which it might have been + // invalidated. E.g. a package's manifest `[dependencies]` table might have changed, a user + // might have edited the `Forc.lock` file when they shouldn't have, a path dependency no + // longer exists at its specified location, etc. We must first remove all invalid nodes + // before we can determine what we need to fetch. + let to_remove = validate_graph(&graph, manifest, sway_git_tag); + remove_deps(&mut graph, &manifest.project.name, &to_remove); + + // We know that the remaining nodes have valid paths, otherwise they would have been + // removed. We can safely produce an initial `path_map`. + let mut path_map = graph_to_path_map(manifest.dir(), &graph, sway_git_tag)?; + + // Attempt to fetch the remainder of the graph. + let added = fetch_graph(manifest, offline, sway_git_tag, &mut graph, &mut path_map)?; + + // If we require any changes to the graph, update the new lock cause. + if !to_remove.is_empty() || !added.is_empty() { + new_lock_cause = Some(anyhow!("lock file did not match manifest")); } + // Determine the compilation order. + let compilation_order = compilation_order(&graph)?; + + let plan = Self { + graph, + path_map, + compilation_order, + }; + if let Some(cause) = new_lock_cause { if locked { bail!( @@ -291,268 +298,228 @@ impl BuildPlan { } info!(" Creating a new `Forc.lock` file. (Cause: {})", cause); - create_new_lock(&plan, &old_lock, manifest, &lock_path)?; + let new_lock = Lock::from_graph(plan.graph()); + let lock_diff = new_lock.diff(&lock); + crate::lock::print_diff(&manifest.project.name, &lock_diff); + let string = toml::ser::to_string_pretty(&new_lock) + .map_err(|e| anyhow!("failed to serialize lock file: {}", e))?; + fs::write(&lock_path, &string) + .map_err(|e| anyhow!("failed to write lock file: {}", e))?; info!(" Created new lock file at {}", lock_path.display()); } Ok(plan) } - /// Create a new build plan from an existing one. Needs the difference with the existing plan with the lock. - fn apply_pkg_diff( - &self, - pkg_diff: &PkgDiff, - sway_git_tag: &str, - offline_mode: bool, - ) -> Result { - self.validate(sway_git_tag)?; - let mut graph = self.graph.clone(); - let mut path_map = self.path_map.clone(); - - let proj_node = *self - .compilation_order - .last() - .ok_or_else(|| anyhow!("Invalid Graph"))?; - let PkgDiff { added, removed } = pkg_diff; - remove_deps(&mut graph, &path_map, proj_node, removed); - - let mut visited_map: HashMap = graph - .node_references() - .into_iter() - .map(|(node_index, pinned)| (pinned.clone(), node_index)) - .collect(); - - add_deps( - &mut graph, - &mut path_map, - &self.compilation_order, - added, - sway_git_tag, - offline_mode, - &mut visited_map, - )?; - let compilation_order = compilation_order(&graph)?; - self.validate(sway_git_tag)?; - Ok(Self { - graph, - path_map, - compilation_order, - }) + /// View the build plan's compilation graph. + pub fn graph(&self) -> &Graph { + &self.graph } - /// Attempt to load the build plan from the `Lock`. - /// Returns the best effort BuildPlan and the packages removed - /// from project's manifest file after the lock file is generated. - /// - /// The returned removed dependencies are already removed from the dependency graph. - fn from_lock( - proj_path: &Path, - lock: &Lock, - sway_git_tag: &str, - ) -> Result<(Self, Vec)> { - let mut graph = lock.to_graph()?; - let tmp_compilation_order = compilation_order(&graph)?; - let (path_map, removed_deps) = - graph_to_path_map(proj_path, &graph, &tmp_compilation_order, sway_git_tag)?; - let removed_deps_vec: Vec = removed_deps - .clone() - .into_iter() - .map(|dep| graph[dep].name.clone()) - .collect(); - - // Remove the unresolveable path dependencies from the graph - apply_diff_after_lock(removed_deps, &mut graph)?; - - // Since apply_diff_after_lock removes removed dependencies from the graph compilation order may change - let compilation_order = compilation_order(&graph)?; - Ok(( - Self { - graph, - path_map, - compilation_order, - }, - removed_deps_vec, - )) + /// View the build plan's map of pinned package IDs to the path containing a local copy of + /// their source. + pub fn path_map(&self) -> &PathMap { + &self.path_map } - /// Attempt to load the build plan from the `Forc.lock` file. - /// Returns the best effort BuildPlan and the packages removed - /// from project's manifest file after the lock file is generated. - /// - /// The returned removed dependencies are already removed from the dependency graph. - fn from_lock_file(lock_path: &Path, sway_git_tag: &str) -> Result<(Self, Vec)> { - let proj_path = lock_path.parent().unwrap(); - let lock = Lock::from_path(lock_path)?; - Self::from_lock(proj_path, &lock, sway_git_tag) + /// The order in which nodes are compiled, determined via a toposort of the package graph. + pub fn compilation_order(&self) -> &[NodeIx] { + &self.compilation_order } +} - /// Attempt to find the difference between the graph constructed from the lock file - /// and the graph that is described by the manifest file. The difference is used to - /// update BuildPlan. - pub fn generate_diff(&self, manifest: &Manifest) -> Result { - let mut added = vec![]; - let mut removed = vec![]; - // Retrieve project's graph node. - let proj_node = *self - .compilation_order - .last() - .ok_or_else(|| anyhow!("Invalid Graph"))?; - - // Collect dependency `Source`s from graph. - let plan_dep_pkgs: BTreeSet<_> = self - .graph - .edges_directed(proj_node, Direction::Outgoing) - .map(|e| { - let dep_name = e.weight(); - let dep_pkg = self.graph[e.target()].unpinned(&self.path_map); - (dep_name, dep_pkg) - }) - .collect(); - - // Collect dependency `Source`s from manifest. - let proj_id = self.graph[proj_node].id(); - let proj_path = &self.path_map[&proj_id]; - let manifest_dep_pkgs = manifest - .deps() - .map(|(dep_name, dep)| { - // NOTE: Temporarily warn about `version` until we have support for registries. - if let Dependency::Detailed(det) = dep { - if det.version.is_some() { - println_yellow_err(&format!( - " WARNING! Dependency \"{}\" specifies the unused `version` field: \ - consider using `branch` or `tag` instead", - dep_name - )); - } - } - - let name = dep.package().unwrap_or(dep_name).to_string(); - let source = - apply_patch(&name, &dep_to_source(proj_path, dep)?, manifest, proj_path)?; - let dep_pkg = Pkg { name, source }; - Ok((dep_name, dep_pkg)) - }) - .collect::>>()?; - - // Ensure both `pkg::Source` are equal. If not, produce added and removed. - if plan_dep_pkgs != manifest_dep_pkgs { - added = manifest_dep_pkgs - .difference(&plan_dep_pkgs) - .into_iter() - .map(|pkg| (pkg.0.clone(), pkg.1.clone())) - .collect(); - removed = plan_dep_pkgs - .difference(&manifest_dep_pkgs) - .into_iter() - .map(|pkg| (pkg.0.clone(), pkg.1.clone())) - .collect(); - } +/// Given a graph and the known project name retrieved from the manifest, produce an iterator +/// yielding any nodes from the graph that might potentially be the project node. There should only +/// ever be one, and this is used during initial graph validation to ensure that's the case. +fn potential_proj_nodes<'a>(g: &'a Graph, proj_name: &'a str) -> impl 'a + Iterator { + g.node_indices() + .filter(|&n| g.edges_directed(n, Direction::Incoming).next().is_none()) + .filter(move |&n| g[n].name == proj_name) +} - Ok(PkgDiff { added, removed }) +/// Given a graph, find the project node. +/// +/// This should be the only node that satisfies the following conditions: +/// +/// - The package name matches `proj_name` +/// - The node has no incoming edges, i.e. is not a dependency of another node. +fn find_proj_node(graph: &Graph, proj_name: &str) -> Result { + let mut potentials = potential_proj_nodes(graph, proj_name); + let proj_node = potentials + .next() + .ok_or_else(|| anyhow!("graph contains no project node"))?; + match potentials.next() { + None => Ok(proj_node), + Some(_) => Err(anyhow!("graph contains more than one project node")), } +} - /// Ensure that the build plan is valid for the given manifest. - fn validate(&self, sway_git_tag: &str) -> Result<()> { - // Ensure the pkg names of all nodes match their associated manifests. - for node in self.graph.node_indices() { - let pkg = &self.graph[node]; - let id = pkg.id(); - let path = &self.path_map[&id]; - let manifest = ManifestFile::from_dir(path, sway_git_tag)?; - if pkg.name != manifest.project.name { - bail!( - "package name {:?} does not match the associated manifest project name {:?}", - pkg.name, - manifest.project.name, - ); +/// Validates the state of the graph against the given project manifest. +/// +/// Returns the set of invalid nodes detected within the graph that should be removed. +/// +/// Note that nodes whose parents are all invalid are *not* returned, as it is assumed they will be +/// removed along with their invalid parents in `remove_deps` in a BFS from the project node. +fn validate_graph( + graph: &Graph, + proj_manifest: &ManifestFile, + sway_git_tag: &str, +) -> BTreeSet { + // First, check the project node exists. + // If we we don't have exactly one potential project node, remove everything as we can't + // validate the graph without knowing the project node. + let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { + Ok(node) => node, + Err(_) => return graph.node_indices().collect(), + }; + // Collect all invalid dependency nodes. + validate_deps(graph, proj_node, proj_manifest, sway_git_tag) +} + +/// Validate all dependency nodes in the `graph`, traversing from `node`. +/// +/// Returns the set of invalid nodes. +fn validate_deps( + graph: &Graph, + node: NodeIx, + node_manifest: &ManifestFile, + sway_git_tag: &str, +) -> BTreeSet { + let mut remove = BTreeSet::default(); + for edge in graph.edges_directed(node, Direction::Outgoing) { + let dep_name = edge.weight(); + let dep_node = edge.target(); + match validate_dep(graph, node_manifest, dep_name, dep_node, sway_git_tag) { + Err(_) => { + remove.insert(dep_node); + } + Ok(dep_manifest) => { + remove.extend(validate_deps(graph, dep_node, &dep_manifest, sway_git_tag)); + continue; } } - Ok(()) } + remove +} - /// View the build plan's compilation graph. - pub fn graph(&self) -> &Graph { - &self.graph +/// Check the validity of a node's dependency within the graph. +/// +/// Returns the `ManifestFile` in the case that the dependency is valid. +fn validate_dep( + graph: &Graph, + node_manifest: &ManifestFile, + dep_name: &str, + dep_node: NodeIx, + sway_git_tag: &str, +) -> Result { + // Check the validity of the dependency path, including its path root. + let dep_path = dep_path(graph, node_manifest, dep_name, dep_node, sway_git_tag) + .ok_or_else(|| anyhow!("failed to construct path for dependency {:?}", dep_name))?; + + // Ensure the manifest is accessible. + let dep_manifest = ManifestFile::from_dir(&dep_path, sway_git_tag)?; + + // Ensure the name matches the manifest project name. + if graph[dep_node].name != dep_manifest.project.name { + bail!( + "dependency name {:?} must match the manifest project name {:?} \ + unless `package = {:?}` is specified in the dependency declaration", + graph[dep_node].name, + dep_manifest.project.name, + dep_manifest.project.name, + ); } - /// View the build plan's map of pinned package IDs to the path containing a local copy of - /// their source. - pub fn path_map(&self) -> &PathMap { - &self.path_map + // Check that the dependency's source matches the entry in the parent manifest. + let dep_entry = node_manifest + .dep(dep_name) + .ok_or_else(|| anyhow!("no entry in parent manifest"))?; + let dep_source = dep_to_source_patched(node_manifest, dep_name, dep_entry)?; + let dep_pkg = graph[dep_node].unpinned(&dep_path); + if dep_pkg.source != dep_source { + bail!("dependency node's source does not match manifest entry"); } - /// The order in which nodes are compiled, determined via a toposort of the package graph. - pub fn compilation_order(&self) -> &[NodeIx] { - &self.compilation_order + Ok(dep_manifest) +} + +/// Returns the local path to the given dependency node if it exists, `None` otherwise. +/// +/// Also returns `None` in the case that the dependency is a `Path` dependency and the path root is +/// invalid. +fn dep_path( + graph: &Graph, + node_manifest: &ManifestFile, + dep_name: &str, + dep_node: NodeIx, + sway_git_tag: &str, +) -> Option { + let dep = &graph[dep_node]; + match &dep.source { + SourcePinned::Git(git) => { + let repo_path = git_commit_path(&dep.name, &git.source.repo, &git.commit_hash); + find_dir_within(&repo_path, &dep.name, sway_git_tag) + } + SourcePinned::Path(src) => { + if validate_path_root(graph, dep_node, src.path_root).is_err() { + return None; + } + + // Check if the path is directly from the dependency. + if let Some(path) = node_manifest.dep_path(dep_name) { + if path.exists() { + return Some(path); + } + } + + // Otherwise, check if it comes from a patch. + for (_, patch_map) in node_manifest.patches() { + if let Some(Dependency::Detailed(details)) = patch_map.get(dep_name) { + if let Some(ref rel_path) = details.path { + let path = node_manifest.dir().join(rel_path); + return path.canonicalize().ok().filter(|p| p.exists()); + } + } + } + + None + } + SourcePinned::Registry(_reg) => unreachable!("registry dependencies not yet supported"), + SourcePinned::Root => unreachable!("a `Root` node cannot be a dependency"), } } /// Remove the given set of packages from `graph` along with any dependencies that are no /// longer required as a result. -fn remove_deps( - graph: &mut Graph, - path_map: &PathMap, - proj_node: NodeIx, - to_remove: &[(DependencyName, Pkg)], -) { +fn remove_deps(graph: &mut Graph, proj_name: &str, to_remove: &BTreeSet) { use petgraph::visit::Bfs; - // Do a BFS from the root and remove all nodes that does not have any incoming edge or one of the removed dependencies. + // Retrieve the project node. + let proj_node = match find_proj_node(graph, proj_name) { + Ok(node) => node, + Err(_) => { + // If it fails, invalidate everything. + graph.clear(); + return; + } + }; + + // Remove all nodes that either have no parents, or are contained within the `to_remove` set. + // We do so in breadth-first-order to ensure we always remove all parents before children. let mut bfs = Bfs::new(&*graph, proj_node); - bfs.next(&*graph); // Skip the root node (aka project node). + // Skip the root node (aka project node). + bfs.next(&*graph); while let Some(node) = bfs.next(&*graph) { - if graph + let no_parents = graph .edges_directed(node, Direction::Incoming) .next() - .is_none() - || to_remove - .iter() - .any(|removed_dep| removed_dep.1 == graph[node].unpinned(path_map)) - { + .is_none(); + if no_parents || to_remove.contains(&node) { graph.remove_node(node); } } } -/// Add the given set of packages to `graph`. If a dependency of an newly added package is already -/// pinned use that. Otherwise fetch and pin it. -fn add_deps( - graph: &mut Graph, - path_map: &mut PathMap, - compilation_order: &[NodeIx], - to_add: &[(DependencyName, Pkg)], - sway_git_tag: &str, - offline_mode: bool, - visited_map: &mut HashMap, -) -> Result<()> { - let proj_node = *compilation_order - .last() - .ok_or_else(|| anyhow!("Invalid Graph"))?; - let proj_id = graph[proj_node].id(); - let proj_path = &path_map[&proj_id]; - let fetch_ts = std::time::Instant::now(); - let fetch_id = fetch_id(proj_path, fetch_ts); - let path_root = proj_id; - for (added_dep_name, added_package) in to_add { - let pinned_pkg = pin_pkg(fetch_id, proj_id, added_package, path_map, sway_git_tag)?; - let manifest = Manifest::from_dir(&path_map[&pinned_pkg.id()], sway_git_tag)?; - let added_package_node = graph.add_node(pinned_pkg.clone()); - fetch_children( - fetch_id, - offline_mode, - added_package_node, - &manifest, - path_root, - sway_git_tag, - graph, - path_map, - visited_map, - )?; - graph.add_edge(proj_node, added_package_node, added_dep_name.to_string()); - } - Ok(()) -} - impl GitReference { /// Resolves the parsed forc git reference to the associated git ID. pub fn resolve(&self, repo: &git2::Repository) -> Result { @@ -612,12 +579,11 @@ impl Pinned { } /// Retrieve the unpinned version of this source. - pub fn unpinned(&self, path_map: &PathMap) -> Pkg { - let id = self.id(); + pub fn unpinned(&self, path: &Path) -> Pkg { let source = match &self.source { - SourcePinned::Root => Source::Root, + SourcePinned::Root => Source::Root(path.to_owned()), SourcePinned::Git(git) => Source::Git(git.source.clone()), - SourcePinned::Path(_) => Source::Path(path_map[&id].clone()), + SourcePinned::Path(_) => Source::Path(path.to_owned()), SourcePinned::Registry(reg) => Source::Registry(reg.source.clone()), }; let name = self.name.clone(); @@ -851,57 +817,27 @@ pub fn compilation_order(graph: &Graph) -> Result> { }) } -/// Applies the differences between the graph that is described by the lock file and the graph that -/// is actually possible to build. -/// -/// `deps_to_remove` : Indices of path dependencies that are removed from their parent's manifest file after the lock file is generated. -/// -/// There might be some differences between the graph that is described by the lock file and -/// the graph that can actually possible to build. This is happening in the case of a dependency -/// (described by path) removal after the lock file is generated. Since dependency is removed -/// graph_to_path_map cannot find the required path for the dependency entry in the lock file. -/// This requires us to remove those removed nodes from the graph so that we can have a BuildPlan -/// generated from lock file. After the BuildPlan is generated it can be used to apply diffs by -/// apply_pkg_diff(). -fn apply_diff_after_lock(deps_to_remove: HashSet, graph: &mut Graph) -> Result<()> { - for dep in deps_to_remove { - let dep_name = &graph[dep].name.clone(); - println_red(&format!(" Removing {}", dep_name)); - // Remove the deleted node - graph.remove_node(dep); - } - Ok(()) -} -/// Search dependencies and patches for the details for a pinned dependency -fn check_manifest_for_pinned(manifest: &ManifestFile, pinned_dep: &Pinned) -> bool { - manifest.dep(&pinned_dep.name).is_none() - && !manifest - .deps() - .filter_map(|dep| dep.1.package()) - .any(|package| package == pinned_dep.name) - && manifest.patch(&pinned_dep.name).is_none() - && manifest - .patches() - .find_map(|patches| patches.1.get(&pinned_dep.name)) - .is_none() -} - -/// Given graph of pinned dependencies and the directory for the root node, produce a path map +/// Given a graph of pinned packages and the directory for the root node, produce a path map /// containing the path to the local source for every node in the graph. /// -/// Returns the generated path map and the nodes that should be removed from the graph (the nodes -/// that are removed from their parent's manifest file). -pub fn graph_to_path_map( +/// NOTE: This function assumes that the paths are valid for each node in the graph. It is designed +/// to be called after calls to `validate_graph_with_manifest` and `remove_deps`, following which +/// there should be no more nodes with invalid paths remaining in the graph. +fn graph_to_path_map( proj_manifest_dir: &Path, graph: &Graph, - compilation_order: &[NodeIx], sway_git_tag: &str, -) -> Result<(PathMap, HashSet)> { +) -> Result { let mut path_map = PathMap::new(); - let mut removed_deps = HashSet::new(); + + // If the graph is empty, the path map will be too. + if graph.node_count() == 0 { + return Ok(path_map); + } // We resolve all paths in reverse compilation order. // That is, we follow paths starting from the project root. + let compilation_order = compilation_order(graph)?; let mut path_resolve_order = compilation_order.iter().cloned().rev(); // Add the project's package to the map. let proj_node = path_resolve_order @@ -949,13 +885,6 @@ pub fn graph_to_path_map( // Construct the path relative to the parent's path. let parent_path = &path_map[&parent.id()]; let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; - // Check if parent's manifest file includes this dependency. - // If not this is a removed dependency and we should continue with the following dep - // Note that we need to check from package name as well since dependency name and package name is not the same thing - if check_manifest_for_pinned(&parent_manifest, dep) { - removed_deps.insert(dep_node); - continue; - } let detailed = parent_manifest .dependencies .as_ref() @@ -1003,7 +932,7 @@ pub fn graph_to_path_map( path_map.insert(dep.id(), dep_path.canonicalize()?); } - Ok((path_map, removed_deps)) + Ok(path_map) } /// Given a `graph`, the node index of a path dependency within that `graph`, and the supposed @@ -1015,115 +944,45 @@ pub(crate) fn validate_path_root( path_dep: NodeIx, path_root: PinnedId, ) -> Result<()> { - let mut node = path_dep; - let invalid_path_root = || { - anyhow!( + let path_root_node = find_path_root(graph, path_dep)?; + if graph[path_root_node].id() != path_root { + bail!( "invalid `path_root` for path dependency package {:?}", &graph[path_dep].name ) - }; - loop { - let parent = graph - .edges_directed(node, Direction::Incoming) - .next() - .map(|edge| edge.source()) - .ok_or_else(invalid_path_root)?; - let parent_pkg = &graph[parent]; - match &parent_pkg.source { - SourcePinned::Path(src) if src.path_root != path_root => bail!(invalid_path_root()), - SourcePinned::Git(_) | SourcePinned::Registry(_) | SourcePinned::Root => { - if parent_pkg.id() != path_root { - bail!(invalid_path_root()); - } - return Ok(()); - } - _ => node = parent, - } } + Ok(()) } -/// Fetch all depedencies and produce the dependency graph along with a map from each node's unique -/// ID to its local fetched path. -/// -/// This will determine pinned versions and commits for remote dependencies during traversal. -pub(crate) fn fetch_deps( - proj_manifest_dir: PathBuf, - proj_manifest: &Manifest, - sway_git_tag: &str, - offline_mode: bool, -) -> Result<(Graph, PathMap)> { - let mut graph = Graph::new(); - let mut path_map = PathMap::new(); - - // Add the project to the graph as the root node. - let name = proj_manifest.project.name.clone(); - let path = proj_manifest_dir.canonicalize()?; - let source = SourcePinned::Root; - let pkg = Pinned { name, source }; - let pkg_id = pkg.id(); - path_map.insert(pkg_id, path); - let root = graph.add_node(pkg); - - // The set of visited packages, starting with the root. - let mut visited = HashMap::new(); - visited.insert(graph[root].clone(), root); - - // Recursively fetch children and add them to the graph. - // TODO: Convert this recursion to use loop & stack to ensure deps can't cause stack overflow. - let fetch_ts = std::time::Instant::now(); - let fetch_id = fetch_id(&path_map[&pkg_id], fetch_ts); - let manifest = Manifest::from_dir(&path_map[&pkg_id], sway_git_tag)?; - let path_root = pkg_id; - fetch_children( - fetch_id, - offline_mode, - root, - &manifest, - path_root, - sway_git_tag, - &mut graph, - &mut path_map, - &mut visited, - )?; - - Ok((graph, path_map)) -} - -fn apply_patch( - name: &str, - source: &Source, - manifest: &Manifest, - parent_path: &Path, -) -> Result { - match source { - // Check if the patch is for a git dependency. - Source::Git(git) => { - // Check if we got a patch for the git dependency. - if let Some(source_patches) = manifest - .patch - .as_ref() - .and_then(|patches| patches.get(git.repo.as_str())) - { - if let Some(patch) = source_patches.get(name) { - Ok(dep_to_source(parent_path, patch)?) - } else { - bail!( - "Cannot find the patch for the {} for package {}", - git.repo, - name - ) - } - } else { - Ok(source.clone()) +/// Given any node in the graph, find the node that is the path root for that node. +fn find_path_root(graph: &Graph, mut node: NodeIx) -> Result { + loop { + let pkg = &graph[node]; + match &pkg.source { + SourcePinned::Path(src) => { + let parent = graph + .edges_directed(node, Direction::Incoming) + .next() + .map(|edge| edge.source()) + .ok_or_else(|| { + anyhow!( + "Failed to find path root: `path` dependency \"{}\" has no parent", + src + ) + })?; + node = parent; + } + SourcePinned::Git(_) | SourcePinned::Registry(_) | SourcePinned::Root => { + return Ok(node); } } - _ => Ok(source.clone()), } } /// Produce a unique ID for a particular fetch pass. /// -/// This is used in the temporary git directory and allows for avoiding contention over the git repo directory. +/// This is used in the temporary git directory and allows for avoiding contention over the git +/// repo directory. pub fn fetch_id(path: &Path, timestamp: std::time::Instant) -> u64 { let mut hasher = hash_map::DefaultHasher::new(); path.hash(&mut hasher); @@ -1131,71 +990,149 @@ pub fn fetch_id(path: &Path, timestamp: std::time::Instant) -> u64 { hasher.finish() } -/// Fetch children nodes of the given node and add unvisited nodes to the graph. +/// Given an empty or partially completed package `graph`, complete the graph. +/// +/// The given `graph` may be empty, partially complete, or fully complete. All existing nodes +/// should already be confirmed to be valid nodes via `validate_graph`. All invalid nodes should +/// have been removed prior to calling this. +/// +/// Recursively traverses dependencies listed within each package's manifest, fetching and pinning +/// each dependency if it does not already exist within the package graph. +/// +/// The accompanying `path_map` should contain a path entry for every existing node within the +/// `graph` and will `panic!` otherwise. +/// +/// Upon success, returns the set of nodes that were added to the graph during traversal. +fn fetch_graph( + proj_manifest: &ManifestFile, + offline: bool, + sway_git_tag: &str, + graph: &mut Graph, + path_map: &mut PathMap, +) -> Result> { + // Retrieve the project node, or create one if it does not exist. + let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { + Ok(proj_node) => proj_node, + Err(_) => { + let name = proj_manifest.project.name.clone(); + let source = SourcePinned::Root; + let path = proj_manifest.dir().to_owned(); + let pkg = Pinned { name, source }; + let pkg_id = pkg.id(); + path_map.insert(pkg_id, path); + graph.add_node(pkg) + } + }; + + // Traverse the rest of the graph from the root. + let proj_dir = proj_manifest.dir(); + let fetch_ts = std::time::Instant::now(); + let fetch_id = fetch_id(proj_dir, fetch_ts); + let path_root = graph[proj_node].id(); + let mut fetched = graph + .node_indices() + .map(|n| { + let pinned = &graph[n]; + let path = &path_map[&pinned.id()]; + let pkg = pinned.unpinned(path); + (pkg, n) + }) + .collect(); + let mut visited = HashSet::default(); + fetch_deps( + fetch_id, + offline, + proj_node, + proj_manifest, + path_root, + sway_git_tag, + graph, + path_map, + &mut fetched, + &mut visited, + ) +} + +/// Visit the unvisited dependencies of the given node and fetch missing nodes as necessary. #[allow(clippy::too_many_arguments)] -fn fetch_children( +fn fetch_deps( fetch_id: u64, - offline_mode: bool, + offline: bool, node: NodeIx, - manifest: &Manifest, + manifest: &ManifestFile, path_root: PinnedId, sway_git_tag: &str, graph: &mut Graph, path_map: &mut PathMap, - visited: &mut HashMap, -) -> Result<()> { - let parent = &graph[node]; - let parent_id = parent.id(); - let parent_path = path_map[&parent_id].clone(); + fetched: &mut HashMap, + visited: &mut HashSet, +) -> Result> { + let mut added = HashSet::default(); for (dep_name, dep) in manifest.deps() { let name = dep.package().unwrap_or(dep_name).to_string(); - let source = apply_patch( - &name, - &dep_to_source(&parent_path, dep)?, - manifest, - &parent_path, - )?; - if offline_mode && !matches!(source, Source::Path(_)) { - bail!("Unable to fetch pkg {:?} in offline mode", source); - } - let pkg = Pkg { name, source }; - let pinned = pin_pkg(fetch_id, path_root, &pkg, path_map, sway_git_tag)?; - let pkg_id = pinned.id(); - let path_root = match pkg.source { - Source::Root | Source::Git(_) | Source::Registry(_) => pkg_id, - Source::Path(_) => path_root, + let source = dep_to_source_patched(manifest, &name, dep)?; + + // If we haven't yet fetched this dependency, fetch it, pin it and add it to the graph. + let dep_pkg = Pkg { name, source }; + let dep_node = match fetched.entry(dep_pkg) { + hash_map::Entry::Occupied(entry) => *entry.get(), + hash_map::Entry::Vacant(entry) => { + let dep_pinned = pin_pkg( + fetch_id, + path_root, + entry.key(), + path_map, + offline, + sway_git_tag, + )?; + let dep_node = graph.add_node(dep_pinned); + added.insert(dep_node); + *entry.insert(dep_node) + } }; - let manifest = Manifest::from_dir(&path_map[&pkg_id], sway_git_tag)?; - if pinned.name != manifest.project.name { - bail!( - "dependency name {:?} must match the manifest project name {:?} \ - unless `package = {:?}` is specified in the dependency declaration", - pinned.name, - manifest.project.name, - manifest.project.name, - ); + + // Add the edge to the dependency. + graph.add_edge(node, dep_node, dep_name.to_string()); + + // If we've visited this node during this traversal already, no need to traverse it again. + if !visited.insert(dep_node) { + continue; } - let dep_node = if let hash_map::Entry::Vacant(entry) = visited.entry(pinned.clone()) { - let node = graph.add_node(pinned); - entry.insert(node); - fetch_children( - fetch_id, - offline_mode, - node, - &manifest, - path_root, - sway_git_tag, - graph, - path_map, - visited, - )?; - node - } else { - visited[&pinned] + + // This shouldn't fail unless the dependency's package name differs from its manifest. + let dep_manifest = validate_dep(graph, manifest, dep_name, dep_node, sway_git_tag) + .map_err(|e| { + let parent = &graph[node]; + anyhow!( + "dependency of {:?} named {:?} is invalid: {}", + parent.name, + dep_name, + e + ) + })?; + + let dep_pinned = &graph[dep_node]; + let dep_pkg_id = dep_pinned.id(); + let path_root = match dep_pinned.source { + SourcePinned::Root | SourcePinned::Git(_) | SourcePinned::Registry(_) => dep_pkg_id, + SourcePinned::Path(_) => path_root, }; - graph.add_edge(node, dep_node, dep_name.to_string()); + + // Fetch the children. + added.extend(fetch_deps( + fetch_id, + offline, + dep_node, + &dep_manifest, + path_root, + sway_git_tag, + graph, + path_map, + fetched, + visited, + )?); } - Ok(()) + Ok(added) } /// The name to use for a package's git repository under the user's forc directory. @@ -1324,11 +1261,18 @@ fn pin_pkg( path_root: PinnedId, pkg: &Pkg, path_map: &mut PathMap, + offline: bool, sway_git_tag: &str, ) -> Result { let name = pkg.name.clone(); let pinned = match &pkg.source { - Source::Root => unreachable!("Root package is \"pinned\" prior to fetching"), + Source::Root(path) => { + let source = SourcePinned::Root; + let pinned = Pinned { name, source }; + let id = pinned.id(); + path_map.insert(id, path.clone()); + pinned + } Source::Path(path) => { let path_pinned = SourcePathPinned { path_root }; let source = SourcePinned::Path(path_pinned); @@ -1338,6 +1282,16 @@ fn pin_pkg( pinned } Source::Git(ref git_source) => { + // TODO: If the git source directly specifies a full commit hash, we should first check + // to see if we have a local copy. Otherwise we cannot know what commit we should pin + // to without fetching the repo into a temporary directory. + if offline { + bail!( + "Unable to fetch pkg {:?} from {:?} in offline mode", + name, + git_source.repo + ); + } let pinned_git = pin_git(fetch_id, &name, git_source.clone())?; let repo_path = git_commit_path(&name, &pinned_git.source.repo, &pinned_git.commit_hash); @@ -1345,11 +1299,12 @@ fn pin_pkg( let pinned = Pinned { name, source }; let id = pinned.id(); if let hash_map::Entry::Vacant(entry) = path_map.entry(id) { - // TODO: Here we assume that if the local path already exists, that it contains the full and - // correct source for that commit and hasn't been tampered with. This is probably fine for most - // cases as users should never be touching these directories, however we should add some code - // to validate this. E.g. can we recreate the git hash by hashing the directory or something - // along these lines using git? + // TODO: Here we assume that if the local path already exists, that it contains the + // full and correct source for that commit and hasn't been tampered with. This is + // probably fine for most cases as users should never be touching these + // directories, however we should add some code to validate this. E.g. can we + // recreate the git hash by hashing the directory or something along these lines + // using git? if !repo_path.exists() { info!(" Fetching {}", pinned_git.to_string()); fetch_git(fetch_id, &pinned.name, &pinned_git)?; @@ -1367,6 +1322,9 @@ fn pin_pkg( pinned } Source::Registry(ref _source) => { + if offline { + bail!("Unable to fetch pkg {:?} in offline mode", name); + } // TODO: determine registry pkg git URL, fetch to determine latest available // semver-compatible version bail!("registry dependencies are not yet supported"); @@ -1460,6 +1418,48 @@ fn dep_to_source(pkg_path: &Path, dep: &Dependency) -> Result { Ok(source) } +/// Converts the `Dependency` to a `Source` with any relevant patches applied. +fn dep_to_source_patched( + parent_manifest: &ManifestFile, + dep_name: &str, + dep: &Dependency, +) -> Result { + let unpatched = dep_to_source(parent_manifest.dir(), dep)?; + apply_patch(dep_name, &unpatched, parent_manifest, parent_manifest.dir()) +} + +fn apply_patch( + dep_name: &str, + dep_source: &Source, + parent_manifest: &Manifest, + parent_path: &Path, +) -> Result { + match dep_source { + // Check if the patch is for a git dependency. + Source::Git(git) => { + // Check if we got a patch for the git dependency. + if let Some(source_patches) = parent_manifest + .patch + .as_ref() + .and_then(|patches| patches.get(git.repo.as_str())) + { + if let Some(patch) = source_patches.get(dep_name) { + Ok(dep_to_source(parent_path, patch)?) + } else { + bail!( + "Cannot find the patch for the {} for package {}", + git.repo, + dep_name + ) + } + } else { + Ok(dep_source.clone()) + } + } + _ => Ok(dep_source.clone()), + } +} + /// Given a `forc_pkg::BuildProfile`, produce the necessary `sway_core::BuildConfig` required for /// compilation. pub fn sway_build_config( @@ -1884,18 +1884,3 @@ pub fn fuel_core_not_running(node_url: &str) -> anyhow::Error { let message = format!("could not get a response from node at the URL {}. Start a node with `fuel-core`. See https://github.com/FuelLabs/fuel-core#running for more information", node_url); Error::msg(message) } - -fn create_new_lock( - plan: &BuildPlan, - old_lock: &Lock, - manifest: &ManifestFile, - lock_path: &Path, -) -> Result<()> { - let lock = Lock::from_graph(plan.graph()); - let diff = lock.diff(old_lock); - super::lock::print_diff(&manifest.project.name, &diff); - let string = toml::ser::to_string_pretty(&lock) - .map_err(|e| anyhow!("failed to serialize lock file: {}", e))?; - fs::write(&lock_path, &string).map_err(|e| anyhow!("failed to write lock file: {}", e))?; - Ok(()) -} From fd91d28244cd6add54f6b5d74ce95e0cee9f6d93 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 11:12:19 +1000 Subject: [PATCH 22/31] Minimise I/O by replacing BuildProfile's PathMap with a ManifestMap This commit reduces the number of times that we need to read each dependency's manifest from file in order to reduce the amount of I/O performed. Each dependency's manifest should now only be loaded once during either `graph_to_manifest_map` or `fetch_graph`. This required implementing `Clone` for `ManifestFile` to allow for constructing the build plan in a manner that does not require consuming the manifest file. `ManifestFile::clone` should only ever be called for the root node. As a side-effect, the `build` and `check` commands no longer require loading each package's manifest as they are already provided by the build plan. As a result both commands no longer require the sway git tag to be passed through. --- forc-pkg/src/manifest.rs | 16 +- forc-pkg/src/pkg.rs | 362 +++++++++++++++------------------- forc/src/ops/forc_build.rs | 2 +- forc/src/ops/forc_check.rs | 2 +- sway-lsp/src/core/document.rs | 2 +- 5 files changed, 173 insertions(+), 211 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index a88fc7c0c8e..a2facf16f83 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -14,7 +14,7 @@ use sway_utils::constants; type PatchMap = BTreeMap; /// A [Manifest] that was deserialized from a file at a particular path. -#[derive(Debug)] +#[derive(Clone, Debug)] pub struct ManifestFile { /// The deserialized `Forc.toml`. manifest: Manifest, @@ -23,7 +23,7 @@ pub struct ManifestFile { } /// A direct mapping to a `Forc.toml`. -#[derive(Serialize, Deserialize, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] #[serde(rename_all = "kebab-case")] pub struct Manifest { pub project: Project, @@ -33,7 +33,7 @@ pub struct Manifest { build_profile: Option>, } -#[derive(Serialize, Deserialize, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] #[serde(rename_all = "kebab-case")] pub struct Project { pub authors: Option>, @@ -45,14 +45,14 @@ pub struct Project { pub implicit_std: Option, } -#[derive(Serialize, Deserialize, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] #[serde(rename_all = "kebab-case")] pub struct Network { #[serde(default = "default_url")] pub url: String, } -#[derive(Serialize, Deserialize, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] #[serde(untagged)] pub enum Dependency { /// In the simple format, only a version is specified, eg. @@ -64,7 +64,7 @@ pub enum Dependency { Detailed(DependencyDetails), } -#[derive(Serialize, Deserialize, Debug, Default)] +#[derive(Serialize, Deserialize, Clone, Debug, Default)] #[serde(rename_all = "kebab-case")] pub struct DependencyDetails { pub(crate) version: Option, @@ -77,7 +77,7 @@ pub struct DependencyDetails { } /// Parameters to pass through to the `sway_core::BuildConfig` during compilation. -#[derive(Serialize, Deserialize, Debug, Clone)] +#[derive(Serialize, Deserialize, Clone, Debug)] #[serde(rename_all = "kebab-case")] pub struct BuildProfile { pub print_ir: bool, @@ -358,7 +358,7 @@ impl Manifest { pub fn dep_detailed(&self, dep_name: &str) -> Option<&DependencyDetails> { self.dep(dep_name).and_then(|dep| match dep { Dependency::Simple(_) => None, - Dependency::Detailed(detailed) => Some(detailed) + Dependency::Detailed(detailed) => Some(detailed), }) } diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 41e85e7ef8d..5710ec10dd5 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -10,7 +10,11 @@ use forc_util::{ }; use fuel_tx::StorageSlot; use fuels_types::JsonABI; -use petgraph::{self, visit::EdgeRef, Directed, Direction}; +use petgraph::{ + self, + visit::{Bfs, Dfs, EdgeRef, Walker}, + Directed, Direction, +}; use serde::{Deserialize, Serialize}; use std::{ collections::{hash_map, BTreeSet, HashMap, HashSet}, @@ -32,7 +36,7 @@ type Node = Pinned; type Edge = DependencyName; pub type Graph = petgraph::stable_graph::StableGraph; pub type NodeIx = petgraph::graph::NodeIndex; -pub type PathMap = HashMap; +pub type ManifestMap = HashMap; /// A unique ID for a pinned package. /// @@ -161,7 +165,7 @@ pub enum SourcePinned { #[derive(Clone)] pub struct BuildPlan { graph: Graph, - path_map: PathMap, + manifest_map: ManifestMap, compilation_order: Vec, } @@ -210,12 +214,18 @@ impl BuildPlan { /// Create a new build plan for the project by fetching and pinning dependenies. pub fn new(manifest: &ManifestFile, sway_git_tag: &str, offline: bool) -> Result { let mut graph = Graph::default(); - let mut path_map = PathMap::default(); - fetch_graph(manifest, offline, sway_git_tag, &mut graph, &mut path_map)?; + let mut manifest_map = ManifestMap::default(); + fetch_graph( + manifest, + offline, + sway_git_tag, + &mut graph, + &mut manifest_map, + )?; let compilation_order = compilation_order(&graph)?; Ok(Self { graph, - path_map, + manifest_map, compilation_order, }) } @@ -263,15 +273,21 @@ impl BuildPlan { // might have edited the `Forc.lock` file when they shouldn't have, a path dependency no // longer exists at its specified location, etc. We must first remove all invalid nodes // before we can determine what we need to fetch. - let to_remove = validate_graph(&graph, manifest, sway_git_tag); + let to_remove = validate_graph(&graph, &manifest, sway_git_tag); remove_deps(&mut graph, &manifest.project.name, &to_remove); // We know that the remaining nodes have valid paths, otherwise they would have been // removed. We can safely produce an initial `path_map`. - let mut path_map = graph_to_path_map(manifest.dir(), &graph, sway_git_tag)?; + let mut manifest_map = graph_to_manifest_map(manifest.clone(), &graph, sway_git_tag)?; // Attempt to fetch the remainder of the graph. - let added = fetch_graph(manifest, offline, sway_git_tag, &mut graph, &mut path_map)?; + let added = fetch_graph( + manifest, + offline, + sway_git_tag, + &mut graph, + &mut manifest_map, + )?; // If we require any changes to the graph, update the new lock cause. if !to_remove.is_empty() || !added.is_empty() { @@ -283,7 +299,7 @@ impl BuildPlan { let plan = Self { graph, - path_map, + manifest_map, compilation_order, }; @@ -316,10 +332,9 @@ impl BuildPlan { &self.graph } - /// View the build plan's map of pinned package IDs to the path containing a local copy of - /// their source. - pub fn path_map(&self) -> &PathMap { - &self.path_map + /// View the build plan's map of pinned package IDs to their associated manifest. + pub fn manifest_map(&self) -> &ManifestMap { + &self.manifest_map } /// The order in which nodes are compiled, determined via a toposort of the package graph. @@ -413,23 +428,18 @@ fn validate_dep( sway_git_tag: &str, ) -> Result { // Check the validity of the dependency path, including its path root. - let dep_path = dep_path(graph, node_manifest, dep_name, dep_node, sway_git_tag) - .ok_or_else(|| anyhow!("failed to construct path for dependency {:?}", dep_name))?; + let dep_path = + dep_path(graph, node_manifest, dep_name, dep_node, sway_git_tag).map_err(|e| { + anyhow!( + "failed to construct path for dependency {:?}: {}", + dep_name, + e + ) + })?; // Ensure the manifest is accessible. let dep_manifest = ManifestFile::from_dir(&dep_path, sway_git_tag)?; - // Ensure the name matches the manifest project name. - if graph[dep_node].name != dep_manifest.project.name { - bail!( - "dependency name {:?} must match the manifest project name {:?} \ - unless `package = {:?}` is specified in the dependency declaration", - graph[dep_node].name, - dep_manifest.project.name, - dep_manifest.project.name, - ); - } - // Check that the dependency's source matches the entry in the parent manifest. let dep_entry = node_manifest .dep(dep_name) @@ -440,10 +450,27 @@ fn validate_dep( bail!("dependency node's source does not match manifest entry"); } + validate_dep_manifest(&graph[dep_node], &dep_manifest)?; + Ok(dep_manifest) } -/// Returns the local path to the given dependency node if it exists, `None` otherwise. +/// Part of dependency validation, any checks related to the depenency's manifest content. +fn validate_dep_manifest(dep: &Pinned, dep_manifest: &ManifestFile) -> Result<()> { + // Ensure the name matches the manifest project name. + if dep.name != dep_manifest.project.name { + bail!( + "dependency name {:?} must match the manifest project name {:?} \ + unless `package = {:?}` is specified in the dependency declaration", + dep.name, + dep_manifest.project.name, + dep_manifest.project.name, + ); + } + Ok(()) +} + +/// Returns the canonical, local path to the given dependency node if it exists, `None` otherwise. /// /// Also returns `None` in the case that the dependency is a `Path` dependency and the path root is /// invalid. @@ -453,22 +480,26 @@ fn dep_path( dep_name: &str, dep_node: NodeIx, sway_git_tag: &str, -) -> Option { +) -> Result { let dep = &graph[dep_node]; match &dep.source { SourcePinned::Git(git) => { let repo_path = git_commit_path(&dep.name, &git.source.repo, &git.commit_hash); - find_dir_within(&repo_path, &dep.name, sway_git_tag) + find_dir_within(&repo_path, &dep.name, sway_git_tag).ok_or_else(|| { + anyhow!( + "failed to find package `{}` in {}", + dep.name, + git.to_string() + ) + }) } SourcePinned::Path(src) => { - if validate_path_root(graph, dep_node, src.path_root).is_err() { - return None; - } + validate_path_root(graph, dep_node, src.path_root)?; // Check if the path is directly from the dependency. if let Some(path) = node_manifest.dep_path(dep_name) { if path.exists() { - return Some(path); + return Ok(path); } } @@ -476,13 +507,20 @@ fn dep_path( for (_, patch_map) in node_manifest.patches() { if let Some(Dependency::Detailed(details)) = patch_map.get(dep_name) { if let Some(ref rel_path) = details.path { - let path = node_manifest.dir().join(rel_path); - return path.canonicalize().ok().filter(|p| p.exists()); + if let Ok(path) = node_manifest.dir().join(rel_path).canonicalize() { + if path.exists() { + return Ok(path); + } + } } } } - None + bail!( + "no dependency or patch with name {:?} in manifest of {:?}", + dep_name, + node_manifest.project.name + ) } SourcePinned::Registry(_reg) => unreachable!("registry dependencies not yet supported"), SourcePinned::Root => unreachable!("a `Root` node cannot be a dependency"), @@ -492,8 +530,6 @@ fn dep_path( /// Remove the given set of packages from `graph` along with any dependencies that are no /// longer required as a result. fn remove_deps(graph: &mut Graph, proj_name: &str, to_remove: &BTreeSet) { - use petgraph::visit::Bfs; - // Retrieve the project node. let proj_node = match find_proj_node(graph, proj_name) { Ok(node) => node, @@ -820,119 +856,55 @@ pub fn compilation_order(graph: &Graph) -> Result> { /// Given a graph of pinned packages and the directory for the root node, produce a path map /// containing the path to the local source for every node in the graph. /// -/// NOTE: This function assumes that the paths are valid for each node in the graph. It is designed -/// to be called after calls to `validate_graph_with_manifest` and `remove_deps`, following which -/// there should be no more nodes with invalid paths remaining in the graph. -fn graph_to_path_map( - proj_manifest_dir: &Path, +/// This function returns an `Err` if it is unable to resolve the path of one or more of the nodes. +/// It is designed to be called after calls to `validate_graph_with_manifest` and `remove_deps`, +/// following which there should be no more nodes with invalid paths remaining in the graph. +fn graph_to_manifest_map( + proj_manifest: ManifestFile, graph: &Graph, sway_git_tag: &str, -) -> Result { - let mut path_map = PathMap::new(); - - // If the graph is empty, the path map will be too. - if graph.node_count() == 0 { - return Ok(path_map); - } +) -> Result { + let mut manifest_map = ManifestMap::new(); - // We resolve all paths in reverse compilation order. - // That is, we follow paths starting from the project root. - let compilation_order = compilation_order(graph)?; - let mut path_resolve_order = compilation_order.iter().cloned().rev(); - // Add the project's package to the map. - let proj_node = path_resolve_order - .next() - .ok_or_else(|| anyhow!("graph must contain at least the project node"))?; + // We'll traverse the graph from the project node. + let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { + Ok(node) => node, + Err(_) => return Ok(manifest_map), + }; let proj_id = graph[proj_node].id(); - path_map.insert(proj_id, proj_manifest_dir.to_path_buf().canonicalize()?); - - // Produce the unique `fetch_id` in case we need to fetch a missing git dep. - let fetch_ts = std::time::Instant::now(); - let fetch_id = fetch_id(&path_map[&proj_id], fetch_ts); - - // Resolve all following dependencies, knowing their parents' paths will already be resolved. - for dep_node in path_resolve_order { - let dep = &graph[dep_node]; - let dep_path = match &dep.source { - SourcePinned::Root => bail!("more than one root package detected in graph"), - SourcePinned::Git(git) => { - let repo_path = git_commit_path(&dep.name, &git.source.repo, &git.commit_hash); - if !repo_path.exists() { - info!(" Fetching {}", git.to_string()); - fetch_git(fetch_id, &dep.name, git)?; - } - find_dir_within(&repo_path, &dep.name, sway_git_tag).ok_or_else(|| { - anyhow!( - "failed to find package `{}` in {}", - dep.name, - git.to_string() - ) - })? - } - SourcePinned::Path(path) => { - // This is already checked during `Graph::from_lock`, but we check again here just - // in case this is being called with a `Graph` constructed via some other means. - validate_path_root(graph, dep_node, path.path_root)?; - - // Retrieve the parent node to construct the relative path. - let (parent_node, dep_name) = graph - .edges_directed(dep_node, Direction::Incoming) - .next() - .map(|edge| (edge.source(), edge.weight().clone())) - .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; + manifest_map.insert(proj_id, proj_manifest); + + // Resolve all parents before their dependencies as we require the parent path to construct the + // dependency path. Skip the already added project node at the beginning of traversal. + let mut bfs = Bfs::new(graph, proj_node); + bfs.next(graph); + while let Some(dep_node) = bfs.next(graph) { + // Retrieve the parent node whose manifest is already stored. + let (parent_manifest, dep_name) = graph + .edges_directed(dep_node, Direction::Incoming) + .filter_map(|edge| { + let parent_node = edge.source(); + let dep_name = edge.weight(); let parent = &graph[parent_node]; - - // Construct the path relative to the parent's path. - let parent_path = &path_map[&parent.id()]; - let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?; - let detailed = parent_manifest - .dependencies - .as_ref() - .and_then(|deps| deps.get(&dep_name)) - .ok_or_else(|| { - anyhow!( - "dependency required for path reconstruction \ - has been removed from the manifest" - ) - }) - .and_then(|dep| match dep { - Dependency::Detailed(detailed) => Ok(detailed), - Dependency::Simple(_) => { - bail!("missing path info for dependency: {}", &dep_name); - } - })?; - // Check if there is a patch for this dep - let patch = parent_manifest - .patches() - .find_map(|patches| patches.1.get(&dep_name)); - // If there is one fetch the details. - let patch_details = patch.and_then(|patch| match patch { - Dependency::Simple(_) => None, - Dependency::Detailed(detailed) => Some(detailed), - }); - // If there is a detail we should have the path. - // If not either we do not have a patch so we are checking dependencies of parent - // If we can't find the path there, either patch or dep is provided as a basic dependency, so we are missing the path info. - let rel_dep_path = if let Some(patch_details) = patch_details { - patch_details.path.as_ref() - } else { - detailed.path.as_ref() - } - .ok_or_else(|| anyhow!("missing path info for dep: {}", &dep_name))?; - let path = parent_path.join(rel_dep_path); - if !path.exists() { - bail!("pinned `path` dependency \"{}\" source missing", dep.name); - } - path - } - SourcePinned::Registry(_reg) => { - bail!("registry dependencies are not yet supported"); - } - }; - path_map.insert(dep.id(), dep_path.canonicalize()?); + let parent_manifest = manifest_map.get(&parent.id())?; + Some((parent_manifest, dep_name)) + }) + .next() + .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; + let dep_path = dep_path(graph, &parent_manifest, dep_name, dep_node, sway_git_tag) + .map_err(|e| { + anyhow!( + "failed to construct path for dependency {:?}: {}", + dep_name, + e + ) + })?; + let dep_manifest = ManifestFile::from_dir(&dep_path, sway_git_tag)?; + let dep = &graph[dep_node]; + manifest_map.insert(dep.id(), dep_manifest); } - Ok(path_map) + Ok(manifest_map) } /// Given a `graph`, the node index of a path dependency within that `graph`, and the supposed @@ -1008,7 +980,7 @@ fn fetch_graph( offline: bool, sway_git_tag: &str, graph: &mut Graph, - path_map: &mut PathMap, + manifest_map: &mut ManifestMap, ) -> Result> { // Retrieve the project node, or create one if it does not exist. let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { @@ -1016,25 +988,23 @@ fn fetch_graph( Err(_) => { let name = proj_manifest.project.name.clone(); let source = SourcePinned::Root; - let path = proj_manifest.dir().to_owned(); let pkg = Pinned { name, source }; let pkg_id = pkg.id(); - path_map.insert(pkg_id, path); + manifest_map.insert(pkg_id, proj_manifest.clone()); graph.add_node(pkg) } }; // Traverse the rest of the graph from the root. - let proj_dir = proj_manifest.dir(); let fetch_ts = std::time::Instant::now(); - let fetch_id = fetch_id(proj_dir, fetch_ts); + let fetch_id = fetch_id(proj_manifest.dir(), fetch_ts); let path_root = graph[proj_node].id(); let mut fetched = graph .node_indices() .map(|n| { let pinned = &graph[n]; - let path = &path_map[&pinned.id()]; - let pkg = pinned.unpinned(path); + let manifest = &manifest_map[&pinned.id()]; + let pkg = pinned.unpinned(manifest.dir()); (pkg, n) }) .collect(); @@ -1043,34 +1013,39 @@ fn fetch_graph( fetch_id, offline, proj_node, - proj_manifest, path_root, sway_git_tag, graph, - path_map, + manifest_map, &mut fetched, &mut visited, ) } /// Visit the unvisited dependencies of the given node and fetch missing nodes as necessary. +/// +/// Assumes the `node`'s manifest already exists within the `manifest_map`. #[allow(clippy::too_many_arguments)] fn fetch_deps( fetch_id: u64, offline: bool, node: NodeIx, - manifest: &ManifestFile, path_root: PinnedId, sway_git_tag: &str, graph: &mut Graph, - path_map: &mut PathMap, + manifest_map: &mut ManifestMap, fetched: &mut HashMap, visited: &mut HashSet, ) -> Result> { let mut added = HashSet::default(); - for (dep_name, dep) in manifest.deps() { - let name = dep.package().unwrap_or(dep_name).to_string(); - let source = dep_to_source_patched(manifest, &name, dep)?; + let parent_id = graph[node].id(); + let deps: Vec<_> = manifest_map[&parent_id] + .deps() + .map(|(n, d)| (n.clone(), d.clone())) + .collect(); + for (dep_name, dep) in deps { + let name = dep.package().unwrap_or(&dep_name).to_string(); + let source = dep_to_source_patched(&manifest_map[&parent_id], &name, &dep)?; // If we haven't yet fetched this dependency, fetch it, pin it and add it to the graph. let dep_pkg = Pkg { name, source }; @@ -1081,7 +1056,7 @@ fn fetch_deps( fetch_id, path_root, entry.key(), - path_map, + manifest_map, offline, sway_git_tag, )?; @@ -1099,20 +1074,18 @@ fn fetch_deps( continue; } - // This shouldn't fail unless the dependency's package name differs from its manifest. - let dep_manifest = validate_dep(graph, manifest, dep_name, dep_node, sway_git_tag) - .map_err(|e| { - let parent = &graph[node]; - anyhow!( - "dependency of {:?} named {:?} is invalid: {}", - parent.name, - dep_name, - e - ) - })?; - let dep_pinned = &graph[dep_node]; let dep_pkg_id = dep_pinned.id(); + validate_dep_manifest(dep_pinned, &manifest_map[&dep_pkg_id]).map_err(|e| { + let parent = &graph[node]; + anyhow!( + "dependency of {:?} named {:?} is invalid: {}", + parent.name, + dep_name, + e + ) + })?; + let path_root = match dep_pinned.source { SourcePinned::Root | SourcePinned::Git(_) | SourcePinned::Registry(_) => dep_pkg_id, SourcePinned::Path(_) => path_root, @@ -1123,11 +1096,10 @@ fn fetch_deps( fetch_id, offline, dep_node, - &dep_manifest, path_root, sway_git_tag, graph, - path_map, + manifest_map, fetched, visited, )?); @@ -1260,7 +1232,7 @@ fn pin_pkg( fetch_id: u64, path_root: PinnedId, pkg: &Pkg, - path_map: &mut PathMap, + manifest_map: &mut ManifestMap, offline: bool, sway_git_tag: &str, ) -> Result { @@ -1270,7 +1242,8 @@ fn pin_pkg( let source = SourcePinned::Root; let pinned = Pinned { name, source }; let id = pinned.id(); - path_map.insert(id, path.clone()); + let manifest = ManifestFile::from_dir(path, sway_git_tag)?; + manifest_map.insert(id, manifest); pinned } Source::Path(path) => { @@ -1278,7 +1251,8 @@ fn pin_pkg( let source = SourcePinned::Path(path_pinned); let pinned = Pinned { name, source }; let id = pinned.id(); - path_map.insert(id, path.clone()); + let manifest = ManifestFile::from_dir(path, sway_git_tag)?; + manifest_map.insert(id, manifest); pinned } Source::Git(ref git_source) => { @@ -1298,7 +1272,7 @@ fn pin_pkg( let source = SourcePinned::Git(pinned_git.clone()); let pinned = Pinned { name, source }; let id = pinned.id(); - if let hash_map::Entry::Vacant(entry) = path_map.entry(id) { + if let hash_map::Entry::Vacant(entry) = manifest_map.entry(id) { // TODO: Here we assume that if the local path already exists, that it contains the // full and correct source for that commit and hasn't been tampered with. This is // probably fine for most cases as users should never be touching these @@ -1317,7 +1291,8 @@ fn pin_pkg( pinned_git.to_string() ) })?; - entry.insert(path); + let manifest = ManifestFile::from_dir(&path, sway_git_tag)?; + entry.insert(manifest); } pinned } @@ -1518,8 +1493,6 @@ pub fn dependency_namespace( /// Find the `core` dependency (whether direct or transitive) for the given node if it exists. fn find_core_dep(graph: &Graph, node: NodeIx) -> Option { - use petgraph::visit::{Dfs, Walker}; - // If we are `core`, do nothing. let pkg = &graph[node]; if pkg.name == CORE { @@ -1691,11 +1664,7 @@ pub fn compile( /// This compiles all packages (including dependencies) in the order specified by the `BuildPlan`. /// /// Also returns the resulting `sway_core::SourceMap` which may be useful for debugging purposes. -pub fn build( - plan: &BuildPlan, - profile: &BuildProfile, - sway_git_tag: &str, -) -> anyhow::Result<(Compiled, SourceMap)> { +pub fn build(plan: &BuildPlan, profile: &BuildProfile) -> anyhow::Result<(Compiled, SourceMap)> { let mut namespace_map = Default::default(); let mut source_map = SourceMap::new(); let mut json_abi = vec![]; @@ -1704,9 +1673,8 @@ pub fn build( let mut tree_type = None; for &node in &plan.compilation_order { let dep_namespace = dependency_namespace(&namespace_map, &plan.graph, node); - let pkg = &plan.graph[node]; - let path = &plan.path_map[&pkg.id()]; - let manifest = ManifestFile::from_dir(path, sway_git_tag)?; + let pkg = &plan.graph()[node]; + let manifest = &plan.manifest_map()[&pkg.id()]; let res = compile(pkg, &manifest, profile, dep_namespace, &mut source_map)?; let (compiled, maybe_namespace) = res; if let Some(namespace) = maybe_namespace { @@ -1716,7 +1684,7 @@ pub fn build( storage_slots.extend(compiled.storage_slots); bytecode = compiled.bytecode; tree_type = Some(compiled.tree_type); - source_map.insert_dependency(path.clone()); + source_map.insert_dependency(manifest.dir()); } let tree_type = tree_type.ok_or_else(|| anyhow!("build plan must contain at least one package"))?; @@ -1730,30 +1698,24 @@ pub fn build( } /// Compile the entire forc package and return a CompileAstResult. -pub fn check( - plan: &BuildPlan, - silent_mode: bool, - sway_git_tag: &str, -) -> anyhow::Result { +pub fn check(plan: &BuildPlan, silent_mode: bool) -> anyhow::Result { let profile = BuildProfile { silent: silent_mode, ..BuildProfile::debug() }; - let mut namespace_map = Default::default(); let mut source_map = SourceMap::new(); for (i, &node) in plan.compilation_order.iter().enumerate() { let dep_namespace = dependency_namespace(&namespace_map, &plan.graph, node); - let pkg = &plan.graph[node]; - let path = &plan.path_map[&pkg.id()]; - let manifest = ManifestFile::from_dir(path, sway_git_tag)?; + let pkg = &plan.graph()[node]; + let manifest = &plan.manifest_map()[&pkg.id()]; let ast_res = compile_ast(&manifest, &profile, dep_namespace)?; if let CompileAstResult::Success { typed_program, .. } = &ast_res { if let TreeType::Library { .. } = typed_program.kind.tree_type() { namespace_map.insert(node, typed_program.root.namespace.clone()); } } - source_map.insert_dependency(path.clone()); + source_map.insert_dependency(manifest.dir()); // We only need to return the final CompileAstResult if i == plan.compilation_order.len() - 1 { diff --git a/forc/src/ops/forc_build.rs b/forc/src/ops/forc_build.rs index 00f9da19158..37f93b2f460 100644 --- a/forc/src/ops/forc_build.rs +++ b/forc/src/ops/forc_build.rs @@ -79,7 +79,7 @@ pub fn build(command: BuildCommand) -> Result { profile.time_phases |= time_phases; // Build it! - let (compiled, source_map) = pkg::build(&plan, &profile, SWAY_GIT_TAG)?; + let (compiled, source_map) = pkg::build(&plan, &profile)?; if let Some(outfile) = binary_outfile { fs::write(&outfile, &compiled.bytecode)?; diff --git a/forc/src/ops/forc_check.rs b/forc/src/ops/forc_check.rs index be39de30508..09e6be93365 100644 --- a/forc/src/ops/forc_check.rs +++ b/forc/src/ops/forc_check.rs @@ -19,5 +19,5 @@ pub fn check(command: CheckCommand) -> Result { let manifest = ManifestFile::from_dir(&this_dir, SWAY_GIT_TAG)?; let plan = pkg::BuildPlan::load_from_manifest(&manifest, locked, offline, SWAY_GIT_TAG)?; - pkg::check(&plan, silent_mode, SWAY_GIT_TAG) + pkg::check(&plan, silent_mode) } diff --git a/sway-lsp/src/core/document.rs b/sway-lsp/src/core/document.rs index 9ae9bc8490b..091dfa0f30f 100644 --- a/sway-lsp/src/core/document.rs +++ b/sway-lsp/src/core/document.rs @@ -163,7 +163,7 @@ impl TextDocument { let plan = pkg::BuildPlan::load_from_manifest(&manifest, false, false, forc::utils::SWAY_GIT_TAG) .unwrap(); - let res = pkg::check(&plan, silent_mode, forc::utils::SWAY_GIT_TAG).unwrap(); + let res = pkg::check(&plan, silent_mode).unwrap(); match res { CompileAstResult::Failure { .. } => None, From a21666923d61ca8259d84ee938df5eada5e67d6a Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 12:11:12 +1000 Subject: [PATCH 23/31] Ensure only one edge between a package and each dependency --- forc-pkg/src/lock.rs | 2 +- forc-pkg/src/pkg.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/forc-pkg/src/lock.rs b/forc-pkg/src/lock.rs index d811b06d005..ec9e068e376 100644 --- a/forc-pkg/src/lock.rs +++ b/forc-pkg/src/lock.rs @@ -159,7 +159,7 @@ impl Lock { .cloned() .ok_or_else(|| anyhow!("found dep {} without node entry in graph", dep_key))?; let dep_name = dep_name.unwrap_or(&graph[dep_node].name).to_string(); - graph.add_edge(node, dep_node, dep_name); + graph.update_edge(node, dep_node, dep_name); } } diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 5710ec10dd5..043f228721e 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -1067,7 +1067,7 @@ fn fetch_deps( }; // Add the edge to the dependency. - graph.add_edge(node, dep_node, dep_name.to_string()); + graph.update_edge(node, dep_node, dep_name.to_string()); // If we've visited this node during this traversal already, no need to traverse it again. if !visited.insert(dep_node) { From c8a43f34e7dd1fdf264c5129fffeb6ef7ab214c6 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 12:21:25 +1000 Subject: [PATCH 24/31] Ensure lock is updated if path child dependencies change --- forc-pkg/src/pkg.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 043f228721e..115911ef11e 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -281,7 +281,7 @@ impl BuildPlan { let mut manifest_map = graph_to_manifest_map(manifest.clone(), &graph, sway_git_tag)?; // Attempt to fetch the remainder of the graph. - let added = fetch_graph( + let _added = fetch_graph( manifest, offline, sway_git_tag, @@ -289,11 +289,6 @@ impl BuildPlan { &mut manifest_map, )?; - // If we require any changes to the graph, update the new lock cause. - if !to_remove.is_empty() || !added.is_empty() { - new_lock_cause = Some(anyhow!("lock file did not match manifest")); - } - // Determine the compilation order. let compilation_order = compilation_order(&graph)?; @@ -303,6 +298,14 @@ impl BuildPlan { compilation_order, }; + // Construct the new lock and check the diff. + let new_lock = Lock::from_graph(plan.graph()); + let lock_diff = new_lock.diff(&lock); + if !lock_diff.removed.is_empty() || !lock_diff.added.is_empty() { + new_lock_cause.get_or_insert(anyhow!("lock file did not match manifest")); + } + + // If there was some change in the lock file, write the new one and share the cause. if let Some(cause) = new_lock_cause { if locked { bail!( @@ -312,10 +315,7 @@ impl BuildPlan { cause, ); } - info!(" Creating a new `Forc.lock` file. (Cause: {})", cause); - let new_lock = Lock::from_graph(plan.graph()); - let lock_diff = new_lock.diff(&lock); crate::lock::print_diff(&manifest.project.name, &lock_diff); let string = toml::ser::to_string_pretty(&new_lock) .map_err(|e| anyhow!("failed to serialize lock file: {}", e))?; From a11696caa06152feb8c3416bf2eaa90b81fc16f1 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 12:35:39 +1000 Subject: [PATCH 25/31] Address clippy nits --- forc-pkg/src/pkg.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 115911ef11e..56f195d4d43 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -273,7 +273,7 @@ impl BuildPlan { // might have edited the `Forc.lock` file when they shouldn't have, a path dependency no // longer exists at its specified location, etc. We must first remove all invalid nodes // before we can determine what we need to fetch. - let to_remove = validate_graph(&graph, &manifest, sway_git_tag); + let to_remove = validate_graph(&graph, manifest, sway_git_tag); remove_deps(&mut graph, &manifest.project.name, &to_remove); // We know that the remaining nodes have valid paths, otherwise they would have been @@ -891,7 +891,7 @@ fn graph_to_manifest_map( }) .next() .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; - let dep_path = dep_path(graph, &parent_manifest, dep_name, dep_node, sway_git_tag) + let dep_path = dep_path(graph, parent_manifest, dep_name, dep_node, sway_git_tag) .map_err(|e| { anyhow!( "failed to construct path for dependency {:?}: {}", @@ -1675,7 +1675,7 @@ pub fn build(plan: &BuildPlan, profile: &BuildProfile) -> anyhow::Result<(Compil let dep_namespace = dependency_namespace(&namespace_map, &plan.graph, node); let pkg = &plan.graph()[node]; let manifest = &plan.manifest_map()[&pkg.id()]; - let res = compile(pkg, &manifest, profile, dep_namespace, &mut source_map)?; + let res = compile(pkg, manifest, profile, dep_namespace, &mut source_map)?; let (compiled, maybe_namespace) = res; if let Some(namespace) = maybe_namespace { namespace_map.insert(node, namespace.into()); @@ -1709,7 +1709,7 @@ pub fn check(plan: &BuildPlan, silent_mode: bool) -> anyhow::Result Date: Wed, 6 Jul 2022 12:36:09 +1000 Subject: [PATCH 26/31] Address formatting nits --- forc-pkg/src/pkg.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 56f195d4d43..626b7e93760 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -891,8 +891,8 @@ fn graph_to_manifest_map( }) .next() .ok_or_else(|| anyhow!("more than one root package detected in graph"))?; - let dep_path = dep_path(graph, parent_manifest, dep_name, dep_node, sway_git_tag) - .map_err(|e| { + let dep_path = + dep_path(graph, parent_manifest, dep_name, dep_node, sway_git_tag).map_err(|e| { anyhow!( "failed to construct path for dependency {:?}: {}", dep_name, From f8ece94fc769d488d2daeb4aaeb4dd33dda62e29 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 13:39:36 +1000 Subject: [PATCH 27/31] Improve accuracy of dependency removal by invalidating by edges Previously, all dependency nodes were removed in the case that they were invalid for any one of their parents. This commit changes the behaviour to not remove the node, but rather each invalid dependency *edge* instead. This means that if a node is valid for one parent but not another, only the edge to the other parent is removed and not the whole node itself. The node is only removed in the case that it is invalid for all of its parents. This is necessary for handling the case where a dependency no longer exists in one parent's `[dependencies]` but still exists in another. The most common case for this is `std`, as many nodes may depend on `std` however we don't want to remove `std` and have to re-fetch it just because one of the nodes removed their depenency on it. --- forc-pkg/src/pkg.rs | 64 ++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 626b7e93760..200e0cd992c 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -35,6 +35,7 @@ type GraphIx = u32; type Node = Pinned; type Edge = DependencyName; pub type Graph = petgraph::stable_graph::StableGraph; +pub type EdgeIx = petgraph::graph::EdgeIndex; pub type NodeIx = petgraph::graph::NodeIndex; pub type ManifestMap = HashMap; @@ -273,8 +274,8 @@ impl BuildPlan { // might have edited the `Forc.lock` file when they shouldn't have, a path dependency no // longer exists at its specified location, etc. We must first remove all invalid nodes // before we can determine what we need to fetch. - let to_remove = validate_graph(&graph, manifest, sway_git_tag); - remove_deps(&mut graph, &manifest.project.name, &to_remove); + let invalid_deps = validate_graph(&graph, manifest, sway_git_tag); + remove_deps(&mut graph, &manifest.project.name, &invalid_deps); // We know that the remaining nodes have valid paths, otherwise they would have been // removed. We can safely produce an initial `path_map`. @@ -371,21 +372,18 @@ fn find_proj_node(graph: &Graph, proj_name: &str) -> Result { /// Validates the state of the graph against the given project manifest. /// -/// Returns the set of invalid nodes detected within the graph that should be removed. -/// -/// Note that nodes whose parents are all invalid are *not* returned, as it is assumed they will be -/// removed along with their invalid parents in `remove_deps` in a BFS from the project node. +/// Returns the set of invalid dependencies detected within the graph that should be removed. fn validate_graph( graph: &Graph, proj_manifest: &ManifestFile, sway_git_tag: &str, -) -> BTreeSet { +) -> BTreeSet { // First, check the project node exists. // If we we don't have exactly one potential project node, remove everything as we can't // validate the graph without knowing the project node. let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { Ok(node) => node, - Err(_) => return graph.node_indices().collect(), + Err(_) => return graph.edge_indices().collect(), }; // Collect all invalid dependency nodes. validate_deps(graph, proj_node, proj_manifest, sway_git_tag) @@ -399,14 +397,14 @@ fn validate_deps( node: NodeIx, node_manifest: &ManifestFile, sway_git_tag: &str, -) -> BTreeSet { +) -> BTreeSet { let mut remove = BTreeSet::default(); for edge in graph.edges_directed(node, Direction::Outgoing) { let dep_name = edge.weight(); let dep_node = edge.target(); match validate_dep(graph, node_manifest, dep_name, dep_node, sway_git_tag) { Err(_) => { - remove.insert(dep_node); + remove.insert(edge.id()); } Ok(dep_manifest) => { remove.extend(validate_deps(graph, dep_node, &dep_manifest, sway_git_tag)); @@ -527,9 +525,10 @@ fn dep_path( } } -/// Remove the given set of packages from `graph` along with any dependencies that are no -/// longer required as a result. -fn remove_deps(graph: &mut Graph, proj_name: &str, to_remove: &BTreeSet) { +/// Remove the given set of dependency edges from the `graph`. +/// +/// Also removes all nodes that are no longer connected to the project node as a result. +fn remove_deps(graph: &mut Graph, proj_name: &str, edges_to_remove: &BTreeSet) { // Retrieve the project node. let proj_node = match find_proj_node(graph, proj_name) { Ok(node) => node, @@ -540,22 +539,39 @@ fn remove_deps(graph: &mut Graph, proj_name: &str, to_remove: &BTreeSet) } }; - // Remove all nodes that either have no parents, or are contained within the `to_remove` set. - // We do so in breadth-first-order to ensure we always remove all parents before children. - let mut bfs = Bfs::new(&*graph, proj_node); - // Skip the root node (aka project node). - bfs.next(&*graph); - while let Some(node) = bfs.next(&*graph) { - let no_parents = graph - .edges_directed(node, Direction::Incoming) - .next() - .is_none(); - if no_parents || to_remove.contains(&node) { + // Before removing edges, sort the nodes in order of dependency for the node removal pass. + let node_removal_order = match petgraph::algo::toposort(&*graph, None) { + Ok(nodes) => nodes, + Err(_) => { + // If toposort fails the given graph is cyclic, so invalidate everything. + graph.clear(); + return; + } + }; + + // Remove the given set of dependency edges. + for &edge in edges_to_remove { + graph.remove_edge(edge); + } + + // Remove all nodes that are no longer connected to the project node as a result. + // Skip iteration over the project node. + let mut nodes = node_removal_order.into_iter(); + assert_eq!(nodes.next(), Some(proj_node)); + for node in nodes { + if !has_parent(graph, node) { graph.remove_node(node); } } } +fn has_parent(graph: &Graph, node: NodeIx) -> bool { + graph + .edges_directed(node, Direction::Incoming) + .next() + .is_some() +} + impl GitReference { /// Resolves the parsed forc git reference to the associated git ID. pub fn resolve(&self, repo: &git2::Repository) -> Result { From d161d20787c0fc4c20d42325c8ee9594c71de108 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 13:49:57 +1000 Subject: [PATCH 28/31] Avoid visiting a node's dependencies multiple times during validation --- forc-pkg/src/pkg.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 200e0cd992c..88909fe2687 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -386,7 +386,8 @@ fn validate_graph( Err(_) => return graph.edge_indices().collect(), }; // Collect all invalid dependency nodes. - validate_deps(graph, proj_node, proj_manifest, sway_git_tag) + let mut visited = HashSet::new(); + validate_deps(graph, proj_node, proj_manifest, sway_git_tag, &mut visited) } /// Validate all dependency nodes in the `graph`, traversing from `node`. @@ -397,6 +398,7 @@ fn validate_deps( node: NodeIx, node_manifest: &ManifestFile, sway_git_tag: &str, + visited: &mut HashSet, ) -> BTreeSet { let mut remove = BTreeSet::default(); for edge in graph.edges_directed(node, Direction::Outgoing) { @@ -407,7 +409,10 @@ fn validate_deps( remove.insert(edge.id()); } Ok(dep_manifest) => { - remove.extend(validate_deps(graph, dep_node, &dep_manifest, sway_git_tag)); + if visited.insert(dep_node) { + let rm = validate_deps(graph, dep_node, &dep_manifest, sway_git_tag, visited); + remove.extend(rm); + } continue; } } From 02ceb92e9cd8a30193b3f9806f33112592bfe1ff Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 14:09:24 +1000 Subject: [PATCH 29/31] Improve name and docs for BuildPlan constructors --- forc-pkg/src/pkg.rs | 18 ++++++++++++++---- forc/src/ops/forc_build.rs | 2 +- forc/src/ops/forc_check.rs | 2 +- forc/src/ops/forc_update.rs | 2 +- sway-lsp/src/core/document.rs | 10 +++++++--- 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 88909fe2687..f0e0b866499 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -212,8 +212,14 @@ pub struct SourcePinnedParseError; pub type DependencyName = String; impl BuildPlan { - /// Create a new build plan for the project by fetching and pinning dependenies. - pub fn new(manifest: &ManifestFile, sway_git_tag: &str, offline: bool) -> Result { + /// Create a new build plan for the project by fetching and pinning all dependenies. + /// + /// To account for an existing lock file, use `from_lock_and_manifest` instead. + pub fn from_manifest( + manifest: &ManifestFile, + sway_git_tag: &str, + offline: bool, + ) -> Result { let mut graph = Graph::default(); let mut manifest_map = ManifestMap::default(); fetch_graph( @@ -238,12 +244,16 @@ impl BuildPlan { /// graph using the current state of the Manifest. /// /// This includes checking if the [dependencies] or [patch] tables have changed and checking - /// the validity of the local path dependencies. If any changes are detected, the graph is + /// the validity of the local path dependencies. If any changes are detected, the graph is /// updated and any new packages that require fetching are fetched. /// /// The resulting build plan should always be in a valid state that is ready for building or /// checking. - pub fn load_from_manifest( + // TODO: Currently (if `--locked` isn't specified) this writes the updated lock directly. This + // probably should not be the role of the `BuildPlan::constructor` - instead, we should return + // the manifest alongside some lock diff type that can be used to optionally write the updated + // lock file and print the diff. + pub fn from_lock_and_manifest( manifest: &ManifestFile, locked: bool, offline: bool, diff --git a/forc/src/ops/forc_build.rs b/forc/src/ops/forc_build.rs index 37f93b2f460..4498a4f1536 100644 --- a/forc/src/ops/forc_build.rs +++ b/forc/src/ops/forc_build.rs @@ -58,7 +58,7 @@ pub fn build(command: BuildCommand) -> Result { let manifest = ManifestFile::from_dir(&this_dir, SWAY_GIT_TAG)?; - let plan = pkg::BuildPlan::load_from_manifest(&manifest, locked, offline, SWAY_GIT_TAG)?; + let plan = pkg::BuildPlan::from_lock_and_manifest(&manifest, locked, offline, SWAY_GIT_TAG)?; // Retrieve the specified build profile let mut profile = manifest diff --git a/forc/src/ops/forc_check.rs b/forc/src/ops/forc_check.rs index 09e6be93365..b87ed038031 100644 --- a/forc/src/ops/forc_check.rs +++ b/forc/src/ops/forc_check.rs @@ -17,7 +17,7 @@ pub fn check(command: CheckCommand) -> Result { std::env::current_dir()? }; let manifest = ManifestFile::from_dir(&this_dir, SWAY_GIT_TAG)?; - let plan = pkg::BuildPlan::load_from_manifest(&manifest, locked, offline, SWAY_GIT_TAG)?; + let plan = pkg::BuildPlan::from_lock_and_manifest(&manifest, locked, offline, SWAY_GIT_TAG)?; pkg::check(&plan, silent_mode) } diff --git a/forc/src/ops/forc_update.rs b/forc/src/ops/forc_update.rs index 4812e8f86f4..6b52ac18f43 100644 --- a/forc/src/ops/forc_update.rs +++ b/forc/src/ops/forc_update.rs @@ -36,7 +36,7 @@ pub async fn update(command: UpdateCommand) -> Result<()> { let lock_path = lock_path(manifest.dir()); let old_lock = Lock::from_path(&lock_path).ok().unwrap_or_default(); let offline = false; - let new_plan = pkg::BuildPlan::new(&manifest, SWAY_GIT_TAG, offline)?; + let new_plan = pkg::BuildPlan::from_manifest(&manifest, SWAY_GIT_TAG, offline)?; let new_lock = Lock::from_graph(new_plan.graph()); let diff = new_lock.diff(&old_lock); lock::print_diff(&manifest.project.name, &diff); diff --git a/sway-lsp/src/core/document.rs b/sway-lsp/src/core/document.rs index 091dfa0f30f..0c08f576a4e 100644 --- a/sway-lsp/src/core/document.rs +++ b/sway-lsp/src/core/document.rs @@ -160,9 +160,13 @@ impl TextDocument { let silent_mode = true; let manifest = pkg::ManifestFile::from_dir(&manifest_dir, forc::utils::SWAY_GIT_TAG).unwrap(); - let plan = - pkg::BuildPlan::load_from_manifest(&manifest, false, false, forc::utils::SWAY_GIT_TAG) - .unwrap(); + let plan = pkg::BuildPlan::from_lock_and_manifest( + &manifest, + false, + false, + forc::utils::SWAY_GIT_TAG, + ) + .unwrap(); let res = pkg::check(&plan, silent_mode).unwrap(); match res { From 647f0ea5a9c18c47c284a805b306ff7dfaf2ed3f Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 15:28:25 +1000 Subject: [PATCH 30/31] Remove unnecessary path root validation from Lock::to_graph. This is checked during the `validate_graph` phase. Also improves a bunch of comments. --- forc-pkg/src/lock.rs | 7 ------ forc-pkg/src/manifest.rs | 3 +-- forc-pkg/src/pkg.rs | 44 ++++++++++++++--------------------- sway-lsp/src/core/document.rs | 15 +++++------- 4 files changed, 25 insertions(+), 44 deletions(-) diff --git a/forc-pkg/src/lock.rs b/forc-pkg/src/lock.rs index ec9e068e376..1ca284dfc87 100644 --- a/forc-pkg/src/lock.rs +++ b/forc-pkg/src/lock.rs @@ -163,13 +163,6 @@ impl Lock { } } - // Validate the `path_root` of each of the path nodes. - for n in graph.node_indices() { - if let pkg::SourcePinned::Path(ref src) = graph[n].source { - pkg::validate_path_root(&graph, n, src.path_root)?; - } - } - Ok(graph) } diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index a2facf16f83..c04104a77b5 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -208,8 +208,7 @@ impl ManifestFile { .and_then(|profiles| profiles.get(profile_name)) } - /// Given the name of a dependency, if that dependency is `path` dependency, returns the full - /// canonical `Path` to the dependency. + /// Given the name of a `path` dependency, returns the full canonical `Path` to the dependency. pub fn dep_path(&self, dep_name: &str) -> Option { let dir = self.dir(); let details = self.dep_detailed(dep_name)?; diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index f0e0b866499..4f6d58106b4 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -250,7 +250,7 @@ impl BuildPlan { /// The resulting build plan should always be in a valid state that is ready for building or /// checking. // TODO: Currently (if `--locked` isn't specified) this writes the updated lock directly. This - // probably should not be the role of the `BuildPlan::constructor` - instead, we should return + // probably should not be the role of the `BuildPlan` constructor - instead, we should return // the manifest alongside some lock diff type that can be used to optionally write the updated // lock file and print the diff. pub fn from_lock_and_manifest( @@ -288,7 +288,7 @@ impl BuildPlan { remove_deps(&mut graph, &manifest.project.name, &invalid_deps); // We know that the remaining nodes have valid paths, otherwise they would have been - // removed. We can safely produce an initial `path_map`. + // removed. We can safely produce an initial `manifest_map`. let mut manifest_map = graph_to_manifest_map(manifest.clone(), &graph, sway_git_tag)?; // Attempt to fetch the remainder of the graph. @@ -316,7 +316,7 @@ impl BuildPlan { new_lock_cause.get_or_insert(anyhow!("lock file did not match manifest")); } - // If there was some change in the lock file, write the new one and share the cause. + // If there was some change in the lock file, write the new one and print the cause. if let Some(cause) = new_lock_cause { if locked { bail!( @@ -355,8 +355,7 @@ impl BuildPlan { } /// Given a graph and the known project name retrieved from the manifest, produce an iterator -/// yielding any nodes from the graph that might potentially be the project node. There should only -/// ever be one, and this is used during initial graph validation to ensure that's the case. +/// yielding any nodes from the graph that might potentially be the project node. fn potential_proj_nodes<'a>(g: &'a Graph, proj_name: &'a str) -> impl 'a + Iterator { g.node_indices() .filter(|&n| g.edges_directed(n, Direction::Incoming).next().is_none()) @@ -380,17 +379,16 @@ fn find_proj_node(graph: &Graph, proj_name: &str) -> Result { } } -/// Validates the state of the graph against the given project manifest. +/// Validates the state of the pinned package graph against the given project manifest. /// -/// Returns the set of invalid dependencies detected within the graph that should be removed. +/// Returns the set of invalid dependency edges. fn validate_graph( graph: &Graph, proj_manifest: &ManifestFile, sway_git_tag: &str, ) -> BTreeSet { - // First, check the project node exists. - // If we we don't have exactly one potential project node, remove everything as we can't - // validate the graph without knowing the project node. + // If we we don't have a project node, remove everything as we can't validate dependencies + // without knowing where to start. let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { Ok(node) => node, Err(_) => return graph.edge_indices().collect(), @@ -400,9 +398,9 @@ fn validate_graph( validate_deps(graph, proj_node, proj_manifest, sway_git_tag, &mut visited) } -/// Validate all dependency nodes in the `graph`, traversing from `node`. +/// Recursively validate all dependencies of the given `node`. /// -/// Returns the set of invalid nodes. +/// Returns the set of invalid dependency edges. fn validate_deps( graph: &Graph, node: NodeIx, @@ -858,8 +856,8 @@ impl Default for GitReference { pub fn compilation_order(graph: &Graph) -> Result> { let rev_pkg_graph = petgraph::visit::Reversed(&graph); petgraph::algo::toposort(rev_pkg_graph, None).map_err(|_| { - // Find strongly connected components - // If the vector has an element with length more than 1, it contains a cyclic path. + // Find the strongly connected components. + // If the vector has an element with length > 1, it contains a cyclic path. let scc = petgraph::algo::kosaraju_scc(&graph); let mut path = String::new(); scc.iter() @@ -884,12 +882,10 @@ pub fn compilation_order(graph: &Graph) -> Result> { }) } -/// Given a graph of pinned packages and the directory for the root node, produce a path map -/// containing the path to the local source for every node in the graph. +/// Given a graph of pinned packages and the project manifest, produce a map containing the +/// manifest of for every node in the graph. /// -/// This function returns an `Err` if it is unable to resolve the path of one or more of the nodes. -/// It is designed to be called after calls to `validate_graph_with_manifest` and `remove_deps`, -/// following which there should be no more nodes with invalid paths remaining in the graph. +/// Assumes the given `graph` only contains valid dependencies (see `validate_graph`). fn graph_to_manifest_map( proj_manifest: ManifestFile, graph: &Graph, @@ -897,7 +893,7 @@ fn graph_to_manifest_map( ) -> Result { let mut manifest_map = ManifestMap::new(); - // We'll traverse the graph from the project node. + // Traverse the graph from the project node. let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { Ok(node) => node, Err(_) => return Ok(manifest_map), @@ -942,11 +938,7 @@ fn graph_to_manifest_map( /// `path_root` of the path dependency, ensure that the `path_root` is valid. /// /// See the `path_root` field of the [SourcePathPinned] type for further details. -pub(crate) fn validate_path_root( - graph: &Graph, - path_dep: NodeIx, - path_root: PinnedId, -) -> Result<()> { +fn validate_path_root(graph: &Graph, path_dep: NodeIx, path_root: PinnedId) -> Result<()> { let path_root_node = find_path_root(graph, path_dep)?; if graph[path_root_node].id() != path_root { bail!( @@ -1097,7 +1089,7 @@ fn fetch_deps( } }; - // Add the edge to the dependency. + // Ensure we have an edge to the dependency. graph.update_edge(node, dep_node, dep_name.to_string()); // If we've visited this node during this traversal already, no need to traverse it again. diff --git a/sway-lsp/src/core/document.rs b/sway-lsp/src/core/document.rs index 0c08f576a4e..2ddbbcbe610 100644 --- a/sway-lsp/src/core/document.rs +++ b/sway-lsp/src/core/document.rs @@ -156,17 +156,14 @@ impl TextDocument { // private methods impl TextDocument { fn parse_typed_tokens_from_text(&self) -> Option> { + use forc::utils::SWAY_GIT_TAG; let manifest_dir = PathBuf::from(self.get_uri()); let silent_mode = true; - let manifest = - pkg::ManifestFile::from_dir(&manifest_dir, forc::utils::SWAY_GIT_TAG).unwrap(); - let plan = pkg::BuildPlan::from_lock_and_manifest( - &manifest, - false, - false, - forc::utils::SWAY_GIT_TAG, - ) - .unwrap(); + let manifest = pkg::ManifestFile::from_dir(&manifest_dir, SWAY_GIT_TAG).unwrap(); + let locked = false; + let offline = false; + let plan = pkg::BuildPlan::from_lock_and_manifest(&manifest, locked, offline, SWAY_GIT_TAG) + .unwrap(); let res = pkg::check(&plan, silent_mode).unwrap(); match res { From 28eec55cd5d02ca72330729e955afd420dcdac63 Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Wed, 6 Jul 2022 22:11:15 +1000 Subject: [PATCH 31/31] Fix typo in find_proj_node comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kaya Gökalp --- forc-pkg/src/pkg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 4f6d58106b4..cb26be86a46 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -387,7 +387,7 @@ fn validate_graph( proj_manifest: &ManifestFile, sway_git_tag: &str, ) -> BTreeSet { - // If we we don't have a project node, remove everything as we can't validate dependencies + // If we don't have a project node, remove everything as we can't validate dependencies // without knowing where to start. let proj_node = match find_proj_node(graph, &proj_manifest.project.name) { Ok(node) => node,