From 7d440dd1a161116af6df51b4165866b96eef6cb3 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Fri, 14 Jun 2024 10:29:44 -0700 Subject: [PATCH] fix: factor in root dependencies into ... filters (#8465) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description Dependency and dependent filters should factor in root dependencies since they're implicit dependencies of all packages in the workspace. We cannot add the root deps to `immediate_dependencies` or `immediate_ancestors` as these are used during a graph traversal and adding in the root deps can easily cause cycles. The transitive versions of these are safe to add the root dependencies to since they return entire closures. ### Testing Instructions Added integration tests Manual testing: ``` [0 olszewski@chriss-mbp] /tmp/pnpm-test $ turbo_dev build --filter='web...' --dry=json | jq '.packages' WARNING No locally installed `turbo` found. Using version: 2.0.4-canary.2. [ "@repo/eslint-config", "@repo/typescript-config", "@repo/ui", "@repo/util", "web" ] [0 olszewski@chriss-mbp] /tmp/pnpm-test $ turbo_dev build --filter='...web' --dry=json | jq '.packages' WARNING No locally installed `turbo` found. Using version: 2.0.4-canary.2. [ "web" ] [0 olszewski@chriss-mbp] /tmp/pnpm-test $ turbo_dev build --filter='...@repo/util' --dry=json | jq '.packages' WARNING No locally installed `turbo` found. Using version: 2.0.4-canary.2. [ "//", "@repo/eslint-config", "@repo/typescript-config", "@repo/ui", "@repo/util", "docs", "web" ] [0 olszewski@chriss-mbp] /tmp/pnpm-test $ bat package.json ───────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── │ File: package.json ───────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 1 │ { 2 │ "name": "pnpm-test", 3 │ "private": true, 4 │ "scripts": { 5 │ "build": "turbo build", 6 │ "dev": "turbo dev", 7 │ "lint": "turbo lint", 8 │ "format": "prettier --write \"**/*.{ts,tsx,md}\"" 9 │ }, 10 + │ "dependencies": { 11 + │ "@repo/util": "workspace:*" 12 + │ }, 13 │ "devDependencies": { 14 _ │ "prettier": "^3.2.5", 15 │ "typescript": "^5.4.5" 16 │ }, 17 │ "packageManager": "pnpm@8.15.6", 18 │ "engines": { 19 │ "node": ">=18" 20 │ } 21 │ } ``` --- .../src/package_graph/mod.rs | 26 ++++++++++++++++--- .../integration/tests/run-caching/root-deps.t | 17 ++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/crates/turborepo-repository/src/package_graph/mod.rs b/crates/turborepo-repository/src/package_graph/mod.rs index 0f7a77dfbf0a4..65241a3890b65 100644 --- a/crates/turborepo-repository/src/package_graph/mod.rs +++ b/crates/turborepo-repository/src/package_graph/mod.rs @@ -260,6 +260,9 @@ impl PackageGraph { pub fn dependencies<'a>(&'a self, node: &PackageNode) -> HashSet<&'a PackageNode> { let mut dependencies = self.transitive_closure_inner(Some(node), petgraph::Direction::Outgoing); + // Add in all root dependencies as they're implied dependencies for every + // package in the graph. + dependencies.extend(self.root_internal_dependencies()); dependencies.remove(node); dependencies } @@ -273,14 +276,18 @@ impl PackageGraph { /// /// ancestors(c) = {a, b} pub fn ancestors(&self, node: &PackageNode) -> HashSet<&PackageNode> { - let mut dependents = - self.transitive_closure_inner(Some(node), petgraph::Direction::Incoming); + // If node is a root dep, then *every* package is an ancestor of this one + let mut dependents = if self.root_internal_dependencies().contains(node) { + return self.graph.node_weights().collect(); + } else { + self.transitive_closure_inner(Some(node), petgraph::Direction::Incoming) + }; dependents.remove(node); dependents } pub fn root_internal_package_dependencies(&self) -> HashSet { - let dependencies = self.dependencies(&PackageNode::Workspace(PackageName::Root)); + let dependencies = self.root_internal_dependencies(); dependencies .into_iter() .filter_map(|package| match package { @@ -299,7 +306,7 @@ impl PackageGraph { } pub fn root_internal_package_dependencies_paths(&self) -> Vec<&AnchoredSystemPath> { - let dependencies = self.dependencies(&PackageNode::Workspace(PackageName::Root)); + let dependencies = self.root_internal_dependencies(); dependencies .into_iter() .filter_map(|package| match package { @@ -313,6 +320,17 @@ impl PackageGraph { .collect() } + fn root_internal_dependencies(&self) -> HashSet<&PackageNode> { + // We cannot call self.dependencies(&PackageNode::Workspace(PackageName::Root)) + // as it will infinitely recurse. + let mut dependencies = self.transitive_closure_inner( + Some(&PackageNode::Workspace(PackageName::Root)), + petgraph::Direction::Outgoing, + ); + dependencies.remove(&PackageNode::Workspace(PackageName::Root)); + dependencies + } + /// Returns the transitive closure of the given nodes in the package /// graph. Note that this includes the nodes themselves. If you want just /// the dependencies, or the dependents, use `dependencies` or `ancestors`. diff --git a/turborepo-tests/integration/tests/run-caching/root-deps.t b/turborepo-tests/integration/tests/run-caching/root-deps.t index 7e76ecbe70bb9..f5d5ef1b46b13 100644 --- a/turborepo-tests/integration/tests/run-caching/root-deps.t +++ b/turborepo-tests/integration/tests/run-caching/root-deps.t @@ -68,3 +68,20 @@ Cache hit since only tracked files contribute to root dep hash Cached: 1 cached, 1 total Time:\s*[\.0-9]+m?s >>> FULL TURBO (re) + +Verify that all packages are considered dependants of a root dep + $ ${TURBO} build --filter='...util' --dry=json | jq '.packages' + [ + "//", + "another", + "my-app", + "util", + "yet-another" + ] +Verify that a root dependency is considered a dependency of all packages + $ ${TURBO} build --filter='another...' --dry=json | jq '.packages' + [ + "another", + "util", + "yet-another" + ]