Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Add early validation to `rush install`/`rush update` that immediately fails with a meaningful error message if an undeclared cycle is detected among workspace packages, specifying the cycle path.",
"type": "minor"
}
]
}
103 changes: 103 additions & 0 deletions libraries/rush-lib/src/logic/WorkspaceCycleDetector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
// See LICENSE in the project root for license information.

import { AlreadyReportedError } from '@rushstack/node-core-library';
import { Colorize, type ITerminal } from '@rushstack/terminal';

import type { RushConfiguration } from '../api/RushConfiguration';
import type { RushConfigurationProject } from '../api/RushConfigurationProject';
import { RushConstants } from './RushConstants';

/**
* Detects cycles in the workspace package dependency graph (i.e., cycles that are not
* broken by `decoupledLocalDependencies`) and reports them as errors.
*
* @remarks
* A cycle means that pnpm would be unable to install the workspace, so it is better to
* fail fast with a clear message rather than let pnpm produce a cryptic error.
*
* The fix is to refactor the code to eliminate the cycle, for example by extracting shared
* code into a new package that both projects can depend on, or by moving code from one project
* to another. `decoupledLocalDependencies` is intended only for the bootstrapping problem
* (e.g. the version of a compiler used to compile itself) and should not be used as a
* general escape hatch for cycles.
*/
export function detectAndReportWorkspaceCycles(
rushConfiguration: RushConfiguration,
terminal: ITerminal
): void {
const cycle: ReadonlyArray<string> | undefined = _findWorkspaceCycle(rushConfiguration.projects);

if (cycle !== undefined) {
terminal.writeLine();
terminal.writeLine(
Colorize.red(
'A cyclic dependency was detected among workspace packages:\n' +
` ${cycle.join(' -> ')}\n\n` +
`To fix this, refactor the code to eliminate the cycle. For example, extract the shared ` +
`code into a new package that both projects can depend on, or move code from one project ` +
`to another so the dependency only goes in one direction.\n\n` +
`NOTE: The "decoupledLocalDependencies" setting in ${RushConstants.rushJsonFilename} is ` +
`intended only for the bootstrapping problem (for example, the version of a compiler used ` +
`to compile itself). It is not a general solution for cyclic dependencies.`
)
);
throw new AlreadyReportedError();
}
}

/**
* Finds one cycle in the workspace dependency graph, or returns `undefined` if there are none.
*
* Uses depth-first search with a "currently visiting" set for O(V + E) detection.
*/
export function _findWorkspaceCycle(
projects: ReadonlyArray<RushConfigurationProject>
): ReadonlyArray<string> | undefined {
// Nodes that have been fully explored (no cycles reachable from them)
const visited: Set<RushConfigurationProject> = new Set();
// Nodes currently on the DFS recursion stack
const visiting: Set<RushConfigurationProject> = new Set();
// The current DFS path (used to extract the cycle path when one is found)
const path: RushConfigurationProject[] = [];

function dfs(node: RushConfigurationProject): ReadonlyArray<string> | undefined {
if (visited.has(node)) {
return undefined;
}
if (visiting.has(node)) {
// We've found a back-edge — extract the cycle from the current path
const cycleStartIndex: number = path.indexOf(node);
return [
...path.slice(cycleStartIndex).map((p) => p.packageName),
node.packageName // append the closing node to make the cycle explicit
];
}

visiting.add(node);
path.push(node);

for (const dep of node.dependencyProjects) {
const cycle: ReadonlyArray<string> | undefined = dfs(dep);
if (cycle !== undefined) {
return cycle;
}
}

path.pop();
visiting.delete(node);
visited.add(node);
return undefined;
}

for (const project of projects) {
if (!visited.has(project)) {
const cycle: ReadonlyArray<string> | undefined = dfs(project);
if (cycle !== undefined) {
return cycle;
}
}
}

return undefined;
}
6 changes: 6 additions & 0 deletions libraries/rush-lib/src/logic/base/BaseInstallManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import { ProjectImpactGraphGenerator } from '../ProjectImpactGraphGenerator';
import { FlagFile } from '../../api/FlagFile';
import { PnpmSyncUtilities } from '../../utilities/PnpmSyncUtilities';
import { HotlinkManager } from '../../utilities/HotlinkManager';
import { detectAndReportWorkspaceCycles } from '../WorkspaceCycleDetector';

