From 7b933ca46837909b29a2f5486377a36739665eeb Mon Sep 17 00:00:00 2001 From: skrtheboss Date: Mon, 3 Jul 2023 14:52:00 +0200 Subject: [PATCH 1/3] fix(core): ensure external dependency hashes are resolved in a deterministic way External dependency hashes were not deterministic and could cause cache misses, since the hash was dependent on which task was hashed when. This was caused by the externalDepsHashCache which was filled with values which are dependent on the visited set. Fixes #17917 --- packages/nx/src/hasher/task-hasher.spec.ts | 138 +++++++++++++++++++++ packages/nx/src/hasher/task-hasher.ts | 15 +-- 2 files changed, 146 insertions(+), 7 deletions(-) diff --git a/packages/nx/src/hasher/task-hasher.spec.ts b/packages/nx/src/hasher/task-hasher.spec.ts index e108672d31c6a..6ba82a56e424a 100644 --- a/packages/nx/src/hasher/task-hasher.spec.ts +++ b/packages/nx/src/hasher/task-hasher.spec.ts @@ -1161,6 +1161,144 @@ describe('TaskHasher', () => { expect(hash.value).toContain('|5.0.0|'); }); + it('should hash entire subtree of dependencies', async () => { + const createHasher = () => + new InProcessTaskHasher( + {}, + allWorkspaceFiles, + { + nodes: { + appA: { + name: 'appA', + type: 'app', + data: { + root: 'apps/appA', + targets: { build: { executor: '@nx/webpack:webpack' } }, + }, + }, + appB: { + name: 'appB', + type: 'app', + data: { + root: 'apps/appB', + targets: { build: { executor: '@nx/webpack:webpack' } }, + }, + }, + }, + externalNodes: { + 'npm:packageA': { + name: 'npm:packageA', + type: 'npm', + data: { + packageName: 'packageA', + version: '0.0.0', + hash: '$packageA0.0.0$', + }, + }, + 'npm:packageB': { + name: 'npm:packageB', + type: 'npm', + data: { + packageName: 'packageB', + version: '0.0.0', + hash: '$packageB0.0.0$', + }, + }, + 'npm:packageC': { + name: 'npm:packageC', + type: 'npm', + data: { + packageName: 'packageC', + version: '0.0.0', + hash: '$packageC0.0.0$', + }, + }, + }, + dependencies: { + appA: [ + { + source: 'app', + target: 'npm:packageA', + type: DependencyType.static, + }, + { + source: 'app', + target: 'npm:packageB', + type: DependencyType.static, + }, + { + source: 'app', + target: 'npm:packageC', + type: DependencyType.static, + }, + ], + appB: [ + { + source: 'app', + target: 'npm:packageC', + type: DependencyType.static, + }, + ], + 'npm:packageC': [ + { + source: 'app', + target: 'npm:packageA', + type: DependencyType.static, + }, + { + source: 'app', + target: 'npm:packageB', + type: DependencyType.static, + }, + ], + 'npm:packageB': [ + { + source: 'app', + target: 'npm:packageA', + type: DependencyType.static, + }, + ], + }, + }, + { + roots: ['app-build'], + tasks: { + 'app-build': { + id: 'app-build', + target: { project: 'app', target: 'build' }, + overrides: {}, + }, + }, + dependencies: {}, + }, + {} as any, + {}, + fileHasher + ); + + const computeTaskHash = async (hasher, appName) => { + const hashAppA = await hasher.hashTask({ + target: { project: appName, target: 'build' }, + id: `${appName}-build`, + overrides: { prop: 'prop-value' }, + }); + + return hashAppA.value; + }; + + const hasher1 = createHasher(); + + await computeTaskHash(hasher1, 'appA'); + const hashAppB1 = await computeTaskHash(hasher1, 'appB'); + + const hasher2 = createHasher(); + + const hashAppB2 = await computeTaskHash(hasher2, 'appB'); + await computeTaskHash(hasher2, 'appA'); + + expect(hashAppB1).toEqual(hashAppB2); + }); + it('should not hash when nx:run-commands executor', async () => { const hasher = new InProcessTaskHasher( {}, diff --git a/packages/nx/src/hasher/task-hasher.ts b/packages/nx/src/hasher/task-hasher.ts index 05246b9ac559c..e0b7f62904133 100644 --- a/packages/nx/src/hasher/task-hasher.ts +++ b/packages/nx/src/hasher/task-hasher.ts @@ -429,13 +429,14 @@ class TaskHasherImpl { partialHashes.push(node.data.version); } // we want to calculate the hash of the entire dependency tree - if (this.projectGraph.dependencies[projectName]) { - this.projectGraph.dependencies[projectName].forEach((d) => { - if (!visited.has(d.target)) { - partialHashes.push(this.hashExternalDependency(d.target, visited)); - } - }); - } + this.projectGraph.dependencies[projectName]?.forEach((d) => { + if (!visited.has(d.target)) { + partialHashes.push( + this.hashExternalDependency(d.target, new Set(visited)) + ); + } + }); + partialHash = hashArray(partialHashes); } else { // unknown dependency From 3564d41fa8a29d76ba2998e329a253d4e81c4e8a Mon Sep 17 00:00:00 2001 From: skrtheboss Date: Tue, 4 Jul 2023 21:18:56 +0200 Subject: [PATCH 2/3] fixup! fix(core): ensure external dependency hashes are resolved in a deterministic way --- packages/nx/src/hasher/task-hasher.ts | 49 ++++++++++++++++++++------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/packages/nx/src/hasher/task-hasher.ts b/packages/nx/src/hasher/task-hasher.ts index e0b7f62904133..d2afd8c37838a 100644 --- a/packages/nx/src/hasher/task-hasher.ts +++ b/packages/nx/src/hasher/task-hasher.ts @@ -332,7 +332,10 @@ class TaskHasherImpl { visited ); } else { - const hash = this.hashExternalDependency(d.target); + const hash = this.hashExternalDependency( + task.target.target, + d.target + ); return { value: hash, details: { @@ -408,16 +411,29 @@ class TaskHasherImpl { return partialHashes; } + private computeExternalDependencyIdentifier( + sourceProjectName: string, + targetProjectName: string + ): `${string}->${string}` { + return `${sourceProjectName}->${targetProjectName}`; + } + private hashExternalDependency( - projectName: string, + sourceProjectName: string, + targetProjectName: string, visited = new Set() ): string { // try to retrieve the hash from cache - if (this.externalDepsHashCache[projectName]) { - return this.externalDepsHashCache[projectName]; + if (this.externalDepsHashCache[targetProjectName]) { + return this.externalDepsHashCache[targetProjectName]; } - visited.add(projectName); - const node = this.projectGraph.externalNodes[projectName]; + visited.add( + this.computeExternalDependencyIdentifier( + sourceProjectName, + targetProjectName + ) + ); + const node = this.projectGraph.externalNodes[targetProjectName]; let partialHash: string; if (node) { const partialHashes: string[] = []; @@ -429,10 +445,17 @@ class TaskHasherImpl { partialHashes.push(node.data.version); } // we want to calculate the hash of the entire dependency tree - this.projectGraph.dependencies[projectName]?.forEach((d) => { - if (!visited.has(d.target)) { + this.projectGraph.dependencies[targetProjectName]?.forEach((d) => { + if ( + !visited.has( + this.computeExternalDependencyIdentifier( + targetProjectName, + d.target + ) + ) + ) { partialHashes.push( - this.hashExternalDependency(d.target, new Set(visited)) + this.hashExternalDependency(targetProjectName, d.target, visited) ); } }); @@ -443,9 +466,9 @@ class TaskHasherImpl { // this may occur if dependency is not an npm package // but rather symlinked in node_modules or it's pointing to a remote git repo // in this case we have no information about the versioning of the given package - partialHash = `__${projectName}__`; + partialHash = `__${targetProjectName}__`; } - this.externalDepsHashCache[projectName] = partialHash; + this.externalDepsHashCache[targetProjectName] = partialHash; return partialHash; } @@ -471,7 +494,7 @@ class TaskHasherImpl { const executorPackage = target.executor.split(':')[0]; const executorNodeName = this.findExternalDependencyNodeName(executorPackage); - hash = this.hashExternalDependency(executorNodeName); + hash = this.hashExternalDependency(targetName, executorNodeName); } else { // use command external dependencies if available to construct the hash const partialHashes: string[] = []; @@ -483,7 +506,7 @@ class TaskHasherImpl { const externalDependencies = input['externalDependencies']; for (let dep of externalDependencies) { dep = this.findExternalDependencyNodeName(dep); - partialHashes.push(this.hashExternalDependency(dep)); + partialHashes.push(this.hashExternalDependency(targetName, dep)); } } } From 9cd8f873e1da9a4f9ab7a00958b7a593f83e58a0 Mon Sep 17 00:00:00 2001 From: skrtheboss Date: Wed, 5 Jul 2023 08:22:03 +0200 Subject: [PATCH 3/3] fixup! fix(core): ensure external dependency hashes are resolved in a deterministic way --- packages/nx/src/hasher/task-hasher.ts | 37 +++++++++++++-------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/packages/nx/src/hasher/task-hasher.ts b/packages/nx/src/hasher/task-hasher.ts index d2afd8c37838a..4b40e50a03337 100644 --- a/packages/nx/src/hasher/task-hasher.ts +++ b/packages/nx/src/hasher/task-hasher.ts @@ -332,10 +332,7 @@ class TaskHasherImpl { visited ); } else { - const hash = this.hashExternalDependency( - task.target.target, - d.target - ); + const hash = this.hashExternalDependency(d.source, d.target); return { value: hash, details: { @@ -445,20 +442,22 @@ class TaskHasherImpl { partialHashes.push(node.data.version); } // we want to calculate the hash of the entire dependency tree - this.projectGraph.dependencies[targetProjectName]?.forEach((d) => { - if ( - !visited.has( - this.computeExternalDependencyIdentifier( - targetProjectName, - d.target + if (this.projectGraph.dependencies[targetProjectName]) { + this.projectGraph.dependencies[targetProjectName].forEach((d) => { + if ( + !visited.has( + this.computeExternalDependencyIdentifier( + targetProjectName, + d.target + ) ) - ) - ) { - partialHashes.push( - this.hashExternalDependency(targetProjectName, d.target, visited) - ); - } - }); + ) { + partialHashes.push( + this.hashExternalDependency(targetProjectName, d.target, visited) + ); + } + }); + } partialHash = hashArray(partialHashes); } else { @@ -494,7 +493,7 @@ class TaskHasherImpl { const executorPackage = target.executor.split(':')[0]; const executorNodeName = this.findExternalDependencyNodeName(executorPackage); - hash = this.hashExternalDependency(targetName, executorNodeName); + hash = this.hashExternalDependency(projectName, executorNodeName); } else { // use command external dependencies if available to construct the hash const partialHashes: string[] = []; @@ -506,7 +505,7 @@ class TaskHasherImpl { const externalDependencies = input['externalDependencies']; for (let dep of externalDependencies) { dep = this.findExternalDependencyNodeName(dep); - partialHashes.push(this.hashExternalDependency(targetName, dep)); + partialHashes.push(this.hashExternalDependency(projectName, dep)); } } }