Skip to content

Commit 726785f

Browse files
fix: affected_packages's optimization flow (#9950)
### Description This PR addresses the `affected_packages` API and its handling of the optimization for lockfiles. It should not change the current behaviors without this optimization. ### Testing Instructions We released canary.14 in this workflow (straight from this branch), and have tested it on the api side and confirmed that it works. --------- Co-authored-by: Chris Olszewski <[email protected]>
1 parent efbb5e1 commit 726785f

File tree

20 files changed

+234
-118
lines changed

20 files changed

+234
-118
lines changed

Cargo.lock

+4
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/turborepo-lib/src/package_changes_watcher.rs

+5-2
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ use turborepo_filewatch::{
1515
NotifyError, OptionalWatch,
1616
};
1717
use turborepo_repository::{
18-
change_mapper::{ChangeMapper, GlobalDepsPackageChangeMapper, PackageChanges},
18+
change_mapper::{
19+
ChangeMapper, GlobalDepsPackageChangeMapper, LockfileContents, PackageChanges,
20+
},
1921
package_graph::{PackageGraph, PackageGraphBuilder, PackageName, WorkspacePackage},
2022
package_json::PackageJson,
2123
};
@@ -342,7 +344,8 @@ impl Subscriber {
342344
continue;
343345
}
344346

345-
let changed_packages = change_mapper.changed_packages(changed_files.clone(), None);
347+
let changed_packages = change_mapper
348+
.changed_packages(changed_files.clone(), LockfileContents::Unchanged);
346349

347350
tracing::warn!("changed_files: {:?}", changed_files);
348351
tracing::warn!("changed_packages: {:?}", changed_packages);

crates/turborepo-lib/src/run/scope/change_detector.rs

+10-13
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf};
55
use turborepo_repository::{
66
change_mapper::{
77
AllPackageChangeReason, ChangeMapper, DefaultPackageChangeMapper, Error,
8-
GlobalDepsPackageChangeMapper, PackageChanges, PackageInclusionReason,
8+
GlobalDepsPackageChangeMapper, LockfileContents, PackageChanges, PackageInclusionReason,
99
},
1010
package_graph::{PackageGraph, PackageName},
1111
};
@@ -52,13 +52,12 @@ impl<'a> ScopeChangeDetector<'a> {
5252
}
5353

5454
/// Gets the lockfile content from SCM if it has changed.
55-
/// Does *not* error if cannot get content, instead just
56-
/// returns a Some(None)
57-
fn get_lockfile_contents(
55+
/// Does *not* error if cannot get content.
56+
pub fn get_lockfile_contents(
5857
&self,
5958
from_ref: Option<&str>,
6059
changed_files: &HashSet<AnchoredSystemPathBuf>,
61-
) -> Option<Option<Vec<u8>>> {
60+
) -> LockfileContents {
6261
let lockfile_path = self
6362
.pkg_graph
6463
.package_manager()
@@ -70,23 +69,21 @@ impl<'a> ScopeChangeDetector<'a> {
7069
&lockfile_path,
7170
) {
7271
debug!("lockfile did not change");
73-
return None;
72+
return LockfileContents::Unchanged;
7473
}
7574

76-
let lockfile_path = self
77-
.pkg_graph
78-
.package_manager()
79-
.lockfile_path(self.turbo_root);
80-
8175
let Ok(content) = self.scm.previous_content(from_ref, &lockfile_path) else {
82-
return Some(None);
76+
debug!("lockfile did change but could not get previous content");
77+
return LockfileContents::UnknownChange;
8378
};
8479

85-
Some(Some(content))
80+
debug!("lockfile changed, have the previous content");
81+
LockfileContents::Changed(content)
8682
}
8783
}
8884

