From a6449b7c7a4d4d8a733058504a060feb6bff10b8 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Thu, 18 Feb 2021 22:19:23 +0000 Subject: [PATCH] feat: add generate_local_modules_build_files flag to yarn_install and npm_install rules (#2449) --- WORKSPACE | 2 + internal/npm_install/generate_build_file.ts | 15 ++--- internal/npm_install/index.js | 10 +-- internal/npm_install/npm_install.bzl | 72 +++++++++------------ 4 files changed, 41 insertions(+), 58 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 40ab99b58f..d9964a5466 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -203,6 +203,7 @@ yarn_install( environment = { "SOME_USER_ENV": "yarn is great!", }, + generate_local_modules_build_files = False, included_files = [ "", ".js", @@ -226,6 +227,7 @@ npm_install( environment = { "SOME_USER_ENV": "npm is cool!", }, + generate_local_modules_build_files = False, included_files = [ "", ".js", diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 0d24ce9c9e..68df4adfab 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -57,8 +57,9 @@ 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 PACKAGE_PATH = args[8]; +const GENERATE_LOCAL_MODULES_BUILD_FILES = (`${args[7]}`.toLowerCase()) === 'true'; +const BAZEL_VERSION = args[8]; +const PACKAGE_PATH = args[9]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; @@ -221,18 +222,12 @@ function generatePackageBuildFiles(pkg: Dep) { // install a node_module with link:) const isPkgDirASymlink = fs.existsSync(nodeModulesPkgDir) && fs.lstatSync(nodeModulesPkgDir).isSymbolicLink(); - // Check if the current package is also written inside the workspace - // NOTE: It's a corner case but fs.realpathSync(.) will not be the root of - // the workspace if symlink_node_modules = False as yarn & npm are run in the root of the - // external repository and anything linked with a relative path would have to be copied - // over via that data attribute - const isPkgInsideWorkspace = fs.realpathSync(nodeModulesPkgDir).includes(fs.realpathSync(`.`)); // Mark build file as one to symlink instead of generate as the package dir is a symlink, we // have a BUILD file and the pkg is written inside the workspace - const symlinkBuildFile = isPkgDirASymlink && buildFilePath && isPkgInsideWorkspace; + const symlinkBuildFile = isPkgDirASymlink && buildFilePath && !GENERATE_LOCAL_MODULES_BUILD_FILES; // Log if a BUILD file was expected but was not found - if (!symlinkBuildFile && isPkgDirASymlink) { + if (isPkgDirASymlink && !buildFilePath && !GENERATE_LOCAL_MODULES_BUILD_FILES) { console.log(`[yarn_install/npm_install]: package ${ nodeModulesPkgDir} is local symlink and as such a BUILD file for it is expected but none was found. Please add one at ${ fs.realpathSync(nodeModulesPkgDir)}`) diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js index e15e927b16..bdb7d6643f 100644 --- a/internal/npm_install/index.js +++ b/internal/npm_install/index.js @@ -17,8 +17,9 @@ 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 PACKAGE_PATH = args[8]; +const GENERATE_LOCAL_MODULES_BUILD_FILES = (`${args[7]}`.toLowerCase()) === 'true'; +const BAZEL_VERSION = args[8]; +const PACKAGE_PATH = args[9]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY) { @@ -123,9 +124,8 @@ function generatePackageBuildFiles(pkg) { buildFilePath = 'BUILD.bazel'; const nodeModulesPkgDir = `node_modules/${pkg._dir}`; const isPkgDirASymlink = fs.existsSync(nodeModulesPkgDir) && fs.lstatSync(nodeModulesPkgDir).isSymbolicLink(); - const isPkgInsideWorkspace = fs.realpathSync(nodeModulesPkgDir).includes(fs.realpathSync(`.`)); - const symlinkBuildFile = isPkgDirASymlink && buildFilePath && isPkgInsideWorkspace; - if (!symlinkBuildFile && isPkgDirASymlink) { + const symlinkBuildFile = isPkgDirASymlink && buildFilePath && !GENERATE_LOCAL_MODULES_BUILD_FILES; + if (isPkgDirASymlink && !buildFilePath && !GENERATE_LOCAL_MODULES_BUILD_FILES) { console.log(`[yarn_install/npm_install]: package ${nodeModulesPkgDir} is local symlink and as such a BUILD file for it is expected but none was found. Please add one at ${fs.realpathSync(nodeModulesPkgDir)}`); } let buildFile = printPackage(pkg); diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 2f312faae5..b85fa6503e 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -44,6 +44,29 @@ repository so all files that the package manager depends on must be listed. doc = """Environment variables to set before calling the package manager.""", default = {}, ), + "generate_local_modules_build_files": attr.bool( + default = True, + doc = """Enables the BUILD files auto generation for local modules installed with `file:` (npm) or `link:` (yarn) + +When using a monorepo it's common to have modules that we want to use locally and +publish to an external package repository. This can be achieved using a `js_library` rule +with a `package_name` attribute defined inside the local package `BUILD` file. However, +if the project relies on the local package dependency with `file:` (npm) or `link:` (yarn) to be used outside Bazel, this +could introduce a race condition with both `npm_install` or `yarn_install` rules. + +In order to overcome it, a link could be created to the package `BUILD` file from the +npm external Bazel repository (so we can use a local BUILD file instead of an auto generated one), +which require us to set `generate_local_modules_build_files = False` and complete a last step which is writing the +expected targets on that same `BUILD` file to be later used both by `npm_install` or `yarn_install` +rules, which are: ``, ``, +``, `` and the last one just ``. If you doubt what those targets +should look like, check the generated `BUILD` file for a given node module. + +When true, the rule will follow the default behaviour of auto generating BUILD files for each `node_module` at install time. + +When False, the rule will not auto generate BUILD files for `node_modules` that are installed as symlinks for local modules. +""", + ), "included_files": attr.string_list( doc = """List of file extensions to be included in the npm package targets. @@ -123,7 +146,7 @@ data attribute. ), }) -def _create_build_files(repository_ctx, rule_type, node, lock_file): +def _create_build_files(repository_ctx, rule_type, node, lock_file, generate_local_modules_build_files): repository_ctx.report_progress("Processing node_modules: installing Bazel packages and generating BUILD files") if repository_ctx.attr.manual_build_file_contents: repository_ctx.file("manual_build_file_contents", repository_ctx.attr.manual_build_file_contents) @@ -137,6 +160,7 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file): _workspace_root_prefix(repository_ctx), str(repository_ctx.attr.strict_visibility), ",".join(repository_ctx.attr.included_files), + str(generate_local_modules_build_files), native.bazel_version, repository_ctx.attr.package_path, # double the default timeout in case of many packages, see #2231 @@ -344,7 +368,7 @@ cd /D "{root}" && "{npm}" {npm_args} _symlink_node_modules(repository_ctx) - _create_build_files(repository_ctx, "npm_install", node, repository_ctx.attr.package_lock_json) + _create_build_files(repository_ctx, "npm_install", node, repository_ctx.attr.package_lock_json, repository_ctx.attr.generate_local_modules_build_files) npm_install = repository_rule( attrs = dict(COMMON_ATTRIBUTES, **{ @@ -374,26 +398,7 @@ See npm CLI docs https://docs.npmjs.com/cli/install.html for complete list of su This rule will set the environment variable `BAZEL_NPM_INSTALL` to '1' (unless it set to another value in the environment attribute). Scripts may use to this to -check if yarn is being run by the `npm_install` repository rule. - - -**LOCAL MODULES WITH THE NEED TO BE USED BOTH INSIDE AND OUTSIDE BAZEL** - -When using a monorepo it's common to have modules that we want to use locally and -publish to an external package repository. This can be achieved using a `js_library` rule -with a `package_name` attribute defined inside the local package `BUILD` file. However, -if the project relies on the local package dependency with `file:`, this could introduce a -race condition with the `npm_install` rule. - -In order to overcome it, a link will be created to the package `BUILD` file from the -npm external Bazel repository, which require us to complete a last step which is writing -the expected targets on that same `BUILD` file to be later used by the `npm_install` -rule, which are: ``, ``, -``, `` and the last -one just ``. - -If you doubt what those targets should look like, check the -generated `BUILD` file for a given node module.""", +check if yarn is being run by the `npm_install` repository rule.""", implementation = _npm_install_impl, ) @@ -499,7 +504,7 @@ cd /D "{root}" && "{yarn}" {yarn_args} _symlink_node_modules(repository_ctx) - _create_build_files(repository_ctx, "yarn_install", node, repository_ctx.attr.yarn_lock) + _create_build_files(repository_ctx, "yarn_install", node, repository_ctx.attr.yarn_lock, repository_ctx.attr.generate_local_modules_build_files) yarn_install = repository_rule( attrs = dict(COMMON_ATTRIBUTES, **{ @@ -550,25 +555,6 @@ to yarn so that the local cache is contained within the external repository. This rule will set the environment variable `BAZEL_YARN_INSTALL` to '1' (unless it set to another value in the environment attribute). Scripts may use to this to -check if yarn is being run by the `yarn_install` repository rule. - - -**LOCAL MODULES WITH THE NEED TO BE USED BOTH INSIDE AND OUTSIDE BAZEL** - -When using a monorepo it's common to have modules that we want to use locally and -publish to an external package repository. This can be achieved using a `js_library` rule -with a `package_name` attribute defined inside the local package `BUILD` file. However, -if the project relies on the local package dependency with `link:`, this could introduce a -race condition with the `yarn_install` rule. - -In order to overcome it, a link will be created to the package `BUILD` file from the -npm external Bazel repository, which require us to complete a last step which is writing -the expected targets on that same `BUILD` file to be later used by the `yarn_install` -rule, which are: ``, ``, -``, `` and the last -one just ``. - -If you doubt what those targets should look like, check the -generated `BUILD` file for a given node module.""", +check if yarn is being run by the `yarn_install` repository rule.""", implementation = _yarn_install_impl, )