Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 10 commits into from
Jul 31, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a73d5ea
feat(turborepo): Log reason why all packages were considered changed
mehulkar Jul 30, 2024
21202d7
fixes
mehulkar Jul 30, 2024
b4a2c4e
rm import
mehulkar Jul 30, 2024
23ce90d
Merge branch 'main' into mk/log-why-all-changes
mehulkar Jul 30, 2024
6ab0fcd
fixup
mehulkar Jul 30, 2024
15fb4b5
Merge branch 'main' into mk/log-why-all-changes
mehulkar Jul 30, 2024
e23ff50
import
mehulkar Jul 30, 2024
a4f8dde
test
mehulkar Jul 30, 2024
5683838
Update crates/turborepo-repository/src/change_mapper/package.rs
chris-olszewski Jul 31, 2024
8d79542
bring back a debug
mehulkar Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions crates/turborepo-lib/src/global_deps_package_change_mapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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>())?;
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-lib/src/package_changes_watcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions crates/turborepo-lib/src/run/scope/change_detector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
88 changes: 54 additions & 34 deletions crates/turborepo-repository/src/change_mapper/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,18 @@ pub enum LockfileChange {
WithContent(Vec<u8>),
}

#[derive(Debug, PartialEq, Eq)]
pub enum AllPackageChangeReason {
DefaultGlobalFileChanged,
LockfileChangeDetectionFailed,
LockfileChangedWithoutDetails,
RootInternalDepChanged,
NonPackageFileChanged,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called this reason "NonPackageFileChanged" instead of "GlobalFileChanged", because the latter is a little bit confusing. E.g. .github/actions/whatever.yml is a global file and so is examples/some-app/whatever.js if neither path falls in the workspace config. It's not always clear that these are "global files" as far as turbo is concerned. I'm open to better name though.

}

#[derive(Debug, PartialEq, Eq)]
pub enum PackageChanges {
All,
All(AllPackageChangeReason),
Some(HashSet<WorkspacePackage>),
}

Expand Down Expand Up @@ -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);
};
Comment on lines -70 to -74
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to pass through the reason to PackageChanges::All here, so converted this to a longer match resulting in a larger diff in this file.


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)),
}
}

Expand All @@ -120,18 +140,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);
mehulkar marked this conversation as resolved.
Show resolved Hide resolved
}
PackageMapping::None => {}
}
Expand Down
7 changes: 5 additions & 2 deletions crates/turborepo-repository/src/change_mapper/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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)
chris-olszewski marked this conversation as resolved.
Show resolved Hide resolved
);

let turbo_package_detector =
GlobalDepsPackageChangeMapper::new(&pkg_graph, std::iter::empty::<&str>())?;
Expand Down
2 changes: 1 addition & 1 deletion packages/turbo-repository/rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl Workspace {
};

let packages = match package_changes {
PackageChanges::All => self
PackageChanges::All(_) => self
.graph
.packages()
.map(|(name, info)| WorkspacePackage {
Expand Down
Loading