chore: remove hide-build-files functionality

Since 1.3.0, we write a .bazelignore file into the generated workspaces, so Bazel does not see BUILD files. The features to hide the BUILD files are no longer needed. We also stop hiding the BUILD files in packages published by pkg_npm. This means that users who publish packages built by that rule will now require *their* users to have rules_nodejs 1.3.0 or greater, which adds the .bazelignore file. BREAKING CHANGE: rules_nodejs now requires Bazel 2.1 or greater. Also the hide_build_files attribute was removed from pkg_npm, and always_hide_bazel_files was removed from yarn_install and npm_install. Fixes bazel-contrib#1613
alexeagle · Mar 29, 2020 · b706351 · b706351
1 parent cd8520d
commit b706351
Show file tree

Hide file tree

Showing 14 changed files with 21 additions and 366 deletions.
diff --git a/BUILD.bazel b/BUILD.bazel
@@ -67,9 +67,6 @@ pkg_npm(
         "BUILD.bazel",
         "LICENSE",
     ],
-    # Don't rename BUILD files as this package is not published to npm
-    # but is compressed in "release" below and published as a .tar.gz to GitHub
-    hide_build_files = False,
     # Don't replace the default 0.0.0-PLACEHOLDER for this pkg_npm since
     # we are packaging up the packager itself and this replacement will break it
     replace_with_version = "",

diff --git a/commitlint.config.js b/commitlint.config.js
@@ -8,7 +8,6 @@ module.exports = {
         'builtin',
         'create',
         'examples',
-        'hide-bazel-files',
         'jasmine',
         'karma',
         'labs',

diff --git a/e2e/symlinked_node_modules_npm/package-lock.json b/e2e/symlinked_node_modules_npm/package-lock.json
diff --git a/internal/node/node_repositories.bzl b/internal/node/node_repositories.bzl
@@ -710,11 +710,12 @@ def node_repositories(**kwargs):
     # 0.14.0: @bazel_tools//tools/bash/runfiles is required for nodejs
     # 0.17.1: allow @ in package names is required for fine grained deps
     # 0.21.0: repository_ctx.report_progress API
+    # 2.1.0: bazelignore support in external workspaces
     check_bazel_version(
         message = """
-    A minimum Bazel version of 0.21.0 is required to use build_bazel_rules_nodejs.
+    A minimum Bazel version of 2.1.0 is required to use build_bazel_rules_nodejs.
     """,
-        minimum_bazel_version = "0.21.0",
+        minimum_bazel_version = "2.1.0",
     )
 
     # This needs to be setup so toolchains can access nodejs for all different versions

diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js
@@ -65,10 +65,9 @@ package(default_visibility = ["//visibility:public"])
     const args = process.argv.slice(2);
     const WORKSPACE = args[0];
     const RULE_TYPE = args[1];
-    const ERROR_ON_BAZEL_FILES = parseInt(args[2]);
-    const LOCK_FILE_PATH = args[3];
-    const INCLUDED_FILES = args[4] ? args[4].split(',') : [];
-    const BAZEL_VERSION = args[5];
+    const LOCK_FILE_PATH = args[2];
+    const INCLUDED_FILES = args[3] ? args[3].split(',') : [];
+    const BAZEL_VERSION = args[4];
     if (require.main === module) {
         main();
     }
@@ -122,46 +121,6 @@ package(default_visibility = ["//visibility:public"])
         pkgs.forEach(pkg => pkgsMap.set(pkg._dir, pkg));
         pkgs.forEach(pkg => flattenPkgDependencies(pkg, pkg, pkgsMap));
     }
