From a73d5eadf15fa45a4e5c6892c7c6359881d2c2f2 Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Mon, 29 Jul 2024 22:42:05 -0500 Subject: [PATCH 1/8] feat(turborepo): Log reason why all packages were considered changed --- .../src/package_changes_watcher.rs | 2 +- .../src/run/scope/change_detector.rs | 4 +- .../src/change_mapper/mod.rs | 81 +++++++++++-------- .../src/change_mapper/package.rs | 5 +- packages/turbo-repository/rust/src/lib.rs | 6 +- 5 files changed, 59 insertions(+), 39 deletions(-) diff --git a/crates/turborepo-lib/src/package_changes_watcher.rs b/crates/turborepo-lib/src/package_changes_watcher.rs index 3e05ed46cd08e..ceeeb3331f424 100644 --- a/crates/turborepo-lib/src/package_changes_watcher.rs +++ b/crates/turborepo-lib/src/package_changes_watcher.rs @@ -348,7 +348,7 @@ impl Subscriber { tracing::warn!("changed_packages: {:?}", changed_packages); match changed_packages { - Ok(PackageChanges::All) => { + Ok(PackageChanges::All(_)) => { // We tell the client that we need to rediscover the packages, i.e. // all bets are off, just re-run everything let _ = self diff --git a/crates/turborepo-lib/src/run/scope/change_detector.rs b/crates/turborepo-lib/src/run/scope/change_detector.rs index 418cc96666fb3..2bd602822bd36 100644 --- a/crates/turborepo-lib/src/run/scope/change_detector.rs +++ b/crates/turborepo-lib/src/run/scope/change_detector.rs @@ -100,8 +100,8 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> { .change_mapper .changed_packages(changed_files, lockfile_contents)? { - PackageChanges::All => { - debug!("all packages changed"); + PackageChanges::All(reason) => { + debug!("all packages changed: {:?}", reason); Ok(self .pkg_graph .packages() diff --git a/crates/turborepo-repository/src/change_mapper/mod.rs b/crates/turborepo-repository/src/change_mapper/mod.rs index 629b616944461..f2a1a94123b98 100644 --- a/crates/turborepo-repository/src/change_mapper/mod.rs +++ b/crates/turborepo-repository/src/change_mapper/mod.rs @@ -23,9 +23,18 @@ pub enum LockfileChange { WithContent(Vec), } +#[derive(Debug, PartialEq, Eq)] +pub enum AllPackageChangeReason { + DefaultGlobalFileChanged, + LockfileChangeDetectionFailed, + LockfileChangedWithoutDetails, + RootInternalDepChanged, + NonPackageFileChanged, +} + #[derive(Debug, PartialEq, Eq)] pub enum PackageChanges { - All, + All(AllPackageChangeReason), Some(HashSet), } @@ -62,40 +71,46 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { ) -> Result { if Self::default_global_file_changed(&changed_files) { debug!("global file changed"); - return Ok(PackageChanges::All); + return Ok(PackageChanges::All(AllPackageChangeReason::DefaultGlobalFileChanged,)); } // get filtered files and add the packages that contain them let filtered_changed_files = self.filter_ignored_files(changed_files.iter())?; - let PackageChanges::Some(mut changed_pkgs) = - self.get_changed_packages(filtered_changed_files.into_iter()) - else { - return Ok(PackageChanges::All); - }; - - match lockfile_change { - Some(LockfileChange::WithContent(content)) => { - // if we run into issues, don't error, just assume all packages have changed - let Ok(lockfile_changes) = self.get_changed_packages_from_lockfile(content) else { - debug!("unable to determine lockfile changes, assuming all packages changed"); - return Ok(PackageChanges::All); - }; - debug!( - "found {} packages changed by lockfile", - lockfile_changes.len() - ); - changed_pkgs.extend(lockfile_changes); - - Ok(PackageChanges::Some(changed_pkgs)) + match self.get_changed_packages(filtered_changed_files.into_iter()) { + PackageChanges::All(reason) => { + return Ok(PackageChanges::All(reason)); } - // We don't have the actual contents, so just invalidate everything - Some(LockfileChange::Empty) => { - debug!("no previous lockfile available, assuming all packages changed"); - Ok(PackageChanges::All) + + PackageChanges::Some(mut changed_pkgs) => { + return match lockfile_change { + Some(LockfileChange::WithContent(content)) => { + // if we run into issues, don't error, just assume all packages have changed + let Ok(lockfile_changes) = self.get_changed_packages_from_lockfile(content) + else { + debug!("unable to determine lockfile changes, assuming all packages changed"); + return Ok(PackageChanges::All( + AllPackageChangeReason::LockfileChangeDetectionFailed, + )); + }; + debug!( + "found {} packages changed by lockfile", + lockfile_changes.len() + ); + changed_pkgs.extend(lockfile_changes); + + Ok(PackageChanges::Some(changed_pkgs)) + } + + // We don't have the actual contents, so just invalidate everything + Some(LockfileChange::Empty) => Ok(PackageChanges::All( + debug!("no previous lockfile available, assuming all packages changed"); + AllPackageChangeReason::LockfileChangedWithoutDetails, + )), + None => Ok(PackageChanges::Some(changed_pkgs)), + }; } - None => Ok(PackageChanges::Some(changed_pkgs)), - } + }; } fn filter_ignored_files<'b>( @@ -120,18 +135,18 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { // Internal root dependency changed so global hash has changed PackageMapping::Package(pkg) if root_internal_deps.contains(&pkg) => { debug!( - "root internal dependency \"{}\" changed due to: {file:?}", + "{} changes root internal dependency: \"{}\"", + file.to_string(), pkg.name ); - return PackageChanges::All; + return PackageChanges::All(AllPackageChangeReason::RootInternalDepChanged); } PackageMapping::Package(pkg) => { - debug!("package {pkg:?} changed due to {file:?}"); + debug!("{} changes \"{}\"", file.to_string(), pkg.name); changed_packages.insert(pkg); } PackageMapping::All => { - debug!("all packages changed due to {file:?}"); - return PackageChanges::All; + return PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged); } PackageMapping::None => {} } diff --git a/crates/turborepo-repository/src/change_mapper/package.rs b/crates/turborepo-repository/src/change_mapper/package.rs index dd4cdf2281347..90ae84e2855b2 100644 --- a/crates/turborepo-repository/src/change_mapper/package.rs +++ b/crates/turborepo-repository/src/change_mapper/package.rs @@ -172,7 +172,10 @@ mod tests { // We should return All because we don't have global deps and // therefore must be conservative about changes - assert_eq!(package_changes, PackageChanges::All); + assert_eq!( + package_changes, + PackageChanges::All(AllPackageChangeReason::DefaultGlobalFileChanged) + ); let turbo_package_detector = GlobalDepsPackageChangeMapper::new(&pkg_graph, std::iter::empty::<&str>())?; diff --git a/packages/turbo-repository/rust/src/lib.rs b/packages/turbo-repository/rust/src/lib.rs index cbf79e95d9888..c8e5298f9c370 100644 --- a/packages/turbo-repository/rust/src/lib.rs +++ b/packages/turbo-repository/rust/src/lib.rs @@ -7,7 +7,9 @@ use napi::Error; use napi_derive::napi; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; use turborepo_repository::{ - change_mapper::{ChangeMapper, DefaultPackageChangeMapper, PackageChanges}, + change_mapper::{ + AllPackageChangeReason, ChangeMapper, DefaultPackageChangeMapper, PackageChanges, + }, inference::RepoState as WorkspaceState, package_graph::{PackageGraph, PackageName, PackageNode, WorkspacePackage, ROOT_PKG_NAME}, }; @@ -211,7 +213,7 @@ impl Workspace { }; let packages = match package_changes { - PackageChanges::All => self + PackageChanges::All(_) => self .graph .packages() .map(|(name, info)| WorkspacePackage { From 21202d7e46f974a56b17dccc50ae25b17dddff44 Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Mon, 29 Jul 2024 23:07:17 -0500 Subject: [PATCH 2/8] fixes --- crates/turborepo-repository/src/change_mapper/mod.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/turborepo-repository/src/change_mapper/mod.rs b/crates/turborepo-repository/src/change_mapper/mod.rs index f2a1a94123b98..e8ae9c5644860 100644 --- a/crates/turborepo-repository/src/change_mapper/mod.rs +++ b/crates/turborepo-repository/src/change_mapper/mod.rs @@ -78,12 +78,10 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { let filtered_changed_files = self.filter_ignored_files(changed_files.iter())?; match self.get_changed_packages(filtered_changed_files.into_iter()) { - PackageChanges::All(reason) => { - return Ok(PackageChanges::All(reason)); - } + PackageChanges::All(reason) => Ok(PackageChanges::All(reason)), PackageChanges::Some(mut changed_pkgs) => { - return match lockfile_change { + match lockfile_change { Some(LockfileChange::WithContent(content)) => { // if we run into issues, don't error, just assume all packages have changed let Ok(lockfile_changes) = self.get_changed_packages_from_lockfile(content) @@ -108,9 +106,9 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { AllPackageChangeReason::LockfileChangedWithoutDetails, )), None => Ok(PackageChanges::Some(changed_pkgs)), - }; + } } - }; + } } fn filter_ignored_files<'b>( From b4a2c4eb9649e6b6364860487b293ca6d40d0164 Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Mon, 29 Jul 2024 23:12:43 -0500 Subject: [PATCH 3/8] rm import --- packages/turbo-repository/rust/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/turbo-repository/rust/src/lib.rs b/packages/turbo-repository/rust/src/lib.rs index c8e5298f9c370..db3d043b676f0 100644 --- a/packages/turbo-repository/rust/src/lib.rs +++ b/packages/turbo-repository/rust/src/lib.rs @@ -7,9 +7,7 @@ use napi::Error; use napi_derive::napi; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; use turborepo_repository::{ - change_mapper::{ - AllPackageChangeReason, ChangeMapper, DefaultPackageChangeMapper, PackageChanges, - }, + change_mapper::{ChangeMapper, DefaultPackageChangeMapper, PackageChanges}, inference::RepoState as WorkspaceState, package_graph::{PackageGraph, PackageName, PackageNode, WorkspacePackage, ROOT_PKG_NAME}, }; From 6ab0fcd8abb5d4f3d0852450a64f4505ee179f92 Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Tue, 30 Jul 2024 10:02:15 -0500 Subject: [PATCH 4/8] fixup --- .../src/change_mapper/mod.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/turborepo-repository/src/change_mapper/mod.rs b/crates/turborepo-repository/src/change_mapper/mod.rs index e8ae9c5644860..0858a20f9a009 100644 --- a/crates/turborepo-repository/src/change_mapper/mod.rs +++ b/crates/turborepo-repository/src/change_mapper/mod.rs @@ -71,7 +71,9 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { ) -> Result { if Self::default_global_file_changed(&changed_files) { debug!("global file changed"); - return Ok(PackageChanges::All(AllPackageChangeReason::DefaultGlobalFileChanged,)); + return Ok(PackageChanges::All( + AllPackageChangeReason::DefaultGlobalFileChanged, + )); } // get filtered files and add the packages that contain them @@ -86,7 +88,10 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { // if we run into issues, don't error, just assume all packages have changed let Ok(lockfile_changes) = self.get_changed_packages_from_lockfile(content) else { - debug!("unable to determine lockfile changes, assuming all packages changed"); + debug!( + "unable to determine lockfile changes, assuming all packages \ + changed" + ); return Ok(PackageChanges::All( AllPackageChangeReason::LockfileChangeDetectionFailed, )); @@ -101,10 +106,12 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { } // We don't have the actual contents, so just invalidate everything - Some(LockfileChange::Empty) => Ok(PackageChanges::All( + Some(LockfileChange::Empty) => { debug!("no previous lockfile available, assuming all packages changed"); - AllPackageChangeReason::LockfileChangedWithoutDetails, - )), + Ok(PackageChanges::All( + AllPackageChangeReason::LockfileChangedWithoutDetails, + )) + } None => Ok(PackageChanges::Some(changed_pkgs)), } } From e23ff5051d081090c7685e4a77bbb02a5ce8f489 Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Tue, 30 Jul 2024 11:11:52 -0500 Subject: [PATCH 5/8] import --- crates/turborepo-repository/src/change_mapper/package.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-repository/src/change_mapper/package.rs b/crates/turborepo-repository/src/change_mapper/package.rs index 90ae84e2855b2..5b1c59c87dd5f 100644 --- a/crates/turborepo-repository/src/change_mapper/package.rs +++ b/crates/turborepo-repository/src/change_mapper/package.rs @@ -120,7 +120,7 @@ mod tests { use super::{DefaultPackageChangeMapper, GlobalDepsPackageChangeMapper}; use crate::{ - change_mapper::{ChangeMapper, PackageChanges}, + change_mapper::{AllPackageChangeReason, ChangeMapper, PackageChanges}, discovery, discovery::PackageDiscovery, package_graph::{PackageGraphBuilder, WorkspacePackage}, From a4f8dde09e4f3e235615cefeecfee17b308814ae Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Tue, 30 Jul 2024 15:32:40 -0500 Subject: [PATCH 6/8] test --- .../src/global_deps_package_change_mapper.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/turborepo-lib/src/global_deps_package_change_mapper.rs b/crates/turborepo-lib/src/global_deps_package_change_mapper.rs index 8661f30a47cd9..3d686c4955e5b 100644 --- a/crates/turborepo-lib/src/global_deps_package_change_mapper.rs +++ b/crates/turborepo-lib/src/global_deps_package_change_mapper.rs @@ -63,7 +63,9 @@ mod tests { use tempfile::tempdir; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; use turborepo_repository::{ - change_mapper::{ChangeMapper, DefaultPackageChangeMapper, PackageChanges}, + change_mapper::{ + AllPackageChangeReason, ChangeMapper, DefaultPackageChangeMapper, PackageChanges, + }, discovery, discovery::PackageDiscovery, package_graph::{PackageGraphBuilder, WorkspacePackage}, @@ -117,7 +119,10 @@ mod tests { // We should return All because we don't have global deps and // therefore must be conservative about changes - assert_eq!(package_changes, PackageChanges::All); + assert_eq!( + package_changes, + PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged) + ); let turbo_package_detector = GlobalDepsPackageChangeMapper::new(&pkg_graph, std::iter::empty::<&str>())?; From 5683838557895b5b7309ae80a7d58293dec2fca3 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 31 Jul 2024 17:07:34 -0400 Subject: [PATCH 7/8] Update crates/turborepo-repository/src/change_mapper/package.rs --- crates/turborepo-repository/src/change_mapper/package.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-repository/src/change_mapper/package.rs b/crates/turborepo-repository/src/change_mapper/package.rs index 5b1c59c87dd5f..c023b56ffe631 100644 --- a/crates/turborepo-repository/src/change_mapper/package.rs +++ b/crates/turborepo-repository/src/change_mapper/package.rs @@ -174,7 +174,7 @@ mod tests { // therefore must be conservative about changes assert_eq!( package_changes, - PackageChanges::All(AllPackageChangeReason::DefaultGlobalFileChanged) + PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged) ); let turbo_package_detector = From 8d79542d13a9e01ad9b283f1909385dcf29785ac Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Wed, 31 Jul 2024 16:26:13 -0500 Subject: [PATCH 8/8] bring back a debug --- crates/turborepo-repository/src/change_mapper/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/turborepo-repository/src/change_mapper/mod.rs b/crates/turborepo-repository/src/change_mapper/mod.rs index 0858a20f9a009..7ed5ea008cc55 100644 --- a/crates/turborepo-repository/src/change_mapper/mod.rs +++ b/crates/turborepo-repository/src/change_mapper/mod.rs @@ -151,6 +151,7 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { changed_packages.insert(pkg); } PackageMapping::All => { + debug!("all packages changed due to {file:?}"); return PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged); } PackageMapping::None => {}