Skip to content

Commit

Permalink
chore: remove hide-build-files package
Browse files Browse the repository at this point in the history 
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.

We leave support for legacy packages which were publishing with _BUILD files.

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 are no longer needed since 1.3.0

Fixes #1613
  • Loading branch information
@alexeagle
alexeagle committed May 3, 2020
1 parent 9f541ce commit 5d1d006
Show file tree
Hide file tree
Showing 15 changed files with 33 additions and 345 deletions.
3 changes: 0 additions & 3 deletions BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "",
Expand Down
1 change: 0 additions & 1 deletion commitlint.config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ module.exports = {
'builtin',
'create',
'examples',
'hide-bazel-files',
'jasmine',
'karma',
'labs',
Expand Down
6 changes: 0 additions & 6 deletions e2e/symlinked_node_modules_npm/package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions internal/node/node_repositories.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
84 changes: 14 additions & 70 deletions internal/npm_install/generate_build_file.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -316,7 +273,8 @@ def _maybe(repo_rule, name, **kwargs):
}
const basename = path.basename(file);
const basenameUc = basename.toUpperCase();
// Bazel BUILD files from npm distribution would have been renamed earlier with a _ prefix so
// Bazel BUILD files from npm distribution of rules_nodejs 1.x
// would have been renamed before publishing 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));
Expand Down Expand Up @@ -446,9 +404,11 @@ 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' ||
// Also look for the "hidden" version, from older npm packages published
// by rules_nodejs 1.x
fileUc === '_BUILD' || fileUc === '_BUILD.BAZEL') {
return true;
}
}
Expand Down Expand Up @@ -477,20 +437,7 @@ function findPackages(p = 'node_modules') {
.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')))
pkgs.push(parsePackage(f), ...findPackages(path.posix.join(f, 'node_modules')));
});

const scopes = listing.filter(f => f.startsWith('@'))
Expand Down Expand Up @@ -525,7 +472,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;
Expand Down Expand Up @@ -559,11 +506,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;
}

Expand Down Expand Up @@ -818,7 +760,9 @@ 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') {
// NB: we don't bother filtering out _BUILD or _BUILD.bazel files
// that might have been published by rules_nodejs 1.x
if (basenameUc === 'BUILD' || basenameUc === 'BUILD.BAZEL') {
return false;
}
return true;
Expand Down
51 changes: 8 additions & 43 deletions internal/npm_install/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,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();
}
Expand Down Expand Up @@ -52,32 +51,6 @@ function flattenDependencies(pkgs) {
pkgs.forEach(pkg => pkgsMap.set(pkg._dir, pkg));
pkgs.forEach(pkg => flattenPkgDependencies(pkg, pkg, pkgsMap));
}
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 (!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 {
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;
});
}
function generateRootBuildFile(pkgs) {
let pkgFilesStarlark = '';
if (pkgs.length) {
Expand Down Expand Up @@ -268,7 +241,8 @@ function listFiles(rootDir, subDir = '') {
function hasRootBuildFile(pkg, rootPath) {
for (const file of pkg._files) {
const fileUc = path.relative(rootPath, file).toUpperCase();
if (fileUc === '_BUILD' || fileUc === '_BUILD.BAZEL') {
if (fileUc === 'BUILD' || fileUc === 'BUILD.BAZEL' ||
fileUc === '_BUILD' || fileUc === '_BUILD.BAZEL') {
return true;
}
}
Expand All @@ -286,14 +260,7 @@ function findPackages(p = 'node_modules') {
.map(f => path.posix.join(p, f))
.filter(f => isDirectory(f));
packages.forEach(f => {
let hide = true;
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')));
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))
Expand All @@ -313,7 +280,7 @@ function findScopes() {
.map(f => f.replace(/^node_modules\//, ''));
return scopes;
}
function parsePackage(p, hide = true) {
function parsePackage(p) {
const packageJson = path.posix.join(p, 'package.json');
const stripBom = (s) => s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
const pkg = isFile(packageJson) ?
Expand All @@ -326,8 +293,6 @@ function parsePackage(p, hide = true) {
pkg._files = listFiles(p);
pkg._runfiles = pkg._files.filter((f) => !/[^\x21-\x7E]/.test(f));
pkg._dependencies = [];
if (hide)
hideBazelFiles(pkg);
return pkg;
}
exports.parsePackage = parsePackage;
Expand Down Expand Up @@ -460,7 +425,7 @@ function filterFiles(files, exts = []) {
}
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;
Expand Down
39 changes: 0 additions & 39 deletions internal/npm_install/npm_install.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 5d1d006

Please sign in to comment.