Skip to content

Commit

Permalink
fix(builtin): make linker deterministic when resolving from manifest …
Browse files Browse the repository at this point in the history
…& fix link_workspace_root with no runfiles
  • Loading branch information
gregmagolan authored and Alex Eagle committed Nov 24, 2020
1 parent d1e3088 commit f7c342f
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 7 deletions.
20 changes: 18 additions & 2 deletions internal/linker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Object.defineProperty(exports, "__esModule", { value: true });
const fs = require("fs");
const path = require("path");
const VERBOSE_LOGS = !!process.env['VERBOSE_LOGS'];
const BAZEL_OUT_REGEX = /(\/bazel-out\/|\/bazel-~1\/x64_wi~1\/)/;
function log_verbose(...m) {
if (VERBOSE_LOGS)
console.error('[link_node_modules.js]', ...m);
Expand Down Expand Up @@ -124,7 +125,7 @@ function resolveRoot(root, startCwd, isExecroot, runfiles) {
if (isExecroot) {
return root ? `${startCwd}/external/${root}` : `${startCwd}/node_modules`;
}
const match = startCwd.match(/(\/bazel-out\/|\/bazel-~1\/x64_wi~1\/)/);
const match = startCwd.match(BAZEL_OUT_REGEX);
if (!match) {
if (!root) {
return `${startCwd}/node_modules`;
Expand Down Expand Up @@ -176,14 +177,22 @@ class Runfiles {
lookupDirectory(dir) {
if (!this.manifest)
return undefined;
let result;
for (const [k, v] of this.manifest) {
if (k.startsWith(`${dir}/external`))
continue;
if (k.startsWith(dir)) {
const l = k.length - dir.length;
return v.substring(0, v.length - l);
const maybe = v.substring(0, v.length - l);
if (maybe.match(BAZEL_OUT_REGEX)) {
return maybe;
}
else {
result = maybe;
}
}
}
return result;
}
loadRunfilesManifest(manifestPath) {
log_verbose(`using runfiles manifest ${manifestPath}`);
Expand Down Expand Up @@ -470,6 +479,13 @@ function main(args, runfiles) {
}
try {
target = runfiles.resolve(runfilesPath);
if (runfiles.manifest && root == 'execroot' && modulePath.startsWith(`${bin}/`)) {
if (!target.includes(`/${bin}/`)) {
const e = new Error(`could not resolve modulePath ${modulePath}`);
e.code = 'MODULE_NOT_FOUND';
throw e;
}
}
}
catch (_a) {
target = '<runfiles resolution failed>';
Expand Down
26 changes: 22 additions & 4 deletions internal/linker/link_node_modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import * as path from 'path';
// Run Bazel with --define=VERBOSE_LOGS=1 to enable this logging
const VERBOSE_LOGS = !!process.env['VERBOSE_LOGS'];

// NB: on windows thanks to legacy 8-character path segments it might be like
// c:/b/ojvxx6nx/execroot/build_~1/bazel-~1/x64_wi~1/bin/internal/npm_in~1/test
const BAZEL_OUT_REGEX = /(\/bazel-out\/|\/bazel-~1\/x64_wi~1\/)/;

function log_verbose(...m: string[]) {
if (VERBOSE_LOGS) console.error('[link_node_modules.js]', ...m);
}
Expand Down Expand Up @@ -175,9 +179,7 @@ async function resolveRoot(
// runfiles on rbe, bazel runs the process in a directory such as
// `/b/f/w/bazel-out/k8-fastbuild/bin/path/to/pkg/some_test.sh.runfiles/my_wksp`. From here we can
// determine the execroot `b/f/w` by finding the first instance of bazel-out.
// NB: on windows thanks to legacy 8-character path segments it might be like
// c:/b/ojvxx6nx/execroot/build_~1/bazel-~1/x64_wi~1/bin/internal/npm_in~1/test
const match = startCwd.match(/(\/bazel-out\/|\/bazel-~1\/x64_wi~1\/)/);
const match = startCwd.match(BAZEL_OUT_REGEX);
if (!match) {
// No execroot found. This can happen if we are inside a nodejs_image or a nodejs_binary is
// run manually.
Expand Down Expand Up @@ -278,6 +280,7 @@ export class Runfiles {
lookupDirectory(dir: string): string|undefined {
if (!this.manifest) return undefined;

let result: string|undefined;
for (const [k, v] of this.manifest) {
// Account for Bazel --legacy_external_runfiles
// which pollutes the workspace with 'my_wksp/external/...'
Expand All @@ -289,9 +292,15 @@ export class Runfiles {
// calculate l = length(`/semver/LICENSE`)
if (k.startsWith(dir)) {
const l = k.length - dir.length;
return v.substring(0, v.length - l);
const maybe = v.substring(0, v.length - l);
if (maybe.match(BAZEL_OUT_REGEX)) {
return maybe;
} else {
result = maybe;
}
}
}
return result;
}


Expand Down Expand Up @@ -731,6 +740,15 @@ export async function main(args: string[], runfiles: Runfiles) {
}
try {
target = runfiles.resolve(runfilesPath);
// if we're resolving from a manifest then make sure we don't resolve
// into the source tree when we are expecting the output tree
if (runfiles.manifest && root == 'execroot' && modulePath.startsWith(`${bin}/`)) {
if (!target.includes(`/${bin}/`)) {
const e = new Error(`could not resolve modulePath ${modulePath}`);
(e as any).code = 'MODULE_NOT_FOUND';
throw e;
}
}
} catch {
target = '<runfiles resolution failed>';
}
Expand Down
1 change: 0 additions & 1 deletion internal/linker/test/workspace_link/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ jasmine_node_test(
name = "test",
srcs = ["test.js"],
link_workspace_root = True,
tags = ["fix-windows"],
templated_args = ["--nobazel_patch_module_resolver"],
deps = [
"//internal/linker/test/workspace_link/bar",
Expand Down

0 comments on commit f7c342f

Please sign in to comment.