/**
* Pnpm don't support --ignore-compatibility-db, so use --config.ignoreCompatibilityDb for now.
Expand Down Expand Up @@ -442,6 +443,11 @@ export abstract class BaseInstallManager {
// Check the policies
await PolicyValidator.validatePolicyAsync(this.rushConfiguration, subspace, variant, this.options);

// Fail fast if there are undeclared cycles in the workspace package dependency graph.
// Pnpm cannot install a workspace with cycles, so this gives a clearer error message
// than whatever pnpm would emit.
detectAndReportWorkspaceCycles(this.rushConfiguration, terminal);

await this._installGitHooksAsync();

const approvedPackagesChecker: ApprovedPackagesChecker = new ApprovedPackagesChecker(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
// See LICENSE in the project root for license information.

import * as path from 'node:path';

import { RushConfiguration } from '../../api/RushConfiguration';
import { _findWorkspaceCycle } from '../WorkspaceCycleDetector';

describe(_findWorkspaceCycle.name, () => {
function loadProjectsFromRepo(repoName: string): RushConfiguration['projects'] {
const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
path.resolve(__dirname, `workspaceCycleDetector/${repoName}/rush.json`)
);
return rushConfiguration.projects;
}

it('returns undefined when there are no cycles', () => {
const projects: RushConfiguration['projects'] = loadProjectsFromRepo('no-cycle');
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(projects);
expect(result).toBeUndefined();
});

it('returns the cycle path when an undeclared cycle is present', () => {
const projects: RushConfiguration['projects'] = loadProjectsFromRepo('with-cycle');
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(projects);
expect(result).toBeDefined();
// The cycle should form a closed loop: [..., pkg-a] or [..., pkg-b] depending on
// iteration order. Either way the first and last element must be the same package,
// and both pkg-a and pkg-b must appear in the cycle.
expect(result!.length).toBeGreaterThanOrEqual(2);
expect(result![0]).toBe(result![result!.length - 1]);
const uniqueNames: Set<string> = new Set(result);
expect(uniqueNames.has('pkg-a')).toBe(true);
expect(uniqueNames.has('pkg-b')).toBe(true);
});

it('returns undefined when the cycle is intentionally broken with decoupledLocalDependencies', () => {
const projects: RushConfiguration['projects'] = loadProjectsFromRepo('decoupled-cycle');
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(projects);
expect(result).toBeUndefined();
});

it('returns the cycle path when using the workspacePackages test repo (cyclic-dep-1 <-> cyclic-dep-2)', () => {
const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
path.resolve(__dirname, 'workspacePackages/rush.json')
);
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(rushConfiguration.projects);
expect(result).toBeDefined();
expect(result![0]).toBe(result![result!.length - 1]);
const uniqueNames: Set<string> = new Set(result);
expect(uniqueNames.has('cyclic-dep-1')).toBe(true);
expect(uniqueNames.has('cyclic-dep-2')).toBe(true);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "pkg-a",
"version": "1.0.0",
"dependencies": {
"pkg-b": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "pkg-b",
"version": "1.0.0",
"dependencies": {
"pkg-a": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"rushVersion": "0.0.0",
"pnpmVersion": "8.0.0",
"projects": [
{
"packageName": "pkg-a",
"projectFolder": "pkg-a"
},
{
"packageName": "pkg-b",
"projectFolder": "pkg-b",
"decoupledLocalDependencies": ["pkg-a"]
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "pkg-a",
"version": "1.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "pkg-b",
"version": "1.0.0",
"dependencies": {
"pkg-a": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"rushVersion": "0.0.0",
"pnpmVersion": "8.0.0",
"projects": [
{
"packageName": "pkg-a",
"projectFolder": "pkg-a"
},
{
"packageName": "pkg-b",
"projectFolder": "pkg-b"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "pkg-a",
"version": "1.0.0",
"dependencies": {
"pkg-b": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "pkg-b",
"version": "1.0.0",
"dependencies": {
"pkg-a": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"rushVersion": "0.0.0",
"pnpmVersion": "8.0.0",
"projects": [
{
"packageName": "pkg-a",
"projectFolder": "pkg-a"
},
{
"packageName": "pkg-b",
"projectFolder": "pkg-b"
}
]
}