From 3c12dfe779d604e4efa5c6878055c9436cd9e009 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Sun, 31 Jan 2021 23:41:22 -0800 Subject: [PATCH] fix: relative data paths in yarn_install & npm_install when symlink_node_modules=False and package.json is not at root This fixes `yarn_install` & `npm_install` to copy `package.json` & lock files into the external repository to a folder that corresponds to the package.json's workspace folder and the package manager is then run out that location so that relative paths to `data` files are preserved (same as they would be outside of bazel) if the `package.json` is _not_ at the root of the WORKSPACE. If you had a nested `package.json` and were using `yarn_install` or `npm_install` with `symlink_node_modules = False,` and passing in `data` that you were referencing during install, then you would have had to use the work-around of absolute workspace paths to the `data` files. This fix will allow you to use the same relative paths that you would use if you were running `yarn` or `npm` outside of bazel in this case. For example, if the package.json file is located at `my/nested/package.json` then it will end up at `_/my/nested/package.json` in the external repository. When `symlink_node_modules` is `False`, `data` files are copied into the same tree, so that relative paths in `package.json` that refer to `data` files are preserved. A `"postinstall": "patch-package --patch-dir patches"` in a nested `package.json` file expects a `patches` directory relative to the `package.json` file will now work with `symlink_node_modules = False`. ``` yarn_install( name = "my_nested_npm_deps", package_json = "//my/nested:package.json", yarn_lock = "//my/nested:yarn.lock", data = ["//my/nested:patches/jest-haste-map+24.9.0.patch"], symlink_node_modules = False, ) ``` Additional fix is that `data` files are now copied to external repository with `mkdir -p && cp -f` instead of `rtcx.template({})` trick. The latter is slower & does not copy over binary files correctly. We must copy `data` files and _not_ symlink them since a `package.json` file with `file:path/to/data` will fail if `path/to/data` is a symlink. --- e2e/packages/npm1/package.json | 2 +- e2e/packages/npm2/package.json | 2 +- e2e/packages/yarn1/package.json | 2 +- e2e/packages/yarn2/package.json | 2 +- internal/npm_install/generate_build_file.ts | 20 ++-- internal/npm_install/index.js | 21 ++-- internal/npm_install/npm_install.bzl | 108 +++++++++++------- tools/fine_grained_deps_npm/package-lock.json | 2 +- tools/fine_grained_deps_npm/package.json | 4 +- tools/fine_grained_deps_yarn/package.json | 4 +- tools/fine_grained_deps_yarn/yarn.lock | 2 +- 11 files changed, 101 insertions(+), 68 deletions(-) diff --git a/e2e/packages/npm1/package.json b/e2e/packages/npm1/package.json index 396c0aeb47..c0054103f6 100644 --- a/e2e/packages/npm1/package.json +++ b/e2e/packages/npm1/package.json @@ -8,6 +8,6 @@ "tmp": "0.1.0" }, "scripts": { - "postinstall": "node ./postinstall.js" + "postinstall": "node ../postinstall.js" } } diff --git a/e2e/packages/npm2/package.json b/e2e/packages/npm2/package.json index 396c0aeb47..c0054103f6 100644 --- a/e2e/packages/npm2/package.json +++ b/e2e/packages/npm2/package.json @@ -8,6 +8,6 @@ "tmp": "0.1.0" }, "scripts": { - "postinstall": "node ./postinstall.js" + "postinstall": "node ../postinstall.js" } } diff --git a/e2e/packages/yarn1/package.json b/e2e/packages/yarn1/package.json index 396c0aeb47..c0054103f6 100644 --- a/e2e/packages/yarn1/package.json +++ b/e2e/packages/yarn1/package.json @@ -8,6 +8,6 @@ "tmp": "0.1.0" }, "scripts": { - "postinstall": "node ./postinstall.js" + "postinstall": "node ../postinstall.js" } } diff --git a/e2e/packages/yarn2/package.json b/e2e/packages/yarn2/package.json index 396c0aeb47..c0054103f6 100644 --- a/e2e/packages/yarn2/package.json +++ b/e2e/packages/yarn2/package.json @@ -8,6 +8,6 @@ "tmp": "0.1.0" }, "scripts": { - "postinstall": "node ./postinstall.js" + "postinstall": "node ../postinstall.js" } } diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 1b0b254728..8bde06bacd 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -53,9 +53,11 @@ const WORKSPACE = args[0]; const RULE_TYPE = args[1]; const PKG_JSON_FILE_PATH = args[2]; const LOCK_FILE_PATH = args[3]; -const STRICT_VISIBILITY = args[4]?.toLowerCase() === 'true'; -const INCLUDED_FILES = args[5] ? args[5].split(',') : []; -const BAZEL_VERSION = args[6]; +const WORKSPACE_ROOT_PREFIX = args[4]; +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 PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; @@ -122,7 +124,7 @@ export function main() { generateBuildFiles(pkgs) // write a .bazelignore file - writeFileSync('.bazelignore', 'node_modules'); + writeFileSync('.bazelignore', `node_modules\n${WORKSPACE_ROOT_BASE}`); } /** @@ -171,8 +173,7 @@ function generateRootBuildFile(pkgs: Dep[]) { let exportsStarlark = ''; pkgs.forEach(pkg => {pkg._files.forEach(f => { - exportsStarlark += ` "node_modules/${pkg._dir}/${f}", -`; + exportsStarlark += ` "node_modules/${pkg._dir}/${f}",\n`; })}); let buildFile = @@ -556,7 +557,8 @@ function findScopes() { const scopes = listing.filter(f => f.startsWith('@')) .map(f => path.posix.join(p, f)) .filter(f => isDirectory(f)) - .map(f => f.replace(/^node_modules\//, '')); + // strip 'node_modules/' from filename + .map(f => f.substring('node_modules/'.length)); return scopes; } @@ -575,7 +577,7 @@ export function parsePackage(p: string, dependencies: Set = new Set()): // Trim the leading node_modules from the path and // assign to _dir for future use - pkg._dir = p.replace(/^node_modules\//, ''); + pkg._dir = p.substring('node_modules/'.length) // Stash the package directory name for future use pkg._name = pkg._dir.split('/').pop(); @@ -585,7 +587,7 @@ export function parsePackage(p: string, dependencies: Set = new Set()): pkg._moduleName = pkg.name || `${pkg._dir}/${pkg._name}`; // Keep track of whether or not this is a nested package - pkg._isNested = /\/node_modules\//.test(p); + pkg._isNested = /\/node_modules\//.test(pkg._dir); // List all the files in the npm package for later use pkg._files = listFiles(p); diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js index f97261b8a6..b675ed8ce6 100644 --- a/internal/npm_install/index.js +++ b/internal/npm_install/index.js @@ -1,5 +1,5 @@ /* THIS FILE GENERATED FROM .ts; see BUILD.bazel */ /* clang-format off */'use strict'; -var _a; +var _a, _b; Object.defineProperty(exports, "__esModule", { value: true }); const fs = require("fs"); const path = require("path"); @@ -13,9 +13,11 @@ const WORKSPACE = args[0]; const RULE_TYPE = args[1]; const PKG_JSON_FILE_PATH = args[2]; const LOCK_FILE_PATH = args[3]; -const STRICT_VISIBILITY = ((_a = args[4]) === null || _a === void 0 ? void 0 : _a.toLowerCase()) === 'true'; -const INCLUDED_FILES = args[5] ? args[5].split(',') : []; -const BAZEL_VERSION = args[6]; +const WORKSPACE_ROOT_PREFIX = args[4]; +const WORKSPACE_ROOT_BASE = (_a = WORKSPACE_ROOT_PREFIX) === null || _a === void 0 ? void 0 : _a.split('/')[0]; +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 PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY) { @@ -49,7 +51,7 @@ function main() { flattenDependencies(pkgs); generateBazelWorkspaces(pkgs); generateBuildFiles(pkgs); - writeFileSync('.bazelignore', 'node_modules'); + writeFileSync('.bazelignore', `node_modules\n${WORKSPACE_ROOT_BASE}`); } exports.main = main; function generateBuildFiles(pkgs) { @@ -86,8 +88,7 @@ function generateRootBuildFile(pkgs) { let exportsStarlark = ''; pkgs.forEach(pkg => { pkg._files.forEach(f => { - exportsStarlark += ` "node_modules/${pkg._dir}/${f}", -`; + exportsStarlark += ` "node_modules/${pkg._dir}/${f}",\n`; }); }); let buildFile = generateBuildFileHeader() + `load("@build_bazel_rules_nodejs//:index.bzl", "js_library") @@ -327,7 +328,7 @@ function findScopes() { const scopes = listing.filter(f => f.startsWith('@')) .map(f => path.posix.join(p, f)) .filter(f => isDirectory(f)) - .map(f => f.replace(/^node_modules\//, '')); + .map(f => f.substring('node_modules/'.length)); return scopes; } function parsePackage(p, dependencies = new Set()) { @@ -335,10 +336,10 @@ function parsePackage(p, dependencies = new Set()) { const pkg = isFile(packageJson) ? JSON.parse(stripBom(fs.readFileSync(packageJson, { encoding: 'utf8' }))) : { version: '0.0.0' }; - pkg._dir = p.replace(/^node_modules\//, ''); + pkg._dir = p.substring('node_modules/'.length); pkg._name = pkg._dir.split('/').pop(); pkg._moduleName = pkg.name || `${pkg._dir}/${pkg._name}`; - pkg._isNested = /\/node_modules\//.test(p); + pkg._isNested = /\/node_modules\//.test(pkg._dir); pkg._files = listFiles(p); pkg._runfiles = pkg._files.filter((f) => !/[^\x21-\x7E]/.test(f)); pkg._dependencies = []; diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 3141e0fd29..9ee3cda84b 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -126,6 +126,7 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file): rule_type, repository_ctx.path(repository_ctx.attr.package_json), repository_ctx.path(lock_file), + _workspace_root_prefix(repository_ctx), str(repository_ctx.attr.strict_visibility), ",".join(repository_ctx.attr.included_files), native.bazel_version, @@ -147,24 +148,55 @@ def _add_scripts(repository_ctx): {}, ) -def _add_package_json(repository_ctx): - repository_ctx.symlink( - repository_ctx.attr.package_json, - repository_ctx.path("package.json"), +def _workspace_root_path(repository_ctx, f): + segments = ["_"] + if f.package: + segments.append(f.package) + segments.append(f.name) + return "/".join(segments) + +def _workspace_root_prefix(repository_ctx): + package_json = repository_ctx.attr.package_json + segments = ["_"] + if package_json.package: + segments.append(package_json.package) + segments.extend(package_json.name.split("/")) + segments.pop() + return "/".join(segments) + "/" + +def _copy_file(repository_ctx, f): + to = _workspace_root_path(repository_ctx, f) + + # ensure the destination directory exists + to_segments = to.split("/") + if len(to_segments) > 1: + dirname = "/".join(to_segments[:-1]) + result = repository_ctx.execute( + ["mkdir", "-p", dirname], + quiet = repository_ctx.attr.quiet, + ) + if result.return_code: + fail("mkdir -p %s failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (dirname, result.stdout, result.stderr)) + + # copy the file; don't use the repository_ctx.template trick with empty substitution as this + # does not copy over binary files properly + result = repository_ctx.execute( + ["cp", "-f", repository_ctx.path(f), to], + quiet = repository_ctx.attr.quiet, ) + if result.return_code: + fail("cp -f %s %s failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (repository_ctx.path(f), to, result.stdout, result.stderr)) + +def _symlink_file(repository_ctx, f): + repository_ctx.symlink(f, _workspace_root_path(repository_ctx, f)) -def _add_data_dependencies(repository_ctx): +def _copy_data_dependencies(repository_ctx): """Add data dependencies to the repository.""" for f in repository_ctx.attr.data: - to = [] - if f.package: - to.append(f.package) - to.append(f.name) - # Make copies of the data files instead of symlinking # as yarn under linux will have trouble using symlinked # files as npm file:// packages - repository_ctx.template("/".join(to), f, {}) + _copy_file(repository_ctx, f) def _add_node_repositories_info_deps(repository_ctx): # Add a dep to the node_info & yarn_info files from node_repositories @@ -180,7 +212,16 @@ def _add_node_repositories_info_deps(repository_ctx): def _symlink_node_modules(repository_ctx): package_json_dir = repository_ctx.path(repository_ctx.attr.package_json).dirname - repository_ctx.symlink(repository_ctx.path(str(package_json_dir) + "/node_modules"), repository_ctx.path("node_modules")) + if repository_ctx.attr.symlink_node_modules: + repository_ctx.symlink( + repository_ctx.path(str(package_json_dir) + "/node_modules"), + repository_ctx.path("node_modules"), + ) + else: + repository_ctx.symlink( + repository_ctx.path(_workspace_root_prefix(repository_ctx) + "node_modules"), + repository_ctx.path("node_modules"), + ) def _check_min_bazel_version(rule, repository_ctx): if repository_ctx.attr.symlink_node_modules: @@ -213,13 +254,11 @@ def _npm_install_impl(repository_ctx): npm_args.extend(repository_ctx.attr.args) - # If symlink_node_modules is true then run the package manager - # in the package.json folder; otherwise, run it in the root of - # the external repository + # Run the package manager in the package.json folder if repository_ctx.attr.symlink_node_modules: - root = repository_ctx.path(repository_ctx.attr.package_json).dirname + root = str(repository_ctx.path(repository_ctx.attr.package_json).dirname) else: - root = repository_ctx.path("") + root = str(repository_ctx.path(_workspace_root_prefix(repository_ctx))) # The entry points for npm install for osx/linux and windows if not is_windows_host: @@ -250,12 +289,9 @@ cd /D "{root}" && "{npm}" {npm_args} executable = True, ) - repository_ctx.symlink( - repository_ctx.attr.package_lock_json, - repository_ctx.path("package-lock.json"), - ) - _add_package_json(repository_ctx) - _add_data_dependencies(repository_ctx) + _symlink_file(repository_ctx, repository_ctx.attr.package_lock_json) + _copy_file(repository_ctx, repository_ctx.attr.package_json) + _copy_data_dependencies(repository_ctx) _add_scripts(repository_ctx) _add_node_repositories_info_deps(repository_ctx) @@ -288,15 +324,15 @@ cd /D "{root}" && "{npm}" {npm_args} # removeNPMAbsolutePaths is run on node_modules after npm install as the package.json files # generated by npm are non-deterministic. They contain absolute install paths and other private # information fields starting with "_". removeNPMAbsolutePaths removes all fields starting with "_". + print([node, repository_ctx.path(remove_npm_absolute_paths), root + "/node_modules"]) result = repository_ctx.execute( - [node, repository_ctx.path(remove_npm_absolute_paths), "/".join([str(root), "node_modules"])], + [node, repository_ctx.path(remove_npm_absolute_paths), root + "/node_modules"], ) if result.return_code: fail("remove_npm_absolute_paths failed: %s (%s)" % (result.stdout, result.stderr)) - if repository_ctx.attr.symlink_node_modules: - _symlink_node_modules(repository_ctx) + _symlink_node_modules(repository_ctx) _create_build_files(repository_ctx, "npm_install", node, repository_ctx.attr.package_lock_json) @@ -380,13 +416,11 @@ def _yarn_install_impl(repository_ctx): yarn_args.extend(["--mutex", "network"]) yarn_args.extend(repository_ctx.attr.args) - # If symlink_node_modules is true then run the package manager - # in the package.json folder; otherwise, run it in the root of - # the external repository + # Run the package manager in the package.json folder if repository_ctx.attr.symlink_node_modules: - root = repository_ctx.path(repository_ctx.attr.package_json).dirname + root = str(repository_ctx.path(repository_ctx.attr.package_json).dirname) else: - root = repository_ctx.path("") + root = str(repository_ctx.path(_workspace_root_prefix(repository_ctx))) # The entry points for npm install for osx/linux and windows if not is_windows_host: @@ -424,12 +458,9 @@ cd /D "{root}" && "{yarn}" {yarn_args} executable = True, ) - repository_ctx.symlink( - repository_ctx.attr.yarn_lock, - repository_ctx.path("yarn.lock"), - ) - _add_package_json(repository_ctx) - _add_data_dependencies(repository_ctx) + _symlink_file(repository_ctx, repository_ctx.attr.yarn_lock) + _copy_file(repository_ctx, repository_ctx.attr.package_json) + _copy_data_dependencies(repository_ctx) _add_scripts(repository_ctx) _add_node_repositories_info_deps(repository_ctx) @@ -456,8 +487,7 @@ cd /D "{root}" && "{yarn}" {yarn_args} if result.return_code: fail("yarn_install failed: %s (%s)" % (result.stdout, result.stderr)) - if repository_ctx.attr.symlink_node_modules: - _symlink_node_modules(repository_ctx) + _symlink_node_modules(repository_ctx) _create_build_files(repository_ctx, "yarn_install", node, repository_ctx.attr.yarn_lock) diff --git a/tools/fine_grained_deps_npm/package-lock.json b/tools/fine_grained_deps_npm/package-lock.json index e5c85d553c..ca11d23479 100644 --- a/tools/fine_grained_deps_npm/package-lock.json +++ b/tools/fine_grained_deps_npm/package-lock.json @@ -1289,7 +1289,7 @@ } }, "local-module": { - "version": "file:tools/npm_packages/local_module/npm" + "version": "file:../npm_packages/local_module/npm" }, "lodash.debounce": { "version": "4.0.8", diff --git a/tools/fine_grained_deps_npm/package.json b/tools/fine_grained_deps_npm/package.json index 77d8e903e1..9438b00eed 100644 --- a/tools/fine_grained_deps_npm/package.json +++ b/tools/fine_grained_deps_npm/package.json @@ -12,10 +12,10 @@ "chokidar": "2.0.4", "http-server": "github:alexeagle/http-server#97205e945b69091606ed83aa0c8489e9ce65d282", "klaw": "1.3.1", - "local-module": "file:tools/npm_packages/local_module/npm", + "local-module": "file:../../tools/npm_packages/local_module/npm", "rxjs": "6.5.0" }, "scripts": { - "postinstall": "node internal/npm_install/test/postinstall.js" + "postinstall": "node ../../internal/npm_install/test/postinstall.js" } } diff --git a/tools/fine_grained_deps_yarn/package.json b/tools/fine_grained_deps_yarn/package.json index 54be5a1e9e..75ff2055ac 100644 --- a/tools/fine_grained_deps_yarn/package.json +++ b/tools/fine_grained_deps_yarn/package.json @@ -12,10 +12,10 @@ "chokidar": "2.0.4", "http-server": "github:alexeagle/http-server#97205e945b69091606ed83aa0c8489e9ce65d282", "klaw": "1.3.1", - "local-module": "link:tools/npm_packages/local_module/yarn", + "local-module": "link:../../tools/npm_packages/local_module/yarn", "rxjs": "6.5.0" }, "scripts": { - "postinstall": "node internal/npm_install/test/postinstall.js" + "postinstall": "node ../../internal/npm_install/test/postinstall.js" } } diff --git a/tools/fine_grained_deps_yarn/yarn.lock b/tools/fine_grained_deps_yarn/yarn.lock index 03752e0a23..93f950ce04 100644 --- a/tools/fine_grained_deps_yarn/yarn.lock +++ b/tools/fine_grained_deps_yarn/yarn.lock @@ -751,7 +751,7 @@ klaw@1.3.1: optionalDependencies: graceful-fs "^4.1.9" -"local-module@link:tools/npm_packages/local_module/yarn": +"local-module@link:../npm_packages/local_module/yarn": version "0.0.0" uid ""