fix(builtin): linker incorrectly resolves workspace node_modules fo…

…r 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

@@ -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`);

internal/linker/link_node_modules.ts

@@ -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`
@@ -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.