From b947af35552a57c1077b15718a603f44bbed4bb9 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 5 Feb 2021 17:21:25 +0000 Subject: [PATCH 1/3] feat: add generate_local_modules_build_files flag to yarn_install and npm_install --- WORKSPACE | 2 + internal/npm_install/generate_build_file.ts | 13 ++--- internal/npm_install/index.js | 8 +-- internal/npm_install/npm_install.bzl | 63 ++++++++++----------- 4 files changed, 39 insertions(+), 47 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 73471aeb32..25c6610526 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -184,6 +184,7 @@ yarn_install( package_json = "//:tools/fine_grained_deps_yarn/package.json", symlink_node_modules = False, yarn_lock = "//:tools/fine_grained_deps_yarn/yarn.lock", + generate_local_modules_build_files = False, ) npm_install( @@ -208,6 +209,7 @@ npm_install( package_json = "//:tools/fine_grained_deps_npm/package.json", package_lock_json = "//:tools/fine_grained_deps_npm/package-lock.json", symlink_node_modules = False, + generate_local_modules_build_files = False, ) yarn_install( diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 8bde06bacd..156ed85b1d 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -57,7 +57,8 @@ 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 GENERATE_LOCAL_MODULES_BUILD_FILES = JSON.parse(String(args[7]).toLowerCase()); +const BAZEL_VERSION = args[8]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; @@ -219,18 +220,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 b675ed8ce6..f9d0c9818b 100644 --- a/internal/npm_install/index.js +++ b/internal/npm_install/index.js @@ -17,7 +17,8 @@ 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 GENERATE_LOCAL_MODULES_BUILD_FILES = JSON.parse(String(args[7]).toLowerCase()); +const BAZEL_VERSION = args[8]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`; function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY) { @@ -121,9 +122,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 9ee3cda84b..d3e44b3f8e 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -113,9 +113,32 @@ data attribute. default = 3600, doc = """Maximum duration of the package manager execution in seconds.""", ), + "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. +""", + ), }) -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) @@ -129,6 +152,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, # double the default timeout in case of many packages, see #2231 ], timeout = 1200, quiet = repository_ctx.attr.quiet) @@ -334,7 +358,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, **{ @@ -373,17 +397,7 @@ When using a monorepo it's common to have modules that we want to use locally an 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.""", +race condition with the `npm_install` rule.""", implementation = _npm_install_impl, ) @@ -489,7 +503,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, **{ @@ -540,25 +554,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, ) From ea89196b9a2819afccfc9d3fa2d019b69b7d1183 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 5 Feb 2021 17:25:27 +0000 Subject: [PATCH 2/3] chore: apply correct linting --- WORKSPACE | 4 +-- internal/npm_install/npm_install.bzl | 46 ++++++++++++++-------------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 25c6610526..afb137054c 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -174,6 +174,7 @@ yarn_install( environment = { "SOME_USER_ENV": "yarn is great!", }, + generate_local_modules_build_files = False, included_files = [ "", ".js", @@ -184,7 +185,6 @@ yarn_install( package_json = "//:tools/fine_grained_deps_yarn/package.json", symlink_node_modules = False, yarn_lock = "//:tools/fine_grained_deps_yarn/yarn.lock", - generate_local_modules_build_files = False, ) npm_install( @@ -198,6 +198,7 @@ npm_install( environment = { "SOME_USER_ENV": "npm is cool!", }, + generate_local_modules_build_files = False, included_files = [ "", ".js", @@ -209,7 +210,6 @@ npm_install( package_json = "//:tools/fine_grained_deps_npm/package.json", package_lock_json = "//:tools/fine_grained_deps_npm/package-lock.json", symlink_node_modules = False, - generate_local_modules_build_files = False, ) yarn_install( diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index d3e44b3f8e..0cb5506b10 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. @@ -113,29 +136,6 @@ data attribute. default = 3600, doc = """Maximum duration of the package manager execution in seconds.""", ), - "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. -""", - ), }) def _create_build_files(repository_ctx, rule_type, node, lock_file, generate_local_modules_build_files): From a260432efd648a706d845b1f18af817eccf84039 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 5 Feb 2021 17:56:52 +0000 Subject: [PATCH 3/3] test: fix npm_install logic by removing json.parse --- internal/npm_install/generate_build_file.ts | 2 +- internal/npm_install/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 156ed85b1d..63ad51552d 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -57,7 +57,7 @@ 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 GENERATE_LOCAL_MODULES_BUILD_FILES = JSON.parse(String(args[7]).toLowerCase()); +const GENERATE_LOCAL_MODULES_BUILD_FILES = (`${args[7]}`.toLowerCase()) === 'true'; const BAZEL_VERSION = args[8]; const PUBLIC_VISIBILITY = '//visibility:public'; diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js index f9d0c9818b..b58f4a6e7e 100644 --- a/internal/npm_install/index.js +++ b/internal/npm_install/index.js @@ -17,7 +17,7 @@ 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 GENERATE_LOCAL_MODULES_BUILD_FILES = JSON.parse(String(args[7]).toLowerCase()); +const GENERATE_LOCAL_MODULES_BUILD_FILES = (`${args[7]}`.toLowerCase()) === 'true'; const BAZEL_VERSION = args[8]; const PUBLIC_VISIBILITY = '//visibility:public'; const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`;