Skip to content

Commit

Permalink
fix: don't link runfiles node_modules to execroot node_modules if the…
Browse files Browse the repository at this point in the history 
…re is an external workspace node_modules (#3060)
  • Loading branch information
@gregmagolan @alexeagle
gregmagolan authored and alexeagle committed Dec 3, 2021
1 parent 7f4415e commit 1d5defa
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 18 deletions.
16 changes: 8 additions & 8 deletions internal/linker/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
}
Expand Down Expand Up @@ -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;
Expand All @@ -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`);
}
}
}
Expand Down
22 changes: 13 additions & 9 deletions internal/linker/link_node_modules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,9 @@ async function symlink(target: string, p: string): Promise<boolean> {

/** 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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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`);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions internal/linker/test/link_node_modules.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,7 @@ describe('link_node_modules', () => {

// No first-party packages
writeManifest({
workspace: workspace,
bin: BIN_DIR,
roots: {'': 'npm'},
});
Expand Down
6 changes: 6 additions & 0 deletions internal/node/launcher.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<path>:<workspace>,<path>:<workspace>"
# (e.g., "internal/linker/test:npm_internal_linker_test,:npm")
Expand Down
12 changes: 11 additions & 1 deletion internal/node/test/env.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`,
Expand All @@ -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);
});
Expand All @@ -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);
Expand Down

0 comments on commit 1d5defa

Please sign in to comment.