From 0dcae3d100ac7620c266399b4db228944ceb908c Mon Sep 17 00:00:00 2001 From: mitchmindtree Date: Thu, 9 Mar 2023 01:45:12 +1000 Subject: [PATCH] fix(forc-pkg): Unify member source types, fix manifest entries, fix #4235 (#4237) ## Description Fixes an issue where member source types unintentionally omitted entries from the manifest map during the fetch traversal after the source refactor in #4168. Tested locally with the `fuels-rs` test suite using the steps described in #4235. Closes #4235. This patch will likely also apply to other large workspaces too, we might want to consider cutting a patch release soon after landing. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --- forc-pkg/src/pkg.rs | 21 ++++++---------- forc-pkg/src/source/member.rs | 47 +++++++++++++++++++++++++++++++++++ forc-pkg/src/source/mod.rs | 32 ++++++++++++++++-------- 3 files changed, 77 insertions(+), 23 deletions(-) create mode 100644 forc-pkg/src/source/member.rs diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 163b677b293..dd5b85f8b9e 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -679,7 +679,7 @@ impl BuildPlan { self.compilation_order() .iter() .cloned() - .filter(|&n| self.graph[n].source == source::Pinned::Member) + .filter(|&n| self.graph[n].source == source::Pinned::MEMBER) } /// Produce an iterator yielding all workspace member pinned pkgs in order of compilation. @@ -806,7 +806,7 @@ fn validate_pkg_version(pkg_manifest: &PackageManifestFile) -> Result<()> { fn member_nodes(g: &Graph) -> impl Iterator + '_ { g.node_indices() - .filter(|&n| g[n].source == source::Pinned::Member) + .filter(|&n| g[n].source == source::Pinned::MEMBER) } /// Validates the state of the pinned package graph against the given ManifestFile. @@ -1041,12 +1041,7 @@ impl Pinned { /// Retrieve the unpinned version of this source. pub fn unpinned(&self, path: &Path) -> Pkg { - let source = match &self.source { - source::Pinned::Member => Source::Member(path.to_owned()), - source::Pinned::Git(git) => Source::Git(git.source.clone()), - source::Pinned::Path(_) => Source::Path(path.to_owned()), - source::Pinned::Registry(reg) => Source::Registry(reg.source.clone()), - }; + let source = self.source.unpinned(path); let name = self.name.clone(); Pkg { name, source } } @@ -1206,8 +1201,8 @@ fn validate_path_root(graph: &Graph, path_dep: NodeIx, path_root: PinnedId) -> R fn find_path_root(graph: &Graph, mut node: NodeIx) -> Result { loop { let pkg = &graph[node]; - match &pkg.source { - source::Pinned::Path(src) => { + match pkg.source { + source::Pinned::Path(ref src) => { let parent = graph .edges_directed(node, Direction::Incoming) .next() @@ -1220,7 +1215,7 @@ fn find_path_root(graph: &Graph, mut node: NodeIx) -> Result { })?; node = parent; } - source::Pinned::Git(_) | source::Pinned::Registry(_) | source::Pinned::Member => { + source::Pinned::Git(_) | source::Pinned::Registry(_) | source::Pinned::Member(_) => { return Ok(node); } } @@ -1279,7 +1274,7 @@ fn fetch_pkg_graph( Ok(proj_node) => proj_node, Err(_) => { let name = proj_manifest.project.name.clone(); - let source = source::Pinned::Member; + let source = source::Pinned::MEMBER; let pkg = Pinned { name, source }; let pkg_id = pkg.id(); manifest_map.insert(pkg_id, proj_manifest.clone()); @@ -1398,7 +1393,7 @@ fn fetch_deps( })?; let path_root = match dep_pinned.source { - source::Pinned::Member | source::Pinned::Git(_) | source::Pinned::Registry(_) => { + source::Pinned::Member(_) | source::Pinned::Git(_) | source::Pinned::Registry(_) => { dep_pkg_id } source::Pinned::Path(_) => path_root, diff --git a/forc-pkg/src/source/member.rs b/forc-pkg/src/source/member.rs new file mode 100644 index 00000000000..11664f80296 --- /dev/null +++ b/forc-pkg/src/source/member.rs @@ -0,0 +1,47 @@ +use crate::{manifest::PackageManifestFile, source}; +use serde::{Deserialize, Serialize}; +use std::{ + fmt, + path::{Path, PathBuf}, +}; + +/// Member source representation as a canonical path. +#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] +pub struct Source(pub(super) PathBuf); + +/// A pinned instance of a member source requires no information as it's a part +/// of the workspace. +#[derive(Clone, Debug, Eq, Hash, PartialEq, Deserialize, Serialize)] +pub struct Pinned; + +impl fmt::Display for Pinned { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "member") + } +} + +impl source::Pin for Source { + type Pinned = Pinned; + fn pin(&self, _ctx: source::PinCtx) -> anyhow::Result<(Self::Pinned, PathBuf)> { + Ok((Pinned, self.0.clone())) + } +} + +impl source::Fetch for Pinned { + fn fetch(&self, _ctx: source::PinCtx, local: &Path) -> anyhow::Result { + let manifest = PackageManifestFile::from_dir(local)?; + Ok(manifest) + } +} + +impl source::DepPath for Pinned { + fn dep_path(&self, _name: &str) -> anyhow::Result { + Ok(source::DependencyPath::Member) + } +} + +impl From for source::Pinned { + fn from(p: Pinned) -> Self { + Self::Member(p) + } +} diff --git a/forc-pkg/src/source/mod.rs b/forc-pkg/src/source/mod.rs index fd4ea344d78..33e94a1b1eb 100644 --- a/forc-pkg/src/source/mod.rs +++ b/forc-pkg/src/source/mod.rs @@ -8,6 +8,7 @@ //! 4. Add variant support to the `from_manifest_dep` and `FromStr` implementations. pub mod git; +mod member; pub mod path; mod reg; @@ -54,7 +55,7 @@ type FetchId = u64; #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] pub enum Source { /// Used to refer to a workspace member project. - Member(PathBuf), + Member(member::Source), /// A git repo with a `Forc.toml` manifest at its root. Git(git::Source), /// A path to a directory with a `Forc.toml` manifest at its root. @@ -69,7 +70,7 @@ pub enum Source { /// pinned version or commit is updated upon creation of the lock file and on `forc update`. #[derive(Clone, Debug, Eq, Hash, PartialEq, Deserialize, Serialize)] pub enum Pinned { - Member, + Member(member::Pinned), Git(git::Pinned), Path(path::Pinned), Registry(reg::Pinned), @@ -135,7 +136,7 @@ impl Source { .values() .any(|pkg_manifest| pkg_manifest.dir() == canonical_path) { - Source::Member(canonical_path) + Source::Member(member::Source(canonical_path)) } else { Source::Path(canonical_path) } @@ -211,7 +212,7 @@ impl Source { /// Attempt to determine the pinned version or commit for the source. /// - /// Also updates the `path_map` with a path to the local copy of the source. + /// Also updates the manifest map with a path to the local copy of the pkg. /// /// The `path_root` is required for `Path` dependencies and must specify the package that is the /// root of the current subgraph of path dependencies. @@ -223,7 +224,6 @@ impl Source { Pinned: From, { let (pinned, fetch_path) = source.pin(ctx.clone())?; - // TODO: Could potentially omit `name`? Breaks all existing locks though... let id = PinnedId::new(ctx.name(), &Pinned::from(pinned.clone())); if let hash_map::Entry::Vacant(entry) = manifests.entry(id) { entry.insert(pinned.fetch(ctx, &fetch_path)?); @@ -231,7 +231,7 @@ impl Source { Ok(pinned) } match self { - Source::Member(_path) => Ok(Pinned::Member), + Source::Member(source) => Ok(Pinned::Member(f(source, ctx, manifests)?)), Source::Path(source) => Ok(Pinned::Path(f(source, ctx, manifests)?)), Source::Git(source) => Ok(Pinned::Git(f(source, ctx, manifests)?)), Source::Registry(source) => Ok(Pinned::Registry(f(source, ctx, manifests)?)), @@ -240,10 +240,12 @@ impl Source { } impl Pinned { + pub(crate) const MEMBER: Self = Self::Member(member::Pinned); + /// Return how the pinned source for a dependency can be found on the local file system. pub(crate) fn dep_path(&self, name: &str) -> Result { match self { - Self::Member => Ok(DependencyPath::Member), + Self::Member(pinned) => pinned.dep_path(name), Self::Path(pinned) => pinned.dep_path(name), Self::Git(pinned) => pinned.dep_path(name), Self::Registry(pinned) => pinned.dep_path(name), @@ -272,6 +274,16 @@ impl Pinned { manifest_dir, } } + + /// Retrieve the unpinned instance of this source. + pub fn unpinned(&self, path: &Path) -> Source { + match self { + Self::Member(_) => Source::Member(member::Source(path.to_owned())), + Self::Git(git) => Source::Git(git.source.clone()), + Self::Path(_) => Source::Path(path.to_owned()), + Self::Registry(reg) => Source::Registry(reg.source.clone()), + } + } } impl<'a> PinCtx<'a> { @@ -292,7 +304,7 @@ impl<'a> PinCtx<'a> { impl fmt::Display for Pinned { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::Member => write!(f, "member"), + Self::Member(src) => src.fmt(f), Self::Path(src) => src.fmt(f), Self::Git(src) => src.fmt(f), Self::Registry(_reg) => todo!("pkg registries not yet implemented"), @@ -303,7 +315,7 @@ impl fmt::Display for Pinned { impl<'a> fmt::Display for DisplayCompiling<'a, Pinned> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self.source { - Pinned::Member => self.manifest_dir.display().fmt(f), + Pinned::Member(_) => self.manifest_dir.display().fmt(f), Pinned::Path(_src) => self.manifest_dir.display().fmt(f), Pinned::Git(src) => src.fmt(f), Pinned::Registry(_src) => todo!("registry dependencies not yet implemented"), @@ -317,7 +329,7 @@ impl FromStr for Pinned { // Also check `"root"` to support reading the legacy `Forc.lock` format and to // avoid breaking old projects. let source = if s == "root" || s == "member" { - Self::Member + Self::Member(member::Pinned) } else if let Ok(src) = path::Pinned::from_str(s) { Self::Path(src) } else if let Ok(src) = git::Pinned::from_str(s) {