From 9138c101b41a2511a70771e6613a51f8f8dc3b64 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 7 May 2021 23:29:04 +0200 Subject: [PATCH] fix(builtin): linker incorrectly resolves workspace `node_modules` for windows (#2659) Depending on whether there are generated files being stored in `bazel-out/bin/external/npm` or not, the linker might resolve the workspace node modules incorrectly for Windows. e.g. consider the manifest being the followed: `` npm/karma/bin/_karma.module_mappings.json C:/users/paul/<..>/execroot/angular_material/bazel-out/<..>/bin/external/npm/<..>` npm/node_modules/@angular/core/index.js C:/users/paul/projects/material2/node_modules/@angular/core/index.js ``` The linker currently on Windows will resolve the NPM workspace to the `bazel-out` directory. This is incorrect as on Windows, the node modules are not symlinked as with linux/macOS. Hence the node modules are not symlinked to the execroot properly and tests/builds fail due to NodeJS imports being "faulty". The fix is to simply use the runfile helpers to resolve the workspace node modules. There is no need in resolving the workspace root if we can just directly narrow the lookup to the workspace node modules. This is needed as the runfile resolution with manifest naively looks for the first entry starting with `npm` in the manifest. This breaks the assumption that the resolved workspace path refers to a directory that also contains the `node_modules` folder (as seen in the example above). --- internal/linker/index.js | 20 +++++++++---------- internal/linker/link_node_modules.ts | 30 ++++++++++++++-------------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/internal/linker/index.js b/internal/linker/index.js index ba7ce3278d..2a8ccd826e 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -111,24 +111,25 @@ function symlink(target, p) { } }); } -function resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles) { +function resolveWorkspaceNodeModules(workspace, startCwd, isExecroot, execroot, runfiles) { return __awaiter(this, void 0, void 0, function* () { + const targetManifestPath = `${workspace}/node_modules`; if (isExecroot) { - return `${execroot}/external/${workspace}`; + return `${execroot}/external/${targetManifestPath}`; } if (!execroot) { - return path.resolve(`${startCwd}/../${workspace}`); + return path.resolve(`${startCwd}/../${targetManifestPath}`); } - const fromManifest = runfiles.lookupDirectory(workspace); + const fromManifest = runfiles.lookupDirectory(targetManifestPath); if (fromManifest) { return fromManifest; } else { - const maybe = path.resolve(`${execroot}/external/${workspace}`); + const maybe = path.resolve(`${execroot}/external/${targetManifestPath}`); if (yield exists(maybe)) { return maybe; } - return path.resolve(`${startCwd}/../${workspace}`); + return path.resolve(`${startCwd}/../${targetManifestPath}`); } }); } @@ -289,9 +290,8 @@ function main(args, runfiles) { for (const packagePath of Object.keys(roots)) { const workspace = roots[packagePath]; if (workspace) { - const workspacePath = yield resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles); - log_verbose(`resolved ${workspace} workspace path to ${workspacePath}`); - const workspaceNodeModules = `${workspacePath}/node_modules`; + const workspaceNodeModules = yield resolveWorkspaceNodeModules(workspace, startCwd, isExecroot, execroot, runfiles); + log_verbose(`resolved ${workspace} workspace node modules path to ${workspaceNodeModules}`); if (packagePath) { if (yield exists(workspaceNodeModules)) { yield mkdirp(packagePath); @@ -309,7 +309,7 @@ function main(args, runfiles) { } else { log_verbose(`no npm workspace node_modules folder under ${packagePath} to link to; creating node_modules directories in ${process.cwd()} for ${packagePath} 1p deps`); - yield mkdirp('${packagePath}/node_modules'); + yield mkdirp(`${packagePath}/node_modules`); if (!isExecroot) { const runfilesPackagePath = `${startCwd}/${packagePath}`; yield mkdirp(`${runfilesPackagePath}/node_modules`); diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index fe8cd95829..412a9b45db 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -131,24 +131,24 @@ async function symlink(target: string, p: string): Promise { } } -/** - * Resolve to the path to an external workspace - */ -async function resolveExternalWorkspacePath( +/** 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, runfiles: Runfiles) { + const targetManifestPath = `${workspace}/node_modules`; + if (isExecroot) { // Under execroot, the npm workspace will be under an external folder from the startCwd - // `execroot/my_wksp`. For example, `execroot/my_wksp/external/npm`. If there is no + // `execroot/my_wksp`. For example, `execroot/my_wksp/external/npm/node_modules`. If there is no // npm workspace, which will be the case if there are no third-party modules dependencies for // this target, npmWorkspace the root to `execroot/my_wksp/node_modules`. - return `${execroot}/external/${workspace}`; + return `${execroot}/external/${targetManifestPath}`; } if (!execroot) { // This can happen if we are inside a nodejs_image or a nodejs_binary is run manually. // Resolve as if we are in runfiles in a sandbox. - return path.resolve(`${startCwd}/../${workspace}`) + return path.resolve(`${startCwd}/../${targetManifestPath}`) } // Under runfiles, the linker should symlink node_modules at `execroot/my_wksp` @@ -158,11 +158,11 @@ async function resolveExternalWorkspacePath( // If we got a runfilesManifest map, look through it for a resolution // This will happen if we are running a binary that had some npm packages // "statically linked" into its runfiles - const fromManifest = runfiles.lookupDirectory(workspace); + const fromManifest = runfiles.lookupDirectory(targetManifestPath); if (fromManifest) { return fromManifest; } else { - const maybe = path.resolve(`${execroot}/external/${workspace}`); + const maybe = path.resolve(`${execroot}/external/${targetManifestPath}`); if (await exists(maybe)) { // Under runfiles, when not in the sandbox we must symlink node_modules down at the execroot // `execroot/my_wksp/external/npm/node_modules` since `runfiles/npm/node_modules` will be a @@ -173,7 +173,7 @@ async function resolveExternalWorkspacePath( // However, when in the sandbox, `execroot/my_wksp/external/npm/node_modules` does not exist, // so we must symlink into `runfiles/npm/node_modules`. This directory exists whether legacy // external runfiles are on or off. - return path.resolve(`${startCwd}/../${workspace}`) + return path.resolve(`${startCwd}/../${targetManifestPath}`) } } @@ -435,10 +435,10 @@ export async function main(args: string[], runfiles: Runfiles) { for (const packagePath of Object.keys(roots)) { const workspace = roots[packagePath]; if (workspace) { - const workspacePath = - await resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles); - log_verbose(`resolved ${workspace} workspace path to ${workspacePath}`); - const workspaceNodeModules = `${workspacePath}/node_modules`; + const workspaceNodeModules = await resolveWorkspaceNodeModules( + workspace, startCwd, isExecroot, execroot, runfiles); + log_verbose(`resolved ${workspace} workspace node modules path to ${workspaceNodeModules}`); + if (packagePath) { // sub-directory node_modules if (await exists(workspaceNodeModules)) { @@ -474,7 +474,7 @@ export async function main(args: string[], runfiles: Runfiles) { log_verbose(`no npm workspace node_modules folder under ${ packagePath} to link to; creating node_modules directories in ${process.cwd()} for ${ packagePath} 1p deps`); - await mkdirp('${packagePath}/node_modules'); + await mkdirp(`${packagePath}/node_modules`); if (!isExecroot) { // Under runfiles, we symlink into the package in runfiles as well. // When inside the sandbox, the execroot location will not exist to symlink to.