Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(builtin): linker incorrectly resolves workspace node_modules for windows #2659

Merged
merged 1 commit into from
May 7, 2021
Merged

fix(builtin): linker incorrectly resolves workspace node_modules for windows #2659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions internal/linker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
});
}
Expand Down Expand Up @@ -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);
Expand All @@ -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`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱

if (!isExecroot) {
const runfilesPackagePath = `${startCwd}/${packagePath}`;
yield mkdirp(`${runfilesPackagePath}/node_modules`);
Expand Down
30 changes: 15 additions & 15 deletions internal/linker/link_node_modules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,24 +131,24 @@ async function symlink(target: string, p: string): Promise<boolean> {
}
}

/**
* 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`
Expand All @@ -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
Expand All @@ -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}`)
}
}

Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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.
Expand Down