Skip to content

Commit

Permalink
fix(core): throw an error when a circular dep occurs in a graph mixin…
Browse files Browse the repository at this point in the history 
…g buidlable/no-buildable projects (#9714)
  • Loading branch information
@vsavkin
vsavkin committed Apr 7, 2022
1 parent 9d89ea8 commit b661710
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 131 deletions.
36 changes: 0 additions & 36 deletions packages/nx/src/tasks-runner/__snapshots__/task-graph-creator.spec.ts.snap
View file
Expand Up @@ -36,42 +36,6 @@ Object {
}
`;

exports[`TaskGraphCreator (tasks with dependency configurations) should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects) 1`] = `
Object {
"dependencies": Object {
"app1:build": Array [
"common2:build",
],
"common2:build": Array [],
},
"roots": Array [
"common2:build",
],
"tasks": Object {
"app1:build": Object {
"id": "app1:build",
"overrides": Object {},
"projectRoot": "app1-root",
"target": Object {
"configuration": undefined,
"project": "app1",
"target": "build",
},
},
"common2:build": Object {
"id": "common2:build",
"overrides": Object {},
"projectRoot": "common2-root",
"target": Object {
"configuration": undefined,
"project": "common2",
"target": "build",
},
},
},
}
`;

exports[`TaskGraphCreator (tasks with dependency configurations) should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects) 2 1`] = `
Object {
"dependencies": Object {
Expand Down
103 changes: 57 additions & 46 deletions packages/nx/src/tasks-runner/run-command.spec.ts
View file
Expand Up @@ -458,7 +458,8 @@ describe('createTasksForProjectToRun', () => {
});
});

