From 7ddcc84da5b77344f544be7b8bf3c7fb220b2473 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Wed, 3 Nov 2021 22:20:49 -0700 Subject: [PATCH] fix: don't link runfiles node_modules to execroot node_modules if there is an external workspace node_modules --- internal/linker/index.js | 16 +++++++------- internal/linker/link_node_modules.ts | 22 +++++++++++-------- .../linker/test/link_node_modules.spec.ts | 1 + internal/node/launcher.sh | 6 +++++ internal/node/test/env.spec.js | 12 +++++++++- 5 files changed, 39 insertions(+), 18 deletions(-) diff --git a/internal/linker/index.js b/internal/linker/index.js index 7568f02f0d..35b4cdf1c5 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -136,9 +136,9 @@ function symlink(target, p) { } }); } -function resolveWorkspaceNodeModules(workspace, startCwd, isExecroot, execroot, runfiles) { +function resolveWorkspaceNodeModules(externalWorkspace, startCwd, isExecroot, execroot, runfiles) { return __awaiter(this, void 0, void 0, function* () { - const targetManifestPath = `${workspace}/node_modules`; + const targetManifestPath = `${externalWorkspace}/node_modules`; if (isExecroot) { return `${execroot}/external/${targetManifestPath}`; } @@ -319,10 +319,10 @@ function main(args, runfiles) { }); } for (const packagePath of Object.keys(roots)) { - const workspace = roots[packagePath]; - let workspaceNodeModules = yield resolveWorkspaceNodeModules(workspace, startCwd, isExecroot, execroot, runfiles); + const externalWorkspace = roots[packagePath]; + let workspaceNodeModules = yield resolveWorkspaceNodeModules(externalWorkspace, startCwd, isExecroot, execroot, runfiles); if (yield exists(workspaceNodeModules)) { - log_verbose(`resolved ${workspace} workspace node modules path to ${workspaceNodeModules}`); + log_verbose(`resolved ${externalWorkspace} external workspace node modules path to ${workspaceNodeModules}`); } else { workspaceNodeModules = undefined; @@ -345,14 +345,14 @@ function main(args, runfiles) { if (!isExecroot) { const runfilesPackagePath = path.posix.join(startCwd, packagePath); yield mkdirp(`${runfilesPackagePath}`); - yield symlinkWithUnlink(execrootNodeModules, `${runfilesPackagePath}/node_modules`); + yield symlinkWithUnlink(!packagePath && workspaceNodeModules ? workspaceNodeModules : execrootNodeModules, `${runfilesPackagePath}/node_modules`); } if (process.env['RUNFILES']) { const stat = yield gracefulLstat(process.env['RUNFILES']); if (stat && stat.isDirectory()) { - const runfilesPackagePath = path.posix.join(process.env['RUNFILES'], packagePath); + const runfilesPackagePath = path.posix.join(process.env['RUNFILES'], workspace, packagePath); yield mkdirp(`${runfilesPackagePath}`); - yield symlinkWithUnlink(execrootNodeModules, `${runfilesPackagePath}/node_modules`); + yield symlinkWithUnlink(!packagePath && workspaceNodeModules ? workspaceNodeModules : execrootNodeModules, `${runfilesPackagePath}/node_modules`); } } } diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index 5ad836b8a3..dcf7381727 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -164,9 +164,9 @@ async function symlink(target: string, p: string): Promise { /** Determines an absolute path to the given workspace if it contains node modules. */ async function resolveWorkspaceNodeModules( - workspace: string, startCwd: string, isExecroot: boolean, execroot: string|undefined, + externalWorkspace: string, startCwd: string, isExecroot: boolean, execroot: string|undefined, runfiles: Runfiles) { - const targetManifestPath = `${workspace}/node_modules`; + const targetManifestPath = `${externalWorkspace}/node_modules`; if (isExecroot) { // Under execroot, the npm workspace will be under an external folder from the startCwd @@ -471,11 +471,11 @@ export async function main(args: string[], runfiles: Runfiles) { // Symlink all node_modules roots defined. These are 3rd party deps in external npm workspaces // lined to node_modules folders at the root or in sub-directories for (const packagePath of Object.keys(roots)) { - const workspace = roots[packagePath]; + const externalWorkspace = roots[packagePath]; let workspaceNodeModules: string | undefined = await resolveWorkspaceNodeModules( - workspace, startCwd, isExecroot, execroot, runfiles); + externalWorkspace, startCwd, isExecroot, execroot, runfiles); if (await exists(workspaceNodeModules)) { - log_verbose(`resolved ${workspace} workspace node modules path to ${workspaceNodeModules}`); + log_verbose(`resolved ${externalWorkspace} external workspace node modules path to ${workspaceNodeModules}`); } else { // There are no third party node_modules to symlink to workspaceNodeModules = undefined; @@ -511,21 +511,25 @@ export async function main(args: string[], runfiles: Runfiles) { await mkdirp(`${packagePathBin}`); await symlinkWithUnlink(execrootNodeModules, `${packagePathBin}/node_modules`); } - + // Start CWD symlink -> execroot node_modules if (!isExecroot) { const runfilesPackagePath = path.posix.join(startCwd, packagePath); await mkdirp(`${runfilesPackagePath}`); - await symlinkWithUnlink(execrootNodeModules, `${runfilesPackagePath}/node_modules`); + // Don't link to the root execroot node_modules if there is a workspace node_modules. + // Bazel will delete that symlink on rebuild in the ibazel run context. + await symlinkWithUnlink(!packagePath && workspaceNodeModules ? workspaceNodeModules : execrootNodeModules, `${runfilesPackagePath}/node_modules`); } // RUNFILES symlink -> execroot node_modules if (process.env['RUNFILES']) { const stat = await gracefulLstat(process.env['RUNFILES']); if (stat && stat.isDirectory()) { - const runfilesPackagePath = path.posix.join(process.env['RUNFILES'], packagePath); + const runfilesPackagePath = path.posix.join(process.env['RUNFILES'], workspace, packagePath); await mkdirp(`${runfilesPackagePath}`); - await symlinkWithUnlink(execrootNodeModules, `${runfilesPackagePath}/node_modules`); + // Don't link to the root execroot node_modules if there is a workspace node_modules. + // Bazel will delete that symlink on rebuild in the ibazel run context. + await symlinkWithUnlink(!packagePath && workspaceNodeModules ? workspaceNodeModules : execrootNodeModules, `${runfilesPackagePath}/node_modules`); } } } diff --git a/internal/linker/test/link_node_modules.spec.ts b/internal/linker/test/link_node_modules.spec.ts index b26b16b128..4dcdbc1ef4 100644 --- a/internal/linker/test/link_node_modules.spec.ts +++ b/internal/linker/test/link_node_modules.spec.ts @@ -593,6 +593,7 @@ describe('link_node_modules', () => { // No first-party packages writeManifest({ + workspace: workspace, bin: BIN_DIR, roots: {'': 'npm'}, }); diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index 8d651df258..007b7279c4 100644 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -285,6 +285,12 @@ if [[ "${RUNFILES_ROOT}" ]]; then # (e.g., /private/.../execroot/build_bazel_rules_nodejs/bazel-out/darwin-fastbuild/bin/internal/linker/test/multi_linker/test.sh.runfiles/build_bazel_rules_nodejs/node_modules) export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}/${BAZEL_WORKSPACE}/node_modules" fi +if [[ "${RUNFILES}" ]]; then + # If in runfiles, guard the RUNFILES root itself + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES}" + # If RUNFILES is set, guard the RUNFILES node_modules as well + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES}/${BAZEL_WORKSPACE}/node_modules" +fi if [[ -n "${BAZEL_NODE_MODULES_ROOTS:-}" ]]; then # BAZEL_NODE_MODULES_ROOTS is in the format ":,:" # (e.g., "internal/linker/test:npm_internal_linker_test,:npm") diff --git a/internal/node/test/env.spec.js b/internal/node/test/env.spec.js index 46ad198756..88d92cbaae 100644 --- a/internal/node/test/env.spec.js +++ b/internal/node/test/env.spec.js @@ -10,9 +10,12 @@ function normPath(p) { // On Windows, we normalize to lowercase for so path mismatches such as 'C:/Users' and // 'c:/users' don't break the specs. result = result.toLowerCase(); - if (/[a-zA-Z]\:/.test(result)) { + if (/^[a-z]\:/.test(result)) { // Handle c:/ and /c/ mismatch result = `/${result[0]}${result.slice(2)}`; + } else if (/^[a-z];[a-z]\:/.test(result)) { + // Handle c;b:/ and /c/b/ mismatch + result = `/${result[0]}/${result[2]}${result.slice(4)}`; } } return result; @@ -40,11 +43,14 @@ describe('launcher.sh environment', function() { expectPathsToMatch(process.cwd(), `${process.env['RUNFILES_DIR']}/build_bazel_rules_nodejs`); expectPathsToMatch(process.env['PWD'], `${process.env['RUNFILES_DIR']}/build_bazel_rules_nodejs`); expectPathsToMatch(process.env['BAZEL_NODE_MODULES_ROOTS'], ':npm'); + console.log(process.env['RUNFILES']) const expectedRoots = [ `${execroot}`, `${execroot}/node_modules`, `${runfilesRoot}`, `${runfilesRoot}/build_bazel_rules_nodejs/node_modules`, + `${process.env['RUNFILES']}`, + `${process.env['RUNFILES']}/${process.env['BAZEL_WORKSPACE']}/node_modules`, `${execroot}/external/npm/node_modules`, `${runfilesRoot}/npm/node_modules`, `${runfilesRoot}/build_bazel_rules_nodejs/external/npm/node_modules`, @@ -71,6 +77,8 @@ describe('launcher.sh environment', function() { const expectedRoots = [ `${execroot}`, `${execroot}/node_modules`, + `${env['RUNFILES']}`, + `${env['RUNFILES']}/${env['BAZEL_WORKSPACE']}/node_modules`, ] expectPathsToMatch(env['BAZEL_PATCH_ROOTS'].split(','), expectedRoots); }); @@ -92,6 +100,8 @@ describe('launcher.sh environment', function() { const expectedRoots = [ `${execroot}`, `${execroot}/node_modules`, + `${env['RUNFILES']}`, + `${env['RUNFILES']}/${env['BAZEL_WORKSPACE']}/node_modules`, `${execroot}/external/npm/node_modules`, ] expectPathsToMatch(env['BAZEL_PATCH_ROOTS'].split(','), expectedRoots);