From bddad0025bf8564b93ef2c5e3858af1139484b34 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Sun, 5 Apr 2020 13:30:45 -0700 Subject: [PATCH] fix(builtin): fix runfiles linking issue on Windows Fix 1 of 2 for linker issue on Windows when all build actions are cached #1781. This fixes incorrectly symlinks but it does not actually address the underlying issue of programs (tests) running outside of the sandbox and directly out of the execroot on Windows (either out of execroot sources or execroot bazel-out). If all build actions are cached then the linker only runs for the test that is executed. If there are no third-part npm deps then it makes a node_modules folder in the runfiles for that test and puts its symlinks in there. The problem is that when the test runs, the .js spec is in execroot bazel-out and it doesn't see the node_modules created in the runfiles folder. Since no build actions have run that may have created the symlinks needed for the test in the execroot node_modules, the symlinks that it needs do not exist. The 2nd fix (still coming) seems to be to always symlink into the execroot which is below the runfiles folder. Some mental gymnastics needed to figure out if this is correct when in the sandbox as well and if there are third-party deps or not. I'll investigate. --- internal/linker/index.js | 25 ++++++------------------- internal/linker/link_node_modules.ts | 23 ++++++----------------- 2 files changed, 12 insertions(+), 36 deletions(-) diff --git a/internal/linker/index.js b/internal/linker/index.js index 6af11a4b43..d54a70d288 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -440,37 +440,24 @@ Include as much of the build output as you can without disclosing anything confi yield mkdirp(path.dirname(m.name)); if (m.link) { const [root, modulePath] = m.link; - const externalPrefix = 'external/'; let target = ''; switch (root) { case 'execroot': if (runfiles.execroot) { target = path.posix.join(runfiles.workspaceDir, modulePath); + break; } - else { - // If under runfiles, convert from execroot path to runfiles path. - // First strip the bin portion if it exists: - let runfilesPath = modulePath; - if (runfilesPath.startsWith(`${bin}/`)) { - runfilesPath = runfilesPath.slice(bin.length + 1); - } - else if (runfilesPath === bin) { - runfilesPath = ''; - } - // Next replace `external/` with `../` if it exists: - if (runfilesPath.startsWith(externalPrefix)) { - runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`; - } - target = path.posix.join(runfiles.workspaceDir, runfilesPath); - } - break; + // If under runfiles, the fall through to 'runfiles' case + // so that we handle case where there is only a MANIFEST file case 'runfiles': // Transform execroot path to the runfiles manifest path so that - // it can be resolved with runfiles.resolve() + // it can be resolved with runfiles.resolve(). let runfilesPath = modulePath; + // First strip the bin portion if it exists if (runfilesPath.startsWith(`${bin}/`)) { runfilesPath = runfilesPath.slice(bin.length + 1); } + const externalPrefix = 'external/'; if (runfilesPath.startsWith(externalPrefix)) { runfilesPath = runfilesPath.slice(externalPrefix.length); } diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index 6391a0e4ea..11a9409e55 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -503,35 +503,24 @@ export async function main(args: string[], runfiles: Runfiles) { if (m.link) { const [root, modulePath] = m.link; - const externalPrefix = 'external/'; let target: string = ''; switch (root) { case 'execroot': if (runfiles.execroot) { target = path.posix.join(runfiles.workspaceDir, modulePath); - } else { - // If under runfiles, convert from execroot path to runfiles path. - // First strip the bin portion if it exists: - let runfilesPath = modulePath; - if (runfilesPath.startsWith(`${bin}/`)) { - runfilesPath = runfilesPath.slice(bin.length + 1); - } else if (runfilesPath === bin) { - runfilesPath = ''; - } - // Next replace `external/` with `../` if it exists: - if (runfilesPath.startsWith(externalPrefix)) { - runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`; - } - target = path.posix.join(runfiles.workspaceDir, runfilesPath); + break; } - break; + // If under runfiles, the fall through to 'runfiles' case + // so that we handle case where there is only a MANIFEST file case 'runfiles': // Transform execroot path to the runfiles manifest path so that - // it can be resolved with runfiles.resolve() + // it can be resolved with runfiles.resolve(). let runfilesPath = modulePath; + // First strip the bin portion if it exists if (runfilesPath.startsWith(`${bin}/`)) { runfilesPath = runfilesPath.slice(bin.length + 1); } + const externalPrefix = 'external/'; if (runfilesPath.startsWith(externalPrefix)) { runfilesPath = runfilesPath.slice(externalPrefix.length); } else {