Skip to content

Commit

Permalink
fix(Turborepo): Handle new packages in lockfile comparisons (#8127)
Browse files Browse the repository at this point in the history
### Description

When comparing lockfiles between revisions to identify what packages
have been affected, we treated missing packages in the previous lockfile
(added in the new lockfile) as errors, and fell back to considering all
packages as affected. This change adds a boolean to determine what the
behavior should be when we can't find a package that we expect. When
building the package graph, it continues to be an error, as well as when
tracing the dependencies of a package we've already found in the
lockfile. However, for the first round of packages that we look for in a
previous lockfile, we ignore missing packages. This allows us to more
gracefully handle comparisons between commits that add new packages.

### Testing Instructions

Added an integration test inspired by the repro from the linked issue.

Fixes #8125

---------

Co-authored-by: Greg Soltis <Greg Soltis>
  • Loading branch information
Greg Soltis authored May 13, 2024
1 parent cc565e8 commit 04248f4
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 6 deletions.
4 changes: 3 additions & 1 deletion crates/turborepo-lockfiles/src/berry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,7 @@ mod test {
&lockfile,
"apps/docs",
HashMap::from_iter(vec![("lodash".into(), "^4.17.21".into())]),
false,
)
.unwrap();

Expand Down Expand Up @@ -883,7 +884,7 @@ mod test {
.map(|(k, v)| (k.to_string(), v.to_string()))
.collect();

let closure = transitive_closure(&lockfile, "packages/ui", unresolved_deps).unwrap();
let closure = transitive_closure(&lockfile, "packages/ui", unresolved_deps, false).unwrap();

assert!(closure.contains(&Package {
key: "ajv@npm:8.11.2".into(),
Expand Down Expand Up @@ -1063,6 +1064,7 @@ mod test {
)]
.into_iter()
.collect(),
false,
)
.unwrap();

Expand Down
29 changes: 26 additions & 3 deletions crates/turborepo-lockfiles/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,17 @@ pub trait Lockfile: Send + Sync + Any + std::fmt::Debug {
pub fn all_transitive_closures<L: Lockfile + ?Sized>(
lockfile: &L,
workspaces: HashMap<String, HashMap<String, String>>,
ignore_missing_packages: bool,
) -> Result<HashMap<String, HashSet<Package>>, Error> {
workspaces
.into_par_iter()
.map(|(workspace, unresolved_deps)| {
let closure = transitive_closure(lockfile, &workspace, unresolved_deps)?;
let closure = transitive_closure(
lockfile,
&workspace,
unresolved_deps,
ignore_missing_packages,
)?;
Ok((workspace, closure))
})
.collect()
Expand All @@ -85,13 +91,15 @@ pub fn transitive_closure<L: Lockfile + ?Sized>(
lockfile: &L,
workspace_path: &str,
unresolved_deps: HashMap<String, String>,
ignore_missing_packages: bool,
) -> Result<HashSet<Package>, Error> {
let mut transitive_deps = HashSet::new();
transitive_closure_helper(
lockfile,
workspace_path,
unresolved_deps,
&mut transitive_deps,
ignore_missing_packages,
)?;

Ok(transitive_deps)
Expand All @@ -102,9 +110,16 @@ fn transitive_closure_helper<L: Lockfile + ?Sized>(
workspace_path: &str,
unresolved_deps: HashMap<String, impl AsRef<str>>,
resolved_deps: &mut HashSet<Package>,
ignore_missing_packages: bool,
) -> Result<(), Error> {
for (name, specifier) in unresolved_deps {
let pkg = lockfile.resolve_package(workspace_path, &name, specifier.as_ref())?;
let pkg = match lockfile.resolve_package(workspace_path, &name, specifier.as_ref()) {
Ok(pkg) => pkg,
Err(Error::MissingWorkspace(_)) if ignore_missing_packages => {
continue;
}
Err(e) => return Err(e),
};

match pkg {
None => {
Expand All @@ -117,7 +132,15 @@ fn transitive_closure_helper<L: Lockfile + ?Sized>(
let all_deps = lockfile.all_dependencies(&pkg.key)?;
resolved_deps.insert(pkg);
if let Some(deps) = all_deps {
transitive_closure_helper(lockfile, workspace_path, deps, resolved_deps)?;
// we've already found one unresolved dependency, so we can't ignore its set of
// dependencies.
transitive_closure_helper(
lockfile,
workspace_path,
deps,
resolved_deps,
false,
)?;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/turborepo-lockfiles/src/npm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ mod test {
]
.into_iter()
.collect(),
false,
)?;
assert!(closures.get("packages/a").unwrap().contains(&Package {
key: "node_modules/eslint-plugin-turbo".into(),
Expand Down
4 changes: 3 additions & 1 deletion crates/turborepo-lockfiles/src/pnpm/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,7 @@ mod tests {
)]
.into_iter()
.collect(),
false,
)
.unwrap();

Expand Down Expand Up @@ -1037,6 +1038,7 @@ c:
)]
.into_iter()
.collect(),
false,
)
.unwrap();

Expand Down Expand Up @@ -1113,7 +1115,7 @@ c:
.map(|(k, v)| (k.to_owned(), v.to_owned()))
.collect(),
);
let mut closures: Vec<_> = crate::all_transitive_closures(&lockfile, workspaces)
let mut closures: Vec<_> = crate::all_transitive_closures(&lockfile, workspaces, false)
.unwrap()
.into_iter()
.map(|(k, v)| (k, v.into_iter().sorted().collect::<Vec<_>>()))
Expand Down
3 changes: 3 additions & 0 deletions crates/turborepo-repository/src/package_graph/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,9 +507,12 @@ impl<'a, T: PackageDiscovery> BuildState<'a, ResolvedLockfile, T> {
return Ok(());
};

// We cannot ignore missing packages in this context, it would indicate a
// malformed or stale lockfile.
let mut closures = turborepo_lockfiles::all_transitive_closures(
lockfile,
self.all_external_dependencies()?,
false,
)?;
for (_, entry) in self.workspaces.iter_mut() {
entry.transitive_dependencies = closures.remove(&entry.unix_dir_str()?);
Expand Down
7 changes: 6 additions & 1 deletion crates/turborepo-repository/src/package_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,12 @@ impl PackageGraph {
})
.collect::<HashMap<_, HashMap<_, _>>>();

let closures = turborepo_lockfiles::all_transitive_closures(previous, external_deps)?;
// We're comparing to a previous lockfile, it's possible that a package was
// added and thus won't exist in the previous lockfile. In that case,
// we're fine to ignore it. Assuming there is not a commit with a stale
// lockfile, the same commit should add the package, so it will get
// picked up as changed.
let closures = turborepo_lockfiles::all_transitive_closures(previous, external_deps, true)?;

let global_change = current.global_change(previous);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
Setup
$ . ${TESTDIR}/../../../helpers/setup.sh
$ . ${TESTDIR}/setup.sh $(pwd) pnpm

Add new package with an external dependency
$ mkdir -p apps/c
$ echo '{"name":"c", "dependencies": {"has-symbols": "^1.0.3"}}' > apps/c/package.json

Update lockfile
$ pnpm i --frozen-lockfile=false > /dev/null

Now build and verify that only the new package is in scope
Note that we need --skip-infer because we've now installed a local
turbo in this repo
Note that we need to disable path conversion because on windows, git bash considers
'//' to be an escape sequence translating to '/'.
$ MSYS_NO_PATHCONV=1 ${TURBO} --skip-infer build -F '[HEAD]' -F '!//' --dry=json | jq '.packages'
[
"c"
]


0 comments on commit 04248f4

Please sign in to comment.