Skip to content

Commit

Permalink
fix: factor in root dependencies into ... filters (#8465)
Browse files Browse the repository at this point in the history
### 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": "[email protected]",
  18   │   "engines": {
  19   │     "node": ">=18"
  20   │   }
  21   │ }
```
  • Loading branch information
chris-olszewski committed Jun 14, 2024
1 parent 5336fd6 commit 7d440dd
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 4 deletions.
26 changes: 22 additions & 4 deletions crates/turborepo-repository/src/package_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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<WorkspacePackage> {
let dependencies = self.dependencies(&PackageNode::Workspace(PackageName::Root));
let dependencies = self.root_internal_dependencies();
dependencies
.into_iter()
.filter_map(|package| match package {
Expand All @@ -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 {
Expand All @@ -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`.
Expand Down
17 changes: 17 additions & 0 deletions turborepo-tests/integration/tests/run-caching/root-deps.t
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]

0 comments on commit 7d440dd

Please sign in to comment.