From 39f742e9fcd57a2aee4a0050a54728ee7ccd2deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B3man=20Joost?= Date: Fri, 1 Nov 2024 16:10:50 +1000 Subject: [PATCH 1/3] fix: honour handleTransparentWorkspaces setting in Yarn/Berry --- .../src/package_graph/builder.rs | 27 ++++++-- .../src/package_graph/dep_splitter.rs | 4 +- .../src/package_graph/mod.rs | 1 + .../src/package_graph/yarnrc.rs | 63 +++++++++++++++++++ 4 files changed, 90 insertions(+), 5 deletions(-) create mode 100644 crates/turborepo-repository/src/package_graph/yarnrc.rs diff --git a/crates/turborepo-repository/src/package_graph/builder.rs b/crates/turborepo-repository/src/package_graph/builder.rs index 1075189b75171..9c25470fca39b 100644 --- a/crates/turborepo-repository/src/package_graph/builder.rs +++ b/crates/turborepo-repository/src/package_graph/builder.rs @@ -10,8 +10,8 @@ use turborepo_graph_utils as graph; use turborepo_lockfiles::Lockfile; use super::{ - dep_splitter::DependencySplitter, npmrc::NpmRc, PackageGraph, PackageInfo, PackageName, - PackageNode, + dep_splitter::DependencySplitter, npmrc::NpmRc, yarnrc::YarnRc, PackageGraph, PackageInfo, + PackageName, PackageNode, }; use crate::{ discovery::{ @@ -362,6 +362,17 @@ impl<'a, T: PackageDiscovery> BuildState<'a, ResolvedWorkspaces, T> { } _ => None, }; + let yarnrc_path = self.repo_root.join_component(".yarnrc.yml"); + let yarnrc = match package_manager { + PackageManager::Berry => { + // HOME? + match yarnrc_path.read_existing_to_string().ok().flatten() { + Some(contents) => YarnRc::from_reader(contents.as_bytes()).ok(), + None => None, + } + } + _ => None, + }; let split_deps = self .workspaces .iter() @@ -375,6 +386,7 @@ impl<'a, T: PackageDiscovery> BuildState<'a, ResolvedWorkspaces, T> { &self.workspaces, package_manager, npmrc.as_ref(), + yarnrc.as_ref(), entry.package_json.all_dependencies(), ), ) @@ -567,6 +579,7 @@ impl Dependencies { workspaces: &HashMap, package_manager: &PackageManager, npmrc: Option<&NpmRc>, + yarnrc: Option<&YarnRc>, dependencies: I, ) -> Self { let resolved_workspace_json_path = repo_root.resolve(workspace_json_path); @@ -575,8 +588,14 @@ impl Dependencies { .expect("package.json path should have parent"); let mut internal = HashSet::new(); let mut external = BTreeMap::new(); - let splitter = - DependencySplitter::new(repo_root, workspace_dir, workspaces, package_manager, npmrc); + let splitter = DependencySplitter::new( + repo_root, + workspace_dir, + workspaces, + package_manager, + npmrc, + yarnrc, + ); for (name, version) in dependencies.into_iter() { if let Some(workspace) = splitter.is_internal(name, version) { internal.insert(workspace); diff --git a/crates/turborepo-repository/src/package_graph/dep_splitter.rs b/crates/turborepo-repository/src/package_graph/dep_splitter.rs index 1a50cd6d5c35c..dc33a9482e4df 100644 --- a/crates/turborepo-repository/src/package_graph/dep_splitter.rs +++ b/crates/turborepo-repository/src/package_graph/dep_splitter.rs @@ -5,7 +5,7 @@ use turbopath::{ RelativeUnixPathBuf, }; -use super::{npmrc::NpmRc, PackageInfo, PackageName}; +use super::{npmrc::NpmRc, yarnrc::YarnRc, PackageInfo, PackageName}; use crate::package_manager::PackageManager; pub struct DependencySplitter<'a> { @@ -22,6 +22,7 @@ impl<'a> DependencySplitter<'a> { workspaces: &'a HashMap, package_manager: &PackageManager, npmrc: Option<&'a NpmRc>, + yarnrc: Option<&'a YarnRc>, ) -> Self { Self { repo_root, @@ -29,6 +30,7 @@ impl<'a> DependencySplitter<'a> { workspaces, link_workspace_packages: npmrc .and_then(|npmrc| npmrc.link_workspace_packages) + .or_else(|| yarnrc.map(|yarnrc| yarnrc.enableTransparentWorkspaces)) .unwrap_or(!matches!(package_manager, PackageManager::Pnpm9)), } } diff --git a/crates/turborepo-repository/src/package_graph/mod.rs b/crates/turborepo-repository/src/package_graph/mod.rs index 988c8850cad0f..2efe956db5712 100644 --- a/crates/turborepo-repository/src/package_graph/mod.rs +++ b/crates/turborepo-repository/src/package_graph/mod.rs @@ -20,6 +20,7 @@ use crate::{ pub mod builder; mod dep_splitter; mod npmrc; +mod yarnrc; pub use builder::{Error, PackageGraphBuilder}; diff --git a/crates/turborepo-repository/src/package_graph/yarnrc.rs b/crates/turborepo-repository/src/package_graph/yarnrc.rs new file mode 100644 index 0000000000000..61db22478f9fd --- /dev/null +++ b/crates/turborepo-repository/src/package_graph/yarnrc.rs @@ -0,0 +1,63 @@ +use std::io; + +use serde::Deserialize; +use serde_yaml; + +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("encountered error parsing yarnrc.yml: {0}")] + SerdeYaml(#[from] serde_yaml::Error), +} + +/// A yarnrc.yaml file representing settings affecting the package graph. +#[allow(non_snake_case)] +#[derive(Debug, PartialEq, Eq, Clone, Deserialize)] +pub struct YarnRc { + /// Used by Yarn(Berry) as `enableTransparentWorkspaces`. + /// When true, treats local workspaces that match a package name + /// and semver range as correct match resulting in turbo including + /// the package in the dependency graph + pub enableTransparentWorkspaces: bool, +} + +impl Default for YarnRc { + fn default() -> YarnRc { + YarnRc { + enableTransparentWorkspaces: true, + } + } +} + +impl YarnRc { + pub fn from_reader(mut reader: impl io::Read) -> Result { + let config: YarnRc = serde_yaml::from_reader(&mut reader)?; + Ok(config) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_parse_empty_yarnrc() { + let empty = YarnRc::from_reader(b"".as_slice()).unwrap(); + assert_eq!( + empty, + YarnRc { + enableTransparentWorkspaces: true + } + ); + } + + #[test] + fn test_parses_transparent_workspaces() { + let empty = YarnRc::from_reader(b"enableTransparentWorkspaces: false".as_slice()).unwrap(); + assert_eq!( + empty, + YarnRc { + enableTransparentWorkspaces: false + } + ); + } +} From 673efc16445bbbe273c1d814cd770a2698116bd7 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 18 Dec 2024 09:43:15 -0500 Subject: [PATCH 2/3] chore(berry): small serde fixes --- .../src/package_graph/dep_splitter.rs | 2 +- .../src/package_graph/yarnrc.rs | 26 +++++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/crates/turborepo-repository/src/package_graph/dep_splitter.rs b/crates/turborepo-repository/src/package_graph/dep_splitter.rs index dc33a9482e4df..f900e134107ed 100644 --- a/crates/turborepo-repository/src/package_graph/dep_splitter.rs +++ b/crates/turborepo-repository/src/package_graph/dep_splitter.rs @@ -30,7 +30,7 @@ impl<'a> DependencySplitter<'a> { workspaces, link_workspace_packages: npmrc .and_then(|npmrc| npmrc.link_workspace_packages) - .or_else(|| yarnrc.map(|yarnrc| yarnrc.enableTransparentWorkspaces)) + .or_else(|| yarnrc.map(|yarnrc| yarnrc.enable_transparent_workspaces)) .unwrap_or(!matches!(package_manager, PackageManager::Pnpm9)), } } diff --git a/crates/turborepo-repository/src/package_graph/yarnrc.rs b/crates/turborepo-repository/src/package_graph/yarnrc.rs index 61db22478f9fd..b1b81664d8369 100644 --- a/crates/turborepo-repository/src/package_graph/yarnrc.rs +++ b/crates/turborepo-repository/src/package_graph/yarnrc.rs @@ -10,20 +10,25 @@ pub enum Error { } /// A yarnrc.yaml file representing settings affecting the package graph. -#[allow(non_snake_case)] #[derive(Debug, PartialEq, Eq, Clone, Deserialize)] +#[serde(rename_all = "camelCase")] pub struct YarnRc { /// Used by Yarn(Berry) as `enableTransparentWorkspaces`. /// When true, treats local workspaces that match a package name /// and semver range as correct match resulting in turbo including /// the package in the dependency graph - pub enableTransparentWorkspaces: bool, + #[serde(default = "default_enable_transparent_workspaces")] + pub enable_transparent_workspaces: bool, +} + +fn default_enable_transparent_workspaces() -> bool { + true } impl Default for YarnRc { fn default() -> YarnRc { YarnRc { - enableTransparentWorkspaces: true, + enable_transparent_workspaces: default_enable_transparent_workspaces(), } } } @@ -45,7 +50,7 @@ mod test { assert_eq!( empty, YarnRc { - enableTransparentWorkspaces: true + enable_transparent_workspaces: true } ); } @@ -56,7 +61,18 @@ mod test { assert_eq!( empty, YarnRc { - enableTransparentWorkspaces: false + enable_transparent_workspaces: false + } + ); + } + + #[test] + fn test_parses_additional_settings() { + let empty = YarnRc::from_reader(b"httpProxy: \"http://my-proxy.com\"".as_slice()).unwrap(); + assert_eq!( + empty, + YarnRc { + enable_transparent_workspaces: true } ); } From 00f40b69f9abd16a6c34b52a723b6df6080c3b92 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 18 Dec 2024 11:55:44 -0500 Subject: [PATCH 3/3] chore(package manager): move logic regarding internal dependency selection to pm --- .../src/package_graph/builder.rs | 38 +---------- .../src/package_graph/dep_splitter.rs | 10 +-- .../src/package_graph/mod.rs | 2 - .../src/package_manager/mod.rs | 66 +++++++++++++++++++ .../npmrc.rs | 17 ++++- .../yarnrc.rs | 15 +++++ 6 files changed, 103 insertions(+), 45 deletions(-) rename crates/turborepo-repository/src/{package_graph => package_manager}/npmrc.rs (78%) rename crates/turborepo-repository/src/{package_graph => package_manager}/yarnrc.rs (79%) diff --git a/crates/turborepo-repository/src/package_graph/builder.rs b/crates/turborepo-repository/src/package_graph/builder.rs index 9c25470fca39b..ec6f2c17b5122 100644 --- a/crates/turborepo-repository/src/package_graph/builder.rs +++ b/crates/turborepo-repository/src/package_graph/builder.rs @@ -10,8 +10,7 @@ use turborepo_graph_utils as graph; use turborepo_lockfiles::Lockfile; use super::{ - dep_splitter::DependencySplitter, npmrc::NpmRc, yarnrc::YarnRc, PackageGraph, PackageInfo, - PackageName, PackageNode, + dep_splitter::DependencySplitter, PackageGraph, PackageInfo, PackageName, PackageNode, }; use crate::{ discovery::{ @@ -352,27 +351,6 @@ impl<'a, T: PackageDiscovery> BuildState<'a, ResolvedWorkspaces, T> { &mut self, package_manager: &PackageManager, ) -> Result<(), Error> { - let npmrc = match package_manager { - PackageManager::Pnpm | PackageManager::Pnpm6 | PackageManager::Pnpm9 => { - let npmrc_path = self.repo_root.join_component(".npmrc"); - match npmrc_path.read_existing_to_string().ok().flatten() { - Some(contents) => NpmRc::from_reader(contents.as_bytes()).ok(), - None => None, - } - } - _ => None, - }; - let yarnrc_path = self.repo_root.join_component(".yarnrc.yml"); - let yarnrc = match package_manager { - PackageManager::Berry => { - // HOME? - match yarnrc_path.read_existing_to_string().ok().flatten() { - Some(contents) => YarnRc::from_reader(contents.as_bytes()).ok(), - None => None, - } - } - _ => None, - }; let split_deps = self .workspaces .iter() @@ -385,8 +363,6 @@ impl<'a, T: PackageDiscovery> BuildState<'a, ResolvedWorkspaces, T> { &entry.package_json_path, &self.workspaces, package_manager, - npmrc.as_ref(), - yarnrc.as_ref(), entry.package_json.all_dependencies(), ), ) @@ -578,8 +554,6 @@ impl Dependencies { workspace_json_path: &AnchoredSystemPathBuf, workspaces: &HashMap, package_manager: &PackageManager, - npmrc: Option<&NpmRc>, - yarnrc: Option<&YarnRc>, dependencies: I, ) -> Self { let resolved_workspace_json_path = repo_root.resolve(workspace_json_path); @@ -588,14 +562,8 @@ impl Dependencies { .expect("package.json path should have parent"); let mut internal = HashSet::new(); let mut external = BTreeMap::new(); - let splitter = DependencySplitter::new( - repo_root, - workspace_dir, - workspaces, - package_manager, - npmrc, - yarnrc, - ); + let splitter = + DependencySplitter::new(repo_root, workspace_dir, workspaces, package_manager); for (name, version) in dependencies.into_iter() { if let Some(workspace) = splitter.is_internal(name, version) { internal.insert(workspace); diff --git a/crates/turborepo-repository/src/package_graph/dep_splitter.rs b/crates/turborepo-repository/src/package_graph/dep_splitter.rs index f900e134107ed..615ddaab6daea 100644 --- a/crates/turborepo-repository/src/package_graph/dep_splitter.rs +++ b/crates/turborepo-repository/src/package_graph/dep_splitter.rs @@ -5,7 +5,7 @@ use turbopath::{ RelativeUnixPathBuf, }; -use super::{npmrc::NpmRc, yarnrc::YarnRc, PackageInfo, PackageName}; +use super::{PackageInfo, PackageName}; use crate::package_manager::PackageManager; pub struct DependencySplitter<'a> { @@ -21,17 +21,13 @@ impl<'a> DependencySplitter<'a> { workspace_dir: &'a AbsoluteSystemPath, workspaces: &'a HashMap, package_manager: &PackageManager, - npmrc: Option<&'a NpmRc>, - yarnrc: Option<&'a YarnRc>, ) -> Self { + let link_workspace_packages = package_manager.link_workspace_packages(repo_root); Self { repo_root, workspace_dir, workspaces, - link_workspace_packages: npmrc - .and_then(|npmrc| npmrc.link_workspace_packages) - .or_else(|| yarnrc.map(|yarnrc| yarnrc.enable_transparent_workspaces)) - .unwrap_or(!matches!(package_manager, PackageManager::Pnpm9)), + link_workspace_packages, } } diff --git a/crates/turborepo-repository/src/package_graph/mod.rs b/crates/turborepo-repository/src/package_graph/mod.rs index 2efe956db5712..1d9063e754211 100644 --- a/crates/turborepo-repository/src/package_graph/mod.rs +++ b/crates/turborepo-repository/src/package_graph/mod.rs @@ -19,8 +19,6 @@ use crate::{ pub mod builder; mod dep_splitter; -mod npmrc; -mod yarnrc; pub use builder::{Error, PackageGraphBuilder}; diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index eaaa63947704c..07a19a0bdc7fc 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -1,7 +1,9 @@ mod bun; mod npm; +mod npmrc; mod pnpm; mod yarn; +mod yarnrc; use std::{ backtrace, @@ -16,13 +18,16 @@ use lazy_regex::{lazy_regex, Lazy}; use miette::{Diagnostic, NamedSource, SourceSpan}; use node_semver::SemverError; use npm::NpmDetector; +use npmrc::NpmRc; use regex::Regex; use serde::Deserialize; use thiserror::Error; +use tracing::debug; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, RelativeUnixPath}; use turborepo_errors::Spanned; use turborepo_lockfiles::Lockfile; use which::which; +use yarnrc::YarnRc; use crate::{ discovery, @@ -190,6 +195,8 @@ pub enum Error { WorkspaceDiscovery(#[from] discovery::Error), #[error("missing packageManager field in package.json")] MissingPackageManager, + #[error(transparent)] + Yarnrc(#[from] yarnrc::Error), } impl From for Error { @@ -545,6 +552,34 @@ impl PackageManager { PackageManager::Pnpm | PackageManager::Pnpm9 | PackageManager::Berry => None, } } + + /// Returns whether or not the package manager will select a package in the + /// workspace as a dependency if the `workspace:` protocol isn't used. + /// For example if a package in the workspace has `"lib": "1.2.3"` and + /// there's a package in the workspace with the name of `lib` and + /// version `1.2.3` if this is true, then the local `lib` package will + /// be used where `false` would use a `lib` package from the registry. + pub fn link_workspace_packages(&self, repo_root: &AbsoluteSystemPath) -> bool { + match self { + PackageManager::Berry => { + let yarnrc = YarnRc::from_file(repo_root) + .inspect_err(|e| debug!("unable to read yarnrc: {e}")) + .unwrap_or_default(); + yarnrc.enable_transparent_workspaces + } + PackageManager::Pnpm9 | PackageManager::Pnpm | PackageManager::Pnpm6 => { + let npmrc = NpmRc::from_file(repo_root) + .inspect_err(|e| debug!("unable to read npmrc: {e}")) + .unwrap_or_default(); + npmrc + .link_workspace_packages + // The default for pnpm 9 is false if not explicitly set + // All previous versions had a default of true + .unwrap_or(!matches!(self, PackageManager::Pnpm9)) + } + PackageManager::Yarn | PackageManager::Bun | PackageManager::Npm => true, + } + } } #[cfg(test)] @@ -553,6 +588,7 @@ mod tests { use pretty_assertions::assert_eq; use tempfile::TempDir; + use test_case::test_case; use super::*; @@ -823,4 +859,34 @@ mod tests { assert_eq!(nested.workspaces.as_ref(), vec!["packages/**"]); Ok(()) } + + #[test_case(PackageManager::Npm, None, true)] + #[test_case(PackageManager::Yarn, None, true)] + #[test_case(PackageManager::Bun, None, true)] + #[test_case(PackageManager::Pnpm6, None, true)] + #[test_case(PackageManager::Pnpm, None, true)] + #[test_case(PackageManager::Pnpm, Some(false), false)] + #[test_case(PackageManager::Pnpm, Some(true), true)] + #[test_case(PackageManager::Pnpm9, None, false)] + #[test_case(PackageManager::Pnpm9, Some(true), true)] + #[test_case(PackageManager::Pnpm9, Some(false), false)] + #[test_case(PackageManager::Berry, None, true)] + #[test_case(PackageManager::Berry, Some(false), false)] + #[test_case(PackageManager::Berry, Some(true), true)] + fn test_link_workspace_packages(pm: PackageManager, enabled: Option, expected: bool) { + let tmpdir = tempfile::tempdir().unwrap(); + let repo_root = AbsoluteSystemPath::from_std_path(tmpdir.path()).unwrap(); + if let Some(enabled) = enabled { + repo_root + .join_component(npmrc::NPMRC_FILENAME) + .create_with_contents(format!("link-workspace-packages={enabled}")) + .unwrap(); + repo_root + .join_component(yarnrc::YARNRC_FILENAME) + .create_with_contents(format!("enableTransparentWorkspaces: {enabled}")) + .unwrap(); + } + let actual = pm.link_workspace_packages(repo_root); + assert_eq!(actual, expected); + } } diff --git a/crates/turborepo-repository/src/package_graph/npmrc.rs b/crates/turborepo-repository/src/package_manager/npmrc.rs similarity index 78% rename from crates/turborepo-repository/src/package_graph/npmrc.rs rename to crates/turborepo-repository/src/package_manager/npmrc.rs index 8e49118b94b9f..107e743e429f1 100644 --- a/crates/turborepo-repository/src/package_graph/npmrc.rs +++ b/crates/turborepo-repository/src/package_manager/npmrc.rs @@ -1,9 +1,14 @@ use std::io; use ini::Ini; +use turbopath::AbsoluteSystemPath; + +pub const NPMRC_FILENAME: &str = ".npmrc"; #[derive(Debug, thiserror::Error)] pub enum Error { + #[error("encountered error reading .npmrc: {0}")] + Io(#[from] std::io::Error), #[error("encountered error parsing .npmrc: {0}")] Ini(#[from] ini::Error), } @@ -11,7 +16,7 @@ pub enum Error { /// Representation of .npmrc used by both npm and pnpm to configure behavior /// The representation is intentionally incomplete and is only intended to /// contain settings that affect the package graph. -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone, Default)] pub struct NpmRc { /// Used by pnpm to determine whether dependencies /// declared without an explicit workspace protocol @@ -20,6 +25,16 @@ pub struct NpmRc { } impl NpmRc { + pub fn from_file(repo_root: &AbsoluteSystemPath) -> Result { + let npmrc_path = repo_root.join_component(NPMRC_FILENAME); + let npmrc = npmrc_path + .read_existing_to_string()? + .map(|contents| Self::from_reader(contents.as_bytes())) + .transpose()? + .unwrap_or_default(); + Ok(npmrc) + } + pub fn from_reader(mut reader: impl io::Read) -> Result { let ini = Ini::read_from(&mut reader)?; Ok(Self::from_ini(ini)) diff --git a/crates/turborepo-repository/src/package_graph/yarnrc.rs b/crates/turborepo-repository/src/package_manager/yarnrc.rs similarity index 79% rename from crates/turborepo-repository/src/package_graph/yarnrc.rs rename to crates/turborepo-repository/src/package_manager/yarnrc.rs index b1b81664d8369..326bc8e447ec3 100644 --- a/crates/turborepo-repository/src/package_graph/yarnrc.rs +++ b/crates/turborepo-repository/src/package_manager/yarnrc.rs @@ -2,9 +2,14 @@ use std::io; use serde::Deserialize; use serde_yaml; +use turbopath::AbsoluteSystemPath; + +pub const YARNRC_FILENAME: &str = ".yarnrc.yml"; #[derive(Debug, thiserror::Error)] pub enum Error { + #[error("encountered error opening yarnrc.yml: {0}")] + Io(#[from] std::io::Error), #[error("encountered error parsing yarnrc.yml: {0}")] SerdeYaml(#[from] serde_yaml::Error), } @@ -38,6 +43,16 @@ impl YarnRc { let config: YarnRc = serde_yaml::from_reader(&mut reader)?; Ok(config) } + + pub fn from_file(repo_root: &AbsoluteSystemPath) -> Result { + let yarnrc_path = repo_root.join_component(YARNRC_FILENAME); + let yarnrc = yarnrc_path + .read_existing_to_string()? + .map(|contents| Self::from_reader(contents.as_bytes())) + .transpose()? + .unwrap_or_default(); + Ok(yarnrc) + } } #[cfg(test)]