diff --git a/WORKSPACE b/WORKSPACE index 73471aeb32..b4340a2634 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -19,6 +19,7 @@ workspace( # cypress_deps must be a managed directory to ensure it is downloaded before cypress_repository is run. "@cypress_deps": ["packages/cypress/test/node_modules"], "@npm": ["node_modules"], + "@npm_internal_linker_test_multi_linker": ["internal/linker/test/multi_linker/node_modules"], "@npm_node_patches": ["packages/node-patches/node_modules"], }, ) @@ -57,6 +58,21 @@ yarn_install( yarn_lock = "//:yarn.lock", ) +yarn_install( + name = "npm_internal_linker_test_multi_linker", + package_json = "//internal/linker/test/multi_linker:package.json", + package_path = "internal/linker/test/multi_linker", + yarn_lock = "//internal/linker/test/multi_linker:yarn.lock", +) + +yarn_install( + name = "onepa_npm_deps", + package_json = "//internal/linker/test/multi_linker/onepa:package.json", + package_path = "internal/linker/test/multi_linker/onepa", + symlink_node_modules = False, + yarn_lock = "//internal/linker/test/multi_linker/onepa:yarn.lock", +) + npm_install( name = "npm_node_patches", package_json = "//packages/node-patches:package.json", diff --git a/internal/bazel_integration_test/test_runner.js b/internal/bazel_integration_test/test_runner.js index 1fedcee86c..8430ea32be 100644 --- a/internal/bazel_integration_test/test_runner.js +++ b/internal/bazel_integration_test/test_runner.js @@ -358,11 +358,10 @@ for (const bazelCommand of config.bazelCommands) { 'BASH_FUNC_is_absolute%%', 'BASH_FUNC_rlocation%%', 'BASH_FUNC_runfiles_export_envvars%%', - 'BAZEL_NODE_MODULES_ROOT', + 'BAZEL_NODE_MODULES_ROOTS', 'BAZEL_NODE_PATCH_REQUIRE', 'BAZEL_NODE_RUNFILES_HELPER', - 'BAZEL_PATCH_GUARDS', - 'BAZEL_PATCH_ROOT', + 'BAZEL_PATCH_ROOTS', 'BAZEL_TARGET', 'BAZEL_WORKSPACE', 'BAZELISK_SKIP_WRAPPER', diff --git a/internal/js_library/js_library.bzl b/internal/js_library/js_library.bzl index 7b6571df85..5fcce1486a 100644 --- a/internal/js_library/js_library.bzl +++ b/internal/js_library/js_library.bzl @@ -38,7 +38,7 @@ _ATTRS = { ), "deps": attr.label_list(), "external_npm_package": attr.bool( - doc = """Indictates that this js_library target is one or more external npm packages in node_modules. + doc = """Internal use only. Indictates that this js_library target is one or more external npm packages in node_modules. This is used by the yarn_install & npm_install repository rules for npm dependencies installed by yarn & npm. When true, js_library will provide ExternalNpmPackageInfo. @@ -73,6 +73,12 @@ _ATTRS = { See `examples/user_managed_deps` for a working example of user-managed npm dependencies.""", default = False, ), + "external_npm_package_path": attr.string( + doc = """Internal use only. The local workspace path that the linker should link these node_modules to. + + Used only when external_npm_package is True. If empty, the linker will link these node_modules at the root.""", + default = "", + ), "is_windows": attr.bool( doc = "Internal use only. Automatically set by macro", mandatory = True, @@ -230,6 +236,7 @@ def _impl(ctx): direct_sources = depset(transitive = direct_sources_depsets), sources = depset(transitive = npm_sources_depsets), workspace = workspace_name, + path = ctx.attr.external_npm_package_path, )) # Don't provide DeclarationInfo if there are no typings to provide. diff --git a/internal/linker/index.js b/internal/linker/index.js index b9346636b3..9ef712b2ac 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -34,7 +34,7 @@ function mkdirp(p) { return __awaiter(this, void 0, void 0, function* () { if (p && !(yield exists(p))) { yield mkdirp(path.dirname(p)); - log_verbose(`mkdir( ${p} )`); + log_verbose(`creating directory ${p} in ${process.cwd()}`); try { yield fs.promises.mkdir(p); } @@ -98,7 +98,10 @@ function deleteDirectory(p) { } function symlink(target, p) { return __awaiter(this, void 0, void 0, function* () { - log_verbose(`symlink( ${p} -> ${target} )`); + if (!path.isAbsolute(target)) { + target = path.resolve(process.cwd(), target); + } + log_verbose(`creating symlink ${p} -> ${target}`); try { yield fs.promises.symlink(target, p, 'junction'); return true; @@ -117,33 +120,24 @@ function symlink(target, p) { } }); } -function resolveRoot(root, startCwd, isExecroot, runfiles) { +function resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles) { return __awaiter(this, void 0, void 0, function* () { if (isExecroot) { - return root ? `${startCwd}/external/${root}` : `${startCwd}/node_modules`; - } - const match = startCwd.match(BAZEL_OUT_REGEX); - if (!match) { - if (!root) { - return `${startCwd}/node_modules`; - } - return path.resolve(`${startCwd}/../${root}`); + return `${execroot}/external/${workspace}`; } - const symlinkRoot = startCwd.slice(0, match.index); - process.chdir(symlinkRoot); - if (!root) { - return `${symlinkRoot}/node_modules`; + if (!execroot) { + return path.resolve(`${startCwd}/../${workspace}`); } - const fromManifest = runfiles.lookupDirectory(root); + const fromManifest = runfiles.lookupDirectory(workspace); if (fromManifest) { return fromManifest; } else { - const maybe = path.resolve(`${symlinkRoot}/external/${root}`); - if (fs.existsSync(maybe)) { + const maybe = path.resolve(`${execroot}/external/${workspace}`); + if (yield exists(maybe)) { return maybe; } - return path.resolve(`${startCwd}/../${root}`); + return path.resolve(`${startCwd}/../${workspace}`); } }); } @@ -211,7 +205,7 @@ class Runfiles { if (result) { return result; } - const e = new Error(`could not resolve modulePath ${modulePath}`); + const e = new Error(`could not resolve module ${modulePath}`); e.code = 'MODULE_NOT_FOUND'; throw e; } @@ -272,6 +266,9 @@ function exists(p) { }); } function existsSync(p) { + if (!p) { + return false; + } try { fs.lstatSync(p); return true; @@ -328,19 +325,6 @@ function liftElement(element) { } return element; } -function toParentLink(link) { - return [link[0], path.dirname(link[1])]; -} -function allElementsAlign(name, elements) { - if (!elements[0].link) { - return false; - } - const parentLink = toParentLink(elements[0].link); - if (!elements.every(e => !!e.link && isDirectChildLink(parentLink, e.link))) { - return false; - } - return !!elements[0].link && allElementsAlignUnder(name, parentLink, elements); -} function allElementsAlignUnder(parentName, parentLink, elements) { for (const { name, link, children } of elements) { if (!link || children) { @@ -361,16 +345,10 @@ function allElementsAlignUnder(parentName, parentLink, elements) { function isDirectChildPath(parent, child) { return parent === path.dirname(child); } -function isDirectChildLink([parentRel, parentPath], [childRel, childPath]) { - if (parentRel !== childRel) { - return false; - } - if (!isDirectChildPath(parentPath, childPath)) { - return false; - } - return true; +function isDirectChildLink(parentLink, childLink) { + return parentLink === path.dirname(childLink); } -function isNameLinkPathTopAligned(namePath, [, linkPath]) { +function isNameLinkPathTopAligned(namePath, linkPath) { return path.basename(namePath) === path.basename(linkPath); } function visitDirectoryPreserveLinks(dirPath, visit) { @@ -390,28 +368,96 @@ function visitDirectoryPreserveLinks(dirPath, visit) { } }); } +function findExecroot(startCwd) { + if (existsSync(`${startCwd}/bazel-out`)) { + return startCwd; + } + const bazelOutMatch = startCwd.match(BAZEL_OUT_REGEX); + return bazelOutMatch ? startCwd.slice(0, bazelOutMatch.index) : undefined; +} function main(args, runfiles) { return __awaiter(this, void 0, void 0, function* () { if (!args || args.length < 1) throw new Error('requires one argument: modulesManifest path'); const [modulesManifest] = args; - let { bin, root, modules, workspace } = JSON.parse(fs.readFileSync(modulesManifest)); + log_verbose('manifest file:', modulesManifest); + let { workspace, bin, roots, modules } = JSON.parse(fs.readFileSync(modulesManifest)); modules = modules || {}; - log_verbose('manifest file', modulesManifest); - log_verbose('manifest contents', JSON.stringify({ workspace, bin, root, modules }, null, 2)); + log_verbose('manifest contents:', JSON.stringify({ workspace, bin, roots, modules }, null, 2)); const startCwd = process.cwd().replace(/\\/g, '/'); - log_verbose('startCwd', startCwd); - const isExecroot = existsSync(`${startCwd}/bazel-out`); - log_verbose('isExecroot', isExecroot.toString()); - const rootDir = yield resolveRoot(root, startCwd, isExecroot, runfiles); - log_verbose('resolved node_modules root', root, 'to', rootDir); - log_verbose('cwd', process.cwd()); - if (!(yield exists(rootDir))) { - log_verbose('no third-party packages; mkdir node_modules at', root); - yield mkdirp(rootDir); - } - yield symlink(rootDir, 'node_modules'); - process.chdir(rootDir); + log_verbose('startCwd:', startCwd); + const execroot = findExecroot(startCwd); + log_verbose('execroot:', execroot ? execroot : 'not found'); + const isExecroot = startCwd == execroot; + log_verbose('isExecroot:', isExecroot.toString()); + if (!isExecroot && execroot) { + process.chdir(execroot); + log_verbose('changed directory to execroot', execroot); + } + function symlinkWithUnlink(target, p, stats = null) { + return __awaiter(this, void 0, void 0, function* () { + if (!path.isAbsolute(target)) { + target = path.resolve(process.cwd(), target); + } + if (stats === null) { + stats = yield gracefulLstat(p); + } + if (runfiles.manifest && execroot && stats !== null && stats.isSymbolicLink()) { + const symlinkPath = fs.readlinkSync(p).replace(/\\/g, '/'); + if (path.relative(symlinkPath, target) != '' && + !path.relative(execroot, symlinkPath).startsWith('..')) { + log_verbose(`Out-of-date symlink for ${p} to ${symlinkPath} detected. Target should be ${target}. Unlinking.`); + yield unlink(p); + } + } + return symlink(target, p); + }); + } + for (const packagePath of Object.keys(roots)) { + const workspace = roots[packagePath]; + const workspacePath = yield resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles); + log_verbose(`resolved ${workspace} workspace path to ${workspacePath}`); + const workspaceNodeModules = `${workspacePath}/node_modules`; + if (packagePath) { + if (yield exists(workspaceNodeModules)) { + let resolvedPackagePath; + if (yield exists(packagePath)) { + yield symlinkWithUnlink(workspaceNodeModules, `${packagePath}/node_modules`); + resolvedPackagePath = packagePath; + } + if (!isExecroot) { + const runfilesPackagePath = `${startCwd}/${packagePath}`; + if (yield exists(runfilesPackagePath)) { + if (resolvedPackagePath) { + yield symlinkWithUnlink(`${resolvedPackagePath}/node_modules`, `${runfilesPackagePath}/node_modules`); + } + else { + yield symlinkWithUnlink(workspaceNodeModules, `${runfilesPackagePath}/node_modules`); + } + resolvedPackagePath = runfilesPackagePath; + } + } + const packagePathBin = `${bin}/${packagePath}`; + if (resolvedPackagePath && (yield exists(packagePathBin))) { + yield symlinkWithUnlink(`${resolvedPackagePath}/node_modules`, `${packagePathBin}/node_modules`); + } + } + } + else { + if (yield exists(workspaceNodeModules)) { + yield symlinkWithUnlink(workspaceNodeModules, `node_modules`); + } + else { + log_verbose('no root npm workspace node_modules folder to link to; creating node_modules directory in', process.cwd()); + yield mkdirp('node_modules'); + } + } + } + if (!roots || !roots['']) { + log_verbose('no root npm workspace; creating node_modules directory in ', process.cwd()); + yield mkdirp('node_modules'); + } + process.chdir('node_modules'); function isLeftoverDirectoryFromLinker(stats, modulePath) { return __awaiter(this, void 0, void 0, function* () { if (runfiles.manifest === undefined) { @@ -451,44 +497,43 @@ function main(args, runfiles) { return __awaiter(this, void 0, void 0, function* () { yield mkdirp(path.dirname(m.name)); if (m.link) { - const [root, modulePath] = m.link; + const modulePath = m.link; let target; - switch (root) { - case 'execroot': - if (isExecroot) { - target = `${startCwd}/${modulePath}`; - break; - } - case 'runfiles': - let runfilesPath = modulePath; - if (runfilesPath.startsWith(`${bin}/`)) { - runfilesPath = runfilesPath.slice(bin.length + 1); - } - else if (runfilesPath === bin) { - runfilesPath = ''; - } - const externalPrefix = 'external/'; - if (runfilesPath.startsWith(externalPrefix)) { - runfilesPath = runfilesPath.slice(externalPrefix.length); - } - else { - runfilesPath = `${workspace}/${runfilesPath}`; - } - 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; - } + if (isExecroot) { + target = `${startCwd}/${modulePath}`; + } + if (!isExecroot || !existsSync(target)) { + let runfilesPath = modulePath; + if (runfilesPath.startsWith(`${bin}/`)) { + runfilesPath = runfilesPath.slice(bin.length + 1); + } + else if (runfilesPath === bin) { + runfilesPath = ''; + } + const externalPrefix = 'external/'; + if (runfilesPath.startsWith(externalPrefix)) { + runfilesPath = runfilesPath.slice(externalPrefix.length); + } + else { + runfilesPath = `${workspace}/${runfilesPath}`; + } + try { + target = runfiles.resolve(runfilesPath); + if (runfiles.manifest && modulePath.startsWith(`${bin}/`)) { + if (!target.match(BAZEL_OUT_REGEX)) { + const e = new Error(`could not resolve module ${runfilesPath} in output tree`); + e.code = 'MODULE_NOT_FOUND'; + throw e; } } - catch (err) { - target = undefined; - log_verbose(`runfiles resolve failed for module '${m.name}': ${err.message}`); - } - break; + } + catch (err) { + target = undefined; + log_verbose(`runfiles resolve failed for module '${m.name}': ${err.message}`); + } + } + if (target && !path.isAbsolute(target)) { + target = path.resolve(process.cwd(), target); } const stats = yield gracefulLstat(m.name); const isLeftOver = (stats !== null && (yield isLeftoverDirectoryFromLinker(stats, m.name))); @@ -497,7 +542,7 @@ function main(args, runfiles) { yield createSymlinkAndPreserveContents(stats, m.name, target); } else { - yield symlink(target, m.name); + yield symlinkWithUnlink(target, m.name, stats); } } else { diff --git a/internal/linker/link_node_modules.bzl b/internal/linker/link_node_modules.bzl index 4f8124416a..d1e93c0240 100644 --- a/internal/linker/link_node_modules.bzl +++ b/internal/linker/link_node_modules.bzl @@ -38,20 +38,11 @@ def add_arg(args, arg): def _link_mapping(label, mappings, k, v): if k in mappings and mappings[k] != v: - # Allow all other mappings to win over legacy "_tslibrary" - if mappings[k][0] == "_tslibrary": + # Allow all other mappings to win over legacy "__tslibrary__" + if mappings[k][0] == "__tslibrary__": return True - elif v[0] == "_tslibrary": + elif v[0] == "__tslibrary__": return False - if v[1] == mappings[k][1]: - # Allow "execroot" to win over "runfiles" - # For example, - # mappings[k] = ["runfiles", "angular/packages/compiler"] - # v = ["execroot", "angular/packages/compiler"] - if mappings[k][0] == "runfiles": - return True - elif v[0] == "runfiles": - return False fail(("conflicting mapping at '%s': '%s' maps to both %s and %s" % (label, k, mappings[k], v)), "deps") else: return True @@ -67,30 +58,36 @@ def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_work If source files need to be required then they can be copied to the bin_dir with copy_to_bin. """ - mappings = {ctx.workspace_name: ["execroot", ctx.bin_dir.path]} if link_workspace_root else {} - node_modules_root = "" + mappings = {ctx.workspace_name: ["__link__", ctx.bin_dir.path]} if link_workspace_root else {} + node_modules_roots = {} # Look through data/deps attributes to find... for dep in extra_data + getattr(ctx.attr, "data", []) + getattr(ctx.attr, "deps", []): - # ...the root directory for the third-party node_modules; we'll symlink the local "node_modules" to it + # ...the root directories for the third-party node_modules; we'll symlink local "node_modules" to them if ExternalNpmPackageInfo in dep: - possible_root = "/".join([dep[ExternalNpmPackageInfo].workspace, "node_modules"]) - if not node_modules_root: - node_modules_root = possible_root - elif node_modules_root != possible_root: - fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (node_modules_root, possible_root)) - - # ...first-party packages to be linked into the node_modules tree + path = dep[ExternalNpmPackageInfo].path + workspace = dep[ExternalNpmPackageInfo].workspace + if path in node_modules_roots: + other_workspace = node_modules_roots[path] + if workspace != other_workspace: + fail("All npm dependencies at the path '%s' must come from a single workspace. Found '%s' and '%s'." % (path, other_workspace, workspace)) + node_modules_roots[path] = workspace + + # ...first-party packages to be linked into the root node_modules tree for k, v in getattr(dep, _ASPECT_RESULT_NAME, {}).items(): if _link_mapping(dep.label, mappings, k, v): - # Special case for ts_library module_name for legacy behavior and for AMD name - # work-around. Do not propagate tslibrary root type to runtime as it is not - # needed at runtime. See comment below in _get_module_mappings for more info. - if v[0] == "_tslibrary": - v = ["execroot", v[1]] _debug(ctx.var, "Linking %s: %s" % (k, v)) mappings[k] = v + # Simplify mappings before writing them to the manifest file. + # The extra complication is required in bzl to handle the __tslibrary__ special + # case since module_name in ts_library enables the linker. In a future major release, + # ts_library will get a package_name attribute to enable the linker and the __tslibrary__ + # special case can be factored out. + # This is planned for 4.0: https://github.com/bazelbuild/rules_nodejs/issues/2450. + for k, v in mappings.items(): + mappings[k] = v[1] + # Write the result to a file, and use the magic node option --bazel_node_modules_manifest # The launcher.sh will peel off this argument and pass it to the linker rather than the program. prefix = ctx.label.name @@ -100,7 +97,7 @@ def write_node_modules_manifest(ctx, extra_data = [], mnemonic = None, link_work content = { "bin": ctx.bin_dir.path, "modules": mappings, - "root": node_modules_root, + "roots": node_modules_roots, "workspace": ctx.workspace_name, } ctx.actions.write(modules_manifest, str(content)) @@ -127,18 +124,9 @@ def _get_module_mappings(target, ctx): for name in _MODULE_MAPPINGS_DEPS_NAMES: for dep in getattr(ctx.rule.attr, name, []): for k, v in getattr(dep, _ASPECT_RESULT_NAME, {}).items(): - # A package which was reachable transitively via a *_binary or *_test - # rule is assumed to be in the runfiles of that binary, - # so we switch the linker target root. - # In theory we ought to be able to tell that the files really are - # propagated through the runfiles output of the target we are visiting - # but it seems that this info is only available in Bazel Java internals. - # TODO: revisit whether this is the correct condition after downstream testing - if ctx.rule.kind.endswith("_binary") or ctx.rule.kind.endswith("_test"): - v = ["runfiles", v[1]] if _link_mapping(target.label, mappings, k, v): - mappings[k] = v _debug(ctx.var, "target %s propagating module mapping %s: %s" % (dep.label, k, v)) + mappings[k] = v # Look for LinkablePackageInfo mapping in this node if not LinkablePackageInfo in target: @@ -149,10 +137,10 @@ def _get_module_mappings(target, ctx): linkable_package_info = target[LinkablePackageInfo] mn = linkable_package_info.package_name - mr = ["execroot", linkable_package_info.path] + mr = ["__link__", linkable_package_info.path] # Special case for ts_library module_name for legacy behavior and for AMD name work-around - # Tag the mapping as "_tslibrary" so it can be overridden by any other mapping if found. + # Tag the mapping as "__tslibrary__" so it can be overridden by any other mapping if found. # # In short, ts_library module_name attribute results in both setting the AMD name (which # desired and necessary in devmode which outputs UMD) and in making a linkable mapping. Because @@ -162,7 +150,7 @@ def _get_module_mappings(target, ctx): # pkg_npm but not vice-verse at the moment since ts_library cannot handle directory artifacts as # deps. if hasattr(linkable_package_info, "_tslibrary") and linkable_package_info._tslibrary: - mr[0] = "_tslibrary" + mr[0] = "__tslibrary__" if _link_mapping(target.label, mappings, mn, mr): _debug(ctx.var, "target %s adding module mapping %s: %s" % (target.label, mn, mr)) diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index 3cd59bfc19..3b28d7b564 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -39,7 +39,7 @@ Include as much of the build output as you can without disclosing anything confi async function mkdirp(p: string) { if (p && !await exists(p)) { await mkdirp(path.dirname(p)); - log_verbose(`mkdir( ${p} )`); + log_verbose(`creating directory ${p} in ${process.cwd()}`); try { await fs.promises.mkdir(p); } catch (e) { @@ -106,7 +106,10 @@ async function deleteDirectory(p: string) { } async function symlink(target: string, p: string): Promise { - log_verbose(`symlink( ${p} -> ${target} )`); + if (!path.isAbsolute(target)) { + target = path.resolve(process.cwd(), target); + } + log_verbose(`creating symlink ${p} -> ${target}`); // Use junction on Windows since symlinks require elevated permissions. // We only link to directories so junctions work for us. @@ -136,72 +139,38 @@ async function symlink(target: string, p: string): Promise { } /** - * Resolve to an absolute root node_modules directory. - * @param root The bazel managed node_modules root such as 'npm/node_modules', which includes the - * workspace name as the first segment. May be undefined if there are no third_party node_modules - * deps. - * @param startCwd The absolute path that bazel started the action at. - * @param isExecroot True if the action is run in the execroot, false if the action is run in - * runfiles root. - * @param runfiles The runfiles helper object. - * @return The absolute path on disk where node_modules was installed or if no third party - * node_modules are deps of the current target the returns the absolute path to - * `execroot/my_wksp/node_modules`. + * Resolve to the path to an external workspace */ -async function resolveRoot( - root: string|undefined, startCwd: string, isExecroot: boolean, runfiles: Runfiles) { +async function resolveExternalWorkspacePath( + workspace: string, startCwd: string, isExecroot: boolean, execroot: string|undefined, + runfiles: Runfiles) { if (isExecroot) { - // Under execroot, the root will be under an external folder from the startCwd - // `execroot/my_wksp`. For example, `execroot/my_wksp/external/npm/node_modules`. If there is no - // root, which will be the case if there are no third-party modules dependencies for this - // target, set the root to `execroot/my_wksp/node_modules`. - return root ? `${startCwd}/external/${root}` : `${startCwd}/node_modules`; + // 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 + // 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}`; + } + + 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}`) } // Under runfiles, the linker should symlink node_modules at `execroot/my_wksp` // so that when there are no runfiles (default on Windows) and scripts run out of // `execroot/my_wksp` they can resolve node_modules with standard node_module resolution - // Look for bazel-out which is used to determine the the path to `execroot/my_wksp`. This works in - // all cases including on rbe where the execroot is a path such as `/b/f/w`. For example, when in - // 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. - 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. - - if (!root) { - // If there is no root, which will be the case if there are no third-party modules - // dependencies for this target, simply link to node_modules at the cwd. - return `${startCwd}/node_modules`; - } - - // If there is a root then attempt to symlink as if we are in runfiles in a sandbox. This will - // be the case for nodejs_image. - return path.resolve(`${startCwd}/../${root}`) - } - - // We've found the execroot - const symlinkRoot = startCwd.slice(0, match.index); - process.chdir(symlinkRoot); - - if (!root) { - // If there is no root, which will be the case if there are no third-party modules dependencies - // for this target, set the root to `execroot/my_wksp/node_modules`. - return `${symlinkRoot}/node_modules`; - } - // 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(root); + const fromManifest = runfiles.lookupDirectory(workspace); if (fromManifest) { return fromManifest; } else { - const maybe = path.resolve(`${symlinkRoot}/external/${root}`); - if (fs.existsSync(maybe)) { + const maybe = path.resolve(`${execroot}/external/${workspace}`); + 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 // directory and not a symlink back to the root node_modules where we expect @@ -211,10 +180,9 @@ async function resolveRoot( // 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}/../${root}`) + return path.resolve(`${startCwd}/../${workspace}`) } } - export class Runfiles { manifest: Map|undefined; dir: string|undefined; @@ -324,7 +292,7 @@ export class Runfiles { if (result) { return result; } - const e = new Error(`could not resolve modulePath ${modulePath}`); + const e = new Error(`could not resolve module ${modulePath}`); (e as any).code = 'MODULE_NOT_FOUND'; throw e; } @@ -400,7 +368,10 @@ async function exists(p: string) { return (await gracefulLstat(p) !== null); } -function existsSync(p: string) { +function existsSync(p: string|undefined) { + if (!p) { + return false; + } try { fs.lstatSync(p); return true; @@ -499,25 +470,6 @@ function liftElement(element: LinkerTreeElement): LinkerTreeElement { return element; } -function toParentLink(link: Link): Link { - return [link[0], path.dirname(link[1])]; -} - -function allElementsAlign(name: string, elements: LinkerTreeElement[]): boolean { - if (!elements[0].link) { - return false; - } - - const parentLink = toParentLink(elements[0].link!); - - // Every child needs a link with aligning parents - if (!elements.every(e => !!e.link && isDirectChildLink(parentLink, e.link))) { - return false; - } - - return !!elements[0].link && allElementsAlignUnder(name, parentLink, elements); -} - function allElementsAlignUnder( parentName: string, parentLink: Link, elements: LinkerTreeElement[]) { for (const {name, link, children} of elements) { @@ -545,21 +497,11 @@ function isDirectChildPath(parent: string, child: string) { return parent === path.dirname(child); } -function isDirectChildLink([parentRel, parentPath]: Link, [childRel, childPath]: Link) { - // Ensure same link-relation type - if (parentRel !== childRel) { - return false; - } - - // Ensure child path is a direct-child of the parent path - if (!isDirectChildPath(parentPath, childPath)) { - return false; - } - - return true; +function isDirectChildLink(parentLink: Link, childLink: Link) { + return parentLink === path.dirname(childLink); } -function isNameLinkPathTopAligned(namePath: string, [, linkPath]: Link) { +function isNameLinkPathTopAligned(namePath: string, linkPath: Link) { return path.basename(namePath) === path.basename(linkPath); } @@ -579,18 +521,7 @@ async function visitDirectoryPreserveLinks( } } -// See link_node_modules.bzl where these link roots types -// are used to indicate which root the linker should target -// for each package: -// execroot: Path from execroot/wksp. Either an output dir path such as -// 'bazel-out/-/bin/path/to/package' or -// 'bazel-out/-/bin/external//path/to/package' -// or a source file path such as 'path/to/package' or -// 'external//path/to/package' -// runfiles: Look in the runfiles dir/manifest -export type LinkerRoot = 'execroot'|'runfiles'; - -export type Link = [LinkerRoot, string]; +export type Link = string; export type LinkerTreeElement = { name: string, link?: Link, @@ -600,43 +531,143 @@ export type LinkerAliases = { [name: string]: Link }; +function findExecroot(startCwd: string): string|undefined { + // We can derive if the process is being run in the execroot if there is a bazel-out folder + if (existsSync(`${startCwd}/bazel-out`)) { + return startCwd; + } + + // Look for bazel-out which is used to determine the the path to `execroot/my_wksp`. This works in + // all cases including on rbe where the execroot is a path such as `/b/f/w`. For example, when in + // 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: If we are inside nodejs_image or a nodejs_binary run manually there may be no execroot + // found. + const bazelOutMatch = startCwd.match(BAZEL_OUT_REGEX); + return bazelOutMatch ? startCwd.slice(0, bazelOutMatch.index) : undefined; +} + export async function main(args: string[], runfiles: Runfiles) { if (!args || args.length < 1) throw new Error('requires one argument: modulesManifest path'); const [modulesManifest] = args; - let {bin, root, modules, workspace} = JSON.parse(fs.readFileSync(modulesManifest)); + log_verbose('manifest file:', modulesManifest); + + let {workspace, bin, roots, modules} = JSON.parse(fs.readFileSync(modulesManifest)); modules = modules || {}; - log_verbose('manifest file', modulesManifest); - log_verbose('manifest contents', JSON.stringify({workspace, bin, root, modules}, null, 2)); + log_verbose('manifest contents:', JSON.stringify({workspace, bin, roots, modules}, null, 2)); // Bazel starts actions with pwd=execroot/my_wksp when under execroot or pwd=runfiles/my_wksp // when under runfiles. // Normalize the slashes in startCwd for easier matching and manipulation. const startCwd = process.cwd().replace(/\\/g, '/'); - log_verbose('startCwd', startCwd); + log_verbose('startCwd:', startCwd); - // We can derive if the process is being run in the execroot if there is a bazel-out folder. - const isExecroot = existsSync(`${startCwd}/bazel-out`); - log_verbose('isExecroot', isExecroot.toString()); + const execroot = findExecroot(startCwd); + log_verbose('execroot:', execroot ? execroot : 'not found'); - // NB: resolveRoot will change the cwd when under runfiles to `execroot/my_wksp` - const rootDir = await resolveRoot(root, startCwd, isExecroot, runfiles); - log_verbose('resolved node_modules root', root, 'to', rootDir); - log_verbose('cwd', process.cwd()); + const isExecroot = startCwd == execroot; + log_verbose('isExecroot:', isExecroot.toString()); - // Create rootDir if it does not exists. This will be the case if there are no third-party deps - // for this target or if outside of the sandbox and there are no node_modules installed. - if (!(await exists(rootDir))) { - log_verbose('no third-party packages; mkdir node_modules at', root); - await mkdirp(rootDir); + if (!isExecroot && execroot) { + // If we're not in the execroot and we've found one then change to the execroot + // directory to create the node_modules symlinks + process.chdir(execroot); + log_verbose('changed directory to execroot', execroot); } - // Create the node_modules symlink to the node_modules root that node will resolve from - await symlink(rootDir, 'node_modules'); + async function symlinkWithUnlink( + target: string, p: string, stats: fs.Stats|null = null): Promise { + if (!path.isAbsolute(target)) { + target = path.resolve(process.cwd(), target); + } + if (stats === null) { + stats = await gracefulLstat(p); + } + // Check if this an an old out-of-date symlink + // If we are running without a runfiles manifest (i.e. in sandbox or with symlinked runfiles), + // then this is guaranteed to be not an artifact from a previous linker run. If not we need to + // check. + if (runfiles.manifest && execroot && stats !== null && stats.isSymbolicLink()) { + const symlinkPath = fs.readlinkSync(p).replace(/\\/g, '/'); + if (path.relative(symlinkPath, target) != '' && + !path.relative(execroot, symlinkPath).startsWith('..')) { + // Left-over out-of-date symlink from previous run. This can happen if switching between + // root configuration options such as `--noenable_runfiles` and/or + // `--spawn_strategy=standalone`. It can also happen if two different targets link the same + // module name to different targets in a non-sandboxed environment. The latter will lead to + // undeterministic behavior. + // TODO: can we detect the latter case and throw an apprioriate error? + log_verbose(`Out-of-date symlink for ${p} to ${symlinkPath} detected. Target should be ${ + target}. Unlinking.`); + await unlink(p); + } + } + return symlink(target, p); + } + + // 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 workspacePath = + await resolveExternalWorkspacePath(workspace, startCwd, isExecroot, execroot, runfiles); + log_verbose(`resolved ${workspace} workspace path to ${workspacePath}`); + const workspaceNodeModules = `${workspacePath}/node_modules`; + if (packagePath) { + // sub-directory node_modules + if (await exists(workspaceNodeModules)) { + let resolvedPackagePath; + if (await exists(packagePath)) { + await symlinkWithUnlink(workspaceNodeModules, `${packagePath}/node_modules`); + resolvedPackagePath = packagePath; + } + 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. + const runfilesPackagePath = `${startCwd}/${packagePath}`; + if (await exists(runfilesPackagePath)) { + if (resolvedPackagePath) { + await symlinkWithUnlink( + `${resolvedPackagePath}/node_modules`, `${runfilesPackagePath}/node_modules`); + } else { + await symlinkWithUnlink(workspaceNodeModules, `${runfilesPackagePath}/node_modules`); + } + resolvedPackagePath = runfilesPackagePath; + } + } + const packagePathBin = `${bin}/${packagePath}`; + if (resolvedPackagePath && await exists(packagePathBin)) { + // if bin path exists, symlink bin/package/node_modules -> package/node_modules + await symlinkWithUnlink( + `${resolvedPackagePath}/node_modules`, `${packagePathBin}/node_modules`); + } + } + } else { + // root node_modules + if (await exists(workspaceNodeModules)) { + await symlinkWithUnlink(workspaceNodeModules, `node_modules`); + } else { + // Special case if there no target to symlink to for the root workspace, create a + // root node_modules folder for 1st party deps + log_verbose( + 'no root npm workspace node_modules folder to link to; creating node_modules directory in', + process.cwd()); + await mkdirp('node_modules'); + } + } + } + + if (!roots || !roots['']) { + // Special case if there is no root npm workspace, create a root node_modules folder for 1st + // party deps + log_verbose('no root npm workspace; creating node_modules directory in ', process.cwd()); + await mkdirp('node_modules'); + } - // Change directory to the node_modules root directory so that all subsequent - // symlinks will be created under node_modules - process.chdir(rootDir); + // Change directory to the root node_modules directory for creation of 1st party deps symlinks + process.chdir('node_modules'); /** * Whether the given module resolves to a directory that has been created by a previous linker @@ -701,47 +732,51 @@ export async function main(args: string[], runfiles: Runfiles) { await mkdirp(path.dirname(m.name)); if (m.link) { - const [root, modulePath] = m.link; + const modulePath = m.link; let target: string|undefined; - switch (root) { - case 'execroot': - if (isExecroot) { - target = `${startCwd}/${modulePath}`; - 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() - let runfilesPath = modulePath; - if (runfilesPath.startsWith(`${bin}/`)) { - runfilesPath = runfilesPath.slice(bin.length + 1); - } else if (runfilesPath === bin) { - runfilesPath = ''; - } - const externalPrefix = 'external/'; - if (runfilesPath.startsWith(externalPrefix)) { - runfilesPath = runfilesPath.slice(externalPrefix.length); - } else { - runfilesPath = `${workspace}/${runfilesPath}`; - } - 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; - } + if (isExecroot) { + // If we're running out of the execroot, try the execroot path first. + // If the dependency came in exclusively from a transitive binary target + // then the module won't be at this path but in the runfiles of the binary. + // In that case we'll fallback to resolving via runfiles below. + target = `${startCwd}/${modulePath}`; + } + if (!isExecroot || !existsSync(target)) { + // Transform execroot path to the runfiles manifest path so that + // it can be resolved with runfiles.resolve() + let runfilesPath = modulePath; + if (runfilesPath.startsWith(`${bin}/`)) { + runfilesPath = runfilesPath.slice(bin.length + 1); + } else if (runfilesPath === bin) { + runfilesPath = ''; + } + const externalPrefix = 'external/'; + if (runfilesPath.startsWith(externalPrefix)) { + runfilesPath = runfilesPath.slice(externalPrefix.length); + } else { + runfilesPath = `${workspace}/${runfilesPath}`; + } + 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 && modulePath.startsWith(`${bin}/`)) { + // Check for BAZEL_OUT_REGEX and not /${bin}/ since resolution + // may be in the `/bazel-out/host` if cfg = "host" + if (!target.match(BAZEL_OUT_REGEX)) { + const e = new Error(`could not resolve module ${runfilesPath} in output tree`); + (e as any).code = 'MODULE_NOT_FOUND'; + throw e; } - } catch (err) { - target = undefined; - log_verbose(`runfiles resolve failed for module '${m.name}': ${err.message}`); } - break; + } catch (err) { + target = undefined; + log_verbose(`runfiles resolve failed for module '${m.name}': ${err.message}`); + } + } + // Ensure target path absolute for consistency + if (target && !path.isAbsolute(target)) { + target = path.resolve(process.cwd(), target); } // In environments where runfiles are not symlinked (e.g. Windows), existing linked @@ -762,7 +797,7 @@ export async function main(args: string[], runfiles: Runfiles) { if (stats !== null && isLeftOver) { await createSymlinkAndPreserveContents(stats, m.name, target); } else { - await symlink(target, m.name); + await symlinkWithUnlink(target, m.name, stats); } } else { if (!target) { diff --git a/internal/linker/test/link_node_modules.spec.ts b/internal/linker/test/link_node_modules.spec.ts index 34fa7a9d1e..76dcfb2079 100644 --- a/internal/linker/test/link_node_modules.spec.ts +++ b/internal/linker/test/link_node_modules.spec.ts @@ -54,48 +54,46 @@ describe('link_node_modules', () => { it('should pull aligned child paths up', () => { const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], - '@foo/a/1': ['execroot', `${BIN_DIR}/root/sub/a/1`], - '@foo/a/2': ['execroot', `${BIN_DIR}/root/sub/a/2`], + '@foo/a': `${BIN_DIR}/root/sub/a`, + '@foo/a/b': `${BIN_DIR}/root/sub/a/b`, + '@foo/a/1': `${BIN_DIR}/root/sub/a/1`, + '@foo/a/2': `${BIN_DIR}/root/sub/a/2`, }; - const OUT: LinkerTreeElement[] = [ - {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} - ]; + const OUT: LinkerTreeElement[] = + [{name: '@foo', children: [{name: '@foo/a', link: `${BIN_DIR}/root/sub/a`}]}]; expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should pull deep aligned child paths up', () => { const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], - '@foo/a/b/1': ['execroot', `${BIN_DIR}/root/sub/a/b/1`], - '@foo/a/b/2': ['execroot', `${BIN_DIR}/root/sub/a/b/2`], + '@foo/a': `${BIN_DIR}/root/sub/a`, + '@foo/a/b': `${BIN_DIR}/root/sub/a/b`, + '@foo/a/b/1': `${BIN_DIR}/root/sub/a/b/1`, + '@foo/a/b/2': `${BIN_DIR}/root/sub/a/b/2`, }; - const OUT: LinkerTreeElement[] = [ - {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} - ]; + const OUT: LinkerTreeElement[] = + [{name: '@foo', children: [{name: '@foo/a', link: `${BIN_DIR}/root/sub/a`}]}]; expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not change aligned paths with a misaligned parent', () => { const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], - '@foo/a/b/1': ['execroot', `${BIN_DIR}/root/sub/other/a/b/1`], - '@foo/a/b/2': ['execroot', `${BIN_DIR}/root/sub/a/b/2`], + '@foo/a': `${BIN_DIR}/root/sub/a`, + '@foo/a/b': `${BIN_DIR}/root/sub/a/b`, + '@foo/a/b/1': `${BIN_DIR}/root/sub/other/a/b/1`, + '@foo/a/b/2': `${BIN_DIR}/root/sub/a/b/2`, }; const OUT: LinkerTreeElement[] = [{ name: '@foo', children: [{ name: '@foo/a', - link: ['execroot', `${BIN_DIR}/root/sub/a`], + link: `${BIN_DIR}/root/sub/a`, children: [{ name: '@foo/a/b', - link: ['execroot', `${BIN_DIR}/root/sub/a/b`], + link: `${BIN_DIR}/root/sub/a/b`, children: [ - {name: '@foo/a/b/1', link: ['execroot', `${BIN_DIR}/root/sub/other/a/b/1`]}, - {name: '@foo/a/b/2', link: ['execroot', `${BIN_DIR}/root/sub/a/b/2`]} + {name: '@foo/a/b/1', link: `${BIN_DIR}/root/sub/other/a/b/1`}, + {name: '@foo/a/b/2', link: `${BIN_DIR}/root/sub/a/b/2`} ] }] }] @@ -105,17 +103,17 @@ describe('link_node_modules', () => { it('should not reduce parent/child when parent linking to different path', () => { const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/foo`], - '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], - '@foo/a/b/1': ['execroot', `${BIN_DIR}/root/sub/a/b/1`], - '@foo/a/b/2': ['execroot', `${BIN_DIR}/root/sub/a/b/2`], + '@foo/a': `${BIN_DIR}/root/foo`, + '@foo/a/b': `${BIN_DIR}/root/sub/a/b`, + '@foo/a/b/1': `${BIN_DIR}/root/sub/a/b/1`, + '@foo/a/b/2': `${BIN_DIR}/root/sub/a/b/2`, }; const OUT: LinkerTreeElement[] = [{ name: '@foo', children: [{ name: '@foo/a', - link: ['execroot', `${BIN_DIR}/root/foo`], - children: [{name: '@foo/a/b', link: ['execroot', `${BIN_DIR}/root/sub/a/b`]}] + link: `${BIN_DIR}/root/foo`, + children: [{name: '@foo/a/b', link: `${BIN_DIR}/root/sub/a/b`}] }] }]; expect(linker.reduceModules(IN)).toEqual(OUT); @@ -123,77 +121,39 @@ describe('link_node_modules', () => { it('should reduce aligned parent+children aliases to single parent alias', () => { const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/1': ['execroot', `${BIN_DIR}/root/sub/a/1`], - '@foo/a/2': ['execroot', `${BIN_DIR}/root/sub/a/2`], - }; - const OUT: LinkerTreeElement[] = [ - {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} - ]; - expect(linker.reduceModules(IN)).toEqual(OUT); - }); - - it('should not reduce parent+children aliases aligned to different link types', () => { - const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/1': ['runfiles', `${BIN_DIR}/root/sub/a/1`], + '@foo/a': `${BIN_DIR}/root/sub/a`, + '@foo/a/1': `${BIN_DIR}/root/sub/a/1`, + '@foo/a/2': `${BIN_DIR}/root/sub/a/2`, }; - const OUT: LinkerTreeElement[] = [{ - name: '@foo', - children: [{ - name: '@foo/a', - link: ['execroot', `${BIN_DIR}/root/sub/a`], - children: [{name: '@foo/a/1', link: ['runfiles', `${BIN_DIR}/root/sub/a/1`]}] - }] - }]; - expect(linker.reduceModules(IN)).toEqual(OUT); - }); - - it('should not reduce sibling aliases with different link types', () => { - const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/1': ['runfiles', `${BIN_DIR}/root/sub/a/1`], - '@foo/a/2': ['execroot', `${BIN_DIR}/root/sub/a/2`], - }; - const OUT: LinkerTreeElement[] = [{ - name: '@foo', - children: [{ - name: '@foo/a', - link: ['execroot', `${BIN_DIR}/root/sub/a`], - children: [ - {name: '@foo/a/1', link: ['runfiles', `${BIN_DIR}/root/sub/a/1`]}, - {name: '@foo/a/2', link: ['execroot', `${BIN_DIR}/root/sub/a/2`]} - ] - }] - }]; + const OUT: LinkerTreeElement[] = + [{name: '@foo', children: [{name: '@foo/a', link: `${BIN_DIR}/root/sub/a`}]}]; expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should reduce deeply-aligned siblings', () => { const IN: LinkerAliases = { - '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], - '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], - '@foo/a/b/c': ['execroot', `${BIN_DIR}/root/sub/a/b/c`], - '@foo/a/b/c/d1': ['execroot', `${BIN_DIR}/root/sub/a/b/c/d1`], - '@foo/a/b/c/d2': ['execroot', `${BIN_DIR}/root/sub/a/b/c/d2`], + '@foo/a': `${BIN_DIR}/root/sub/a`, + '@foo/a/b': `${BIN_DIR}/root/sub/a/b`, + '@foo/a/b/c': `${BIN_DIR}/root/sub/a/b/c`, + '@foo/a/b/c/d1': `${BIN_DIR}/root/sub/a/b/c/d1`, + '@foo/a/b/c/d2': `${BIN_DIR}/root/sub/a/b/c/d2`, }; - const OUT: LinkerTreeElement[] = [ - {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} - ]; + const OUT: LinkerTreeElement[] = + [{name: '@foo', children: [{name: '@foo/a', link: `${BIN_DIR}/root/sub/a`}]}]; expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not reduce parent/child with different ancestor link paths', () => { const IN: LinkerAliases = { - 'b/b': ['execroot', 'external/other_wksp/path/to/lib_bb'], - 'b': ['execroot', 'external/other_wksp/path/to/lib_b'], + 'b/b': 'external/other_wksp/path/to/lib_bb', + 'b': 'external/other_wksp/path/to/lib_b', }; const OUT: LinkerTreeElement[] = [{ name: 'b', - link: ['execroot', 'external/other_wksp/path/to/lib_b'], + link: 'external/other_wksp/path/to/lib_b', children: [{ name: 'b/b', - link: ['execroot', 'external/other_wksp/path/to/lib_bb'], + link: 'external/other_wksp/path/to/lib_bb', }], }]; @@ -202,20 +162,20 @@ describe('link_node_modules', () => { it('should not reduce aligned paths when link has extra dir', () => { const IN: LinkerAliases = { - '@foo/lib': ['execroot', `${BIN_DIR}/path/to/lib`], - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/noseeme/a`], - '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/noseeme/b`], - '@foo/lib/c': ['execroot', `${BIN_DIR}/path/to/lib/noseeme/c`], + '@foo/lib': `${BIN_DIR}/path/to/lib`, + '@foo/lib/a': `${BIN_DIR}/path/to/lib/noseeme/a`, + '@foo/lib/b': `${BIN_DIR}/path/to/lib/noseeme/b`, + '@foo/lib/c': `${BIN_DIR}/path/to/lib/noseeme/c`, }; const OUT: LinkerTreeElement[] = [{ name: '@foo', children: [{ name: '@foo/lib', - link: ['execroot', `${BIN_DIR}/path/to/lib`], + link: `${BIN_DIR}/path/to/lib`, children: [ - {name: '@foo/lib/a', link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme/a`]}, - {name: '@foo/lib/b', link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme/b`]}, - {name: '@foo/lib/c', link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme/c`]} + {name: '@foo/lib/a', link: `${BIN_DIR}/path/to/lib/noseeme/a`}, + {name: '@foo/lib/b', link: `${BIN_DIR}/path/to/lib/noseeme/b`}, + {name: '@foo/lib/c', link: `${BIN_DIR}/path/to/lib/noseeme/c`} ] }] }]; @@ -224,18 +184,18 @@ describe('link_node_modules', () => { it('should not reduce sibling mappings with inconsistent paths', () => { const IN: LinkerAliases = { - '@foo/lib': ['execroot', `${BIN_DIR}/path/to/lib`], - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/x`], - '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], + '@foo/lib': `${BIN_DIR}/path/to/lib`, + '@foo/lib/a': `${BIN_DIR}/path/to/lib/x`, + '@foo/lib/b': `${BIN_DIR}/path/to/lib/b`, }; const OUT: LinkerTreeElement[] = [{ name: '@foo', children: [{ name: '@foo/lib', - link: ['execroot', `${BIN_DIR}/path/to/lib`], + link: `${BIN_DIR}/path/to/lib`, children: [ - {name: '@foo/lib/a', link: ['execroot', `${BIN_DIR}/path/to/lib/x`]}, - {name: '@foo/lib/b', link: ['execroot', `${BIN_DIR}/path/to/lib/b`]} + {name: '@foo/lib/a', link: `${BIN_DIR}/path/to/lib/x`}, + {name: '@foo/lib/b', link: `${BIN_DIR}/path/to/lib/b`}, ] }] }]; @@ -244,18 +204,18 @@ describe('link_node_modules', () => { it('should not reduce sibling mappings with inconsistent path parents', () => { const IN: LinkerAliases = { - '@foo/lib': ['execroot', `${BIN_DIR}/path/to/lib`], - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/x/a`], - '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], + '@foo/lib': `${BIN_DIR}/path/to/lib`, + '@foo/lib/a': `${BIN_DIR}/path/to/lib/x/a`, + '@foo/lib/b': `${BIN_DIR}/path/to/lib/b`, }; const OUT: LinkerTreeElement[] = [{ name: '@foo', children: [{ name: '@foo/lib', - link: ['execroot', `${BIN_DIR}/path/to/lib`], + link: `${BIN_DIR}/path/to/lib`, children: [ - {name: '@foo/lib/a', link: ['execroot', `${BIN_DIR}/path/to/lib/x/a`]}, - {name: '@foo/lib/b', link: ['execroot', `${BIN_DIR}/path/to/lib/b`]} + {name: '@foo/lib/a', link: `${BIN_DIR}/path/to/lib/x/a`}, + {name: '@foo/lib/b', link: `${BIN_DIR}/path/to/lib/b`} ] }] }]; @@ -264,32 +224,23 @@ describe('link_node_modules', () => { it('should not reduce to parent mapping with inconcsisten parent link path', () => { const IN: LinkerAliases = { - '@foo/lib': ['execroot', `${BIN_DIR}/path/to/other/lib`], - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/a`], - '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], + '@foo/lib': `${BIN_DIR}/path/to/other/lib`, + '@foo/lib/a': `${BIN_DIR}/path/to/lib/a`, + '@foo/lib/b': `${BIN_DIR}/path/to/lib/b`, }; const OUT: LinkerTreeElement[] = [{ name: '@foo', children: [{ name: '@foo/lib', - link: [ - 'execroot', - `${BIN_DIR}/path/to/other/lib`, - ], + link: `${BIN_DIR}/path/to/other/lib`, children: [ { name: '@foo/lib/a', - link: [ - 'execroot', - `${BIN_DIR}/path/to/lib/a`, - ] + link: `${BIN_DIR}/path/to/lib/a`, }, { name: '@foo/lib/b', - link: [ - 'execroot', - `${BIN_DIR}/path/to/lib/b`, - ] + link: `${BIN_DIR}/path/to/lib/b`, } ] }] @@ -299,14 +250,14 @@ describe('link_node_modules', () => { it('should reduce complicated example', () => { const IN: LinkerAliases = { - 'a': ['execroot', `path/to/lib_a`], - '@foo/c/c/c/c': ['execroot', `${BIN_DIR}/path/to/foo_cccc`], - 'b/b': ['execroot', 'external/other_wksp/path/to/lib_bb'], - 'b': ['execroot', 'external/other_wksp/path/to/lib_b'], - '@foo/c': ['execroot', `${BIN_DIR}/path/to/foo_c`], - '@foo/c/c': ['execroot', `${BIN_DIR}/path/to/foo_cc`], - '@foo/d/bar/fum/far': ['execroot', `${BIN_DIR}/path/to/foo_d/bar/fum/far`], - '@foo/d/bar': ['execroot', `${BIN_DIR}/path/to/foo_d/bar`], + 'a': `path/to/lib_a`, + '@foo/c/c/c/c': `${BIN_DIR}/path/to/foo_cccc`, + 'b/b': 'external/other_wksp/path/to/lib_bb', + 'b': 'external/other_wksp/path/to/lib_b', + '@foo/c': `${BIN_DIR}/path/to/foo_c`, + '@foo/c/c': `${BIN_DIR}/path/to/foo_cc`, + '@foo/d/bar/fum/far': `${BIN_DIR}/path/to/foo_d/bar/fum/far`, + '@foo/d/bar': `${BIN_DIR}/path/to/foo_d/bar`, // don't include `@foo/d` as the linker should derive that symlink // from the lowest common denominator of the module name & module path }; @@ -316,15 +267,16 @@ describe('link_node_modules', () => { children: [ { name: '@foo/c', - link: ['execroot', `${BIN_DIR}/path/to/foo_c`], + link: `${BIN_DIR}/path/to/foo_c`, children: [{ name: '@foo/c/c', - link: ['execroot', `${BIN_DIR}/path/to/foo_cc`], + link: `${BIN_DIR}/path/to/foo_cc`, children: [{ name: '@foo/c/c/c', - children: [ - {name: '@foo/c/c/c/c', link: ['execroot', `${BIN_DIR}/path/to/foo_cccc`]} - ] + children: [{ + name: '@foo/c/c/c/c', + link: `${BIN_DIR}/path/to/foo_cccc`, + }] }] }] }, @@ -332,22 +284,22 @@ describe('link_node_modules', () => { name: '@foo/d', children: [{ name: '@foo/d/bar', - link: ['execroot', `${BIN_DIR}/path/to/foo_d/bar`], + link: `${BIN_DIR}/path/to/foo_d/bar`, children: [{ name: '@foo/d/bar/fum', children: [{ name: '@foo/d/bar/fum/far', - link: ['execroot', `${BIN_DIR}/path/to/foo_d/bar/fum/far`] + link: `${BIN_DIR}/path/to/foo_d/bar/fum/far`, }] }] }] } ] }, - {name: 'a', link: ['execroot', 'path/to/lib_a']}, { + {name: 'a', link: 'path/to/lib_a'}, { name: 'b', - link: ['execroot', 'external/other_wksp/path/to/lib_b'], - children: [{name: 'b/b', link: ['execroot', 'external/other_wksp/path/to/lib_bb']}] + link: 'external/other_wksp/path/to/lib_b', + children: [{name: 'b/b', link: 'external/other_wksp/path/to/lib_bb'}] } ]; expect(linker.reduceModules(IN)).toEqual(OUT); @@ -407,21 +359,22 @@ describe('link_node_modules', () => { 'utf-8'); writeManifest({ - 'bin': BIN_DIR, + bin: BIN_DIR, // intentionally out of order so that linker has to sort // and create nested modules in the correct order - 'modules': { - 'a': ['execroot', `path/to/lib_a`], - '@foo/c/c/c/c': ['execroot', `${BIN_DIR}/path/to/foo_cccc`], - 'b/b': ['execroot', 'external/other_wksp/path/to/lib_bb'], - 'b': ['execroot', 'external/other_wksp/path/to/lib_b'], - '@foo/c': ['execroot', `${BIN_DIR}/path/to/foo_c`], - '@foo/c/c': ['execroot', `${BIN_DIR}/path/to/foo_cc`], - '@foo/d/bar/fum/far': ['execroot', `${BIN_DIR}/path/to/foo_d/bar/fum/far`], - '@foo/d/bar': ['execroot', `${BIN_DIR}/path/to/foo_d/bar`], - '@foo/d': ['execroot', `${BIN_DIR}/path/to/foo_d`], + modules: { + 'a': `path/to/lib_a`, + '@foo/c/c/c/c': `${BIN_DIR}/path/to/foo_cccc`, + 'b/b': 'external/other_wksp/path/to/lib_bb', + 'b': 'external/other_wksp/path/to/lib_b', + '@foo/c': `${BIN_DIR}/path/to/foo_c`, + '@foo/c/c': `${BIN_DIR}/path/to/foo_cc`, + '@foo/d/bar/fum/far': `${BIN_DIR}/path/to/foo_d/bar/fum/far`, + '@foo/d/bar': `${BIN_DIR}/path/to/foo_d/bar`, + '@foo/d': `${BIN_DIR}/path/to/foo_d`, }, - 'workspace': workspace, + workspace: workspace, + roots: {}, }); // TODO(alexeagle): test should control the environment, not just pass through @@ -461,9 +414,10 @@ describe('link_node_modules', () => { bin: BIN_DIR, workspace: workspace, modules: { - '@angular/cdk/bidi': ['execroot', `${BIN_DIR}/src/cdk/bidi`], - '@angular/cdk/core': ['execroot', `${BIN_DIR}/src/other-core-folder`], + '@angular/cdk/bidi': `${BIN_DIR}/src/cdk/bidi`, + '@angular/cdk/core': `${BIN_DIR}/src/other-core-folder`, }, + roots: {} }); await linker.main(['manifest.json'], new linker.Runfiles({ @@ -485,10 +439,11 @@ describe('link_node_modules', () => { bin: BIN_DIR, workspace: workspace, modules: { - '@angular/cdk': ['execroot', `${BIN_DIR}/src/cdk`], - '@angular/cdk/bidi': ['execroot', `${BIN_DIR}/src/cdk/bidi`], - '@angular/cdk/core': ['execroot', `${BIN_DIR}/src/other-core-folder`], + '@angular/cdk': `${BIN_DIR}/src/cdk`, + '@angular/cdk/bidi': `${BIN_DIR}/src/cdk/bidi`, + '@angular/cdk/core': `${BIN_DIR}/src/other-core-folder`, }, + roots: {} }); // In the second run, we added a mapping for `@angular/cdk`. This means @@ -521,13 +476,14 @@ describe('link_node_modules', () => { fs.writeFileSync(`${BIN_DIR}/path/to/lib/c/index.js`, '/*c*/exports = {}', 'utf-8'); writeManifest({ - 'bin': BIN_DIR, - 'modules': { - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/a`], - '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], - '@foo/lib/c': ['execroot', `${BIN_DIR}/path/to/lib/c`], + bin: BIN_DIR, + modules: { + '@foo/lib/a': `${BIN_DIR}/path/to/lib/a`, + '@foo/lib/b': `${BIN_DIR}/path/to/lib/b`, + '@foo/lib/c': `${BIN_DIR}/path/to/lib/c`, }, - 'workspace': workspace, + workspace: workspace, + roots: {}, }); // TODO(alexeagle): test should control the environment, not just pass through @@ -553,13 +509,14 @@ describe('link_node_modules', () => { fs.writeFileSync('path/to/lib/c/index.js', '/*c*/exports = {}', 'utf-8'); writeManifest({ - 'bin': BIN_DIR, - 'modules': { - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/x`], - '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], - '@foo/lib/c': ['execroot', `path/to/lib/c`], + bin: BIN_DIR, + modules: { + '@foo/lib/a': `${BIN_DIR}/path/to/lib/x`, + '@foo/lib/b': `${BIN_DIR}/path/to/lib/b`, + '@foo/lib/c': `path/to/lib/c`, }, - 'workspace': workspace, + workspace: workspace, + roots: {}, }); // TODO(alexeagle): test should control the environment, not just pass through @@ -585,13 +542,14 @@ describe('link_node_modules', () => { fs.writeFileSync(`${BIN_DIR}/path/to/bar/b/index.js`, '/*ab*/exports = {}', 'utf-8'); writeManifest({ - 'bin': BIN_DIR, - 'modules': { - '@foo/lib': ['execroot', `${BIN_DIR}/path/to/other/lib`], - '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/foo/a`], - '@foo/lib/a/b': ['execroot', `${BIN_DIR}/path/to/bar/b`], + bin: BIN_DIR, + modules: { + '@foo/lib': `${BIN_DIR}/path/to/other/lib`, + '@foo/lib/a': `${BIN_DIR}/path/to/foo/a`, + '@foo/lib/a/b': `${BIN_DIR}/path/to/bar/b`, }, - 'workspace': workspace, + workspace: workspace, + roots: {}, }); // TODO(alexeagle): test should control the environment, not just pass through @@ -616,8 +574,8 @@ describe('link_node_modules', () => { // No first-party packages writeManifest({ - 'bin': BIN_DIR, - 'root': 'npm/node_modules', + bin: BIN_DIR, + roots: {'': 'npm'}, }); writeRunfiles(runfilesManifest); diff --git a/internal/linker/test/multi_linker/BUILD.bazel b/internal/linker/test/multi_linker/BUILD.bazel new file mode 100644 index 0000000000..9b3bf5ba8f --- /dev/null +++ b/internal/linker/test/multi_linker/BUILD.bazel @@ -0,0 +1,38 @@ +load("//packages/jasmine:index.bzl", "jasmine_node_test") +load("//packages/typescript:checked_in_ts_project.bzl", "checked_in_ts_project") + +checked_in_ts_project( + name = "checked_in_from_sources_test", + src = "test.ts", + checked_in_js = "from_sources_test.js", + deps = [ + "@npm//@types/jest", + "@npm//@types/node", + ], +) + +# Test with a .js file in the output-tree to ensure that we can +# resolve multi-linked node_modules from that context +jasmine_node_test( + name = "test", + srcs = ["test.js"], + deps = [ + "//internal/linker/test/multi_linker/onepa", + "//internal/linker/test/multi_linker/onepb", + "@npm//semver", + "@npm_internal_linker_test_multi_linker//semver", + ], +) + +# Test with a .js file from the source tree to ensure that we can +# resolve multi-linked node_modules from that context +jasmine_node_test( + name = "from_sources_test", + srcs = ["from_sources_test.js"], + deps = [ + "//internal/linker/test/multi_linker/onepa", + "//internal/linker/test/multi_linker/onepb", + "@npm//semver", + "@npm_internal_linker_test_multi_linker//semver", + ], +) diff --git a/internal/linker/test/multi_linker/from_sources_test.js b/internal/linker/test/multi_linker/from_sources_test.js new file mode 100644 index 0000000000..5c9e7c590c --- /dev/null +++ b/internal/linker/test/multi_linker/from_sources_test.js @@ -0,0 +1,16 @@ +/* THIS FILE GENERATED FROM .ts; see BUILD.bazel */ /* clang-format off */"use strict"; +describe('linker', () => { + it('should link nested node modules', () => { + const semverVersion = require(require.resolve('semver/package.json')).version; + expect(semverVersion).toBe('1.0.0'); + }); + it('should get transitive nested node_modules from 1p dep', () => { + const onepa = require('@rules_nodejs/onepa'); + expect(onepa.semverVersion()).toBe('1.0.1'); + }); + it('should get semver from root pacakge.json (which is currently 5.6.0) if the is no transitive in 1p dep', () => { + const onepb = require('@rules_nodejs/onepb'); + const major = onepb.semver().major(onepb.semverVersion()); + expect(major).toBeGreaterThanOrEqual(5); + }); +}); diff --git a/internal/linker/test/multi_linker/onepa/BUILD.bazel b/internal/linker/test/multi_linker/onepa/BUILD.bazel new file mode 100644 index 0000000000..fd4f56ab0b --- /dev/null +++ b/internal/linker/test/multi_linker/onepa/BUILD.bazel @@ -0,0 +1,14 @@ +load("//:index.bzl", "js_library") + +js_library( + name = "onepa", + package_name = "@rules_nodejs/onepa", + srcs = [ + "index.js", + "package.json", + ], + visibility = ["//internal/linker/test/multi_linker:__pkg__"], + deps = [ + "@onepa_npm_deps//:node_modules", + ], +) diff --git a/internal/linker/test/multi_linker/onepa/index.js b/internal/linker/test/multi_linker/onepa/index.js new file mode 100644 index 0000000000..b8d91601f9 --- /dev/null +++ b/internal/linker/test/multi_linker/onepa/index.js @@ -0,0 +1,6 @@ +function semverVersion() { + const semverVersion = require(require.resolve('semver/package.json')).version; + return semverVersion; +} + +module.exports = {semverVersion} diff --git a/internal/linker/test/multi_linker/onepa/package.json b/internal/linker/test/multi_linker/onepa/package.json new file mode 100644 index 0000000000..ed02f69b72 --- /dev/null +++ b/internal/linker/test/multi_linker/onepa/package.json @@ -0,0 +1,7 @@ +{ + "private": true, + "//": "semver intentionally pinned to unique 1.0.1 to assert this version is found", + "dependencies": { + "semver": "1.0.1" + } +} diff --git a/internal/linker/test/multi_linker/onepa/yarn.lock b/internal/linker/test/multi_linker/onepa/yarn.lock new file mode 100644 index 0000000000..4ed9911db4 --- /dev/null +++ b/internal/linker/test/multi_linker/onepa/yarn.lock @@ -0,0 +1,8 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 + + +semver@1.0.1: + version "1.0.1" + resolved "https://registry.yarnpkg.com/semver/-/semver-1.0.1.tgz#93b90b9a3e00c7a143f2e49f6e2b32fd72237cdb" + integrity sha1-k7kLmj4Ax6FD8uSfbisy/XIjfNs= diff --git a/internal/linker/test/multi_linker/onepb/BUILD.bazel b/internal/linker/test/multi_linker/onepb/BUILD.bazel new file mode 100644 index 0000000000..9293fc46f9 --- /dev/null +++ b/internal/linker/test/multi_linker/onepb/BUILD.bazel @@ -0,0 +1,14 @@ +load("//:index.bzl", "js_library") + +# For negative testing, an alternative to 'onepb' +# that doesn't include the transitive semver npm dep +# that @onepa_npm_deps//:node_modules does +js_library( + name = "onepb", + package_name = "@rules_nodejs/onepb", + srcs = [ + "index.js", + "package.json", + ], + visibility = ["//internal/linker/test/multi_linker:__pkg__"], +) diff --git a/internal/linker/test/multi_linker/onepb/index.js b/internal/linker/test/multi_linker/onepb/index.js new file mode 100644 index 0000000000..3be6046768 --- /dev/null +++ b/internal/linker/test/multi_linker/onepb/index.js @@ -0,0 +1,13 @@ +function semverVersion() { + const semverVersion = require(require.resolve('semver/package.json')).version; + return semverVersion; +} + +function semver() { + return require('semver'); +} + +module.exports = { + semverVersion, + semver +} diff --git a/internal/linker/test/multi_linker/onepb/package.json b/internal/linker/test/multi_linker/onepb/package.json new file mode 100644 index 0000000000..8e74f7912b --- /dev/null +++ b/internal/linker/test/multi_linker/onepb/package.json @@ -0,0 +1,6 @@ +{ + "private": true, + "peerDependency": { + "semver": ">=5.0.0" + } +} diff --git a/internal/linker/test/multi_linker/package.json b/internal/linker/test/multi_linker/package.json new file mode 100644 index 0000000000..86d5883760 --- /dev/null +++ b/internal/linker/test/multi_linker/package.json @@ -0,0 +1,7 @@ +{ + "private": true, + "//": "semver intentionally pinned to unique 1.0.0 to assert this version is found", + "devDependencies": { + "semver": "1.0.0" + } +} diff --git a/internal/linker/test/multi_linker/test.ts b/internal/linker/test/multi_linker/test.ts new file mode 100644 index 0000000000..4e98d93cd5 --- /dev/null +++ b/internal/linker/test/multi_linker/test.ts @@ -0,0 +1,24 @@ +describe('linker', () => { + it('should link nested node modules', () => { + // The nested internal/linker/test/multi_linker/package.json file pulls in semver 1.0.0 + // require('semver') should resolve to that version and not the root package.json version + const semverVersion = require(require.resolve('semver/package.json')).version; + expect(semverVersion).toBe('1.0.0'); + }); + + it('should get transitive nested node_modules from 1p dep', () => { + // The nested transitive internal/linker/test/multi_linker/onepa/package.json file pulls + // in semver 1.0.1 into @onepa_npm_deps//:node_modules. onepa should find that version + // as @onepa_npm_deps//:node_modules is pulled in transitively via + // //internal/linker/test/multi_linker/onepa + const onepa = require('@rules_nodejs/onepa'); + expect(onepa.semverVersion()).toBe('1.0.1'); + }); + + it('should get semver from root pacakge.json (which is currently 5.6.0) if the is no transitive in 1p dep', + () => { + const onepb = require('@rules_nodejs/onepb'); + const major = onepb.semver().major(onepb.semverVersion()); + expect(major).toBeGreaterThanOrEqual(5); + }); +}); diff --git a/internal/linker/test/multi_linker/tsconfig.json b/internal/linker/test/multi_linker/tsconfig.json new file mode 100644 index 0000000000..e89edbac6c --- /dev/null +++ b/internal/linker/test/multi_linker/tsconfig.json @@ -0,0 +1,5 @@ +{ + "compilerOptions": { + "types": ["node", "jest"] + } +} diff --git a/internal/linker/test/multi_linker/yarn.lock b/internal/linker/test/multi_linker/yarn.lock new file mode 100644 index 0000000000..49b7a00fa1 --- /dev/null +++ b/internal/linker/test/multi_linker/yarn.lock @@ -0,0 +1,8 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 + + +semver@1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/semver/-/semver-1.0.0.tgz#11f18a0c08ed21c988fc2b0257f1951969816615" + integrity sha1-EfGKDAjtIcmI/CsCV/GVGWmBZhU= diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index ea03bf8621..d602cd2f78 100644 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -240,12 +240,7 @@ if [ "$NODE_PATCHES" = true ]; then LAUNCHER_NODE_OPTIONS+=( "--require" "$node_patches_script" ) fi -# Tell the node_patches_script that programs should not escape the execroot -# Bazel always sets the PWD to execroot/my_wksp so we go up one directory. -export BAZEL_PATCH_ROOT=$(dirname $PWD) - -# Set all bazel managed node_modules directories as guarded so no symlinks may -# escape and no symlinks may enter +# Find the execroot if [[ "$PWD" == *"/bazel-out/"* ]]; then # We in runfiles, find the execroot. # Look for `bazel-out` which is used to determine the the path to `execroot/my_wksp`. This works in @@ -257,24 +252,80 @@ if [[ "$PWD" == *"/bazel-out/"* ]]; then readonly rest=${PWD#*$bazel_out} readonly index=$(( ${#PWD} - ${#rest} - ${#bazel_out} )) if [[ ${index} < 0 ]]; then - echo "No 'bazel-out' folder found in path '${PWD}'!" + echo "No 'bazel-out' folder found in path '${PWD}'!" >&2 exit 1 fi EXECROOT=${PWD:0:${index}} + RUNFILES_ROOT=$(dirname $PWD) else # We are in execroot or in some other context all together such as a nodejs_image or a manually # run nodejs_binary. If this is execroot then linker node_modules is in the PWD. If this another # context then it is safe to assume the node_modules are there and guard that directory if it exists. EXECROOT=${PWD} + RUNFILES_ROOT= fi -export BAZEL_PATCH_GUARDS="${EXECROOT}/node_modules" -if [[ -n "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then - if [[ "${BAZEL_NODE_MODULES_ROOT}" != "${BAZEL_WORKSPACE}/node_modules" ]]; then - # If BAZEL_NODE_MODULES_ROOT is set and it is not , add it to the list of bazel patch guards - # Also, add the external/${BAZEL_NODE_MODULES_ROOT} which is the correct path under execroot - # and under runfiles it is the legacy external runfiles path - export BAZEL_PATCH_GUARDS="${BAZEL_PATCH_GUARDS},${BAZEL_PATCH_ROOT}/${BAZEL_NODE_MODULES_ROOT},${PWD}/external/${BAZEL_NODE_MODULES_ROOT}" + +# Tell the node_patches_script that programs should not escape the execroot +export BAZEL_PATCH_ROOTS="${EXECROOT}" +# Set all bazel managed node_modules directories as guarded so no symlinks may +# escape and no symlinks may enter. +# We always guard against the root node_modules where 1st party deps go. +# (e.g., /private/.../execroot/build_bazel_rules_nodejs/node_modules) +export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${EXECROOT}/node_modules" +if [[ "${RUNFILES_ROOT}" ]]; then + # If in runfiles, guard the runfiles root itself + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}" + # If in runfiles guard the node_modules location in runfiles as well + # (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 [[ -n "${BAZEL_NODE_MODULES_ROOTS:-}" ]]; then + # BAZEL_NODE_MODULES_ROOTS is in the format ":,:" + # (e.g., "internal/linker/test:npm_internal_linker_test,:npm") + if [[ -n "${VERBOSE_LOGS:-}" ]]; then + echo "BAZEL_NODE_MODULES_ROOTS=${BAZEL_NODE_MODULES_ROOTS}" >&2 fi + OLDIFS="${IFS}" + IFS="," + roots=(${BAZEL_NODE_MODULES_ROOTS}) + IFS="${OLDIFS}" + for root in "${roots[@]}"; do + if [[ "${root}" ]]; then + OLDIFS="${IFS}" + IFS=":" + root_pair=(${root}) + IFS="${OLDIFS}" + root_path="${root_pair[0]}" + root_workspace="${root_pair[1]}" + if [[ "${root_path}" ]]; then + # Guard non-root node_modules as well + # (e.g., /private/.../execroot/build_bazel_rules_nodejs/internal/linker/test/node_modules) + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${EXECROOT}/${root_path}/node_modules" + if [[ "${RUNFILES_ROOT}" ]]; then + # If in runfiles guard the node_modules location in runfiles as well + # (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/internal/linker/test/node_modules) + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}/${BAZEL_WORKSPACE}/${root_path}/node_modules" + fi + fi + # TODO: the following guards on the external workspaces may not be necessary and could be removed in the future with care + if [[ "${root_workspace}" ]] && [[ "${root_workspace}" != "${BAZEL_WORKSPACE}" ]]; then + # Guard the external workspaces if they are not the user workspace + # (e.g., /private/.../execroot/build_bazel_rules_nodejs/external/npm_internal_linker_test/node_modules) + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${EXECROOT}/external/${root_workspace}/node_modules" + if [[ "${RUNFILES_ROOT}" ]]; then + # If in runfiles guard the external workspace location in runfiles as well + # (e.g., /private/.../execroot/build_bazel_rules_nodejs/bazel-out/darwin-fastbuild/bin/internal/linker/test/multi_linker/test.sh.runfiles/npm_internal_linker_test/node_modules) + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}/${root_workspace}/node_modules" + # and include the legacy runfiles location incase legacy runfiles are enabled + # (e.g., /private/.../bazel-out/darwin-fastbuild/bin/internal/linker/test/multi_linker/test.sh.runfiles/build_bazel_rules_nodejs/external/npm_internal_linker_test/node_modules) + export BAZEL_PATCH_ROOTS="${BAZEL_PATCH_ROOTS},${RUNFILES_ROOT}/${BAZEL_WORKSPACE}/external/${root_workspace}/node_modules" + fi + fi + fi + done +fi +if [[ -n "${VERBOSE_LOGS:-}" ]]; then + echo "BAZEL_PATCH_ROOTS=${BAZEL_PATCH_ROOTS}" >&2 fi if [ "$PATCH_REQUIRE" = true ]; then @@ -311,7 +362,7 @@ readonly EXPECTED_EXIT_CODE="TEMPLATED_expected_exit_code" if [[ -n "${COVERAGE_DIR:-}" ]]; then if [[ -n "${VERBOSE_LOGS:-}" ]]; then - echo "Turning on node coverage with NODE_V8_COVERAGE=${COVERAGE_DIR}" + echo "Turning on node coverage with NODE_V8_COVERAGE=${COVERAGE_DIR}" >&2 fi # Setting NODE_V8_COVERAGE=${COVERAGE_DIR} causes NodeJS to write coverage # information our to the COVERAGE_DIR once the process exits @@ -387,10 +438,10 @@ fi # Post process the coverage information after the process has exited if [[ -n "${COVERAGE_DIR:-}" ]]; then if [[ -n "${VERBOSE_LOGS:-}" ]]; then - echo "Running coverage lcov merger script with arguments" - echo " --coverage_dir="${COVERAGE_DIR}"" - echo " --output_file="${COVERAGE_OUTPUT_FILE}"" - echo " --source_file_manifest="${COVERAGE_MANIFEST}"" + echo "Running coverage lcov merger script with arguments" >&2 + echo " --coverage_dir="${COVERAGE_DIR}"" >&2 + echo " --output_file="${COVERAGE_OUTPUT_FILE}"" >&2 + echo " --source_file_manifest="${COVERAGE_MANIFEST}"" >&2 fi set +e @@ -399,7 +450,7 @@ if [[ -n "${COVERAGE_DIR:-}" ]]; then set -e if [[ -n "${EXIT_CODE_CAPTURE}" ]]; then - echo "${RESULT}" > "${EXIT_CODE_CAPTURE}" + echo "${RESULT}" > "${EXIT_CODE_CAPTURE}" >&2 fi fi diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 4a8d70a771..8a8b6cabdb 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -40,27 +40,19 @@ def _trim_package_node_modules(package_name): segments.append(n) return "/".join(segments) -def _compute_node_modules_root(ctx): - """Computes the node_modules root from the deps attributes. - - Args: - ctx: the skylark execution context - - Returns: - The node_modules root as a string - """ - node_modules_root = None +def _compute_node_modules_roots(ctx): + """Computes the node_modules root (if any) from data attribute.""" + node_modules_roots = {} for d in ctx.attr.data: if ExternalNpmPackageInfo in d: - possible_root = "/".join([d[ExternalNpmPackageInfo].workspace, "node_modules"]) - if not node_modules_root: - node_modules_root = possible_root - elif node_modules_root != possible_root: - fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (node_modules_root, possible_root)) - if not node_modules_root: - # there are no fine grained deps but we still need a node_modules_root even if it is a non-existant one - node_modules_root = "build_bazel_rules_nodejs/node_modules" - return node_modules_root + path = d[ExternalNpmPackageInfo].path + workspace = d[ExternalNpmPackageInfo].workspace + if path in node_modules_roots: + other_workspace = node_modules_roots[path] + if other_workspace != workspace: + fail("All npm dependencies at the path '%s' must come from a single workspace. Found '%s' and '%s'." % (path, other_workspace, workspace)) + node_modules_roots[path] = workspace + return node_modules_roots def _write_require_patch_script(ctx, node_modules_root): # Generates the JavaScript snippet of module roots mappings, with each entry @@ -167,9 +159,15 @@ def _nodejs_binary_impl(ctx): sources_depsets.append(d.files) sources = depset(transitive = sources_depsets) - node_modules_root = _compute_node_modules_root(ctx) + node_modules_roots = _compute_node_modules_roots(ctx) + if "" in node_modules_roots: + node_modules_root = node_modules_roots[""] + "/node_modules" + else: + # there are no fine grained deps but we still need a node_modules_root even if it is a non-existant one + node_modules_root = "build_bazel_rules_nodejs/node_modules" _write_require_patch_script(ctx, node_modules_root) + _write_loader_script(ctx) # Provide the target name as an environment variable avaiable to all actions for the @@ -184,12 +182,18 @@ def _nodejs_binary_impl(ctx): # runfiles helpers to use. env_vars += "export BAZEL_WORKSPACE=%s\n" % ctx.workspace_name - # if BAZEL_NODE_MODULES_ROOT has not already been set by + bazel_node_module_roots = "" + for path, root in node_modules_roots.items(): + if bazel_node_module_roots: + bazel_node_module_roots = bazel_node_module_roots + "," + bazel_node_module_roots = bazel_node_module_roots + "%s:%s" % (path, root) + + # if BAZEL_NODE_MODULES_ROOTS has not already been set by # run_node, then set it to the computed value - env_vars += """if [[ -z "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then - export BAZEL_NODE_MODULES_ROOT=%s + env_vars += """if [[ -z "${BAZEL_NODE_MODULES_ROOTS:-}" ]]; then + export BAZEL_NODE_MODULES_ROOTS=%s fi -""" % node_modules_root +""" % bazel_node_module_roots for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars: # Check ctx.var first & if env var not in there then check diff --git a/internal/node/node_patches.js b/internal/node/node_patches.js index 860076e31f..fce861f9cf 100644 --- a/internal/node/node_patches.js +++ b/internal/node/node_patches.js @@ -67,17 +67,16 @@ Object.defineProperty(exports, "__esModule", { value: true }); // es modules // tslint:disable-next-line:no-any -exports.patcher = (fs = fs$1, root, guards) => { +exports.patcher = (fs = fs$1, roots) => { fs = fs || fs$1; - root = root || ''; - guards = guards || []; - if (!root) { + roots = roots || []; + roots = roots.filter(root => fs.existsSync(root)); + if (!roots.length) { if (process.env.VERBOSE_LOGS) { - console.error('fs patcher called without root path ' + __filename); + console.error('fs patcher called without any valid root paths ' + __filename); } return; } - root = fs.realpathSync(root); const origRealpath = fs.realpath.bind(fs); const origRealpathNative = fs.realpath.native; const origLstat = fs.lstat.bind(fs); @@ -90,7 +89,7 @@ exports.patcher = (fs = fs$1, root, guards) => { const origReadlinkSync = fs.readlinkSync.bind(fs); const origReaddir = fs.readdir.bind(fs); const origReaddirSync = fs.readdirSync.bind(fs); - const { isEscape } = exports.escapeFunction(root, guards); + const { isEscape } = exports.escapeFunction(roots); // tslint:disable-next-line:no-any fs.lstat = (...args) => { let cb = args.length > 1 ? args[args.length - 1] : undefined; @@ -401,7 +400,7 @@ exports.patcher = (fs = fs$1, root, guards) => { return reject(err); } if (fs.DEBUG) - console.error(handleCounter + ' opendir: escapes? [target]', path.resolve(target), '[link] ' + linkName, isEscape(path.resolve(target), linkName), root); + console.error(handleCounter + ' opendir: escapes? [target]', path.resolve(target), '[link] ' + linkName, isEscape(path.resolve(target), linkName), roots); if (!isEscape(path.resolve(target), linkName)) { return resolve(v); } @@ -489,10 +488,13 @@ exports.patcher = (fs = fs$1, root, guards) => { } } }; -exports.escapeFunction = (root, guards) => { - // ensure root & guards are always absolute. - root = path.resolve(root); - guards = guards.map(g => path.resolve(g)); +function isOutPath(root, str) { + return !root || (!str.startsWith(root + path.sep) && str !== root); +} +exports.isOutPath = isOutPath; +exports.escapeFunction = (roots) => { + // ensure roots are always absolute + roots = roots.map(root => path.resolve(root)); function isEscape(linkTarget, linkPath) { if (!path.isAbsolute(linkPath)) { linkPath = path.resolve(linkPath); @@ -500,29 +502,15 @@ exports.escapeFunction = (root, guards) => { if (!path.isAbsolute(linkTarget)) { linkTarget = path.resolve(linkTarget); } - if (isGuardPath(linkPath) || isGuardPath(linkTarget)) { - // don't escape out of guard paths and don't symlink into guard paths - return true; - } - if (root) { - if (isOutPath(linkTarget) && !isOutPath(linkPath)) { + for (const root of roots) { + if (isOutPath(root, linkTarget) && !isOutPath(root, linkPath)) { // don't escape out of the root return true; } } return false; } - function isGuardPath(str) { - for (const g of guards) { - if (str === g || str.startsWith(g + path.sep)) - return true; - } - return false; - } - function isOutPath(str) { - return !root || (!str.startsWith(root + path.sep) && str !== root); - } - return { isEscape, isGuardPath, isOutPath }; + return { isEscape, isOutPath }; }; function once(fn) { let called = false; @@ -656,15 +644,15 @@ exports.subprocess = subprocess.patcher; * @fileoverview Description of this file. */ -const { BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_NODE_DIR, VERBOSE_LOGS } = process.env; -if (BAZEL_PATCH_ROOT) { - const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : []; +const { BAZEL_PATCH_ROOTS, NP_SUBPROCESS_NODE_DIR, VERBOSE_LOGS } = process.env; +if (BAZEL_PATCH_ROOTS) { + const roots = BAZEL_PATCH_ROOTS ? BAZEL_PATCH_ROOTS.split(',') : []; if (VERBOSE_LOGS) - console.error(`bazel node patches enabled. root: ${BAZEL_PATCH_ROOT} symlinks in this directory will not escape`); + console.error(`bazel node patches enabled. roots: ${roots} symlinks in these directories will not escape`); const fs = fs$1; - src.fs(fs, BAZEL_PATCH_ROOT, guards); + src.fs(fs, roots); } else if (VERBOSE_LOGS) { - console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`); + console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOTS`); } src.subprocess(__filename, NP_SUBPROCESS_NODE_DIR); diff --git a/internal/node/test/env.spec.js b/internal/node/test/env.spec.js index b23c94c984..0544c7ec87 100644 --- a/internal/node/test/env.spec.js +++ b/internal/node/test/env.spec.js @@ -39,14 +39,17 @@ describe('launcher.sh environment', function() { expectPathsToMatch(process.env['BAZEL_TARGET'], '//internal/node/test:env_test'); 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_PATCH_ROOT'], process.env['RUNFILES_DIR']); - expectPathsToMatch(process.env['BAZEL_NODE_MODULES_ROOT'], 'npm/node_modules'); - const expectedGuards = [ + expectPathsToMatch(process.env['BAZEL_NODE_MODULES_ROOTS'], ':npm'); + const expectedRoots = [ + `${execroot}`, `${execroot}/node_modules`, + `${runfilesRoot}`, + `${runfilesRoot}/build_bazel_rules_nodejs/node_modules`, + `${execroot}/external/npm/node_modules`, `${runfilesRoot}/npm/node_modules`, `${runfilesRoot}/build_bazel_rules_nodejs/external/npm/node_modules`, ] - expectPathsToMatch(process.env['BAZEL_PATCH_GUARDS'].split(','), expectedGuards); + expectPathsToMatch(process.env['BAZEL_PATCH_ROOTS'].split(','), expectedRoots); }); it('should setup correct bazel environment variables when in execroot with no third party deps', @@ -61,12 +64,15 @@ describe('launcher.sh environment', function() { expectPathsToMatch(env['BAZEL_WORKSPACE'], 'build_bazel_rules_nodejs'); expectPathsToMatch(env['BAZEL_TARGET'], '//internal/node/test:dump_build_env'); expectPathsToMatch(env['PWD'], execroot); - expectPathsToMatch(env['BAZEL_PATCH_ROOT'], path.dirname(execroot)); - expectPathsToMatch(env['BAZEL_NODE_MODULES_ROOT'], 'build_bazel_rules_nodejs/node_modules'); - const expectedGuards = [ + // On Windows, an empty string value for 'BAZEL_NODE_MODULES_ROOTS' does not make it into + // dump_build_env.json + expectPathsToMatch( + env['BAZEL_NODE_MODULES_ROOTS'] ? env['BAZEL_NODE_MODULES_ROOTS'] : '', ''); + const expectedRoots = [ + `${execroot}`, `${execroot}/node_modules`, ] - expectPathsToMatch(env['BAZEL_PATCH_GUARDS'].split(','), expectedGuards); + expectPathsToMatch(env['BAZEL_PATCH_ROOTS'].split(','), expectedRoots); }); it('should setup correct bazel environment variables when in execroot with third party deps', @@ -82,13 +88,12 @@ describe('launcher.sh environment', function() { expectPathsToMatch(env['BAZEL_WORKSPACE'], 'build_bazel_rules_nodejs'); expectPathsToMatch(env['BAZEL_TARGET'], '//internal/node/test:dump_build_env_alt'); expectPathsToMatch(env['PWD'], execroot); - expectPathsToMatch(env['BAZEL_PATCH_ROOT'], path.dirname(execroot)); - expectPathsToMatch(env['BAZEL_NODE_MODULES_ROOT'], 'npm/node_modules'); - const expectedGuards = [ + expectPathsToMatch(env['BAZEL_NODE_MODULES_ROOTS'], ':npm'); + const expectedRoots = [ + `${execroot}`, `${execroot}/node_modules`, - `${path.dirname(execroot)}/npm/node_modules`, `${execroot}/external/npm/node_modules`, ] - expectPathsToMatch(env['BAZEL_PATCH_GUARDS'].split(','), expectedGuards); + expectPathsToMatch(env['BAZEL_PATCH_ROOTS'].split(','), expectedRoots); }); }); diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 8bde06bacd..0d24ce9c9e 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -58,6 +58,7 @@ const WORKSPACE_ROOT_BASE = WORKSPACE_ROOT_PREFIX ?.split('/')[0]; const STRICT_VISIBILITY = args[5]?.toLowerCase() === 'true'; const INCLUDED_FILES = args[6] ? args[6].split(',') : []; const BAZEL_VERSION = args[7]; +const PACKAGE_PATH = args[8]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; @@ -188,7 +189,8 @@ ${exportsStarlark}]) # See https://github.com/bazelbuild/bazel/issues/5153. js_library( name = "node_modules", - external_npm_package = True,${pkgFilesStarlark}${depsStarlark} + external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}",${pkgFilesStarlark}${depsStarlark} ) ` @@ -1002,6 +1004,7 @@ filegroup( js_library( name = "${pkg._name}", external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}", # direct sources listed for strict deps support srcs = [":${pkg._name}__files"], # nested node_modules for this package plus flattened list of direct and transitive dependencies @@ -1015,6 +1018,7 @@ js_library( js_library( name = "${pkg._name}__contents", external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}", srcs = [":${pkg._name}__files", ":${pkg._name}__nested_node_modules"],${namedSourcesStarlark} visibility = ["//:__subpackages__"], ) @@ -1022,7 +1026,8 @@ js_library( # Typings files that are part of the npm package not including nested node_modules js_library( name = "${pkg._name}__typings", - external_npm_package = True,${dtsStarlark} + external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}",${dtsStarlark} ) `; @@ -1218,7 +1223,8 @@ function printScope(scope: string, pkgs: Dep[]) { # Generated target for npm scope ${scope} js_library( name = "${scope}", - external_npm_package = True,${pkgFilesStarlark}${depsStarlark} + external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}",${pkgFilesStarlark}${depsStarlark} ) `; diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js index b675ed8ce6..e15e927b16 100644 --- a/internal/npm_install/index.js +++ b/internal/npm_install/index.js @@ -18,6 +18,7 @@ const WORKSPACE_ROOT_BASE = (_a = WORKSPACE_ROOT_PREFIX) === null || _a === void const STRICT_VISIBILITY = ((_b = args[5]) === null || _b === void 0 ? void 0 : _b.toLowerCase()) === 'true'; const INCLUDED_FILES = args[6] ? args[6].split(',') : []; const BAZEL_VERSION = args[7]; +const PACKAGE_PATH = args[8]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY) { @@ -102,7 +103,8 @@ ${exportsStarlark}]) # See https://github.com/bazelbuild/bazel/issues/5153. js_library( name = "node_modules", - external_npm_package = True,${pkgFilesStarlark}${depsStarlark} + external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}",${pkgFilesStarlark}${depsStarlark} ) `; @@ -571,6 +573,7 @@ filegroup( js_library( name = "${pkg._name}", external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}", # direct sources listed for strict deps support srcs = [":${pkg._name}__files"], # nested node_modules for this package plus flattened list of direct and transitive dependencies @@ -584,6 +587,7 @@ js_library( js_library( name = "${pkg._name}__contents", external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}", srcs = [":${pkg._name}__files", ":${pkg._name}__nested_node_modules"],${namedSourcesStarlark} visibility = ["//:__subpackages__"], ) @@ -591,7 +595,8 @@ js_library( # Typings files that are part of the npm package not including nested node_modules js_library( name = "${pkg._name}__typings", - external_npm_package = True,${dtsStarlark} + external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}",${dtsStarlark} ) `; @@ -741,7 +746,8 @@ function printScope(scope, pkgs) { # Generated target for npm scope ${scope} js_library( name = "${scope}", - external_npm_package = True,${pkgFilesStarlark}${depsStarlark} + external_npm_package = True, + external_npm_package_path = "${PACKAGE_PATH}",${pkgFilesStarlark}${depsStarlark} ) `; diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 9ee3cda84b..5964239a18 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -79,6 +79,14 @@ fine grained npm dependencies. mandatory = True, allow_single_file = True, ), + "package_path": attr.string( + default = "", + doc = """If set, link the 3rd party node_modules dependencies under the package path specified. + +In most cases, this should be the directory of the package.json file so that the linker links the node_modules +in the same location they are found in the source tree. In a future release, this will default to the package.json +directory. This is planned for 4.0: https://github.com/bazelbuild/rules_nodejs/issues/2451""", + ), "quiet": attr.bool( default = True, doc = "If stdout and stderr should be printed to the terminal.", @@ -130,6 +138,7 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file): str(repository_ctx.attr.strict_visibility), ",".join(repository_ctx.attr.included_files), native.bazel_version, + repository_ctx.attr.package_path, # double the default timeout in case of many packages, see #2231 ], timeout = 1200, quiet = repository_ctx.attr.quiet) if result.return_code: diff --git a/internal/npm_install/test/golden/@angular/core/BUILD.bazel.golden b/internal/npm_install/test/golden/@angular/core/BUILD.bazel.golden index dc2ef85164..6ed6edd6aa 100644 --- a/internal/npm_install/test/golden/@angular/core/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/@angular/core/BUILD.bazel.golden @@ -681,6 +681,7 @@ filegroup( js_library( name = "core", external_npm_package = True, + external_npm_package_path = "", srcs = [":core__files"], deps = [ "//@angular/core:core__contents", @@ -692,6 +693,7 @@ js_library( js_library( name = "core__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":core__files", ":core__nested_node_modules"], named_module_srcs = [ "//:node_modules/@angular/core/bundles/core-testing.umd.js", @@ -702,6 +704,7 @@ js_library( js_library( name = "core__typings", external_npm_package = True, + external_npm_package_path = "", srcs = [ "//:node_modules/@angular/core/core.d.ts", "//:node_modules/@angular/core/schematics/migrations/missing-injectable/import_manager.d.ts", diff --git a/internal/npm_install/test/golden/@gregmagolan/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/BUILD.bazel.golden index a2ecf4f2dd..5142f87d98 100644 --- a/internal/npm_install/test/golden/@gregmagolan/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/@gregmagolan/BUILD.bazel.golden @@ -4,6 +4,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "js_library") js_library( name = "@gregmagolan", external_npm_package = True, + external_npm_package_path = "", srcs = [ "//@gregmagolan/test-a:test-a__files", "//@gregmagolan/test-b:test-b__files", diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden index ed8f1934ee..3a533e8010 100644 --- a/internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden @@ -28,6 +28,7 @@ filegroup( js_library( name = "test-a", external_npm_package = True, + external_npm_package_path = "", srcs = [":test-a__files"], deps = [ "//@gregmagolan/test-a:test-a__contents", @@ -36,12 +37,14 @@ js_library( js_library( name = "test-a__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":test-a__files", ":test-a__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "test-a__typings", external_npm_package = True, + external_npm_package_path = "", ) load("@build_bazel_rules_nodejs//internal/npm_install:npm_umd_bundle.bzl", "npm_umd_bundle") npm_umd_bundle( diff --git a/internal/npm_install/test/golden/@gregmagolan/test-b/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-b/BUILD.bazel.golden index 02ba0a853a..678f90ee00 100644 --- a/internal/npm_install/test/golden/@gregmagolan/test-b/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/@gregmagolan/test-b/BUILD.bazel.golden @@ -27,6 +27,7 @@ filegroup( js_library( name = "test-b", external_npm_package = True, + external_npm_package_path = "", srcs = [":test-b__files"], deps = [ "//@gregmagolan/test-b:test-b__contents", @@ -35,12 +36,14 @@ js_library( js_library( name = "test-b__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":test-b__files", ":test-b__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "test-b__typings", external_npm_package = True, + external_npm_package_path = "", ) load("@build_bazel_rules_nodejs//internal/npm_install:npm_umd_bundle.bzl", "npm_umd_bundle") npm_umd_bundle( diff --git a/internal/npm_install/test/golden/BUILD.bazel.golden b/internal/npm_install/test/golden/BUILD.bazel.golden index 33bde3f2ce..ee8c2e3b12 100644 --- a/internal/npm_install/test/golden/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/BUILD.bazel.golden @@ -4727,6 +4727,7 @@ exports_files([ js_library( name = "node_modules", external_npm_package = True, + external_npm_package_path = "", srcs = [ "//ajv:ajv__files", "//ajv:ajv__nested_node_modules", diff --git a/internal/npm_install/test/golden/ajv/BUILD.bazel.golden b/internal/npm_install/test/golden/ajv/BUILD.bazel.golden index cf40a12d36..9214c70147 100644 --- a/internal/npm_install/test/golden/ajv/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/ajv/BUILD.bazel.golden @@ -111,6 +111,7 @@ filegroup( js_library( name = "ajv", external_npm_package = True, + external_npm_package_path = "", srcs = [":ajv__files"], deps = [ "//ajv:ajv__contents", @@ -123,12 +124,14 @@ js_library( js_library( name = "ajv__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":ajv__files", ":ajv__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "ajv__typings", external_npm_package = True, + external_npm_package_path = "", srcs = [ "//:node_modules/ajv/lib/ajv.d.ts", ], diff --git a/internal/npm_install/test/golden/jasmine/BUILD.bazel.golden b/internal/npm_install/test/golden/jasmine/BUILD.bazel.golden index fa35dbbe66..2d8a97a0b2 100644 --- a/internal/npm_install/test/golden/jasmine/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/jasmine/BUILD.bazel.golden @@ -58,6 +58,7 @@ filegroup( js_library( name = "jasmine", external_npm_package = True, + external_npm_package_path = "", srcs = [":jasmine__files"], deps = [ "//jasmine:jasmine__contents", @@ -77,12 +78,14 @@ js_library( js_library( name = "jasmine__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":jasmine__files", ":jasmine__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "jasmine__typings", external_npm_package = True, + external_npm_package_path = "", ) load("@build_bazel_rules_nodejs//internal/npm_install:npm_umd_bundle.bzl", "npm_umd_bundle") npm_umd_bundle( diff --git a/internal/npm_install/test/golden/rxjs/BUILD.bazel.golden b/internal/npm_install/test/golden/rxjs/BUILD.bazel.golden index cb3dd8763c..a5989610c5 100644 --- a/internal/npm_install/test/golden/rxjs/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/rxjs/BUILD.bazel.golden @@ -3630,6 +3630,7 @@ filegroup( js_library( name = "rxjs", external_npm_package = True, + external_npm_package_path = "", srcs = [":rxjs__files"], deps = [ "//rxjs:rxjs__contents", @@ -3639,12 +3640,14 @@ js_library( js_library( name = "rxjs__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":rxjs__files", ":rxjs__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "rxjs__typings", external_npm_package = True, + external_npm_package_path = "", srcs = [ "//:node_modules/rxjs/AsyncSubject.d.ts", "//:node_modules/rxjs/BehaviorSubject.d.ts", diff --git a/internal/npm_install/test/golden/unidiff/BUILD.bazel.golden b/internal/npm_install/test/golden/unidiff/BUILD.bazel.golden index 29c1b84418..968b5f31b8 100644 --- a/internal/npm_install/test/golden/unidiff/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/unidiff/BUILD.bazel.golden @@ -33,6 +33,7 @@ filegroup( js_library( name = "unidiff", external_npm_package = True, + external_npm_package_path = "", srcs = [":unidiff__files"], deps = [ "//unidiff:unidiff__contents", @@ -42,12 +43,14 @@ js_library( js_library( name = "unidiff__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":unidiff__files", ":unidiff__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "unidiff__typings", external_npm_package = True, + external_npm_package_path = "", ) load("@build_bazel_rules_nodejs//internal/npm_install:npm_umd_bundle.bzl", "npm_umd_bundle") npm_umd_bundle( diff --git a/internal/npm_install/test/golden/zone.js/BUILD.bazel.golden b/internal/npm_install/test/golden/zone.js/BUILD.bazel.golden index 5724bd739f..5686fad042 100644 --- a/internal/npm_install/test/golden/zone.js/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/zone.js/BUILD.bazel.golden @@ -151,6 +151,7 @@ filegroup( js_library( name = "zone.js", external_npm_package = True, + external_npm_package_path = "", srcs = [":zone.js__files"], deps = [ "//zone.js:zone.js__contents", @@ -159,12 +160,14 @@ js_library( js_library( name = "zone.js__contents", external_npm_package = True, + external_npm_package_path = "", srcs = [":zone.js__files", ":zone.js__nested_node_modules"], visibility = ["//:__subpackages__"], ) js_library( name = "zone.js__typings", external_npm_package = True, + external_npm_package_path = "", srcs = [ "//:node_modules/zone.js/dist/zone.js.d.ts", ], diff --git a/internal/providers/external_npm_package_info.bzl b/internal/providers/external_npm_package_info.bzl index fbfeb14e30..730aac0e01 100644 --- a/internal/providers/external_npm_package_info.bzl +++ b/internal/providers/external_npm_package_info.bzl @@ -22,28 +22,41 @@ ExternalNpmPackageInfo = provider( doc = "Provides information about one or more external npm packages", fields = { "direct_sources": "Depset of direct source files in these external npm package(s)", + "path": "The local workspace path that these external npm deps should be linked at. If empty, they will be linked at the root.", "sources": "Depset of direct & transitive source files in these external npm package(s) and transitive dependencies", "workspace": "The workspace name that these external npm package(s) are provided from", }, ) def _node_modules_aspect_impl(target, ctx): - providers = [] + if ExternalNpmPackageInfo in target: + return [] # provide ExternalNpmPackageInfo if it is not already provided there are ExternalNpmPackageInfo deps - if not ExternalNpmPackageInfo in target: - sources_depsets = [] - workspace = None - if hasattr(ctx.rule.attr, "deps"): - for dep in ctx.rule.attr.deps: - if ExternalNpmPackageInfo in dep: - if workspace and dep[ExternalNpmPackageInfo].workspace != workspace: - fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (workspace, dep[ExternalNpmPackageInfo].workspace)) - workspace = dep[ExternalNpmPackageInfo].workspace - sources_depsets.append(dep[ExternalNpmPackageInfo].sources) - if workspace: - providers.extend([ExternalNpmPackageInfo(direct_sources = depset(), sources = depset(transitive = sources_depsets), workspace = workspace)]) + providers = [] + # map of 'path' to [workspace, sources_depsets] + paths = {} + if hasattr(ctx.rule.attr, "deps"): + for dep in ctx.rule.attr.deps: + if ExternalNpmPackageInfo in dep: + path = dep[ExternalNpmPackageInfo].path + workspace = dep[ExternalNpmPackageInfo].workspace + sources_depsets = [] + if path in paths: + path_entry = paths[path] + if path_entry[0] != workspace: + fail("All npm dependencies at the path '%s' must come from a single workspace. Found '%s' and '%s'." % (path, workspace, path_entry[0])) + sources_depsets = path_entry[1] + sources_depsets.append(dep[ExternalNpmPackageInfo].sources) + paths[path] = [workspace, sources_depsets] + for path, path_entry in paths.items(): + providers.extend([ExternalNpmPackageInfo( + direct_sources = depset(), + sources = depset(transitive = path_entry[1]), + workspace = path_entry[0], + path = path, + )]) return providers node_modules_aspect = aspect( diff --git a/internal/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl index b9ca4ab168..fbb9ea7c40 100644 --- a/internal/providers/node_runtime_deps_info.bzl +++ b/internal/providers/node_runtime_deps_info.bzl @@ -39,9 +39,9 @@ do the same. }, ) -def _compute_node_modules_root(ctx): +def _compute_node_modules_roots(ctx): """Computes the node_modules root (if any) from data & deps targets.""" - node_modules_root = "" + node_modules_roots = {} deps = [] if hasattr(ctx.attr, "data"): deps += ctx.attr.data @@ -49,12 +49,14 @@ def _compute_node_modules_root(ctx): deps += ctx.attr.deps for d in deps: if ExternalNpmPackageInfo in d: - possible_root = "/".join([d[ExternalNpmPackageInfo].workspace, "node_modules"]) - if not node_modules_root: - node_modules_root = possible_root - elif node_modules_root != possible_root: - fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (node_modules_root, possible_root)) - return node_modules_root + path = d[ExternalNpmPackageInfo].path + workspace = d[ExternalNpmPackageInfo].workspace + if path in node_modules_roots: + other_workspace = node_modules_roots[path] + if other_workspace != workspace: + fail("All npm dependencies at the path '%s' must come from a single workspace. Found '%s' and '%s'." % (path, other_workspace, workspace)) + node_modules_roots[path] = workspace + return node_modules_roots def run_node(ctx, inputs, arguments, executable, chdir = None, **kwargs): """Helper to replace ctx.actions.run @@ -127,7 +129,13 @@ def run_node(ctx, inputs, arguments, executable, chdir = None, **kwargs): env[var] = ctx.var[var] elif var in ctx.configuration.default_shell_env.keys(): env[var] = ctx.configuration.default_shell_env[var] - env["BAZEL_NODE_MODULES_ROOT"] = _compute_node_modules_root(ctx) + bazel_node_module_roots = "" + node_modules_roots = _compute_node_modules_roots(ctx) + for path, root in node_modules_roots.items(): + if bazel_node_module_roots: + bazel_node_module_roots = bazel_node_module_roots + "," + bazel_node_module_roots = bazel_node_module_roots + "%s:%s" % (path, root) + env["BAZEL_NODE_MODULES_ROOTS"] = bazel_node_module_roots # ctx.actions.run accepts both lists and a depset for inputs. Coerce the original inputs to a # depset if they're a list, so that extra inputs can be combined in a performant manner. diff --git a/packages/node-patches/README.md b/packages/node-patches/README.md index 2e3eb55028..40a9ac8c22 100644 --- a/packages/node-patches/README.md +++ b/packages/node-patches/README.md @@ -32,10 +32,10 @@ this should not change the behavior of any paths that are outside of the root. ### loader you can use the register script to include it in a -r flag to preload the patch before user code. -This depends on setting the environment variable BAZEL_PATCH_ROOT +This depends on setting the environment variable BAZEL_PATCH_ROOTS ```sh -BAZEL_PATCH_ROOT=~/.cache/bazel node -r @bazel/node-patches/register +BAZEL_PATCH_ROOTS=~/.cache/bazel node -r @bazel/node-patches/register ``` ### api diff --git a/packages/node-patches/register.ts b/packages/node-patches/register.ts index 534b9c4d65..bc6eebc974 100644 --- a/packages/node-patches/register.ts +++ b/packages/node-patches/register.ts @@ -18,17 +18,17 @@ * @fileoverview Description of this file. */ const patcher = require('./src'); -const {BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_NODE_DIR, VERBOSE_LOGS} = process.env; +const {BAZEL_PATCH_ROOTS, NP_SUBPROCESS_NODE_DIR, VERBOSE_LOGS} = process.env; -if (BAZEL_PATCH_ROOT) { - const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : []; +if (BAZEL_PATCH_ROOTS) { + const roots = BAZEL_PATCH_ROOTS ? BAZEL_PATCH_ROOTS.split(',') : []; if (VERBOSE_LOGS) - console.error(`bazel node patches enabled. root: ${ - BAZEL_PATCH_ROOT} symlinks in this directory will not escape`); + console.error(`bazel node patches enabled. roots: ${ + roots} symlinks in these directories will not escape`); const fs = require('fs'); - patcher.fs(fs, BAZEL_PATCH_ROOT, guards); + patcher.fs(fs, roots); } else if (VERBOSE_LOGS) { - console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`); + console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOTS`); } patcher.subprocess(__filename, NP_SUBPROCESS_NODE_DIR); diff --git a/packages/node-patches/src/fs.ts b/packages/node-patches/src/fs.ts index 942cf6de83..9d6d0fbcb4 100644 --- a/packages/node-patches/src/fs.ts +++ b/packages/node-patches/src/fs.ts @@ -29,19 +29,17 @@ type Dirent = any; const _fs = require('fs'); // tslint:disable-next-line:no-any -export const patcher = (fs: any = _fs, root: string, guards: string[]) => { +export const patcher = (fs: any = _fs, roots: string[]) => { fs = fs || _fs; - root = root || ''; - guards = guards || []; - if (!root) { + roots = roots || []; + roots = roots.filter(root => fs.existsSync(root)); + if (!roots.length) { if (process.env.VERBOSE_LOGS) { - console.error('fs patcher called without root path ' + __filename); + console.error('fs patcher called without any valid root paths ' + __filename); } return; } - root = fs.realpathSync(root); - const origRealpath = fs.realpath.bind(fs); const origRealpathNative = fs.realpath.native; const origLstat = fs.lstat.bind(fs); @@ -55,7 +53,7 @@ export const patcher = (fs: any = _fs, root: string, guards: string[]) => { const origReaddir = fs.readdir.bind(fs); const origReaddirSync = fs.readdirSync.bind(fs); - const {isEscape} = escapeFunction(root, guards); + const {isEscape} = escapeFunction(roots); const logged: {[k: string]: boolean} = {}; @@ -375,7 +373,7 @@ export const patcher = (fs: any = _fs, root: string, guards: string[]) => { if (fs.DEBUG) console.error( handleCounter + ' opendir: escapes? [target]', path.resolve(target), - '[link] ' + linkName, isEscape(path.resolve(target), linkName), root); + '[link] ' + linkName, isEscape(path.resolve(target), linkName), roots); if (!isEscape(path.resolve(target), linkName)) { return resolve(v); @@ -472,10 +470,13 @@ export const patcher = (fs: any = _fs, root: string, guards: string[]) => { } }; -export const escapeFunction = (root: string, guards: string[]) => { - // ensure root & guards are always absolute. - root = path.resolve(root); - guards = guards.map(g => path.resolve(g)); +export function isOutPath(root: string, str: string) { + return !root || (!str.startsWith(root + path.sep) && str !== root); +} + +export const escapeFunction = (roots: string[]) => { + // ensure roots are always absolute + roots = roots.map(root => path.resolve(root)); function isEscape(linkTarget: string, linkPath: string) { if (!path.isAbsolute(linkPath)) { linkPath = path.resolve(linkPath); @@ -485,32 +486,17 @@ export const escapeFunction = (root: string, guards: string[]) => { linkTarget = path.resolve(linkTarget); } - if (isGuardPath(linkPath) || isGuardPath(linkTarget)) { - // don't escape out of guard paths and don't symlink into guard paths - return true; - } - - if (root) { - if (isOutPath(linkTarget) && !isOutPath(linkPath)) { + for (const root of roots) { + if (isOutPath(root, linkTarget) && !isOutPath(root, linkPath)) { // don't escape out of the root return true; } } - return false; - } - function isGuardPath(str) { - for (const g of guards) { - if (str === g || str.startsWith(g + path.sep)) return true; - } return false; } - function isOutPath(str: string) { - return !root || (!str.startsWith(root + path.sep) && str !== root); - } - - return {isEscape, isGuardPath, isOutPath}; + return {isEscape, isOutPath}; }; function once(fn: (...args: unknown[]) => T) { diff --git a/packages/node-patches/test/fs/escape.ts b/packages/node-patches/test/fs/escape.ts index 86d155308a..96df7d54d2 100644 --- a/packages/node-patches/test/fs/escape.ts +++ b/packages/node-patches/test/fs/escape.ts @@ -17,43 +17,24 @@ import * as assert from 'assert'; import * as path from 'path'; -import {escapeFunction} from '../../src/fs'; +import {escapeFunction, isOutPath} from '../../src/fs'; describe('escape function', () => { - it('isGuardPath & isOutPath is correct', () => { - const root = '/a/b'; - const guards = [ - '/a/b/g/1', - '/a/b/g/a/2', - '/a/b/g/a/3', - ]; - const {isGuardPath, isOutPath} = escapeFunction(root, guards); - - assert.ok(isGuardPath('/a/b/g/1')); - assert.ok(isGuardPath('/a/b/g/1/foo')); - assert.ok(!isGuardPath('/a/b/g/h')); - assert.ok(!isGuardPath('/a/b/g/h/i')); - assert.ok(isGuardPath('/a/b/g/a/2')); - assert.ok(isGuardPath('/a/b/g/a/2/foo')); - assert.ok(isGuardPath('/a/b/g/a/3')); - assert.ok(isGuardPath('/a/b/g/a/3/foo')); - assert.ok(!isGuardPath('/a/b/g/a/h')); - assert.ok(!isGuardPath('/a/b/g/a/h/i')); - - assert.ok(isOutPath('/a')); - assert.ok(isOutPath('/a/c/b')); - assert.ok(!isOutPath('/a/b')); - assert.ok(!isOutPath('/a/b/c/d')); + it('isOutPath is correct', () => { + assert.ok(isOutPath('/a/b', '/a')); + assert.ok(isOutPath('/a/b', '/a/c/b')); + assert.ok(!isOutPath('/a/b', '/a/b')); + assert.ok(!isOutPath('/a/b', '/a/b/c/d')); }); it('isEscape is correct', () => { - const root = '/a/b'; - const guards = [ + const roots = [ + '/a/b', '/a/b/g/1', '/a/b/g/a/2', '/a/b/g/a/3', ]; - const {isEscape} = escapeFunction(root, guards); + const {isEscape} = escapeFunction(roots); assert.ok(isEscape('/a/c/boop', '/a/b/l')); assert.ok(isEscape('/a/c/boop', '/a/b')); @@ -93,27 +74,16 @@ describe('escape function', () => { assert.ok(isEscape('/a/b/c', '/a/b/g/a/3/foo')); assert.ok(!isEscape('/a/b/c', '/a/b/g/a/h')); assert.ok(!isEscape('/a/b/c', '/a/b/g/a/h/i')); - - assert.ok(isEscape('/a/b/g/1', '/some/path')); - assert.ok(isEscape('/a/b/g/1/foo', '/some/path')); - assert.ok(!isEscape('/a/b/g/h', '/some/path')); - assert.ok(!isEscape('/a/b/g/h/i', '/some/path')); - assert.ok(isEscape('/a/b/g/a/2', '/some/path')); - assert.ok(isEscape('/a/b/g/a/2/foo', '/some/path')); - assert.ok(isEscape('/a/b/g/a/3', '/some/path')); - assert.ok(isEscape('/a/b/g/a/3/foo', '/some/path')); - assert.ok(!isEscape('/a/b/g/a/h', '/some/path')); - assert.ok(!isEscape('/a/b/g/a/h/i', '/some/path')); }); it('isEscape handles relative paths', () => { - const root = './a/b'; - const guards = [ + const roots = [ + './a/b', './a/b/g/1', './a/b/g/a/2', './a/b/g/a/3', ]; - const {isEscape} = escapeFunction(root, guards); + const {isEscape} = escapeFunction(roots); assert.ok(isEscape('./a/c/boop', './a/b/l')); assert.ok(isEscape('./a/c/boop', './a/b')); @@ -153,16 +123,5 @@ describe('escape function', () => { assert.ok(isEscape('./a/b/c', './a/b/g/a/3/foo')); assert.ok(!isEscape('./a/b/c', './a/b/g/a/h')); assert.ok(!isEscape('./a/b/c', './a/b/g/a/h/i')); - - assert.ok(isEscape('./a/b/g/1', './some/path')); - assert.ok(isEscape('./a/b/g/1/foo', './some/path')); - assert.ok(!isEscape('./a/b/g/h', './some/path')); - assert.ok(!isEscape('./a/b/g/h/i', './some/path')); - assert.ok(isEscape('./a/b/g/a/2', './some/path')); - assert.ok(isEscape('./a/b/g/a/2/foo', './some/path')); - assert.ok(isEscape('./a/b/g/a/3', './some/path')); - assert.ok(isEscape('./a/b/g/a/3/foo', './some/path')); - assert.ok(!isEscape('./a/b/g/a/h', './some/path')); - assert.ok(!isEscape('./a/b/g/a/h/i', './some/path')); }); }); diff --git a/packages/node-patches/test/fs/lstat.ts b/packages/node-patches/test/fs/lstat.ts index 5cecd219a2..61808fb1d9 100644 --- a/packages/node-patches/test/fs/lstat.ts +++ b/packages/node-patches/test/fs/lstat.ts @@ -38,7 +38,7 @@ describe('testing lstat', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir), []); + patcher(patchedFs, [path.join(fixturesDir)]); const linkPath = path.join(fixturesDir, 'a', 'link'); assert.ok( @@ -69,7 +69,7 @@ describe('testing lstat', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir), [path.join(fixturesDir, 'a', 'g')]); + patcher(patchedFs, [path.join(fixturesDir), path.join(fixturesDir, 'a', 'g')]); const linkPath = path.join(fixturesDir, 'a', 'g', 'link'); assert.ok( @@ -99,7 +99,7 @@ describe('testing lstat', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); const linkPath = path.join(fixturesDir, 'a', 'link'); @@ -145,7 +145,7 @@ describe('testing lstat', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); const linkPath = path.join(fixturesDir, 'b', 'link'); diff --git a/packages/node-patches/test/fs/opendir.ts b/packages/node-patches/test/fs/opendir.ts index 96e4e1707b..cf898366f5 100644 --- a/packages/node-patches/test/fs/opendir.ts +++ b/packages/node-patches/test/fs/opendir.ts @@ -39,7 +39,7 @@ describe('testing opendir', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, fixturesDir, []); + patcher(patchedFs, [fixturesDir]); (patchedFs as any).DEBUG = true; let dir; @@ -74,7 +74,7 @@ describe('testing opendir', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); (patchedFs as any).DEBUG = true; let dir; @@ -109,7 +109,7 @@ describe('testing opendir', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir), []); + patcher(patchedFs, [path.join(fixturesDir)]); (patchedFs as any).DEBUG = true; const dir = await util.promisify(patchedFs.opendir)(path.join(fixturesDir, 'a')); @@ -140,7 +140,7 @@ describe('testing opendir', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); (patchedFs as any).DEBUG = true; const dir = await util.promisify(patchedFs.opendir)(path.join(fixturesDir, 'a')); diff --git a/packages/node-patches/test/fs/readdir.ts b/packages/node-patches/test/fs/readdir.ts index 14a95bef36..2a52bd1048 100644 --- a/packages/node-patches/test/fs/readdir.ts +++ b/packages/node-patches/test/fs/readdir.ts @@ -38,7 +38,7 @@ describe('testing readdir', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, fixturesDir, []); + patcher(patchedFs, [fixturesDir]); let dirents = patchedFs.readdirSync(path.join(fixturesDir, 'a'), { withFileTypes: true, @@ -77,7 +77,7 @@ describe('testing readdir', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); let dirents = patchedFs.readdirSync(path.join(fixturesDir, 'a'), { withFileTypes: true, diff --git a/packages/node-patches/test/fs/readlink.ts b/packages/node-patches/test/fs/readlink.ts index b26ce50ce8..614b8234a9 100644 --- a/packages/node-patches/test/fs/readlink.ts +++ b/packages/node-patches/test/fs/readlink.ts @@ -38,7 +38,7 @@ describe('testing readlink', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir), []); + patcher(patchedFs, [path.join(fixturesDir)]); const linkPath = path.join(fixturesDir, 'a', 'link'); assert.deepStrictEqual( @@ -70,7 +70,7 @@ describe('testing readlink', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); const linkPath = path.join(fs.realpathSync(fixturesDir), 'a', 'link'); assert.throws(() => { diff --git a/packages/node-patches/test/fs/realpath.ts b/packages/node-patches/test/fs/realpath.ts index d8496c96ed..53c73afbc1 100644 --- a/packages/node-patches/test/fs/realpath.ts +++ b/packages/node-patches/test/fs/realpath.ts @@ -40,7 +40,7 @@ describe('testing realpath', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir), []); + patcher(patchedFs, [path.join(fixturesDir)]); const linkPath = path.join(fs.realpathSync(fixturesDir), 'a', 'link'); assert.deepStrictEqual( @@ -83,7 +83,7 @@ describe('testing realpath', () => { const patchedFs = Object.assign({}, fs); patchedFs.promises = Object.assign({}, fs.promises); - patcher(patchedFs, path.join(fixturesDir, 'a'), []); + patcher(patchedFs, [path.join(fixturesDir, 'a')]); const linkPath = path.join(fs.realpathSync(fixturesDir), 'a', 'link'); assert.deepStrictEqual( diff --git a/packages/typescript/internal/build_defs.bzl b/packages/typescript/internal/build_defs.bzl index 3c13b5f195..7ccc50ee46 100644 --- a/packages/typescript/internal/build_defs.bzl +++ b/packages/typescript/internal/build_defs.bzl @@ -51,8 +51,6 @@ def _trim_package_node_modules(package_name): segments += [n] return "/".join(segments) -# This function is similar but slightly different than _compute_node_modules_root -# in /internal/node/node.bzl. TODO(gregmagolan): consolidate these functions def _compute_node_modules_root(ctx): """Computes the node_modules root from the node_modules and deps attributes.