-    /**
-     * Handles Bazel files in npm distributions.
-     */
-    function hideBazelFiles(pkg) {
-        const hasHideBazelFiles = isDirectory('node_modules/@bazel/hide-bazel-files');
-        pkg._files = pkg._files.map(file => {
-            const basename = path.basename(file);
-            const basenameUc = basename.toUpperCase();
-            if (basenameUc === 'BUILD' || basenameUc === 'BUILD.BAZEL') {
-                // If bazel files are detected and there is no @bazel/hide-bazel-files npm
-                // package then error out and suggest adding the package. It is possible to
-                // have bazel BUILD files with the package installed as it's postinstall
-                // step, which hides bazel BUILD files, only runs when the @bazel/hide-bazel-files
-                // is installed and not when new packages are added (via `yarn add`
-                // for example) after the initial install. In this case, however, the repo rule
-                // will re-run as the package.json && lock file has changed so we just
-                // hide the added BUILD files during the repo rule run here since @bazel/hide-bazel-files
-                // was not run.
-                if (!hasHideBazelFiles && ERROR_ON_BAZEL_FILES) {
-                    console.error(`npm package '${pkg._dir}' from @${WORKSPACE} ${RULE_TYPE} rule
-has a Bazel BUILD file '${file}'. We recommend updating to Bazel 2.1 or greater which ignores such files.
-
-If you can't update Bazel from ${BAZEL_VERSION}, you can use the @bazel/hide-bazel-files utility to hide these files.
-See https://github.com/bazelbuild/rules_nodejs/blob/master/packages/hide-bazel-files/README.md
-for installation instructions.`);
-                    process.exit(1);
-                }
-                else {
-                    // All Bazel files in the npm distribution should be renamed by
-                    // adding a `_` prefix so that file targets don't cross package boundaries.
-                    const newFile = path.posix.join(path.dirname(file), `_${basename}`);
-                    const srcPath = path.posix.join('node_modules', pkg._dir, file);
-                    const dstPath = path.posix.join('node_modules', pkg._dir, newFile);
-                    fs.renameSync(srcPath, dstPath);
-                    return newFile;
-                }
-            }
-            return file;
-        });
-    }
     /**
      * Generates the root BUILD file.
      */
@@ -432,22 +391,7 @@ def _maybe(repo_rule, name, **kwargs):
             .filter(f => !f.startsWith('.'))
             .map(f => path.posix.join(p, f))
             .filter(f => isDirectory(f));
-        packages.forEach(f => {
-            let hide = true;
-            // Starting in version 2.1, Bazel honors the .bazelignore file we wrote into the
-            // root of the external repository, and won't see BUILD files under node_modules
-            // This parsing of the version number isn't accurate in some cases
-            // (eg. install bazel from commit hash)
-            // Do a cheap semver check that the major version is at least 2.1
-            // (we don't want to depend on a third-party library like semver here)
-            if (Number(BAZEL_VERSION.split('.')[0]) >= 2 && !BAZEL_VERSION.startsWith('2.0')) {
-                hide = false;
-            }
-            if (fs.lstatSync(f).isSymbolicLink()) {
-                hide = false;
-            }
-            pkgs.push(parsePackage(f, hide), ...findPackages(path.posix.join(f, 'node_modules')));
-        });
+        packages.forEach(f => { pkgs.push(parsePackage(f), ...findPackages(path.posix.join(f, 'node_modules'))); });
         const scopes = listing.filter(f => f.startsWith('@'))
             .map(f => path.posix.join(p, f))
             .filter(f => isDirectory(f));
@@ -474,7 +418,7 @@ def _maybe(repo_rule, name, **kwargs):
      * package json and return it as an object along with
      * some additional internal attributes prefixed with '_'.
      */
