From 5c9379392a17e9223e12c1741f5f1998237886d4 Mon Sep 17 00:00:00 2001 From: Nicholas Yang Date: Thu, 10 Oct 2024 17:06:29 -0400 Subject: [PATCH] feat(query): package changes reason (#9240) ### Description Plumbing through reason for why a package changed for query. Adds a `reason` field which is a union of the different reasons a package could be added. This can be reviewed commit by commit, although there is a little bit of churn around types and some behavior. ### Testing Instructions Added some tests to `affected.t` --- .../src/global_deps_package_change_mapper.rs | 147 ----------- crates/turborepo-lib/src/lib.rs | 1 - .../src/package_changes_watcher.rs | 4 +- crates/turborepo-lib/src/query/mod.rs | 203 +++++++++++++- crates/turborepo-lib/src/run/builder.rs | 24 +- crates/turborepo-lib/src/run/mod.rs | 2 +- .../src/run/scope/change_detector.rs | 40 +-- crates/turborepo-lib/src/run/scope/filter.rs | 249 +++++++++++++----- crates/turborepo-lib/src/run/scope/mod.rs | 11 +- .../src/change_mapper/mod.rs | 106 ++++++-- .../src/change_mapper/package.rs | 78 ++++-- crates/turborepo-scm/src/git.rs | 46 ++-- packages/turbo-repository/rust/src/lib.rs | 2 +- turborepo-tests/integration/tests/affected.t | 44 +++- .../tests/lockfile-aware-caching/pnpm.t | 27 +- .../tests/lockfile-aware-caching/yarn.t | 27 +- 16 files changed, 697 insertions(+), 314 deletions(-) delete mode 100644 crates/turborepo-lib/src/global_deps_package_change_mapper.rs diff --git a/crates/turborepo-lib/src/global_deps_package_change_mapper.rs b/crates/turborepo-lib/src/global_deps_package_change_mapper.rs deleted file mode 100644 index 3d686c4955e5b..0000000000000 --- a/crates/turborepo-lib/src/global_deps_package_change_mapper.rs +++ /dev/null @@ -1,147 +0,0 @@ -use thiserror::Error; -use turbopath::AnchoredSystemPath; -use turborepo_repository::{ - change_mapper::{DefaultPackageChangeMapper, PackageChangeMapper, PackageMapping}, - package_graph::{PackageGraph, WorkspacePackage}, -}; -use wax::{BuildError, Program}; - -#[derive(Error, Debug)] -pub enum Error { - #[error(transparent)] - InvalidFilter(#[from] BuildError), -} - -/// A package detector that uses a global deps list to determine -/// if a file should cause all packages to be marked as changed. -/// This is less conservative than the `DefaultPackageChangeMapper`, -/// which assumes that any changed file that is not in a package -/// changes all packages. Since we have a list of global deps, -/// we can check against that and avoid invalidating in unnecessary cases. -pub struct GlobalDepsPackageChangeMapper<'a> { - pkg_dep_graph: &'a PackageGraph, - global_deps_matcher: wax::Any<'a>, -} - -impl<'a> GlobalDepsPackageChangeMapper<'a> { - pub fn new, I: Iterator>( - pkg_dep_graph: &'a PackageGraph, - global_deps: I, - ) -> Result { - let global_deps_matcher = wax::any(global_deps)?; - - Ok(Self { - pkg_dep_graph, - global_deps_matcher, - }) - } -} - -impl<'a> PackageChangeMapper for GlobalDepsPackageChangeMapper<'a> { - fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping { - match DefaultPackageChangeMapper::new(self.pkg_dep_graph).detect_package(path) { - // Since `DefaultPackageChangeMapper` is overly conservative, we can check here if - // the path is actually in globalDeps and if not, return it as - // PackageDetection::Package(WorkspacePackage::root()). - PackageMapping::All => { - let cleaned_path = path.clean(); - let in_global_deps = self.global_deps_matcher.is_match(cleaned_path.as_str()); - - if in_global_deps { - PackageMapping::All - } else { - PackageMapping::Package(WorkspacePackage::root()) - } - } - result => result, - } - } -} - -#[cfg(test)] -mod tests { - use tempfile::tempdir; - use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; - use turborepo_repository::{ - change_mapper::{ - AllPackageChangeReason, ChangeMapper, DefaultPackageChangeMapper, PackageChanges, - }, - discovery, - discovery::PackageDiscovery, - package_graph::{PackageGraphBuilder, WorkspacePackage}, - package_json::PackageJson, - }; - - use super::GlobalDepsPackageChangeMapper; - - #[allow(dead_code)] - pub struct MockDiscovery; - - impl PackageDiscovery for MockDiscovery { - async fn discover_packages( - &self, - ) -> Result { - Ok(discovery::DiscoveryResponse { - package_manager: turborepo_repository::package_manager::PackageManager::Npm, - workspaces: vec![], - }) - } - - async fn discover_packages_blocking( - &self, - ) -> Result { - self.discover_packages().await - } - } - - #[tokio::test] - async fn test_different_package_detectors() -> Result<(), anyhow::Error> { - let repo_root = tempdir()?; - let root_package_json = PackageJson::default(); - - let pkg_graph = PackageGraphBuilder::new( - AbsoluteSystemPath::from_std_path(repo_root.path())?, - root_package_json, - ) - .with_package_discovery(MockDiscovery) - .build() - .await?; - - let default_package_detector = DefaultPackageChangeMapper::new(&pkg_graph); - let change_mapper = ChangeMapper::new(&pkg_graph, vec![], default_package_detector); - - let package_changes = change_mapper.changed_packages( - [AnchoredSystemPathBuf::from_raw("README.md")?] - .into_iter() - .collect(), - None, - )?; - - // We should return All because we don't have global deps and - // therefore must be conservative about changes - assert_eq!( - package_changes, - PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged) - ); - - let turbo_package_detector = - GlobalDepsPackageChangeMapper::new(&pkg_graph, std::iter::empty::<&str>())?; - let change_mapper = ChangeMapper::new(&pkg_graph, vec![], turbo_package_detector); - - let package_changes = change_mapper.changed_packages( - [AnchoredSystemPathBuf::from_raw("README.md")?] - .into_iter() - .collect(), - None, - )?; - - // We only get a root workspace change since we have global deps specified and - // README.md is not one of them - assert_eq!( - package_changes, - PackageChanges::Some([WorkspacePackage::root()].into_iter().collect()) - ); - - Ok(()) - } -} diff --git a/crates/turborepo-lib/src/lib.rs b/crates/turborepo-lib/src/lib.rs index 0014816ab3d38..5124adb23d036 100644 --- a/crates/turborepo-lib/src/lib.rs +++ b/crates/turborepo-lib/src/lib.rs @@ -21,7 +21,6 @@ mod engine; mod framework; mod gitignore; -mod global_deps_package_change_mapper; pub(crate) mod globwatcher; mod hash; mod opts; diff --git a/crates/turborepo-lib/src/package_changes_watcher.rs b/crates/turborepo-lib/src/package_changes_watcher.rs index d60ace1ffda93..2cb9857a18767 100644 --- a/crates/turborepo-lib/src/package_changes_watcher.rs +++ b/crates/turborepo-lib/src/package_changes_watcher.rs @@ -370,7 +370,9 @@ impl Subscriber { } } } - Ok(PackageChanges::Some(mut filtered_pkgs)) => { + Ok(PackageChanges::Some(filtered_pkgs)) => { + let mut filtered_pkgs: HashSet = + filtered_pkgs.into_keys().collect(); // If the root package has changed, we only send it if we have root // tasks. Otherwise it's not worth sending as it will only // pollute up the output logs diff --git a/crates/turborepo-lib/src/query/mod.rs b/crates/turborepo-lib/src/query/mod.rs index b1c210308ec7d..91338601cc586 100644 --- a/crates/turborepo-lib/src/query/mod.rs +++ b/crates/turborepo-lib/src/query/mod.rs @@ -15,7 +15,7 @@ use thiserror::Error; use tokio::select; use turbo_trace::TraceError; use turbopath::AbsoluteSystemPathBuf; -use turborepo_repository::package_graph::PackageName; +use turborepo_repository::{change_mapper::AllPackageChangeReason, package_graph::PackageName}; use crate::{ get_version, @@ -46,6 +46,9 @@ pub enum Error { Path(#[from] turbopath::PathError), #[error(transparent)] UI(#[from] turborepo_ui::Error), + #[error(transparent)] + #[diagnostic(transparent)] + Resolution(#[from] crate::run::scope::filter::ResolutionError), } pub struct RepositoryQuery { @@ -61,6 +64,7 @@ impl RepositoryQuery { #[derive(Debug, SimpleObject)] #[graphql(concrete(name = "RepositoryTasks", params(RepositoryTask)))] #[graphql(concrete(name = "Packages", params(Package)))] +#[graphql(concrete(name = "ChangedPackages", params(ChangedPackage)))] pub struct Array { items: Vec, length: usize, @@ -290,6 +294,188 @@ impl PackagePredicate { } } +// why write few types when many work? +#[derive(SimpleObject)] +struct GlobalDepsChanged { + // we're using slightly awkward names so we can reserve the nicer name for the "correct" + // GraphQL type, e.g. a `file` field for the `File` type + file_path: String, +} + +#[derive(SimpleObject)] +struct DefaultGlobalFileChanged { + file_path: String, +} + +#[derive(SimpleObject)] +struct LockfileChangeDetectionFailed { + /// This is a nothing field + empty: bool, +} + +#[derive(SimpleObject)] +struct LockfileChangedWithoutDetails { + /// This is a nothing field + empty: bool, +} + +#[derive(SimpleObject)] +struct RootInternalDepChanged { + root_internal_dep: String, +} + +#[derive(SimpleObject)] +struct NonPackageFileChanged { + file: String, +} + +#[derive(SimpleObject)] +struct GitRefNotFound { + from_ref: Option, + to_ref: Option, +} + +#[derive(SimpleObject)] +struct IncludedByFilter { + filters: Vec, +} + +#[derive(SimpleObject)] +struct RootTask { + task_name: String, +} + +#[derive(SimpleObject)] +struct ConservativeRootLockfileChanged { + /// This is a nothing field + empty: bool, +} + +#[derive(SimpleObject)] +struct LockfileChanged { + /// This is a nothing field + empty: bool, +} + +#[derive(SimpleObject)] +struct DependencyChanged { + dependency_name: String, +} + +#[derive(SimpleObject)] +struct DependentChanged { + dependent_name: String, +} + +#[derive(SimpleObject)] +struct FileChanged { + file_path: String, +} + +#[derive(SimpleObject)] +struct InFilteredDirectory { + directory_path: String, +} + +#[derive(Union)] +enum PackageChangeReason { + GlobalDepsChanged(GlobalDepsChanged), + DefaultGlobalFileChanged(DefaultGlobalFileChanged), + LockfileChangeDetectionFailed(LockfileChangeDetectionFailed), + LockfileChangedWithoutDetails(LockfileChangedWithoutDetails), + RootInternalDepChanged(RootInternalDepChanged), + NonPackageFileChanged(NonPackageFileChanged), + GitRefNotFound(GitRefNotFound), + IncludedByFilter(IncludedByFilter), + RootTask(RootTask), + ConservativeRootLockfileChanged(ConservativeRootLockfileChanged), + LockfileChanged(LockfileChanged), + DependencyChanged(DependencyChanged), + DependentChanged(DependentChanged), + FileChanged(FileChanged), + InFilteredDirectory(InFilteredDirectory), +} + +impl From for PackageChangeReason { + fn from(value: turborepo_repository::change_mapper::PackageInclusionReason) -> Self { + match value { + turborepo_repository::change_mapper::PackageInclusionReason::All( + AllPackageChangeReason::GlobalDepsChanged { file }, + ) => PackageChangeReason::GlobalDepsChanged(GlobalDepsChanged { + file_path: file.to_string(), + }), + turborepo_repository::change_mapper::PackageInclusionReason::All( + AllPackageChangeReason::DefaultGlobalFileChanged { file }, + ) => PackageChangeReason::DefaultGlobalFileChanged(DefaultGlobalFileChanged { + file_path: file.to_string(), + }), + turborepo_repository::change_mapper::PackageInclusionReason::All( + AllPackageChangeReason::LockfileChangeDetectionFailed, + ) => { + PackageChangeReason::LockfileChangeDetectionFailed(LockfileChangeDetectionFailed { + empty: false, + }) + } + turborepo_repository::change_mapper::PackageInclusionReason::All( + AllPackageChangeReason::GitRefNotFound { from_ref, to_ref }, + ) => PackageChangeReason::GitRefNotFound(GitRefNotFound { from_ref, to_ref }), + turborepo_repository::change_mapper::PackageInclusionReason::All( + AllPackageChangeReason::LockfileChangedWithoutDetails, + ) => { + PackageChangeReason::LockfileChangedWithoutDetails(LockfileChangedWithoutDetails { + empty: false, + }) + } + turborepo_repository::change_mapper::PackageInclusionReason::All( + AllPackageChangeReason::RootInternalDepChanged { root_internal_dep }, + ) => PackageChangeReason::RootInternalDepChanged(RootInternalDepChanged { + root_internal_dep: root_internal_dep.to_string(), + }), + turborepo_repository::change_mapper::PackageInclusionReason::RootTask { task } => { + PackageChangeReason::RootTask(RootTask { + task_name: task.to_string(), + }) + } + turborepo_repository::change_mapper::PackageInclusionReason::ConservativeRootLockfileChanged => { + PackageChangeReason::ConservativeRootLockfileChanged(ConservativeRootLockfileChanged { empty: false }) + } + turborepo_repository::change_mapper::PackageInclusionReason::LockfileChanged => { + PackageChangeReason::LockfileChanged(LockfileChanged { empty: false }) + } + turborepo_repository::change_mapper::PackageInclusionReason::DependencyChanged { + dependency, + } => PackageChangeReason::DependencyChanged(DependencyChanged { + dependency_name: dependency.to_string(), + }), + turborepo_repository::change_mapper::PackageInclusionReason::DependentChanged { + dependent, + } => PackageChangeReason::DependentChanged(DependentChanged { + dependent_name: dependent.to_string(), + }), + turborepo_repository::change_mapper::PackageInclusionReason::FileChanged { file } => { + PackageChangeReason::FileChanged(FileChanged { + file_path: file.to_string(), + }) + } + turborepo_repository::change_mapper::PackageInclusionReason::InFilteredDirectory { + directory, + } => PackageChangeReason::InFilteredDirectory(InFilteredDirectory { + directory_path: directory.to_string(), + }), + turborepo_repository::change_mapper::PackageInclusionReason::IncludedByFilter { + filters, + } => PackageChangeReason::IncludedByFilter(IncludedByFilter { filters }), + } + } +} + +#[derive(SimpleObject)] +struct ChangedPackage { + reason: PackageChangeReason, + #[graphql(flatten)] + package: Package, +} + #[Object] impl RepositoryQuery { async fn affected_packages( @@ -297,7 +483,7 @@ impl RepositoryQuery { base: Option, head: Option, filter: Option, - ) -> Result, Error> { + ) -> Result, Error> { let mut opts = self.run.opts().clone(); opts.scope_opts.affected_range = Some((base, head)); @@ -309,12 +495,15 @@ impl RepositoryQuery { self.run.root_turbo_json(), )? .into_iter() - .map(|package| Package { - run: self.run.clone(), - name: package, + .map(|(package, reason)| ChangedPackage { + package: Package { + run: self.run.clone(), + name: package, + }, + reason: reason.into(), }) - .filter(|package| filter.as_ref().map_or(true, |f| f.check(package))) - .sorted_by(|a, b| a.name.cmp(&b.name)) + .filter(|package| filter.as_ref().map_or(true, |f| f.check(&package.package))) + .sorted_by(|a, b| a.package.name.cmp(&b.package.name)) .collect()) } /// Gets a single package by name diff --git a/crates/turborepo-lib/src/run/builder.rs b/crates/turborepo-lib/src/run/builder.rs index 0169515663b4c..7f10b64ab1cc5 100644 --- a/crates/turborepo-lib/src/run/builder.rs +++ b/crates/turborepo-lib/src/run/builder.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashSet, + collections::{HashMap, HashSet}, io::{ErrorKind, IsTerminal}, sync::Arc, time::SystemTime, @@ -14,6 +14,7 @@ use turborepo_cache::AsyncCache; use turborepo_env::EnvironmentVariableMap; use turborepo_errors::Spanned; use turborepo_repository::{ + change_mapper::PackageInclusionReason, package_graph::{PackageGraph, PackageName}, package_json, package_json::PackageJson, @@ -148,7 +149,7 @@ impl RunBuilder { pkg_dep_graph: &PackageGraph, scm: &SCM, root_turbo_json: &TurboJson, - ) -> Result, Error> { + ) -> Result, Error> { let (mut filtered_pkgs, is_all_packages) = scope::resolve_packages( &opts.scope_opts, repo_root, @@ -166,7 +167,12 @@ impl RunBuilder { } if root_turbo_json.tasks.contains_key(&task_name) { - filtered_pkgs.insert(PackageName::Root); + filtered_pkgs.insert( + PackageName::Root, + PackageInclusionReason::RootTask { + task: task_name.to_string(), + }, + ); break; } } @@ -411,7 +417,7 @@ impl RunBuilder { let mut engine = self.build_engine( &pkg_dep_graph, &root_turbo_json, - &filtered_pkgs, + filtered_pkgs.keys(), turbo_json_loader.clone(), )?; @@ -420,7 +426,7 @@ impl RunBuilder { engine = self.build_engine( &pkg_dep_graph, &root_turbo_json, - &filtered_pkgs, + filtered_pkgs.keys(), turbo_json_loader, )?; } @@ -453,7 +459,7 @@ impl RunBuilder { api_client: self.api_client, api_auth: self.api_auth, env_at_execution_start, - filtered_pkgs, + filtered_pkgs: filtered_pkgs.keys().cloned().collect(), pkg_dep_graph: Arc::new(pkg_dep_graph), root_turbo_json, scm, @@ -465,11 +471,11 @@ impl RunBuilder { }) } - fn build_engine( + fn build_engine<'a>( &self, pkg_dep_graph: &PackageGraph, root_turbo_json: &TurboJson, - filtered_pkgs: &HashSet, + filtered_pkgs: impl Iterator, turbo_json_loader: TurboJsonLoader, ) -> Result { let mut builder = EngineBuilder::new( @@ -480,7 +486,7 @@ impl RunBuilder { ) .with_root_tasks(root_turbo_json.tasks.keys().cloned()) .with_tasks_only(self.opts.run_opts.only) - .with_workspaces(filtered_pkgs.clone().into_iter().collect()) + .with_workspaces(filtered_pkgs.cloned().collect()) .with_tasks(self.opts.run_opts.tasks.iter().map(|task| { // TODO: Pull span info from command Spanned::new(TaskName::from(task.as_str()).into_owned()) diff --git a/crates/turborepo-lib/src/run/mod.rs b/crates/turborepo-lib/src/run/mod.rs index 5b50452c10be5..0f3a90676c67d 100644 --- a/crates/turborepo-lib/src/run/mod.rs +++ b/crates/turborepo-lib/src/run/mod.rs @@ -6,7 +6,7 @@ mod error; pub(crate) mod global_hash; mod graph_visualizer; pub(crate) mod package_discovery; -mod scope; +pub(crate) mod scope; pub(crate) mod summary; pub mod task_access; pub mod task_id; diff --git a/crates/turborepo-lib/src/run/scope/change_detector.rs b/crates/turborepo-lib/src/run/scope/change_detector.rs index b2527a74cc40d..3b8459e352836 100644 --- a/crates/turborepo-lib/src/run/scope/change_detector.rs +++ b/crates/turborepo-lib/src/run/scope/change_detector.rs @@ -1,17 +1,17 @@ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use tracing::debug; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; use turborepo_repository::{ - change_mapper::{ChangeMapper, DefaultPackageChangeMapper, LockfileChange, PackageChanges}, + change_mapper::{ + AllPackageChangeReason, ChangeMapper, DefaultPackageChangeMapper, Error, + GlobalDepsPackageChangeMapper, LockfileChange, PackageChanges, PackageInclusionReason, + }, package_graph::{PackageGraph, PackageName}, }; -use turborepo_scm::{git::ChangedFiles, SCM}; +use turborepo_scm::{git::InvalidRange, SCM}; -use crate::{ - global_deps_package_change_mapper::{Error, GlobalDepsPackageChangeMapper}, - run::scope::ResolutionError, -}; +use crate::run::scope::ResolutionError; /// Given two git refs, determine which packages have changed between them. pub trait GitChangeDetector { @@ -22,7 +22,7 @@ pub trait GitChangeDetector { include_uncommitted: bool, allow_unknown_objects: bool, merge_base: bool, - ) -> Result, ResolutionError>; + ) -> Result, ResolutionError>; } pub struct ScopeChangeDetector<'a> { @@ -94,7 +94,7 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> { include_uncommitted: bool, allow_unknown_objects: bool, merge_base: bool, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { let changed_files = match self.scm.changed_files( self.turbo_root, from_ref, @@ -103,15 +103,23 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> { allow_unknown_objects, merge_base, )? { - ChangedFiles::All => { + Err(InvalidRange { from_ref, to_ref }) => { debug!("all packages changed"); return Ok(self .pkg_graph .packages() - .map(|(name, _)| name.to_owned()) + .map(|(name, _)| { + ( + name.to_owned(), + PackageInclusionReason::All(AllPackageChangeReason::GitRefNotFound { + from_ref: from_ref.clone(), + to_ref: to_ref.clone(), + }), + ) + }) .collect()); } - ChangedFiles::Some(changed_files) => changed_files, + Ok(changed_files) => changed_files, }; let lockfile_contents = self.get_lockfile_contents(from_ref, &changed_files); @@ -133,22 +141,22 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> { Ok(self .pkg_graph .packages() - .map(|(name, _)| name.to_owned()) + .map(|(name, _)| (name.to_owned(), PackageInclusionReason::All(reason.clone()))) .collect()) } PackageChanges::Some(packages) => { debug!( "{} packages changed: {:?}", packages.len(), - &packages - .iter() + packages + .keys() .map(|x| x.name.to_string()) .collect::>() ); Ok(packages .iter() - .map(|package| package.name.to_owned()) + .map(|(package, reason)| (package.name.clone(), reason.clone())) .collect()) } } diff --git a/crates/turborepo-lib/src/run/scope/filter.rs b/crates/turborepo-lib/src/run/scope/filter.rs index 331d53f0526f5..918afc2a3c8e4 100644 --- a/crates/turborepo-lib/src/run/scope/filter.rs +++ b/crates/turborepo-lib/src/run/scope/filter.rs @@ -4,10 +4,11 @@ use std::{ str::FromStr, }; +use miette::Diagnostic; use tracing::debug; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPathBuf}; use turborepo_repository::{ - change_mapper::ChangeMapError, + change_mapper::{merge_changed_packages, ChangeMapError, PackageInclusionReason}, package_graph::{self, PackageGraph, PackageName}, }; use turborepo_scm::SCM; @@ -18,10 +19,7 @@ use super::{ simple_glob::{Match, SimpleGlob}, target_selector::{GitRange, InvalidSelectorError, TargetSelector}, }; -use crate::{ - global_deps_package_change_mapper, run::scope::change_detector::ScopeChangeDetector, - turbo_json::TurboJson, -}; +use crate::{run::scope::change_detector::ScopeChangeDetector, turbo_json::TurboJson}; pub struct PackageInference { package_name: Option, @@ -165,7 +163,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { &self, affected: &Option<(Option, Option)>, patterns: &[String], - ) -> Result<(HashSet, bool), ResolutionError> { + ) -> Result<(HashMap, bool), ResolutionError> { // inference is None only if we are in the root let is_all_packages = patterns.is_empty() && self.inference.is_none() && affected.is_none(); @@ -174,7 +172,14 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { self.pkg_graph .packages() .filter(|(name, _)| matches!(name, PackageName::Other(_))) - .map(|(name, _)| name.to_owned()) + .map(|(name, _)| { + ( + name.to_owned(), + PackageInclusionReason::IncludedByFilter { + filters: patterns.to_vec(), + }, + ) + }) .collect() } else { self.get_packages_from_patterns(affected, patterns)? @@ -187,7 +192,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { &self, affected: &Option<(Option, Option)>, patterns: &[String], - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { let mut selectors = patterns .iter() .map(|pattern| TargetSelector::from_str(pattern)) @@ -213,7 +218,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn get_filtered_packages( &self, selectors: Vec, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { let (_prod_selectors, all_selectors) = self .apply_inference(selectors) .into_iter() @@ -249,7 +254,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn filter_graph( &self, selectors: Vec, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { let (include_selectors, exclude_selectors) = selectors.into_iter().partition::, _>(|t| !t.exclude); @@ -261,13 +266,28 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { .packages() // todo: a type-level way of dealing with non-root packages .filter(|(name, _)| !PackageName::Root.eq(name)) // the root package has to be explicitly included - .map(|(name, _)| name.to_owned()) + .map(|(name, _)| { + ( + name.to_owned(), + PackageInclusionReason::IncludedByFilter { + filters: exclude_selectors + .iter() + .map(|s| s.raw.to_string()) + .collect(), + }, + ) + }) .collect() }; - let exclude = self.filter_graph_with_selectors(exclude_selectors)?; + // We want to just collect the names, not the reasons, so when we check for + // inclusion we don't need to check the reason + let exclude: HashSet = self + .filter_graph_with_selectors(exclude_selectors)? + .into_keys() + .collect(); - include.retain(|i| !exclude.contains(i)); + include.retain(|i, _| !exclude.contains(i)); Ok(include) } @@ -275,12 +295,12 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn filter_graph_with_selectors( &self, selectors: Vec, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { let mut unmatched_selectors = Vec::new(); - let mut walked_dependencies = HashSet::new(); - let mut walked_dependents = HashSet::new(); - let mut walked_dependent_dependencies = HashSet::new(); - let mut cherry_picked_packages = HashSet::new(); + let mut walked_dependencies = HashMap::new(); + let mut walked_dependents = HashMap::new(); + let mut walked_dependent_dependencies = HashMap::new(); + let mut cherry_picked_packages = HashMap::new(); for selector in selectors { let selector_packages = self.filter_graph_with_selector(&selector)?; @@ -290,7 +310,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { continue; } - for package in selector_packages { + for (package, reason) in selector_packages { let node = package_graph::PackageNode::Workspace(package.clone()); if selector.include_dependencies { @@ -298,17 +318,35 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { let dependencies = dependencies .iter() .filter(|node| !matches!(node, package_graph::PackageNode::Root)) - .map(|i| i.as_package_name().to_owned()) + .map(|i| { + ( + i.as_package_name().to_owned(), + // While we're adding dependencies, from their + // perspective, they were changed because + // of a *dependent* + PackageInclusionReason::DependentChanged { + dependent: package.to_owned(), + }, + ) + }) .collect::>(); // flatmap through the option, the set, and then the optional package name - walked_dependencies.extend(dependencies); + merge_changed_packages(&mut walked_dependencies, dependencies); } if selector.include_dependents { let dependents = self.pkg_graph.ancestors(&node); for dependent in dependents.iter().map(|i| i.as_package_name()) { - walked_dependents.insert(dependent.clone()); + walked_dependents.insert( + dependent.clone(), + // While we're adding dependents, from their + // perspective, they were changed because + // of a *dependency* + PackageInclusionReason::DependencyChanged { + dependency: package.to_owned(), + }, + ); // get the dependent's dependencies if selector.include_dependencies { @@ -321,10 +359,20 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { let dependent_dependencies = dependent_dependencies .iter() .filter(|node| !matches!(node, package_graph::PackageNode::Root)) - .map(|i| i.as_package_name().to_owned()) + .map(|i| { + ( + i.as_package_name().to_owned(), + PackageInclusionReason::DependencyChanged { + dependency: package.to_owned(), + }, + ) + }) .collect::>(); - walked_dependent_dependencies.extend(dependent_dependencies); + merge_changed_packages( + &mut walked_dependent_dependencies, + dependent_dependencies, + ); } } } @@ -334,20 +382,20 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { { // if we are including dependents or dependencies, and we are not excluding // ourselves, then we should add ourselves to the list of packages - walked_dependencies.insert(package); + walked_dependencies.insert(package, reason); } else if !selector.include_dependencies && !selector.include_dependents { // if we are neither including dependents or dependencies, then // add to the list of cherry picked packages - cherry_picked_packages.insert(package); + cherry_picked_packages.insert(package, reason); } } } - let mut all_packages = HashSet::new(); - all_packages.extend(walked_dependencies); - all_packages.extend(walked_dependents); - all_packages.extend(walked_dependent_dependencies); - all_packages.extend(cherry_picked_packages); + let mut all_packages = HashMap::new(); + merge_changed_packages(&mut all_packages, walked_dependencies); + merge_changed_packages(&mut all_packages, walked_dependents); + merge_changed_packages(&mut all_packages, walked_dependent_dependencies); + merge_changed_packages(&mut all_packages, cherry_picked_packages); Ok(all_packages) } @@ -355,7 +403,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn filter_graph_with_selector( &self, selector: &TargetSelector, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { if selector.match_dependencies { self.filter_subtrees_with_selector(selector) } else { @@ -375,8 +423,8 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn filter_subtrees_with_selector( &self, selector: &TargetSelector, - ) -> Result, ResolutionError> { - let mut entry_packages = HashSet::new(); + ) -> Result, ResolutionError> { + let mut entry_packages = HashMap::new(); for (name, info) in self.pkg_graph.packages() { if let Some(parent_dir) = selector.parent_dir.as_deref() { @@ -385,10 +433,20 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { let matches = parent_dir_matcher.is_match(info.package_path().as_path()); if matches { - entry_packages.insert(name.to_owned()); + entry_packages.insert( + name.to_owned(), + PackageInclusionReason::InFilteredDirectory { + directory: parent_dir.to_owned(), + }, + ); } } else { - entry_packages.insert(name.to_owned()); + entry_packages.insert( + name.to_owned(), + PackageInclusionReason::IncludedByFilter { + filters: vec![selector.raw.to_string()], + }, + ); } } @@ -399,26 +457,26 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { entry_packages }; - let mut roots = HashSet::new(); + let mut roots = HashMap::new(); let mut matched = HashSet::new(); let changed_packages = if let Some(git_range) = selector.git_range.as_ref() { self.packages_changed_in_range(git_range)? } else { - HashSet::new() + HashMap::default() }; - for package in filtered_entry_packages { + for (package, reason) in filtered_entry_packages { if matched.contains(&package) { - roots.insert(package); + roots.insert(package, reason); continue; } let workspace_node = package_graph::PackageNode::Workspace(package.clone()); let dependencies = self.pkg_graph.dependencies(&workspace_node); - for changed_package in &changed_packages { + for changed_package in changed_packages.keys() { if !selector.exclude_self && package.eq(changed_package) { - roots.insert(package); + roots.insert(package, reason); break; } @@ -426,7 +484,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { package_graph::PackageNode::Workspace(changed_package.to_owned()); if dependencies.contains(&changed_node) { - roots.insert(package.clone()); + roots.insert(package.clone(), reason); matched.insert(package); break; } @@ -439,8 +497,8 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn filter_nodes_with_selector( &self, selector: &TargetSelector, - ) -> Result, ResolutionError> { - let mut entry_packages = HashSet::new(); + ) -> Result, ResolutionError> { + let mut entry_packages = HashMap::new(); let mut selector_valid = false; let parent_dir_unix = selector.parent_dir.as_deref().map(|path| path.to_unix()); @@ -480,13 +538,13 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { .map(|(name, entry)| (name, entry.package_path())) .collect::>(); - for package in changed_packages { + for (package, reason) in changed_packages { if let Some(parent_dir_globber) = parent_dir_globber.as_ref() { if package == PackageName::Root { // The root package changed, only add it if // the parentDir is equivalent to the root if parent_dir_globber.matched(&Path::new(".").into()).is_some() { - entry_packages.insert(package); + entry_packages.insert(package, reason); } } else { let path = package_path_lookup @@ -494,11 +552,11 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { .ok_or(ResolutionError::MissingPackageInfo(package.to_string()))?; if parent_dir_globber.is_match(path.as_path()) { - entry_packages.insert(package); + entry_packages.insert(package, reason); } } } else { - entry_packages.insert(package); + entry_packages.insert(package, reason); } } } else if let Some((parent_dir, parent_dir_globber)) = selector @@ -508,21 +566,42 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { { selector_valid = true; if parent_dir == &*AnchoredSystemPathBuf::from_raw(".").expect("valid anchored") { - entry_packages.insert(PackageName::Root); + entry_packages.insert( + PackageName::Root, + PackageInclusionReason::InFilteredDirectory { + directory: parent_dir.to_owned(), + }, + ); } else { let packages = self.pkg_graph.packages(); for (name, _) in packages.filter(|(_name, info)| { let path = info.package_path().as_path(); parent_dir_globber.is_match(path) }) { - entry_packages.insert(name.to_owned()); + entry_packages.insert( + name.to_owned(), + PackageInclusionReason::InFilteredDirectory { + directory: parent_dir.to_owned(), + }, + ); } } } if !selector.name_pattern.is_empty() { if !selector_valid { - entry_packages = self.all_packages(); + entry_packages = self + .all_packages() + .into_iter() + .map(|name| { + ( + name, + PackageInclusionReason::IncludedByFilter { + filters: vec![selector.raw.to_string()], + }, + ) + }) + .collect(); selector_valid = true; } let all_packages = self.all_packages(); @@ -541,10 +620,10 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { } } - fn packages_changed_in_range( + pub fn packages_changed_in_range( &self, git_range: &GitRange, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { self.change_detector.changed_packages( git_range.from_ref.as_deref(), git_range.to_ref.as_deref(), @@ -572,8 +651,8 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> { fn match_package_names( name_pattern: &str, all_packages: &HashSet, - mut packages: HashSet, -) -> Result, ResolutionError> { + mut packages: HashMap, +) -> Result, ResolutionError> { let matcher = SimpleGlob::new(name_pattern)?; let matched_packages = all_packages .iter() @@ -588,12 +667,12 @@ fn match_package_names( )); } - packages.retain(|pkg| matched_packages.contains(pkg)); + packages.retain(|pkg, _| matched_packages.contains(pkg)); Ok(packages) } -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, Diagnostic)] pub enum ResolutionError { #[error("missing info for package")] MissingPackageInfo(String), @@ -623,7 +702,7 @@ pub enum ResolutionError { #[error("Directory '{0}' specified in filter does not exist")] DirectoryDoesNotExist(AbsoluteSystemPathBuf), #[error("failed to construct glob for globalDependencies")] - GlobalDependenciesGlob(#[from] global_deps_package_change_mapper::Error), + GlobalDependenciesGlob(#[from] turborepo_repository::change_mapper::Error), } #[cfg(test)] @@ -635,6 +714,7 @@ mod test { use test_case::test_case; use turbopath::{AbsoluteSystemPathBuf, AnchoredSystemPathBuf, RelativeUnixPathBuf}; use turborepo_repository::{ + change_mapper::PackageInclusionReason, discovery::PackageDiscovery, package_graph::{PackageGraph, PackageName, ROOT_PKG_NAME}, package_json::PackageJson, @@ -1024,7 +1104,10 @@ mod test { let packages = resolver.get_filtered_packages(selectors).unwrap(); assert_eq!( - packages, + packages + .into_iter() + .map(|(name, _)| name) + .collect::>(), expected.iter().map(|s| PackageName::from(*s)).collect() ); } @@ -1040,15 +1123,21 @@ mod test { let packages = resolver .get_filtered_packages(vec![TargetSelector { name_pattern: "bar".to_string(), + raw: "bar".to_string(), ..Default::default() }]) .unwrap(); assert_eq!( packages, - vec![PackageName::Other("bar".to_string())] - .into_iter() - .collect() + vec![( + PackageName::Other("bar".to_string()), + PackageInclusionReason::IncludedByFilter { + filters: vec!["bar".to_string()] + } + )] + .into_iter() + .collect() ); } @@ -1062,6 +1151,7 @@ mod test { ); let packages = resolver.get_filtered_packages(vec![TargetSelector { name_pattern: "bar".to_string(), + raw: "bar".to_string(), ..Default::default() }]); @@ -1070,12 +1160,21 @@ mod test { let packages = resolver .get_filtered_packages(vec![TargetSelector { name_pattern: "@foo/bar".to_string(), + raw: "@foo/bar".to_string(), ..Default::default() }]) .unwrap(); + assert_eq!( packages, - vec![PackageName::from("@foo/bar")].into_iter().collect() + vec![( + PackageName::from("@foo/bar"), + PackageInclusionReason::IncludedByFilter { + filters: vec!["@foo/bar".to_string()] + } + )] + .into_iter() + .collect() ); } @@ -1240,12 +1339,17 @@ mod test { let packages = resolver.get_filtered_packages(selectors).unwrap(); assert_eq!( - packages, + packages + .into_iter() + .map(|(name, _)| name) + .collect::>(), expected.iter().map(|s| PackageName::from(*s)).collect() ); } - struct TestChangeDetector<'a>(HashMap<(&'a str, Option<&'a str>), HashSet>); + struct TestChangeDetector<'a>( + HashMap<(&'a str, Option<&'a str>), HashMap>, + ); impl<'a> TestChangeDetector<'a> { fn new(pairs: &[(&'a str, Option<&'a str>, &[&'a str])]) -> Self { @@ -1253,7 +1357,16 @@ mod test { for (from, to, changed) in pairs { map.insert( (*from, *to), - changed.iter().map(|s| PackageName::from(*s)).collect(), + changed + .iter() + .map(|s| { + ( + PackageName::from(*s), + // This is just a random reason, + PackageInclusionReason::IncludedByFilter { filters: vec![] }, + ) + }) + .collect(), ); } @@ -1269,7 +1382,7 @@ mod test { _include_uncommitted: bool, _allow_unknown_objects: bool, _merge_base: bool, - ) -> Result, ResolutionError> { + ) -> Result, ResolutionError> { Ok(self .0 .get(&(from.expect("expected base branch"), to)) diff --git a/crates/turborepo-lib/src/run/scope/mod.rs b/crates/turborepo-lib/src/run/scope/mod.rs index c009f61a1329e..7bed4b3b19f00 100644 --- a/crates/turborepo-lib/src/run/scope/mod.rs +++ b/crates/turborepo-lib/src/run/scope/mod.rs @@ -1,13 +1,16 @@ mod change_detector; -mod filter; +pub mod filter; mod simple_glob; pub mod target_selector; -use std::collections::HashSet; +use std::collections::HashMap; use filter::{FilterResolver, PackageInference}; use turbopath::AbsoluteSystemPath; -use turborepo_repository::package_graph::{PackageGraph, PackageName}; +use turborepo_repository::{ + change_mapper::PackageInclusionReason, + package_graph::{PackageGraph, PackageName}, +}; use turborepo_scm::SCM; pub use crate::run::scope::filter::ResolutionError; @@ -20,7 +23,7 @@ pub fn resolve_packages( pkg_graph: &PackageGraph, scm: &SCM, root_turbo_json: &TurboJson, -) -> Result<(HashSet, bool), ResolutionError> { +) -> Result<(HashMap, bool), ResolutionError> { let pkg_inference = opts.pkg_inference_root.as_ref().map(|pkg_inference_path| { PackageInference::calculate(turbo_root, pkg_inference_path, pkg_graph) }); diff --git a/crates/turborepo-repository/src/change_mapper/mod.rs b/crates/turborepo-repository/src/change_mapper/mod.rs index c4e0b14d5f4f7..5b861802e7f9c 100644 --- a/crates/turborepo-repository/src/change_mapper/mod.rs +++ b/crates/turborepo-repository/src/change_mapper/mod.rs @@ -1,16 +1,20 @@ //! Maps changed files to changed packages in a repository. //! Used for both `--filter` and for isolated builds. -use std::collections::HashSet; +use std::{ + collections::{HashMap, HashSet}, + hash::Hash, +}; pub use package::{ - DefaultPackageChangeMapper, GlobalDepsPackageChangeMapper, PackageChangeMapper, PackageMapping, + DefaultPackageChangeMapper, Error, GlobalDepsPackageChangeMapper, PackageChangeMapper, + PackageMapping, }; use tracing::debug; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; use wax::Program; -use crate::package_graph::{ChangedPackagesError, PackageGraph, WorkspacePackage}; +use crate::package_graph::{ChangedPackagesError, PackageGraph, PackageName, WorkspacePackage}; mod package; @@ -23,19 +27,63 @@ pub enum LockfileChange { WithContent(Vec), } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Hash, Clone)] +pub enum PackageInclusionReason { + /// All the packages are invalidated + All(AllPackageChangeReason), + /// Root task was run + RootTask { task: String }, + /// We conservatively assume that the root package is changed because + /// the lockfile changed. + ConservativeRootLockfileChanged, + /// The lockfile changed and caused this package to be invalidated + LockfileChanged, + /// A transitive dependency of this package changed + DependencyChanged { dependency: PackageName }, + /// A transitive dependent of this package changed + DependentChanged { dependent: PackageName }, + /// A file contained in this package changed + FileChanged { file: AnchoredSystemPathBuf }, + /// The filter selected a directory which contains this package + InFilteredDirectory { directory: AnchoredSystemPathBuf }, + /// Package is automatically included because of the filter (or lack + /// thereof) + IncludedByFilter { filters: Vec }, +} + +#[derive(Debug, PartialEq, Eq, Hash, Clone)] pub enum AllPackageChangeReason { - DefaultGlobalFileChanged, + GlobalDepsChanged { + file: AnchoredSystemPathBuf, + }, + /// A file like `package.json` or `turbo.json` changed + DefaultGlobalFileChanged { + file: AnchoredSystemPathBuf, + }, LockfileChangeDetectionFailed, LockfileChangedWithoutDetails, - RootInternalDepChanged, - NonPackageFileChanged, + RootInternalDepChanged { + root_internal_dep: PackageName, + }, + GitRefNotFound { + from_ref: Option, + to_ref: Option, + }, +} + +pub fn merge_changed_packages( + changed_packages: &mut HashMap, + new_changes: impl IntoIterator, +) { + for (package, reason) in new_changes { + changed_packages.entry(package).or_insert(reason); + } } #[derive(Debug, PartialEq, Eq)] pub enum PackageChanges { All(AllPackageChangeReason), - Some(HashSet), + Some(HashMap), } pub struct ChangeMapper<'a, PD> { @@ -58,10 +106,12 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { } } - fn default_global_file_changed(changed_files: &HashSet) -> bool { + fn default_global_file_changed( + changed_files: &HashSet, + ) -> Option<&AnchoredSystemPathBuf> { changed_files .iter() - .any(|f| DEFAULT_GLOBAL_DEPS.iter().any(|dep| *dep == f.as_str())) + .find(|f| DEFAULT_GLOBAL_DEPS.iter().any(|dep| *dep == f.as_str())) } pub fn changed_packages( @@ -69,10 +119,12 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { changed_files: HashSet, lockfile_change: Option, ) -> Result { - if Self::default_global_file_changed(&changed_files) { + if let Some(file) = Self::default_global_file_changed(&changed_files) { debug!("global file changed"); return Ok(PackageChanges::All( - AllPackageChangeReason::DefaultGlobalFileChanged, + AllPackageChangeReason::DefaultGlobalFileChanged { + file: file.to_owned(), + }, )); } @@ -86,7 +138,8 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { 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) + let Ok(lockfile_changes) = + self.get_changed_packages_from_lockfile(&content) else { debug!( "unable to determine lockfile changes, assuming all packages \ @@ -100,7 +153,12 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { "found {} packages changed by lockfile", lockfile_changes.len() ); - changed_pkgs.extend(lockfile_changes); + merge_changed_packages( + &mut changed_pkgs, + lockfile_changes + .into_iter() + .map(|pkg| (pkg, PackageInclusionReason::LockfileChanged)), + ); Ok(PackageChanges::Some(changed_pkgs)) } @@ -134,11 +192,11 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { files: impl Iterator, ) -> PackageChanges { let root_internal_deps = self.pkg_graph.root_internal_package_dependencies(); - let mut changed_packages = HashSet::new(); + let mut changed_packages = HashMap::new(); for file in files { match self.package_detector.detect_package(file) { // Internal root dependency changed so global hash has changed - PackageMapping::Package(pkg) if root_internal_deps.contains(&pkg) => { + PackageMapping::Package((pkg, _)) if root_internal_deps.contains(&pkg) => { debug!( "{} changes root internal dependency: \"{}\"\nshortest path from root: \ {:?}", @@ -146,15 +204,17 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { pkg.name, self.pkg_graph.root_internal_dependency_explanation(&pkg), ); - return PackageChanges::All(AllPackageChangeReason::RootInternalDepChanged); + return PackageChanges::All(AllPackageChangeReason::RootInternalDepChanged { + root_internal_dep: pkg.name.clone(), + }); } - PackageMapping::Package(pkg) => { + PackageMapping::Package((pkg, reason)) => { debug!("{} changes \"{}\"", file.to_string(), pkg.name); - changed_packages.insert(pkg); + changed_packages.insert(pkg, reason); } - PackageMapping::All => { + PackageMapping::All(reason) => { debug!("all packages changed due to {file:?}"); - return PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged); + return PackageChanges::All(reason); } PackageMapping::None => {} } @@ -165,12 +225,12 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> { fn get_changed_packages_from_lockfile( &self, - lockfile_content: Vec, + lockfile_content: &[u8], ) -> Result, ChangeMapError> { let previous_lockfile = self .pkg_graph .package_manager() - .parse_lockfile(self.pkg_graph.root_package_json(), &lockfile_content)?; + .parse_lockfile(self.pkg_graph.root_package_json(), lockfile_content)?; let additional_packages = self .pkg_graph diff --git a/crates/turborepo-repository/src/change_mapper/package.rs b/crates/turborepo-repository/src/change_mapper/package.rs index c023b56ffe631..fa24e05d8e10e 100644 --- a/crates/turborepo-repository/src/change_mapper/package.rs +++ b/crates/turborepo-repository/src/change_mapper/package.rs @@ -1,16 +1,19 @@ use thiserror::Error; -use turbopath::AnchoredSystemPath; +use turbopath::{AnchoredSystemPath, AnchoredSystemPathBuf}; use wax::{BuildError, Program}; -use crate::package_graph::{PackageGraph, PackageName, WorkspacePackage}; +use crate::{ + change_mapper::{AllPackageChangeReason, PackageInclusionReason}, + package_graph::{PackageGraph, PackageName, WorkspacePackage}, +}; pub enum PackageMapping { /// We've hit a global file, so all packages have changed - All, + All(AllPackageChangeReason), /// This change is meaningless, no packages have changed None, /// This change has affected one package - Package(WorkspacePackage), + Package((WorkspacePackage, PackageInclusionReason)), } /// Maps a single file change to affected packages. This can be a single @@ -49,15 +52,22 @@ impl<'a> PackageChangeMapper for DefaultPackageChangeMapper<'a> { } if let Some(package_path) = entry.package_json_path.parent() { if Self::is_file_in_package(file, package_path) { - return PackageMapping::Package(WorkspacePackage { - name: name.clone(), - path: package_path.to_owned(), - }); + return PackageMapping::Package(( + WorkspacePackage { + name: name.clone(), + path: package_path.to_owned(), + }, + PackageInclusionReason::FileChanged { + file: file.to_owned(), + }, + )); } } } - PackageMapping::All + PackageMapping::All(AllPackageChangeReason::GlobalDepsChanged { + file: file.to_owned(), + }) } } @@ -94,18 +104,43 @@ impl<'a> GlobalDepsPackageChangeMapper<'a> { impl<'a> PackageChangeMapper for GlobalDepsPackageChangeMapper<'a> { fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping { + // If we have a lockfile change, we consider this as a root package change, + // since there's a chance that the root package uses a workspace package + // dependency (this is cursed behavior but sadly possible). There's a chance + // that we can make this more accurate by checking which package + // manager, since not all package managers may permit root pulling from + // workspace package dependencies + if matches!( + path.as_str(), + "package.json" | "pnpm-lock.yaml" | "yarn.lock" + ) { + return PackageMapping::Package(( + WorkspacePackage { + name: PackageName::Root, + path: AnchoredSystemPathBuf::from_raw("").unwrap(), + }, + PackageInclusionReason::ConservativeRootLockfileChanged, + )); + } match DefaultPackageChangeMapper::new(self.pkg_dep_graph).detect_package(path) { // Since `DefaultPackageChangeMapper` is overly conservative, we can check here if // the path is actually in globalDeps and if not, return it as // PackageDetection::Package(WorkspacePackage::root()). - PackageMapping::All => { + PackageMapping::All(_) => { let cleaned_path = path.clean(); let in_global_deps = self.global_deps_matcher.is_match(cleaned_path.as_str()); if in_global_deps { - PackageMapping::All + PackageMapping::All(AllPackageChangeReason::GlobalDepsChanged { + file: path.to_owned(), + }) } else { - PackageMapping::Package(WorkspacePackage::root()) + PackageMapping::Package(( + WorkspacePackage::root(), + PackageInclusionReason::FileChanged { + file: path.to_owned(), + }, + )) } } result => result, @@ -120,7 +155,9 @@ mod tests { use super::{DefaultPackageChangeMapper, GlobalDepsPackageChangeMapper}; use crate::{ - change_mapper::{AllPackageChangeReason, ChangeMapper, PackageChanges}, + change_mapper::{ + AllPackageChangeReason, ChangeMapper, PackageChanges, PackageInclusionReason, + }, discovery, discovery::PackageDiscovery, package_graph::{PackageGraphBuilder, WorkspacePackage}, @@ -174,7 +211,9 @@ mod tests { // therefore must be conservative about changes assert_eq!( package_changes, - PackageChanges::All(AllPackageChangeReason::NonPackageFileChanged) + PackageChanges::All(AllPackageChangeReason::GlobalDepsChanged { + file: AnchoredSystemPathBuf::from_raw("README.md")?, + }) ); let turbo_package_detector = @@ -192,7 +231,16 @@ mod tests { // README.md is not one of them assert_eq!( package_changes, - PackageChanges::Some([WorkspacePackage::root()].into_iter().collect()) + PackageChanges::Some( + [( + WorkspacePackage::root(), + PackageInclusionReason::FileChanged { + file: AnchoredSystemPathBuf::from_raw("README.md")?, + } + )] + .into_iter() + .collect() + ) ); Ok(()) diff --git a/crates/turborepo-scm/src/git.rs b/crates/turborepo-scm/src/git.rs index fecfbae6f7bf8..0c6751a9dffc8 100644 --- a/crates/turborepo-scm/src/git.rs +++ b/crates/turborepo-scm/src/git.rs @@ -16,10 +16,10 @@ use turborepo_ci::Vendor; use crate::{Error, Git, SCM}; -#[derive(Debug)] -pub enum ChangedFiles { - All, - Some(HashSet), +#[derive(Debug, PartialEq, Eq)] +pub struct InvalidRange { + pub from_ref: Option, + pub to_ref: Option, } impl SCM { @@ -45,13 +45,17 @@ impl SCM { include_uncommitted: bool, allow_unknown_objects: bool, merge_base: bool, - ) -> Result { - fn unable_to_detect_range(error: impl std::error::Error) -> Result { + ) -> Result, InvalidRange>, Error> { + fn unable_to_detect_range( + error: impl std::error::Error, + from_ref: Option, + to_ref: Option, + ) -> Result, InvalidRange>, Error> { warn!( "unable to detect git range, assuming all files have changed: {}", error ); - Ok(ChangedFiles::All) + Ok(Err(InvalidRange { from_ref, to_ref })) } match self { Self::Git(git) => { @@ -62,17 +66,23 @@ impl SCM { include_uncommitted, merge_base, ) { - Ok(files) => Ok(ChangedFiles::Some(files)), + Ok(files) => Ok(Ok(files)), Err(ref error @ Error::Git(ref message, _)) if allow_unknown_objects && (message.contains("no merge base") || message.contains("bad object")) => { - unable_to_detect_range(error) - } - Err(Error::UnableToResolveRef) => { - unable_to_detect_range(Error::UnableToResolveRef) + unable_to_detect_range( + error, + from_commit.map(|c| c.to_string()), + to_commit.map(|c| c.to_string()), + ) } + Err(Error::UnableToResolveRef) => unable_to_detect_range( + Error::UnableToResolveRef, + from_commit.map(|c| c.to_string()), + to_commit.map(|c| c.to_string()), + ), Err(e) => Err(e), } } @@ -402,7 +412,7 @@ mod tests { use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, PathError}; use which::which; - use super::{previous_content, CIEnv, ChangedFiles}; + use super::{previous_content, CIEnv, InvalidRange}; use crate::{ git::{GitHubCommit, GitHubEvent}, Error, Git, SCM, @@ -438,7 +448,7 @@ mod tests { // Replicating the `--filter` behavior where we only do a merge base // if both ends of the git range are specified. let merge_base = to_commit.is_some(); - let ChangedFiles::Some(files) = scm.changed_files( + let Ok(files) = scm.changed_files( &turbo_root, from_commit, to_commit, @@ -1010,7 +1020,13 @@ mod tests { .changed_files(&root, None, Some("HEAD"), true, true, false) .unwrap(); - assert_matches!(actual, ChangedFiles::All); + assert_eq!( + actual, + Err(InvalidRange { + from_ref: None, + to_ref: Some("HEAD".to_string()), + }) + ); Ok(()) } diff --git a/packages/turbo-repository/rust/src/lib.rs b/packages/turbo-repository/rust/src/lib.rs index db3d043b676f0..626d0b38e6206 100644 --- a/packages/turbo-repository/rust/src/lib.rs +++ b/packages/turbo-repository/rust/src/lib.rs @@ -219,7 +219,7 @@ impl Workspace { path: info.package_path().to_owned(), }) .collect::>(), - PackageChanges::Some(packages) => packages.into_iter().collect(), + PackageChanges::Some(packages) => packages.into_iter().map(|(p, _)| p).collect(), }; let mut serializable_packages: Vec = packages diff --git a/turborepo-tests/integration/tests/affected.t b/turborepo-tests/integration/tests/affected.t index 50cb1f13bdbea..eed12e6dbe6ad 100644 --- a/turborepo-tests/integration/tests/affected.t +++ b/turborepo-tests/integration/tests/affected.t @@ -43,14 +43,17 @@ Do the same thing with the `ls` command Do the same thing with the `query` command - $ ${TURBO} query "query { affectedPackages { items { name } } }" + $ ${TURBO} query "query { affectedPackages { items { name reason { __typename } } } }" WARNING query command is experimental and may change in the future { "data": { "affectedPackages": { "items": [ { - "name": "my-app" + "name": "my-app", + "reason": { + "__typename": "FileChanged" + } } ] } @@ -61,6 +64,36 @@ Do the same thing with the `query` command Remove the new file $ rm apps/my-app/new.js +Add a file in `util` + $ echo "hello world" > packages/util/new.js + +Validate that both `my-app` and `util` are affected + $ ${TURBO} query "query { affectedPackages { items { name reason { __typename } } } }" + WARNING query command is experimental and may change in the future + { + "data": { + "affectedPackages": { + "items": [ + { + "name": "my-app", + "reason": { + "__typename": "DependencyChanged" + } + }, + { + "name": "util", + "reason": { + "__typename": "FileChanged" + } + } + ] + } + } + } + +Remove the new file + $ rm packages/util/new.js + Add field to `apps/my-app/package.json` $ jq '. += {"description": "foo"}' apps/my-app/package.json | tr -d '\r' > apps/my-app/package.json.new $ mv apps/my-app/package.json.new apps/my-app/package.json @@ -92,14 +125,17 @@ Do the same thing with the `ls` command Do the same thing with the `query` command - $ ${TURBO} query "query { affectedPackages { items { name } } }" + $ ${TURBO} query "query { affectedPackages { items { name reason { __typename } } } }" WARNING query command is experimental and may change in the future { "data": { "affectedPackages": { "items": [ { - "name": "my-app" + "name": "my-app", + "reason": { + "__typename": "FileChanged" + } } ] } diff --git a/turborepo-tests/integration/tests/lockfile-aware-caching/pnpm.t b/turborepo-tests/integration/tests/lockfile-aware-caching/pnpm.t index 4c453bb7cfea5..45ff462ccf3b8 100644 --- a/turborepo-tests/integration/tests/lockfile-aware-caching/pnpm.t +++ b/turborepo-tests/integration/tests/lockfile-aware-caching/pnpm.t @@ -80,7 +80,32 @@ Only root and b should be rebuilt since only the deps for b had a version bump "//", "b" ] - + +This should be annotated as a `ConservativeRootLockfileChanged` because the root package may pull from the workspace packages' dependencies (even though this is cursed) + $ ${TURBO} query "query { affectedPackages(base: \"HEAD~1\") { items { name reason { __typename } } } }" | jq + WARNING query command is experimental and may change in the future + { + "data": { + "affectedPackages": { + "items": [ + { + "name": "//", + "reason": { + "__typename": "ConservativeRootLockfileChanged" + } + }, + { + "name": "b", + "reason": { + "__typename": "LockfileChanged" + } + } + ] + } + } + } + + Bump of root workspace invalidates all packages $ patch pnpm-lock.yaml turbo-bump.patch patching file pnpm-lock.yaml diff --git a/turborepo-tests/integration/tests/lockfile-aware-caching/yarn.t b/turborepo-tests/integration/tests/lockfile-aware-caching/yarn.t index c9dee24495ca1..00e687b46999f 100644 --- a/turborepo-tests/integration/tests/lockfile-aware-caching/yarn.t +++ b/turborepo-tests/integration/tests/lockfile-aware-caching/yarn.t @@ -80,7 +80,32 @@ Only root and b should be rebuilt since only the deps for b had a version bump "//", "b" ] - + +This should be annotated as a `ConservativeRootLockfileChanged` because the root package may pull from the workspace packages' dependencies (even though this is cursed) + $ ${TURBO} query "query { affectedPackages(base: \"HEAD~1\") { items { name reason { __typename } } } }" | jq + WARNING query command is experimental and may change in the future + { + "data": { + "affectedPackages": { + "items": [ + { + "name": "//", + "reason": { + "__typename": "ConservativeRootLockfileChanged" + } + }, + { + "name": "b", + "reason": { + "__typename": "LockfileChanged" + } + } + ] + } + } + } + + Bump of root workspace invalidates all packages $ patch yarn.lock turbo-bump.patch patching file yarn.lock