feat(turborepo): log reason why all packages were considered changed (#…

…8872) The ultimate goal for me is to figure out why I'm seeing _all_ packages invalidated when running with `--filter='[sha]'` . In a lot of cases, we return a sigil for `PackageChanges::All`. I'm not sure how to bubble this up with higher fidelity information in a good way, so this PR settles for a more granular "reason" why all packages changed and includes it in debug logging. This should help a little bit with debugging. --------- Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>
vercel · Jul 31, 2024 · e563d20 · e563d20
1 parent 86c89a9
commit e563d20
Show file tree

Hide file tree

Showing 6 changed files with 70 additions and 41 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
@@ -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>())?;

diff --git 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
@@ -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
@@ -23,9 +23,18 @@ pub enum LockfileChange {
     WithContent(Vec<u8>),
 }
 
+#[derive(Debug, PartialEq, Eq)]
+pub enum AllPackageChangeReason {
+    DefaultGlobalFileChanged,
+    LockfileChangeDetectionFailed,
+    LockfileChangedWithoutDetails,
+    RootInternalDepChanged,
+    NonPackageFileChanged,
+}
+
 #[derive(Debug, PartialEq, Eq)]
 pub enum PackageChanges {
-    All,
+    All(AllPackageChangeReason),
     Some(HashSet<WorkspacePackage>),
 }
 
@@ -62,39 +71,50 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> {
     ) -> Result<PackageChanges, ChangeMapError> {
         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))
-            }
-            // 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)
+
+        match self.get_changed_packages(filtered_changed_files.into_iter()) {
+            PackageChanges::All(reason) => Ok(PackageChanges::All(reason)),
+
+            PackageChanges::Some(mut changed_pkgs) => {
+                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) => {
+                        debug!("no previous lockfile available, assuming all packages changed");
+                        Ok(PackageChanges::All(
+                            AllPackageChangeReason::LockfileChangedWithoutDetails,
+                        ))
+                    }
+                    None => Ok(PackageChanges::Some(changed_pkgs)),
+                }
             }
-            None => Ok(PackageChanges::Some(changed_pkgs)),
         }
     }
 
@@ -120,18 +140,19 @@ 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
@@ -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},
@@ -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::NonPackageFileChanged)
+        );
 
         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
@@ -211,7 +211,7 @@ impl Workspace {
         };
 
         let packages = match package_changes {
-            PackageChanges::All => self
+            PackageChanges::All(_) => self
                 .graph
                 .packages()
                 .map(|(name, info)| WorkspacePackage {