-    function parsePackage(p, hide = true) {
+    function parsePackage(p) {
         // Parse the package.json file of this package
         const packageJson = path.posix.join(p, 'package.json');
         const stripBom = (s) => s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
@@ -500,11 +444,6 @@ def _maybe(repo_rule, name, **kwargs):
         // Initialize _dependencies to an empty array
         // which is later filled with the flattened dependency list
         pkg._dependencies = [];
-        // Hide bazel files in this package. We do this before parsing
-        // the next package to prevent issues caused by symlinks between
-        // package and nested packages setup by the package manager.
-        if (hide)
-            hideBazelFiles(pkg);
         return pkg;
     }
     exports.parsePackage = parsePackage;

diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts
@@ -59,10 +59,9 @@ package(default_visibility = ["//visibility:public"])
 const args = process.argv.slice(2);
 const WORKSPACE = args[0];
 const RULE_TYPE = args[1];
-const ERROR_ON_BAZEL_FILES = parseInt(args[2]);
-const LOCK_FILE_PATH = args[3];
-const INCLUDED_FILES = args[4] ? args[4].split(',') : [];
-const BAZEL_VERSION = args[5];
+const LOCK_FILE_PATH = args[2];
+const INCLUDED_FILES = args[3] ? args[3].split(',') : [];
+const BAZEL_VERSION = args[4];
 
 if (require.main === module) {
   main();
@@ -126,48 +125,6 @@ function flattenDependencies(pkgs: Dep[]) {
   pkgs.forEach(pkg => flattenPkgDependencies(pkg, pkg, pkgsMap));
 }
 
-/**
- * Handles Bazel files in npm distributions.
- */
-function hideBazelFiles(pkg: Dep) {
-  const hasHideBazelFiles = isDirectory('node_modules/@bazel/hide-bazel-files');
-  pkg._files = pkg._files.map(file => {
-    const basename = path.basename(file);
-    const basenameUc = basename.toUpperCase();
-    if (basenameUc === 'BUILD' || basenameUc === 'BUILD.BAZEL') {
-      // If bazel files are detected and there is no @bazel/hide-bazel-files npm
-      // package then error out and suggest adding the package. It is possible to
-      // have bazel BUILD files with the package installed as it's postinstall
-      // step, which hides bazel BUILD files, only runs when the @bazel/hide-bazel-files
-      // is installed and not when new packages are added (via `yarn add`
-      // for example) after the initial install. In this case, however, the repo rule
-      // will re-run as the package.json && lock file has changed so we just
-      // hide the added BUILD files during the repo rule run here since @bazel/hide-bazel-files
-      // was not run.
-      if (!hasHideBazelFiles && ERROR_ON_BAZEL_FILES) {
-        console.error(`npm package '${pkg._dir}' from @${WORKSPACE} ${RULE_TYPE} rule
-has a Bazel BUILD file '${
-            file}'. We recommend updating to Bazel 2.1 or greater which ignores such files.
-
-If you can't update Bazel from ${
-            BAZEL_VERSION}, you can use the @bazel/hide-bazel-files utility to hide these files.
-See https://github.com/bazelbuild/rules_nodejs/blob/master/packages/hide-bazel-files/README.md
-for installation instructions.`);
-        process.exit(1);
-      } else {
-        // All Bazel files in the npm distribution should be renamed by
-        // adding a `_` prefix so that file targets don't cross package boundaries.
-        const newFile = path.posix.join(path.dirname(file), `_${basename}`);
-        const srcPath = path.posix.join('node_modules', pkg._dir, file);
-        const dstPath = path.posix.join('node_modules', pkg._dir, newFile);
-        fs.renameSync(srcPath, dstPath);
-        return newFile;
-      }
-    }
-    return file;
-  });
-}
-
 /**
  * Generates the root BUILD file.
  */
@@ -476,22 +433,8 @@ function findPackages(p = 'node_modules') {
                        .map(f => path.posix.join(p, f))
                        .filter(f => isDirectory(f));
 
-  packages.forEach(f => {
-    let hide = true;
-    // Starting in version 2.1, Bazel honors the .bazelignore file we wrote into the
-    // root of the external repository, and won't see BUILD files under node_modules
-    // This parsing of the version number isn't accurate in some cases
-    // (eg. install bazel from commit hash)
-    // Do a cheap semver check that the major version is at least 2.1
-    // (we don't want to depend on a third-party library like semver here)
-    if (Number(BAZEL_VERSION.split('.')[0]) >= 2 && !BAZEL_VERSION.startsWith('2.0')) {
-      hide = false;
-    }
-    if (fs.lstatSync(f).isSymbolicLink()) {
-      hide = false;
-    }
-    pkgs.push(parsePackage(f, hide), ...findPackages(path.posix.join(f, 'node_modules')))
-  });
+  packages.forEach(
+      f => {pkgs.push(parsePackage(f), ...findPackages(path.posix.join(f, 'node_modules')))});
 
   const scopes = listing.filter(f => f.startsWith('@'))
                      .map(f => path.posix.join(p, f))
@@ -525,7 +468,7 @@ function findScopes() {
  * package json and return it as an object along with
  * some additional internal attributes prefixed with '_'.
  */
-export function parsePackage(p: string, hide: boolean = true): Dep {
+export function parsePackage(p: string): Dep {
   // Parse the package.json file of this package
   const packageJson = path.posix.join(p, 'package.json');
   const stripBom = (s: string) => s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
@@ -559,11 +502,6 @@ export function parsePackage(p: string, hide: boolean = true): Dep {
   // which is later filled with the flattened dependency list
   pkg._dependencies = [];
 
-  // Hide bazel files in this package. We do this before parsing
-  // the next package to prevent issues caused by symlinks between
-  // package and nested packages setup by the package manager.
-  if (hide) hideBazelFiles(pkg);
-
   return pkg;
 }
 

diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl
@@ -30,42 +30,6 @@ COMMON_ATTRIBUTES = dict(dict(), **{
         default = 3600,
         doc = """Maximum duration of the package manager execution in seconds.""",
     ),
-    "always_hide_bazel_files": attr.bool(
-        doc = """Always hide Bazel build files such as `BUILD` and BUILD.bazel` by prefixing them with `_`.
-
-This is only needed in Bazel 2.0 or earlier.
-We recommend upgrading to a later version to avoid the problem this works around.
-
-Defaults to False, in which case Bazel files are _not_ hidden when `symlink_node_modules`
-is True. In this case, the rule will report an error when there are Bazel files detected
-in npm packages.
-
-Reporting the error is desirable as relying on this repository rule to hide
-these files does not work in the case where a user deletes their node_modules folder
-and manually re-creates it with yarn or npm outside of Bazel which would restore them.
-On a subsequent Bazel build, this repository rule does not re-run and the presence
-of the Bazel files leads to a build failure that looks like the following:
-
-```
-ERROR: /private/var/tmp/_bazel_greg/37b273501bbecefcf5ce4f3afcd7c47a/external/npm/BUILD.bazel:9:1:
-Label '@npm//:node_modules/rxjs/src/AsyncSubject.ts' crosses boundary of subpackage '@npm//node_modules/rxjs/src'
-(perhaps you meant to put the colon here: '@npm//node_modules/rxjs/src:AsyncSubject.ts'?)
-```
-
-See https://github.com/bazelbuild/rules_nodejs/issues/802 for more details.
-
-The recommended solution is to use the @bazel/hide-bazel-files utility to hide these files.
-See https://github.com/bazelbuild/rules_nodejs/blob/master/packages/hide-bazel-files/README.md
-for installation instructions.
-
-The alternate solution is to set `always_hide_bazel_files` to True which tell
-this rule to hide Bazel files even when `symlink_node_modules` is True. This means
-you won't need to use `@bazel/hide-bazel-files` utility but if you manually recreate
-your `node_modules` folder via yarn or npm outside of Bazel you may run into the above
-error.
-""",
-        default = False,
-    ),
     "data": attr.label_list(
         doc = """Data files required by this rule.
 
@@ -136,8 +100,6 @@ data attribute.
 })
 
 def _create_build_files(repository_ctx, rule_type, node, lock_file):
-    error_on_build_files = repository_ctx.attr.symlink_node_modules and not repository_ctx.attr.always_hide_bazel_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)
@@ -146,7 +108,6 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file):
         "generate_build_file.js",
         repository_ctx.attr.name,
         rule_type,
-        "1" if error_on_build_files else "0",
         repository_ctx.path(lock_file),
         ",".join(repository_ctx.attr.included_files),
         native.bazel_version,