From 68675c40a9f564ecbe31d112410739924844f393 Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Wed, 15 Feb 2023 16:00:12 +0300 Subject: [PATCH 1/9] feat: workspace level patch table added --- forc-pkg/src/manifest.rs | 53 +++++++++++++++++++++++++++++++++----- forc-pkg/src/source/mod.rs | 3 ++- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index 90ed23637fe..5eca85e6ff7 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -240,6 +240,35 @@ impl PackageManifestFile { Ok(Self { manifest, path }) } + /// Apply patch declarations from the workspace (which declares this package manifest as a + /// member) to this package manifest. + /// + /// This checks whether there is such workspace present before attempting to apply patches so + /// it is safe to use with standalone package manifests as well. + pub fn with_workspace_patches(&self) -> Self { + let mut workspace_patches = self + .workspace() + .ok() + .flatten() + .and_then(|workspace| workspace.patch.clone()); + let mut package_patches = self.patch.clone().unwrap_or_default(); + if let Some(workspace_patches) = &mut workspace_patches { + package_patches.append(workspace_patches); + } + let updated_patches = if package_patches.is_empty() { + None + } else { + Some(package_patches) + }; + Self { + manifest: PackageManifest { + patch: updated_patches, + ..self.manifest.clone() + }, + ..self.clone() + } + } + /// Read the manifest from the `Forc.toml` in the directory specified by the given `path` or /// any of its parent directories. /// @@ -364,6 +393,7 @@ impl PackageManifestFile { if e.to_string().contains("could not find") { return Ok(None); } else { + println!("{e:?}"); return Err(e); } } @@ -486,6 +516,13 @@ impl PackageManifest { .flat_map(|patches| patches.iter()) } + /// Retrieve the listed patches for the given name. + pub fn patch(&self, patch_name: &str) -> Option<&PatchMap> { + self.patch + .as_ref() + .and_then(|patches| patches.get(patch_name)) + } + /// Check for the `core` and `std` packages under `[dependencies]`. If both are missing, add /// `std` implicitly. /// @@ -545,13 +582,6 @@ impl PackageManifest { }) } - /// Retrieve the listed patches for the given name. - pub fn patch(&self, patch_name: &str) -> Option<&PatchMap> { - self.patch - .as_ref() - .and_then(|patches| patches.get(patch_name)) - } - /// Retrieve a reference to the contract dependency with the given name. pub fn contract_dep(&self, contract_dep_name: &str) -> Option<&ContractDependency> { self.contract_dependencies @@ -694,6 +724,7 @@ pub struct WorkspaceManifestFile { #[serde(rename_all = "kebab-case")] pub struct WorkspaceManifest { workspace: Workspace, + patch: Option>, } #[derive(Serialize, Deserialize, Clone, Debug)] @@ -798,6 +829,14 @@ impl WorkspaceManifestFile { pub fn lock_path(&self) -> PathBuf { self.dir().to_path_buf().join(constants::LOCK_FILE_NAME) } + + /// Produce an iterator yielding all listed patches. + pub fn patches(&self) -> impl Iterator { + self.patch + .as_ref() + .into_iter() + .flat_map(|patches| patches.iter()) + } } impl WorkspaceManifest { diff --git a/forc-pkg/src/source/mod.rs b/forc-pkg/src/source/mod.rs index 41a284c0071..50509e30363 100644 --- a/forc-pkg/src/source/mod.rs +++ b/forc-pkg/src/source/mod.rs @@ -204,7 +204,8 @@ impl Source { manifest: &PackageManifestFile, members: &MemberManifestFiles, ) -> Result { - match self.dep_patch(dep_name, manifest) { + let manifest = PackageManifestFile::with_workspace_patches(manifest); + match self.dep_patch(dep_name, &manifest) { Some(patch) => Self::from_manifest_dep(manifest.dir(), patch, members), None => Ok(self.clone()), } From ea9bb666aa7b03719a9eb69d528445c7e3c0e519 Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Thu, 16 Feb 2023 14:23:37 +0300 Subject: [PATCH 2/9] docs: added docs to workspace reference --- docs/book/src/forc/workspaces.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/book/src/forc/workspaces.md b/docs/book/src/forc/workspaces.md index 4cdf5b6d4a2..230cfda762c 100644 --- a/docs/book/src/forc/workspaces.md +++ b/docs/book/src/forc/workspaces.md @@ -10,6 +10,7 @@ The key points for workspaces are: Workspace manifests are declared within `Forc.toml` files and support the following fields: * [`members`](#the-members-field) - Packages to include in the workspace. +* [`[patch]`](#the-patch-section) - Defines the patches. An empty workspace can be created with `forc new --workspace` or `forc init --workspace`. @@ -25,6 +26,23 @@ members = ["member1", "path/to/member2"] The `members` field accepts entries to be given in relative path with respect to the workspace root. Packages that are located within a workspace directory but are *not* contained within the `members` set are ignored. +## The `[patch]` section + +The `[patch]` section can be used to override any dependency in the workspace dependency graph. The usage is the same with package level `[patch]` section and details can be seen [here](./manifest_reference.md#the-patch-section). + +Example: + +```toml +[workspace] +members = ["member1", "path/to/member2"] + + +[patch.'https://github.com/fuellabs/sway'] +std = { git = "https://github.com/fuellabs/sway", branch = "test" } +``` + +In the above example each occurance of `std` as a dependency in the workspace will be changed with `std` from `test` branch of sway repo. + ## Some `forc` commands that support workspaces * `forc build` - Builds an entire workspace. From 9dfb3fd600a1f2f1a5e9d7f2933d8ef21929367f Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Thu, 16 Feb 2023 16:12:56 +0300 Subject: [PATCH 3/9] update docs to mention conflicting patch declarations between workspace and package --- docs/book/src/forc/workspaces.md | 4 ++++ forc-pkg/src/manifest.rs | 3 +++ 2 files changed, 7 insertions(+) diff --git a/docs/book/src/forc/workspaces.md b/docs/book/src/forc/workspaces.md index 230cfda762c..fd7441f8cb3 100644 --- a/docs/book/src/forc/workspaces.md +++ b/docs/book/src/forc/workspaces.md @@ -30,6 +30,10 @@ Packages that are located within a workspace directory but are *not* contained w The `[patch]` section can be used to override any dependency in the workspace dependency graph. The usage is the same with package level `[patch]` section and details can be seen [here](./manifest_reference.md#the-patch-section). +If a member of a workspace has a patch table in its package manifest, the workspace level patch declarations are merged with package level declarations. If there are conflicting declarations (patching the same source with different target) between workspace level patch table and a member's patch table, workspace level patch declarations. + +> **NOTE:** Workspace level patch declarations only apply to members declared in `members` field. + Example: ```toml diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index 5eca85e6ff7..998f8d1b95c 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -243,6 +243,9 @@ impl PackageManifestFile { /// Apply patch declarations from the workspace (which declares this package manifest as a /// member) to this package manifest. /// + /// If there are conflicting patch declarations from `PackageManifestFile` and + /// `WorkspaceManifestFile`, the priority is given to workspace level declarations. + /// /// This checks whether there is such workspace present before attempting to apply patches so /// it is safe to use with standalone package manifests as well. pub fn with_workspace_patches(&self) -> Self { From a59b216155050a32a53dd935c20f4b64cfac4ed0 Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Thu, 16 Feb 2023 16:13:41 +0300 Subject: [PATCH 4/9] remove debug probe --- forc-pkg/src/manifest.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index 998f8d1b95c..32041908ca9 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -396,7 +396,6 @@ impl PackageManifestFile { if e.to_string().contains("could not find") { return Ok(None); } else { - println!("{e:?}"); return Err(e); } } From 259e2d52acaf0ec06090f6a23017f384979a2b69 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 6 Apr 2023 15:08:37 -0400 Subject: [PATCH 5/9] disallow conflicting patch tables --- forc-pkg/src/manifest.rs | 28 +++++++++++++--------------- forc-pkg/src/source/mod.rs | 2 +- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index 32041908ca9..0fda65750b8 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -244,32 +244,30 @@ impl PackageManifestFile { /// member) to this package manifest. /// /// If there are conflicting patch declarations from `PackageManifestFile` and - /// `WorkspaceManifestFile`, the priority is given to workspace level declarations. + /// `WorkspaceManifestFile`, returns an error. /// /// This checks whether there is such workspace present before attempting to apply patches so /// it is safe to use with standalone package manifests as well. - pub fn with_workspace_patches(&self) -> Self { - let mut workspace_patches = self + pub fn with_workspace_patches(&self) -> Result { + let workspace_patches = self .workspace() .ok() .flatten() .and_then(|workspace| workspace.patch.clone()); - let mut package_patches = self.patch.clone().unwrap_or_default(); - if let Some(workspace_patches) = &mut workspace_patches { - package_patches.append(workspace_patches); - } - let updated_patches = if package_patches.is_empty() { - None - } else { - Some(package_patches) - }; - Self { + let package_patches = self.patch.clone(); + let patch = match (workspace_patches, package_patches) { + (Some(_), Some(_)) => bail!("Found [patch] table both in workspace and member package's manifest file. Consider removing [patch] table from package's manifest file."), + (Some(workspace_patches), None) => anyhow::Ok(Some(workspace_patches)), + (None, Some(pkg_patches)) => anyhow::Ok(Some(pkg_patches)), + (None, None) => Ok(None), + }?; + Ok(Self { manifest: PackageManifest { - patch: updated_patches, + patch, ..self.manifest.clone() }, ..self.clone() - } + }) } /// Read the manifest from the `Forc.toml` in the directory specified by the given `path` or diff --git a/forc-pkg/src/source/mod.rs b/forc-pkg/src/source/mod.rs index 50509e30363..8c5c1b52786 100644 --- a/forc-pkg/src/source/mod.rs +++ b/forc-pkg/src/source/mod.rs @@ -204,7 +204,7 @@ impl Source { manifest: &PackageManifestFile, members: &MemberManifestFiles, ) -> Result { - let manifest = PackageManifestFile::with_workspace_patches(manifest); + let manifest = PackageManifestFile::with_workspace_patches(manifest)?; match self.dep_patch(dep_name, &manifest) { Some(patch) => Self::from_manifest_dep(manifest.dir(), patch, members), None => Ok(self.clone()), From 4b3278b7eb0cff87e794bf6150fce2070ed94a45 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Sun, 9 Apr 2023 22:24:49 -0400 Subject: [PATCH 6/9] doc: update docs with manifest package patch conflicts --- docs/book/src/forc/workspaces.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/book/src/forc/workspaces.md b/docs/book/src/forc/workspaces.md index fd7441f8cb3..7f4ab6d608a 100644 --- a/docs/book/src/forc/workspaces.md +++ b/docs/book/src/forc/workspaces.md @@ -30,9 +30,7 @@ Packages that are located within a workspace directory but are *not* contained w The `[patch]` section can be used to override any dependency in the workspace dependency graph. The usage is the same with package level `[patch]` section and details can be seen [here](./manifest_reference.md#the-patch-section). -If a member of a workspace has a patch table in its package manifest, the workspace level patch declarations are merged with package level declarations. If there are conflicting declarations (patching the same source with different target) between workspace level patch table and a member's patch table, workspace level patch declarations. - -> **NOTE:** Workspace level patch declarations only apply to members declared in `members` field. +It is not allowed to declare patch table in member of a workspace if the workspace manifest file contains a patch table. Example: From f0bcfde88a871774fbd988c9102af8e0998af062 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kaya=20G=C3=B6kalp?= Date: Tue, 11 Apr 2023 23:03:22 +0300 Subject: [PATCH 7/9] remove unnecessary `Ok`s Co-authored-by: bing --- forc-pkg/src/manifest.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index 0fda65750b8..f795bc2a192 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -257,10 +257,10 @@ impl PackageManifestFile { let package_patches = self.patch.clone(); let patch = match (workspace_patches, package_patches) { (Some(_), Some(_)) => bail!("Found [patch] table both in workspace and member package's manifest file. Consider removing [patch] table from package's manifest file."), - (Some(workspace_patches), None) => anyhow::Ok(Some(workspace_patches)), - (None, Some(pkg_patches)) => anyhow::Ok(Some(pkg_patches)), - (None, None) => Ok(None), - }?; + (Some(workspace_patches), None) => Some(workspace_patches), + (None, Some(pkg_patches)) => Some(pkg_patches), + (None, None) => None, + }; Ok(Self { manifest: PackageManifest { patch, From 808510a15bae84f4988575d7892d0ac4b71088d6 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Thu, 13 Apr 2023 23:57:51 +0300 Subject: [PATCH 8/9] refactor: introduce resolve_patch and resolve_patches --- forc-pkg/src/manifest.rs | 42 ++++++++++++++++++++------------------ forc-pkg/src/source/mod.rs | 17 ++++++++------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/forc-pkg/src/manifest.rs b/forc-pkg/src/manifest.rs index f795bc2a192..fc32fbdd6f0 100644 --- a/forc-pkg/src/manifest.rs +++ b/forc-pkg/src/manifest.rs @@ -240,34 +240,36 @@ impl PackageManifestFile { Ok(Self { manifest, path }) } - /// Apply patch declarations from the workspace (which declares this package manifest as a - /// member) to this package manifest. + /// Returns an iterator over patches defined in underlying `PackageManifest` if this is a + /// standalone package. /// - /// If there are conflicting patch declarations from `PackageManifestFile` and - /// `WorkspaceManifestFile`, returns an error. - /// - /// This checks whether there is such workspace present before attempting to apply patches so - /// it is safe to use with standalone package manifests as well. - pub fn with_workspace_patches(&self) -> Result { + /// If this package is a member of a workspace, patches are fetched from + /// the workspace manifest file. + pub fn resolve_patches(&self) -> Result> { let workspace_patches = self .workspace() .ok() .flatten() .and_then(|workspace| workspace.patch.clone()); let package_patches = self.patch.clone(); - let patch = match (workspace_patches, package_patches) { + match (workspace_patches, package_patches) { (Some(_), Some(_)) => bail!("Found [patch] table both in workspace and member package's manifest file. Consider removing [patch] table from package's manifest file."), - (Some(workspace_patches), None) => Some(workspace_patches), - (None, Some(pkg_patches)) => Some(pkg_patches), - (None, None) => None, - }; - Ok(Self { - manifest: PackageManifest { - patch, - ..self.manifest.clone() - }, - ..self.clone() - }) + (Some(workspace_patches), None) => Ok(workspace_patches.into_iter()), + (None, Some(pkg_patches)) => Ok(pkg_patches.into_iter()), + (None, None) => Ok(BTreeMap::default().into_iter()), + } + } + + /// Retrieve the listed patches for the given name from underlying `PackageManifest` if this is + /// a standalone package. + /// + /// If this package is a member of a workspace, patch is fetched from + /// the workspace manifest file. + pub fn resolve_patch(&self, patch_name: &str) -> Result> { + Ok(self + .resolve_patches()? + .find(|(p_name, _)| patch_name == p_name.as_str()) + .map(|(_, patch)| patch)) } /// Read the manifest from the `Forc.toml` in the directory specified by the given `path` or diff --git a/forc-pkg/src/source/mod.rs b/forc-pkg/src/source/mod.rs index 8c5c1b52786..6214b094b3b 100644 --- a/forc-pkg/src/source/mod.rs +++ b/forc-pkg/src/source/mod.rs @@ -179,19 +179,19 @@ impl Source { /// If a patch exists for this dependency source within the given project /// manifest, this returns the patch. - fn dep_patch<'manifest>( + fn dep_patch( &self, dep_name: &str, - manifest: &'manifest PackageManifestFile, - ) -> Option<&'manifest manifest::Dependency> { + manifest: &PackageManifestFile, + ) -> Result> { if let Source::Git(git) = self { - if let Some(patches) = manifest.patch(&git.repo.to_string()) { + if let Some(patches) = manifest.resolve_patch(&git.repo.to_string())? { if let Some(patch) = patches.get(dep_name) { - return Some(patch); + return Ok(Some(patch.clone())); } } } - None + Ok(None) } /// If a patch exists for the dependency associated with this source within @@ -204,9 +204,8 @@ impl Source { manifest: &PackageManifestFile, members: &MemberManifestFiles, ) -> Result { - let manifest = PackageManifestFile::with_workspace_patches(manifest)?; - match self.dep_patch(dep_name, &manifest) { - Some(patch) => Self::from_manifest_dep(manifest.dir(), patch, members), + match self.dep_patch(dep_name, &manifest)? { + Some(patch) => Self::from_manifest_dep(manifest.dir(), &patch, members), None => Ok(self.clone()), } } From 6d6f5ed66118afe70363b0bf593c21fbb78b88d5 Mon Sep 17 00:00:00 2001 From: kayagokalp Date: Mon, 17 Apr 2023 13:53:37 +0300 Subject: [PATCH 9/9] chore: clippy --- forc-pkg/src/source/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forc-pkg/src/source/mod.rs b/forc-pkg/src/source/mod.rs index 6214b094b3b..1b0b76f7c86 100644 --- a/forc-pkg/src/source/mod.rs +++ b/forc-pkg/src/source/mod.rs @@ -204,7 +204,7 @@ impl Source { manifest: &PackageManifestFile, members: &MemberManifestFiles, ) -> Result { - match self.dep_patch(dep_name, &manifest)? { + match self.dep_patch(dep_name, manifest)? { Some(patch) => Self::from_manifest_dep(manifest.dir(), &patch, members), None => Ok(self.clone()), }