8985
impl<'a> GitChangeDetector for ScopeChangeDetector<'a> {
86+
/// get the actual changed packages between two git refs
9087
fn changed_packages(
9188
&self,
9289
from_ref: Option<&str>,

crates/turborepo-repository/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ biome_diagnostics = { workspace = true }
1717
biome_json_parser = { workspace = true }
1818
biome_json_syntax = { workspace = true }
1919

20+
either = { workspace = true }
2021
globwalk = { version = "0.1.0", path = "../turborepo-globwalk" }
2122
itertools = { workspace = true }
2223
lazy-regex = "2.5.0"

crates/turborepo-repository/src/change_mapper/mod.rs

+32-15
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ use std::{
77
};
88

99
pub use package::{
10-
DefaultPackageChangeMapper, Error, GlobalDepsPackageChangeMapper, PackageChangeMapper,
11-
PackageMapping,
10+
DefaultPackageChangeMapper, DefaultPackageChangeMapperWithLockfile, Error,
11+
GlobalDepsPackageChangeMapper, PackageChangeMapper, PackageMapping,
1212
};
1313
use tracing::debug;
1414
use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf};
@@ -29,6 +29,19 @@ pub enum LockfileChange {
2929
ChangedPackages(HashSet<turborepo_lockfiles::Package>),
3030
}
3131

32+
/// This describes the state of a change to a lockfile.
33+
pub enum LockfileContents {
34+
/// We know the lockfile did not change
35+
Unchanged,
36+
/// We know the lockfile changed but don't have the file contents of the
37+
/// previous lockfile (i.e. `git status`, or perhaps a lockfile that was
38+
/// deleted or otherwise inaccessible with the information we have)
39+
UnknownChange,
40+
/// We know the lockfile changed and have the contents of the previous
41+
/// lockfile
42+
Changed(Vec<u8>),
43+
}
44+
3245
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
3346
pub enum PackageInclusionReason {
3447
/// All the packages are invalidated
@@ -122,13 +135,7 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> {
122135
pub fn changed_packages(
123136
&self,
124137
changed_files: HashSet<AnchoredSystemPathBuf>,
125-
// None - we don't know if the lockfile changed
126-
//
127-
// Some(None) - we know the lockfile changed, but don't know exactly why (i.e. `git status`
128-
// and the lockfile is there)
129-
//
130-
// Some(Some(content)) - we know the lockfile changed and have the contents
131-
lockfile_change: Option<Option<Vec<u8>>>,
138+
lockfile_contents: LockfileContents,
132139
) -> Result<PackageChanges, ChangeMapError> {
133140
if let Some(file) = Self::default_global_file_changed(&changed_files) {
134141
debug!("global file changed");
@@ -142,15 +149,16 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> {
142149
// get filtered files and add the packages that contain them
143150
let filtered_changed_files = self.filter_ignored_files(changed_files.iter())?;
144151

152+
// calculate lockfile_change here based on changed_files
145153
match self.get_changed_packages(filtered_changed_files.into_iter()) {
146154
PackageChanges::All(reason) => Ok(PackageChanges::All(reason)),
147155

148156
PackageChanges::Some(mut changed_pkgs) => {
149-
match lockfile_change {
150-
Some(Some(content)) => {
157+
match lockfile_contents {
158+
LockfileContents::Changed(previous_lockfile_contents) => {
151159
// if we run into issues, don't error, just assume all packages have changed
152160
let Ok(lockfile_changes) =
153-
self.get_changed_packages_from_lockfile(&content)
161+
self.get_changed_packages_from_lockfile(&previous_lockfile_contents)
154162
else {
155163
debug!(
156164
"unable to determine lockfile changes, assuming all packages \
@@ -183,13 +191,22 @@ impl<'a, PD: PackageChangeMapper> ChangeMapper<'a, PD> {
183191
}
184192

185193
// We don't have the actual contents, so just invalidate everything
186-
Some(None) => {
187-
debug!("no previous lockfile available, assuming all packages changed");
194+
LockfileContents::UnknownChange => {
195+
// this can happen in a blobless checkout
196+
debug!(
197+
"we know the lockfile changed but we don't have the contents so we \
198+
have to assume all packages changed and rebuild everything"
199+
);
188200
Ok(PackageChanges::All(
189201
AllPackageChangeReason::LockfileChangedWithoutDetails,
190202
))
191203
}
192-
None => Ok(PackageChanges::Some(changed_pkgs)),
204+
205+
// We don't know if the lockfile changed or not, so we can't assume anything
206+
LockfileContents::Unchanged => {
207+
debug!("the lockfile did not change");
208+
Ok(PackageChanges::Some(changed_pkgs))
209+
}
193210
}
194211
}
195212
}

crates/turborepo-repository/src/change_mapper/package.rs

+58-24
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,19 @@ pub trait PackageChangeMapper {
2424
fn detect_package(&self, file: &AnchoredSystemPath) -> PackageMapping;
2525
}
2626

27+
impl<L, R> PackageChangeMapper for either::Either<L, R>
28+
where
29+
L: PackageChangeMapper,
30+
R: PackageChangeMapper,
31+
{
32+
fn detect_package(&self, file: &AnchoredSystemPath) -> PackageMapping {
33+
match self {
34+
either::Either::Left(l) => l.detect_package(file),
35+
either::Either::Right(r) => r.detect_package(file),
36+
}
37+
}
38+
}
39+
2740
/// Detects package by checking if the file is inside the package.
2841
///
2942
/// Does *not* use the `globalDependencies` in turbo.json.
@@ -73,6 +86,43 @@ impl PackageChangeMapper for DefaultPackageChangeMapper<'_> {
7386
}
7487
}
7588

89+
pub struct DefaultPackageChangeMapperWithLockfile<'a> {
90+
base: DefaultPackageChangeMapper<'a>,
91+
}
92+
93+
impl<'a> DefaultPackageChangeMapperWithLockfile<'a> {
94+
pub fn new(pkg_dep_graph: &'a PackageGraph) -> Self {
95+
Self {
96+
base: DefaultPackageChangeMapper::new(pkg_dep_graph),
97+
}
98+
}
99+
}
100+
101+
impl PackageChangeMapper for DefaultPackageChangeMapperWithLockfile<'_> {
102+
fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping {
103+
// If we have a lockfile change, we consider this as a root package change,
104+
// since there's a chance that the root package uses a workspace package
105+
// dependency (this is cursed behavior but sadly possible). There's a chance
106+
// that we can make this more accurate by checking which package
107+
// manager, since not all package managers may permit root pulling from
108+
// workspace package dependencies
109+
if PackageManager::supported_managers()
110+
.iter()
111+
.any(|pm| pm.lockfile_name() == path.as_str())
112+
{
113+
PackageMapping::Package((
114+
WorkspacePackage {
115+
name: PackageName::Root,
116+
path: AnchoredSystemPathBuf::from_raw("").unwrap(),
117+
},
118+
PackageInclusionReason::ConservativeRootLockfileChanged,
119+
))
120+
} else {
121+
self.base.detect_package(path)
122+
}
123+
}
124+
}
125+
76126
#[derive(Error, Debug)]
77127
pub enum Error {
78128
#[error(transparent)]
@@ -88,7 +138,7 @@ pub enum Error {
88138
/// changes all packages. Since we have a list of global deps,
89139
/// we can check against that and avoid invalidating in unnecessary cases.
90140
pub struct GlobalDepsPackageChangeMapper<'a> {
91-
pkg_dep_graph: &'a PackageGraph,
141+
base: DefaultPackageChangeMapperWithLockfile<'a>,
92142
global_deps_matcher: wax::Any<'a>,
93143
}
94144

@@ -97,36 +147,19 @@ impl<'a> GlobalDepsPackageChangeMapper<'a> {
97147
pkg_dep_graph: &'a PackageGraph,
98148
global_deps: I,
99149
) -> Result<Self, Error> {
150+
let base = DefaultPackageChangeMapperWithLockfile::new(pkg_dep_graph);
100151
let global_deps_matcher = wax::any(global_deps)?;
101152

102153
Ok(Self {
103-
pkg_dep_graph,
154+
base,
104155
global_deps_matcher,
105156
})
106157
}
107158
}
108159

109160
impl PackageChangeMapper for GlobalDepsPackageChangeMapper<'_> {
110161
fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping {
111-
// If we have a lockfile change, we consider this as a root package change,
112-
// since there's a chance that the root package uses a workspace package
113-
// dependency (this is cursed behavior but sadly possible). There's a chance
114-
// that we can make this more accurate by checking which package
115-
// manager, since not all package managers may permit root pulling from
116-
// workspace package dependencies
117-
if PackageManager::supported_managers()
118-
.iter()
119-
.any(|pm| pm.lockfile_name() == path.as_str())
120-
{
121-
return PackageMapping::Package((
122-
WorkspacePackage {
123-
name: PackageName::Root,
124-
path: AnchoredSystemPathBuf::from_raw("").unwrap(),
125-
},
126-
PackageInclusionReason::ConservativeRootLockfileChanged,
127-
));
128-
}
129-
match DefaultPackageChangeMapper::new(self.pkg_dep_graph).detect_package(path) {
162+
match self.base.detect_package(path) {
130163
// Since `DefaultPackageChangeMapper` is overly conservative, we can check here if
131164
// the path is actually in globalDeps and if not, return it as
132165
// PackageDetection::Package(WorkspacePackage::root()).
@@ -160,7 +193,8 @@ mod tests {
160193
use super::{DefaultPackageChangeMapper, GlobalDepsPackageChangeMapper};
161194
use crate::{
162195
change_mapper::{
163-
AllPackageChangeReason, ChangeMapper, PackageChanges, PackageInclusionReason,
196+
AllPackageChangeReason, ChangeMapper, LockfileContents, PackageChanges,
197+
PackageInclusionReason,
164198
},
165199
discovery::{self, PackageDiscovery},
166200
package_graph::{PackageGraphBuilder, WorkspacePackage},
@@ -208,7 +242,7 @@ mod tests {
208242
[AnchoredSystemPathBuf::from_raw("README.md")?]
209243
.into_iter()
210244
.collect(),
211-
None,
245+
LockfileContents::Unchanged,
212246
)?;
213247

214248
// We should return All because we don't have global deps and
@@ -228,7 +262,7 @@ mod tests {
228262
[AnchoredSystemPathBuf::from_raw("README.md")?]
229263
.into_iter()
230264
.collect(),
231-
None,
265+
LockfileContents::Unchanged,
232266
)?;
233267

234268
// We only get a root workspace change since we have global deps specified and

crates/turborepo-scm/src/git.rs

+1
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ impl SCM {
3737
}
3838
}
3939

40+
/// get the actual changed files between two git refs
4041
pub fn changed_files(
4142
&self,
4243
turbo_root: &AbsoluteSystemPath,

0 commit comments

Comments
 (0)