it('should handle circular dependencies between projects', () => {
it('should handle APP1 <=> LIB1(no build) => LIB2', () => {
// APP1 <=> LIB1 => LIB2
// App 1 depends on builds of its dependencies
projectGraph.nodes.app1.data.targets.build.dependsOn = [
{
Expand Down Expand Up @@ -501,13 +502,7 @@ describe('createTasksForProjectToRun', () => {
},
},
};
projectGraph.dependencies.lib2 = [
{
type: DependencyType.static,
source: 'lib2',
target: 'lib1',
},
];
projectGraph.dependencies.lib2 = [];

const tasks = createTasksForProjectToRun(
[projectGraph.nodes.app1],
Expand All @@ -534,24 +529,49 @@ describe('createTasksForProjectToRun', () => {
});
});

it('should handle circular dependencies between projects with no tasks', () => {
// App 1 depends on builds of its dependencies
it('should error when APP1 <=> LIB1(no build) <=> LIB2', () => {
jest.spyOn(process, 'exit').mockImplementation(() => {
throw new Error();
});

// Define Lib 2 with a build
projectGraph.nodes.lib2 = {
name: 'lib2',
type: 'lib',
data: {
root: 'lib2-root',
files: [],
targets: {
build: {},
},
},
};

// Lib 1 does not have "build"
delete projectGraph.nodes.lib1.data.targets.build;

// APP1 <=> LIB1 <=> LIB2
projectGraph.nodes.app1.data.targets.build.dependsOn = [
{
target: 'build',
projects: 'dependencies',
},
];

projectGraph.nodes.lib2.data.targets.build.dependsOn = [
{
target: 'build',
projects: 'dependencies',
},
];

// App 1 depends on Lib 1
projectGraph.dependencies.app1.push({
type: DependencyType.static,
source: 'app1',
target: 'lib1',
});

// Lib 1 does not have build but depends on Lib 2
delete projectGraph.nodes.lib1.data.targets.build;
projectGraph.dependencies.lib1.push(
{
type: DependencyType.static,
Expand All @@ -565,45 +585,36 @@ describe('createTasksForProjectToRun', () => {
}
);

// Lib 2 has a build
projectGraph.nodes.lib2 = {
name: 'lib2',
type: 'lib',
data: {
root: 'lib2-root',
files: [],
targets: {
build: {},
},
},
};
projectGraph.dependencies.lib2 = [];

const tasks = createTasksForProjectToRun(
[projectGraph.nodes.app1],
projectGraph.dependencies.lib2 = [
{
target: 'build',
configuration: undefined,
overrides: {},
type: DependencyType.static,
source: 'lib2',
target: 'lib1',
},
projectGraph,
projectGraph.nodes.app1.name
);
];

expect(tasks).toContainEqual({
id: 'app1:build',
target: { project: 'app1', target: 'build' },
projectRoot: 'app1-root',
overrides: {},
});
expect(tasks).toContainEqual({
id: 'lib2:build',
target: { project: 'lib2', target: 'build' },
projectRoot: 'lib2-root',
overrides: {},
});
try {
createTasksForProjectToRun(
[projectGraph.nodes.app1],
{
target: 'build',
configuration: undefined,
overrides: {},
},
projectGraph,
projectGraph.nodes.app1.name
);
fail();
} catch (e) {
expect(process.exit).toHaveBeenCalledWith(1);
}
});

// Technically, this could work but it creates a lot of problems in the implementation,
// so instead we error saying that the circular dependency cannot be handled.
// xit('should handle APP1 => LIB1(no build) <=> LIB2', () => {
// });

it('should throw an error for an invalid target', () => {
jest.spyOn(process, 'exit').mockImplementation(() => {
throw new Error();
Expand Down
69 changes: 53 additions & 16 deletions packages/nx/src/tasks-runner/run-command.ts
View file
Expand Up @@ -258,7 +258,7 @@ function addTasksForProjectTarget(
projectGraph: ProjectGraph,
originalTargetExecutor: string,
tasksMap: Map<string, Task>,
path: string[],
path: { targetIdentifier: string; hasTarget: boolean }[],
seenSet: Set<string>
) {
const task = createTask({
Expand Down Expand Up @@ -356,23 +356,22 @@ function addTasksForProjectDependencyConfig(
projectGraph: ProjectGraph,
originalTargetExecutor: string,
tasksMap: Map<string, Task>,
path: string[],
path: { targetIdentifier: string; hasTarget: boolean }[],
seenSet: Set<string>
) {
const targetIdentifier = getId({
project: project.name,
target,
configuration,
});
seenSet.add(project.name);

if (path.includes(targetIdentifier)) {
output.error({
title: `Could not execute ${path[0]} because it has a circular dependency`,
bodyLines: [`${[...path, targetIdentifier].join(' --> ')}`],
});
process.exit(1);
}
const pathFragment = {
targetIdentifier,
hasTarget: projectHasTarget(project, target),
};

const newPath = [...path, pathFragment];
seenSet.add(project.name);

if (tasksMap.has(targetIdentifier)) {
return;
Expand All @@ -385,10 +384,21 @@ function addTasksForProjectDependencyConfig(
const depProject = projectGraph.nodes[
dep.target
] as ProjectGraphProjectNode;

if (
depProject &&
projectHasTarget(depProject, dependencyConfig.target)
) {
const depTargetId = getId({
project: depProject.name,
target,
configuration: configuration,
});
exitOnCircularDep(newPath, depTargetId);
if (seenSet.has(dep.target)) {
continue;
}

addTasksForProjectTarget(
{
project: depProject,
Expand All @@ -401,17 +411,26 @@ function addTasksForProjectDependencyConfig(
projectGraph,
originalTargetExecutor,
tasksMap,
[...path, targetIdentifier],
newPath,
seenSet
);
} else {
if (seenSet.has(dep.target)) {
continue;
}
if (!depProject) {
seenSet.add(dep.target);
continue;
}
const depTargetId = getId({
project: depProject.name,
target,
configuration: configuration,
});

exitOnCircularDep(newPath, depTargetId);

if (seenSet.has(dep.target)) {
continue;
}

addTasksForProjectDependencyConfig(
depProject,
{ target, configuration, overrides },
Expand All @@ -420,7 +439,7 @@ function addTasksForProjectDependencyConfig(
projectGraph,
originalTargetExecutor,
tasksMap,
path,
newPath,
seenSet
);
}
Expand All @@ -439,12 +458,30 @@ function addTasksForProjectDependencyConfig(
projectGraph,
originalTargetExecutor,
tasksMap,
[...path, targetIdentifier],
newPath,
seenSet
);
}
}

function exitOnCircularDep(
path: { targetIdentifier: string; hasTarget: boolean }[],
targetIdentifier: string
) {
if (
path.length > 0 &&
path[path.length - 1].hasTarget &&
path.filter((p) => p.targetIdentifier === targetIdentifier).length > 0
) {
const identifiers = path.map((p) => p.targetIdentifier);
output.error({
title: `Could not execute ${identifiers[0]} because it has a circular dependency`,
bodyLines: [`${[...identifiers, targetIdentifier].join(' --> ')}`],
});
process.exit(1);
}
}

function getId({
project,
target,
Expand Down
32 changes: 0 additions & 32 deletions packages/nx/src/tasks-runner/task-graph-creator.spec.ts
View file
Expand Up @@ -320,38 +320,6 @@ describe('TaskGraphCreator', () => {
expect(taskGraph).toMatchSnapshot();
});

it('should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects)', () => {
delete projectGraph.nodes.common1.data.targets.build;
projectGraph.dependencies.common1.push({
type: DependencyType.static,
source: 'common1',
target: 'common2',
});

projectGraph.dependencies.common2.push({
type: DependencyType.static,
source: 'common2',
target: 'common1',
});

const tasks = createTasksForProjectToRun(
[projectGraph.nodes.app1],
{
target: 'build',
configuration: undefined,
overrides: {},
},
projectGraph,
null
);

const taskGraph = new TaskGraphCreator(projectGraph, {}).createTaskGraph(
tasks
);

expect(taskGraph).toMatchSnapshot();
});

it('should create a task graph (builds depend on builds of dependencies with intermediate projects and circular dependencies between projects) 2', () => {
delete projectGraph.nodes.common1.data.targets.build;
projectGraph.dependencies.common1.push({
Expand Down
6 changes: 5 additions & 1 deletion packages/nx/src/utils/project-graph-utils.ts
View file
Expand Up @@ -10,7 +10,11 @@ export function projectHasTarget(
project: ProjectGraphProjectNode,
target: string
) {
return project.data && project.data.targets && project.data.targets[target];
return !!(
project.data &&
project.data.targets &&
project.data.targets[target]
);
}

export function projectHasTargetAndConfiguration(
Expand Down

0 comments on commit b661710

Please sign in to comment.