From 362ba699fddea6e907ff0c3e25558e121b26b669 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 24 Mar 2020 19:15:24 -0700 Subject: [PATCH] refactor(rollup): simplify by removing the .from_src.bzl Also enable the new rollup worker mode for e2e and packages/rollup tests. This is a first step towards making it the default in rules_nodejs 2.0 --- e2e/webapp/BUILD.bazel | 2 ++ internal/pkg_npm/test/BUILD.bazel | 2 +- internal/pkg_web/test/BUILD.bazel | 2 +- internal/pkg_web/test2/BUILD.bazel | 2 +- packages/labs/test/grpc_web/BUILD.bazel | 2 +- packages/node-patches/BUILD.bazel | 2 +- .../protractor/test/protractor-2/BUILD.bazel | 2 +- packages/rollup/BUILD.bazel | 3 +++ packages/rollup/src/BUILD.bazel | 2 +- packages/rollup/src/index.from_src.bzl | 25 ------------------- packages/rollup/src/index.js | 2 +- packages/rollup/src/package.json | 3 --- packages/rollup/src/rollup_bundle.bzl | 13 +++++----- .../rollup/test/code_splitting/BUILD.bazel | 3 ++- packages/rollup/test/integration/BUILD.bazel | 3 ++- .../test/multiple_entry_points/BUILD.bazel | 3 ++- packages/rollup/test/plugins/BUILD.bazel | 3 ++- packages/rollup/test/sourcemaps/BUILD.bazel | 3 ++- .../rollup/test/version_stamp/BUILD.bazel | 3 ++- 19 files changed, 32 insertions(+), 48 deletions(-) delete mode 100644 packages/rollup/src/index.from_src.bzl diff --git a/e2e/webapp/BUILD.bazel b/e2e/webapp/BUILD.bazel index ab1216eddf..1b0c57d9e3 100644 --- a/e2e/webapp/BUILD.bazel +++ b/e2e/webapp/BUILD.bazel @@ -7,6 +7,8 @@ rollup_bundle( srcs = ["strings.js"], entry_point = "app.js", output_dir = True, + # Enable experimental faster builds with Bazel workers + supports_workers = True, ) terser_minified( diff --git a/internal/pkg_npm/test/BUILD.bazel b/internal/pkg_npm/test/BUILD.bazel index 516def1141..4b743e1ea7 100644 --- a/internal/pkg_npm/test/BUILD.bazel +++ b/internal/pkg_npm/test/BUILD.bazel @@ -1,6 +1,6 @@ load("@build_bazel_rules_nodejs//:index.bzl", "pkg_npm") load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("@npm_bazel_typescript//:index.from_src.bzl", "ts_library") load("//internal/node:context.bzl", "node_context_data") load("//third_party/github.com/bazelbuild/bazel-skylib:rules/write_file.bzl", "write_file") diff --git a/internal/pkg_web/test/BUILD.bazel b/internal/pkg_web/test/BUILD.bazel index 003053b562..ed146b9eb4 100644 --- a/internal/pkg_web/test/BUILD.bazel +++ b/internal/pkg_web/test/BUILD.bazel @@ -1,7 +1,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "pkg_web") load("@npm//@babel/cli:index.bzl", "babel") load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("@npm_bazel_terser//:index.from_src.bzl", "terser_minified") load("@npm_bazel_typescript//:index.from_src.bzl", "ts_library") diff --git a/internal/pkg_web/test2/BUILD.bazel b/internal/pkg_web/test2/BUILD.bazel index 0458b76569..96121038b3 100644 --- a/internal/pkg_web/test2/BUILD.bazel +++ b/internal/pkg_web/test2/BUILD.bazel @@ -1,7 +1,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "pkg_web") load("@npm//@babel/cli:index.bzl", "babel") load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("@npm_bazel_terser//:index.from_src.bzl", "terser_minified") rollup_bundle( diff --git a/packages/labs/test/grpc_web/BUILD.bazel b/packages/labs/test/grpc_web/BUILD.bazel index 204bca20f1..eaa77c4f5e 100644 --- a/packages/labs/test/grpc_web/BUILD.bazel +++ b/packages/labs/test/grpc_web/BUILD.bazel @@ -1,6 +1,6 @@ load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") load("@npm_bazel_karma//:index.from_src.bzl", "karma_web_test_suite") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("@npm_bazel_typescript//:index.from_src.bzl", "ts_library") # This test checks that the protos can be resolved in a nodejs environment diff --git a/packages/node-patches/BUILD.bazel b/packages/node-patches/BUILD.bazel index 81a7194ab0..5e218ed028 100644 --- a/packages/node-patches/BUILD.bazel +++ b/packages/node-patches/BUILD.bazel @@ -13,7 +13,7 @@ # limitations under the License. load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("@npm_node_patches//typescript:index.bzl", "tsc") package(default_visibility = ["//:__subpackages__"]) diff --git a/packages/protractor/test/protractor-2/BUILD.bazel b/packages/protractor/test/protractor-2/BUILD.bazel index f62a7b03cf..70075aaef1 100644 --- a/packages/protractor/test/protractor-2/BUILD.bazel +++ b/packages/protractor/test/protractor-2/BUILD.bazel @@ -1,6 +1,6 @@ load("@npm//http-server:index.bzl", "http_server") load("@npm_bazel_protractor//:index.from_src.bzl", "protractor_web_test_suite") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("@npm_bazel_terser//:index.from_src.bzl", "terser_minified") load("@npm_bazel_typescript//:index.bzl", "ts_config") load("@npm_bazel_typescript//:index.from_src.bzl", "ts_devserver", "ts_library") diff --git a/packages/rollup/BUILD.bazel b/packages/rollup/BUILD.bazel index a35dc9e4af..135e8a9fac 100644 --- a/packages/rollup/BUILD.bazel +++ b/packages/rollup/BUILD.bazel @@ -59,6 +59,9 @@ pkg_npm( "@npm_bazel_rollup//:package_contents", ], build_file_content = """exports_files(["rollup.config.js"])""", + substitutions = { + "//:rollup-worker-local": "@npm//@bazel/rollup/bin:rollup-worker", + }, vendor_external = [ "npm_bazel_rollup", ], diff --git a/packages/rollup/src/BUILD.bazel b/packages/rollup/src/BUILD.bazel index 972d9215cb..2e7e284947 100644 --- a/packages/rollup/src/BUILD.bazel +++ b/packages/rollup/src/BUILD.bazel @@ -51,7 +51,7 @@ filegroup( ) nodejs_binary( - name = "rollup-worker", + name = "rollup-worker-local", data = [ # NOTE: we ought to link to the original location instead of copying # "@build_bazel_rules_nodejs//packages/worker:npm_package", diff --git a/packages/rollup/src/index.from_src.bzl b/packages/rollup/src/index.from_src.bzl deleted file mode 100644 index 7fdf0a6041..0000000000 --- a/packages/rollup/src/index.from_src.bzl +++ /dev/null @@ -1,25 +0,0 @@ -# Copyright 2019 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""Defaults for usage without @npm//@bazel/rollup -""" - -load(":index.bzl", _rollup_bundle = "rollup_bundle") - -def rollup_bundle(**kwargs): - _rollup_bundle( - # Override this attribute to point to our local one, not @npm - rollup_worker_bin = "@npm_bazel_rollup//:rollup-worker", - **kwargs - ) diff --git a/packages/rollup/src/index.js b/packages/rollup/src/index.js index e13787f2cd..a31693b4bd 100644 --- a/packages/rollup/src/index.js +++ b/packages/rollup/src/index.js @@ -22,7 +22,7 @@ const PID = process.pid; let worker; try { - worker = require('@bazel/worker'); + worker = require('./worker'); } catch { // TODO: rely on the linker to link the first-party package const helper = process.env['BAZEL_NODE_RUNFILES_HELPER']; diff --git a/packages/rollup/src/package.json b/packages/rollup/src/package.json index 0b57af4728..e63bdcac83 100644 --- a/packages/rollup/src/package.json +++ b/packages/rollup/src/package.json @@ -25,8 +25,5 @@ "npm_bazel_rollup": { "rootPath": "." } - }, - "dependencies": { - "@bazel/worker": "0.0.0-PLACEHOLDER" } } \ No newline at end of file diff --git a/packages/rollup/src/rollup_bundle.bzl b/packages/rollup/src/rollup_bundle.bzl index 5a75b37830..11be430438 100644 --- a/packages/rollup/src/rollup_bundle.bzl +++ b/packages/rollup/src/rollup_bundle.bzl @@ -182,7 +182,8 @@ Otherwise, the outputs are assumed to be a single file. doc = "Internal use only", executable = True, cfg = "host", - default = "@npm//@bazel/rollup:rollup-worker", + # NB: will be substituted with "@npm//@bazel/rollup/bin:rollup-worker" when the pkg_npm target is built + default = "//:rollup-worker-local", ), "silent": attr.bool( doc = """Whether to execute the rollup binary with the --silent flag, defaults to False. @@ -201,11 +202,11 @@ Passed to the [`--sourcemap` option](https://github.com/rollup/rollup/blob/maste values = ["inline", "hidden", "true", "false"], ), "supports_workers": attr.bool( - doc = """Intended for internal use only. + doc = """Experimental! Use only with caution. -Allows you to disable the Bazel Worker strategy for this library. -Typically used together with the "rollup_bin" setting when using a -non-worker aware binary.""", +Allows you to enable the Bazel Worker strategy for this library. +When enabled, this rule invokes the "rollup_worker_bin" +worker aware binary rather than "rollup_bin".""", default = False, ), "deps": attr.label_list( @@ -383,7 +384,7 @@ def _rollup_bundle(ctx): if ctx.attr.supports_workers: executable = "rollup_worker_bin" - execution_requirements = {"supports-workers": str(int(ctx.attr.supports_workers))} + execution_requirements["supports-workers"] = str(int(ctx.attr.supports_workers)) run_node( ctx, diff --git a/packages/rollup/test/code_splitting/BUILD.bazel b/packages/rollup/test/code_splitting/BUILD.bazel index 256f3cc22d..0316fe3e66 100644 --- a/packages/rollup/test/code_splitting/BUILD.bazel +++ b/packages/rollup/test/code_splitting/BUILD.bazel @@ -1,5 +1,5 @@ load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") rollup_bundle( name = "bundle", @@ -10,6 +10,7 @@ rollup_bundle( # [!] Error: You must set output.dir instead of output.file when generating multiple chunks. # which will confuse users because it references output.dir instead of output_dir output_dir = True, + supports_workers = True, ) jasmine_node_test( diff --git a/packages/rollup/test/integration/BUILD.bazel b/packages/rollup/test/integration/BUILD.bazel index 3c2d00332b..3f6177d424 100644 --- a/packages/rollup/test/integration/BUILD.bazel +++ b/packages/rollup/test/integration/BUILD.bazel @@ -1,6 +1,6 @@ load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary", "npm_package_bin") load("@build_bazel_rules_nodejs//internal/golden_file_test:golden_file_test.bzl", "golden_file_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") _BUNDLE_FORMATS = [ "amd", @@ -23,6 +23,7 @@ _BUNDLE_FORMATS = [ entry_point = "foo.js", format = format, sourcemap = "true", + supports_workers = True, deps = [ "//%s/fum:fumlib" % package_name(), "//%s/foo:foo_lib" % package_name(), diff --git a/packages/rollup/test/multiple_entry_points/BUILD.bazel b/packages/rollup/test/multiple_entry_points/BUILD.bazel index 72f97b74ca..adf2289bb8 100644 --- a/packages/rollup/test/multiple_entry_points/BUILD.bazel +++ b/packages/rollup/test/multiple_entry_points/BUILD.bazel @@ -1,5 +1,5 @@ load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") rollup_bundle( name = "chunks", @@ -8,6 +8,7 @@ rollup_bundle( "entry2.js": "two", }, output_dir = True, + supports_workers = True, ) jasmine_node_test( diff --git a/packages/rollup/test/plugins/BUILD.bazel b/packages/rollup/test/plugins/BUILD.bazel index 574d9cff61..dbfa295263 100644 --- a/packages/rollup/test/plugins/BUILD.bazel +++ b/packages/rollup/test/plugins/BUILD.bazel @@ -1,5 +1,5 @@ load("@build_bazel_rules_nodejs//internal/golden_file_test:golden_file_test.bzl", "golden_file_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") rollup_bundle( name = "plugins", @@ -7,6 +7,7 @@ rollup_bundle( config_file = "rollup.config.js", entry_point = "input.js", sourcemap = "false", + supports_workers = True, deps = ["@npm//rollup-plugin-json"], ) diff --git a/packages/rollup/test/sourcemaps/BUILD.bazel b/packages/rollup/test/sourcemaps/BUILD.bazel index bb8b6d0062..de8182174e 100644 --- a/packages/rollup/test/sourcemaps/BUILD.bazel +++ b/packages/rollup/test/sourcemaps/BUILD.bazel @@ -1,5 +1,5 @@ load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") rollup_bundle( name = "bundle", @@ -12,6 +12,7 @@ rollup_bundle( # Use the desugared form to assert that it works entry_points = {"input.js": "bundle"}, sourcemap = "true", + supports_workers = True, ) jasmine_node_test( diff --git a/packages/rollup/test/version_stamp/BUILD.bazel b/packages/rollup/test/version_stamp/BUILD.bazel index 4398c6ca30..c05c20b02b 100644 --- a/packages/rollup/test/version_stamp/BUILD.bazel +++ b/packages/rollup/test/version_stamp/BUILD.bazel @@ -1,5 +1,5 @@ load("@build_bazel_rules_nodejs//internal/golden_file_test:golden_file_test.bzl", "golden_file_test") -load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") load("//internal/node:context.bzl", "node_context_data") # Force stamping behavior even in builds without --stamp config @@ -15,6 +15,7 @@ rollup_bundle( entry_point = "input.js", node_context_data = ":force_stamp", sourcemap = "false", + supports_workers = True, ) golden_file_test(