From b353351a0884af8e88a8226602bf262dda94856d Mon Sep 17 00:00:00 2001 From: Mehul Kar Date: Mon, 29 Jul 2024 22:42:05 -0500 Subject: [PATCH] feat(turborepo): Log reason why all packages were considered changed --- .../src/package_changes_watcher.rs | 2 +- .../src/run/scope/change_detector.rs | 13 ++-- .../src/change_mapper/mod.rs | 69 ++++++++++++------- .../src/change_mapper/package.rs | 5 +- packages/turbo-repository/rust/src/lib.rs | 6 +- 5 files changed, 63 insertions(+), 32 deletions(-) diff --git a/crates/turborepo-lib/src/package_changes_watcher.rs b/crates/turborepo-lib/src/package_changes_watcher.rs index 3e05ed46cd08e9..ceeeb3331f424c 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 c2eebbe496bdfc..b8b386c4cf1322 100644 --- a/crates/turborepo-lib/src/run/scope/change_detector.rs +++ b/crates/turborepo-lib/src/run/scope/change_detector.rs @@ -99,11 +99,14 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> { .change_mapper .changed_packages(changed_files, lockfile_contents)? { - PackageChanges::All => Ok(self - .pkg_graph - .packages() - .map(|(name, _)| name.to_owned()) - .collect()), + PackageChanges::All(reason) => { + debug!("All packages changed: {:?}", reason); + Ok(self + .pkg_graph + .packages() + .map(|(name, _)| name.to_owned()) + .collect()) + } PackageChanges::Some(packages) => Ok(packages .iter() .map(|package| package.name.to_owned()) diff --git a/crates/turborepo-repository/src/change_mapper/mod.rs b/crates/turborepo-repository/src/change_mapper/mod.rs index 1b8ef715a11858..803157b592bdff 100644 --- a/crates/turborepo-repository/src/change_mapper/mod.rs +++ b/crates/turborepo-repository/src/change_mapper/mod.rs @@ -8,6 +8,7 @@ pub use package::{ }; use tracing::debug; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; +use turborepo_lockfiles::Package; use wax::Program; use crate::package_graph::{ChangedPackagesError, PackageGraph, WorkspacePackage}; @@ -23,9 +24,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), } @@ -61,32 +71,43 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { lockfile_change: Option, ) -> Result { if Self::default_global_file_changed(&changed_files) { - 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 { - return Ok(PackageChanges::All); - }; - changed_pkgs.extend(lockfile_changes); + match self.get_changed_packages(filtered_changed_files.into_iter()) { + PackageChanges::All(reason) => { + return Ok(PackageChanges::All(reason)); + } - Ok(PackageChanges::Some(changed_pkgs)) + 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 { + return Ok(PackageChanges::All( + AllPackageChangeReason::LockfileChangeDetectionFailed, + )); + }; + + 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( + AllPackageChangeReason::LockfileChangedWithoutDetails, + )), + None => Ok(PackageChanges::Some(changed_pkgs)), + }; } - // We don't have the actual contents, so just invalidate everything - Some(LockfileChange::Empty) => Ok(PackageChanges::All), - None => Ok(PackageChanges::Some(changed_pkgs)), - } + }; } fn filter_ignored_files<'b>( @@ -111,16 +132,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!("{} changes \"{}\"", file.to_string(), pkg.name); changed_packages.insert(pkg); } PackageMapping::All => { - 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 dd4cdf2281347d..90ae84e2855b24 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 cbf79e95d98885..c8e5298f9c370b 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 {