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.
These attributes are no longer needed because they were a workaround for older versions of Bazel.

Fixes #1613

BUILD.bazel

@@ -68,9 +68,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 = "",

commitlint.config.js

@@ -8,7 +8,6 @@ module.exports = {
         'builtin',
         'create',
         'examples',
-        'hide-bazel-files',
         'jasmine',
         'karma',
         'labs',

internal/node/node_repositories.bzl

@@ -713,11 +713,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

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.
  */
@@ -314,13 +271,6 @@ def _maybe(repo_rule, name, **kwargs):
       // this file is not under the rootPath
       return;
     }
-    const basename = path.basename(file);
-    const basenameUc = basename.toUpperCase();
-    // Bazel BUILD files from npm distribution would have been renamed earlier with a _ prefix so
-    // we restore the name on the copy
-    if (basenameUc === '_BUILD' || basenameUc === '_BUILD.BAZEL') {
-      destFile = path.posix.join(path.dirname(destFile), basename.substr(1));
-    }
     const src = path.posix.join('node_modules', pkg._dir, file);
     const dest = path.posix.join(workspaceSourcePath, destFile);
     mkdirp(path.dirname(dest));
@@ -446,9 +396,8 @@ function listFiles(rootDir: string, subDir: string = ''): string[] {
  */
 function hasRootBuildFile(pkg: Dep, rootPath: string) {
   for (const file of pkg._files) {
-    // Bazel files would have been renamed earlier with a `_` prefix
     const fileUc = path.relative(rootPath, file).toUpperCase();
-    if (fileUc === '_BUILD' || fileUc === '_BUILD.BAZEL') {
+    if (fileUc === 'BUILD' || fileUc === 'BUILD.BAZEL') {
       return true;
     }
   }
@@ -476,22 +425,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 +460,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 +494,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;
 }
 
@@ -818,7 +748,7 @@ function filterFiles(files: string[], exts: string[] = []) {
   // Filter out BUILD files that came with the npm package
   return files.filter(file => {
     const basenameUc = path.basename(file).toUpperCase();
-    if (basenameUc === '_BUILD' || basenameUc === '_BUILD.BAZEL') {
+    if (basenameUc === 'BUILD' || basenameUc === 'BUILD.BAZEL') {
       return false;
     }
     return true;

internal/npm_install/npm_install.bzl

@@ -31,42 +31,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.
 
@@ -137,8 +101,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)
@@ -147,7 +109,6 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file):
         "index.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,

internal/pkg_npm/packager.js

@@ -35,30 +35,21 @@ function mkdirp(p) {
   }
 }
 
-function copyWithReplace(src, dest, substitutions, renameBuildFiles) {
+function copyWithReplace(src, dest, substitutions) {
   mkdirp(path.dirname(dest));
   if (fs.lstatSync(src).isDirectory()) {
     const files = fs.readdirSync(src)
     files.forEach((relativeChildSrc) => {
       const childSrc = path.join(src, relativeChildSrc);
       const childDest = path.join(dest, path.basename(childSrc));
-      copyWithReplace(childSrc, childDest, substitutions, renameBuildFiles);
+      copyWithReplace(childSrc, childDest, substitutions);
     });
   } else if (!isBinary(src)) {
     let content = fs.readFileSync(src, {encoding: 'utf-8'});
     substitutions.forEach(r => {
       const [regexp, newvalue] = r;
       content = content.replace(regexp, newvalue);
     });
-    if (renameBuildFiles) {
-      // Prefix all Bazel BUILD files with _ for npm packages.
-      // npm packages should not publish build files as this
-      // breaks their usage within yarn_install & npm_install rules.
-      const basenameUc = path.basename(dest).toUpperCase();
-      if (basenameUc == 'BUILD' || basenameUc == 'BUILD.BAZEL') {
-        dest = path.posix.join(path.dirname(dest), `_${path.basename(dest)}`);
-      }
-    }
     fs.writeFileSync(dest, content);
   } else {
     fs.copyFileSync(src, dest);
@@ -73,8 +64,7 @@ function main(args) {
   args = fs.readFileSync(args[0], {encoding: 'utf-8'}).split('\n').map(unquoteArgs);
   const
       [outDir, baseDir, srcsArg, binDir, genDir, depsArg, packagesArg, substitutionsArg,
-       replaceWithVersion, stampFile, vendorExternalArg, hideBuildFilesArg] = args;
-  const renameBuildFiles = parseInt(hideBuildFilesArg);
+       replaceWithVersion, stampFile, vendorExternalArg] = args;
 
   const substitutions = [
     // Strip content between BEGIN-INTERNAL / END-INTERNAL comments
@@ -112,18 +102,15 @@ function main(args) {
     src = src.replace(/\\/g, '/');
     if (src.startsWith('external/')) {
       // If srcs is from external workspace drop the external/wksp portion
-      copyWithReplace(
-          src, path.join(outDir, src.split('/').slice(2).join('/')), substitutions,
-          renameBuildFiles);
+      copyWithReplace(src, path.join(outDir, src.split('/').slice(2).join('/')), substitutions);
     } else {
       // Source is from local workspace
       if (baseDir && !src.startsWith(`${baseDir}/`)) {
         throw new Error(
             `${src} in 'srcs' does not reside in the base directory, ` +
             `generated file should belong in 'deps' instead.`);
       }
-      copyWithReplace(
-          src, path.join(outDir, path.relative(baseDir, src)), substitutions, renameBuildFiles);
+      copyWithReplace(src, path.join(outDir, path.relative(baseDir, src)), substitutions);
     }
   }
 
@@ -152,7 +139,7 @@ function main(args) {
   // Deps like bazel-bin/baseDir/my/path is copied to outDir/my/path.
   for (const dep of depsArg.split(',').filter(s => !!s)) {
     try {
-      copyWithReplace(dep, outPath(dep), substitutions, renameBuildFiles);
+      copyWithReplace(dep, outPath(dep), substitutions);
     } catch (e) {
       console.error(`Failed to copy ${dep} to ${outPath(dep)}`);
       throw e;
@@ -179,8 +166,7 @@ function main(args) {
           }
           return file;
         }
-        copyWithReplace(
-            path.join(base, file), path.join(outDir, outFile()), substitutions, renameBuildFiles);
+        copyWithReplace(path.join(base, file), path.join(outDir, outFile()), substitutions);
       }
     }
     fs.readdirSync(pkg).forEach(f => {

internal/pkg_npm/pkg_npm.bzl

@@ -81,16 +81,6 @@ PKG_NPM_ATTRS = {
         doc = """Files inside this directory which are simply copied into the package.""",
         allow_files = True,
     ),
-    "hide_build_files": attr.bool(
-        doc = """If set BUILD and BUILD.bazel files are prefixed with `_` in the npm package.
-        The default is True since npm packages that contain BUILD files don't work with
-        `yarn_install` and `npm_install` without a post-install step that deletes or renames them.
-
-        NB: Bazel has a change in https://github.com/bazelbuild/bazel/pull/10261
-        (expected in version 2.1) that adds .bazelignore
-        support for external repositories, which will make this attribute obsolete.""",
-        default = True,
-    ),
     "nested_packages": attr.label_list(
         doc = """Other pkg_npm rules whose content is copied into this package.""",
         allow_files = True,
@@ -207,7 +197,6 @@ def create_package(ctx, deps_files, nested_packages):
     args.add(ctx.attr.replace_with_version)
     args.add(ctx.version_file.path if stamp else "")
     args.add_joined(ctx.attr.vendor_external, join_with = ",", omit_if_empty = False)
-    args.add("1" if ctx.attr.hide_build_files else "0")
 
     inputs = ctx.files.srcs + deps_files + nested_packages