Skip to content

Commit

Permalink
fix(builtin): linker incorrectly resolves workspace node_modules fo…
Browse files Browse the repository at this point in the history 
…r windows

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).
  • Loading branch information
@devversion
devversion committed May 6, 2021
1 parent 4bea5b2 commit ab1895a
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 83 deletions.
34 changes: 6 additions & 28 deletions internal/linker/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,27 +111,6 @@ function symlink(target, p) {
}
});
}
function resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles) {
return __awaiter(this, void 0, void 0, function* () {
if (isExecroot) {
return `${execroot}/external/${workspace}`;
}
if (!execroot) {
return path.resolve(`${startCwd}/../${workspace}`);
}
const fromManifest = runfiles.lookupDirectory(workspace);
if (fromManifest) {
return fromManifest;
}
else {
const maybe = path.resolve(`${execroot}/external/${workspace}`);
if (yield exists(maybe)) {
return maybe;
}
return path.resolve(`${startCwd}/../${workspace}`);
}
});
}
function exists(p) {
return __awaiter(this, void 0, void 0, function* () {
return ((yield gracefulLstat(p)) !== null);
Expand Down Expand Up @@ -289,11 +268,10 @@ 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 = runfiles.lookupDirectory(`${workspace}/node_modules`);
log_verbose(`resolved ${workspace} workspace node modules path to ${workspaceNodeModules}`);
if (packagePath) {
if (yield exists(workspaceNodeModules)) {
if (workspaceNodeModules !== undefined) {
yield mkdirp(packagePath);
yield symlinkWithUnlink(workspaceNodeModules, `${packagePath}/node_modules`);
if (!isExecroot) {
Expand All @@ -309,7 +287,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`);
Expand All @@ -321,12 +299,12 @@ function main(args, runfiles) {
}
}
else {
if (yield exists(workspaceNodeModules)) {
if (workspaceNodeModules !== undefined) {
yield symlinkWithUnlink(workspaceNodeModules, `node_modules`);
}
else {
log_verbose(`no root npm workspace node_modules folder to link to; creating node_modules directory in ${process.cwd()}`);
yield mkdirp('node_modules');
yield mkdirp(`node_modules`);
}
}
}
Expand Down
61 changes: 6 additions & 55 deletions internal/linker/link_node_modules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,52 +131,6 @@ async function symlink(target: string, p: string): Promise<boolean> {
}
}

/**
* Resolve to the path to an external workspace
*/
async function resolveExternalWorkspacePath(
workspace: string, startCwd: string, isExecroot: boolean, execroot: string|undefined,
runfiles: Runfiles) {
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
// 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}`;
}

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}`)
}

// Under runfiles, the linker should symlink node_modules at `execroot/my_wksp`
// so that when there are no runfiles (default on Windows) and scripts run out of
// `execroot/my_wksp` they can resolve node_modules with standard node_module resolution

// 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);
if (fromManifest) {
return fromManifest;
} else {
const maybe = path.resolve(`${execroot}/external/${workspace}`);
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
// directory and not a symlink back to the root node_modules where we expect
// to resolve from. This case is tested in internal/linker/test/local.
return maybe;
}
// 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}`)
}
}

// TypeScript lib.es5.d.ts has a mistake: JSON.parse does accept Buffer.
declare global {
interface JSON {
Expand Down Expand Up @@ -435,13 +389,11 @@ 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 = runfiles.lookupDirectory(`${workspace}/node_modules`);
log_verbose(`resolved ${workspace} workspace node modules path to ${workspaceNodeModules}`);
if (packagePath) {
// sub-directory node_modules
if (await exists(workspaceNodeModules)) {
if (workspaceNodeModules !== undefined) {
// in some cases packagePath may not exist in sandbox if there are no source deps
// and only generated file deps. we create it so that we that we can link to
// packagePath/node_modules since packagePathBin/node_modules is a symlink to
Expand Down Expand Up @@ -474,7 +426,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.
Expand All @@ -488,16 +440,15 @@ export async function main(args: string[], runfiles: Runfiles) {
await symlinkWithUnlink(`${packagePath}/node_modules`, `${packagePathBin}/node_modules`);
}
} else {
// root node_modules
if (await exists(workspaceNodeModules)) {
if (workspaceNodeModules !== undefined) {
await symlinkWithUnlink(workspaceNodeModules, `node_modules`);
} else {
// Special case if there no target to symlink to for the root workspace, create a
// root node_modules folder for 1st party deps
log_verbose(
`no root npm workspace node_modules folder to link to; creating node_modules directory in ${
process.cwd()}`);
await mkdirp('node_modules');
await mkdirp(`node_modules`);
}
}
} else {
Expand Down

0 comments on commit ab1895a

Please sign in to comment.