From 8a931d8d40543c451d1a273e230b6007cfef26d5 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Mon, 30 Dec 2019 16:49:30 -0800 Subject: [PATCH] fix: don't bake COMPILATION_MODE into launcher as exported environment var This approach is incorrect as tools that are build for host such as "rollup_bin" in the rollup_bundle rule will see the --host_compilation_mode and not the --compilation_mode configuration value when built by Bazel as their cfg in the rule is correctly set to "host". ``` "rollup_bin": attr.label( doc = "Target that executes the rollup binary", executable = True, cfg = "host", default = "@npm//rollup/bin:rollup", ), ``` The correct approach is to pass COMPILATION_MODE via the actions.run env attribute. For the rollup_bundle rollup_bin tool, this is passed via run_node: ``` def run_node(ctx, inputs, arguments, executable, **kwargs): """Helper to replace ctx.actions.run This calls node programs with a node_modules directory in place""" .... # Forward the COMPILATION_MODE to node process as an environment variable env = kwargs.pop("env", {}) if "COMPILATION_MODE" not in env.keys(): env["COMPILATION_MODE"] = ctx.var["COMPILATION_MODE"] ctx.actions.run( inputs = inputs + extra_inputs, arguments = arguments, executable = exec_exec, env = env, **kwargs ) ``` for other tools such the terser_minified terser_bin tool, this is done directly in ctx.actions.run: ``` ctx.actions.run( inputs = inputs, outputs = outputs, executable = ctx.executable.terser_bin, arguments = [args], env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, progress_message = "Minifying JavaScript %s [terser]" % (outputs[0].short_path), ) ``` Accordingly `COMPILATION_MODE` is removed from default_env_vars so it is not baked into the launcher.sh. `DEBUG` is also removed as a DEBUG define should not be default be passed to all nodejs_binary targets as it may unexpectantly affect the build outputs and if you do want to pass --define=DEBUG=1 to a tool such as the bazel_integration_test test_runner.js, also forwarding it to all nodejs_binaries will result in a massive cache invalidation. --- common.bazelrc | 4 ++-- examples/parcel/parcel.bzl | 1 + examples/worker/uses_workers.bzl | 1 + internal/node/node.bzl | 13 ++----------- internal/pkg_web/pkg_web.bzl | 4 +++- internal/providers/node_runtime_deps_info.bzl | 6 ++++++ packages/labs/src/protobufjs/ts_proto_library.bzl | 10 +++++++--- packages/labs/src/webpack/webpack_bundle.bzl | 1 + packages/rollup/test/version_stamp/BUILD.bazel | 1 + packages/rollup/test/version_stamp/golden_debug.js_ | 7 +++++++ packages/rollup/test/version_stamp/rollup.config.js | 5 +++++ packages/terser/src/terser_minified.bzl | 1 + packages/typescript/src/internal/build_defs.bzl | 1 + 13 files changed, 38 insertions(+), 17 deletions(-) create mode 100644 packages/rollup/test/version_stamp/golden_debug.js_ diff --git a/common.bazelrc b/common.bazelrc index 0b5ddf2c8c..19e6d72d13 100644 --- a/common.bazelrc +++ b/common.bazelrc @@ -47,8 +47,8 @@ test --test_output=errors # `nodejs_binary` and `nodejs_test` via the default value of the `default_env_vars` attribute of those rules. # --compilation_mode=dbg # Rules may change their build outputs if the compilation mode is set to dbg. For example, -# mininfiers such as terser may make their output more human readable when this is set. `COMPILATION_MODE` will be passed to -# `nodejs_binary` and `nodejs_test` via the default value of the `default_env_vars` attribute of those rules. +# mininfiers such as terser may make their output more human readable when this is set. Rules will pass `COMPILATION_MODE` +# to `nodejs_binary` executables via the actions.run env attribute. # See https://docs.bazel.build/versions/master/user-manual.html#flag--compilation_mode for more details. test:debug --test_output=streamed --test_strategy=exclusive --test_timeout=9999 --nocache_test_results --define=VERBOSE_LOGS=1 # Use bazel run with `--config=debug` to turn on the NodeJS inspector agent. diff --git a/examples/parcel/parcel.bzl b/examples/parcel/parcel.bzl index d1eb224cd6..458dbffd12 100644 --- a/examples/parcel/parcel.bzl +++ b/examples/parcel/parcel.bzl @@ -36,6 +36,7 @@ def _parcel_impl(ctx): executable = ctx.executable.parcel, outputs = [ctx.outputs.bundle, ctx.outputs.sourcemap], arguments = args, + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, progress_message = "Bundling JavaScript %s [parcel]" % ctx.outputs.bundle.short_path, ) return [DefaultInfo()] diff --git a/examples/worker/uses_workers.bzl b/examples/worker/uses_workers.bzl index 0456cdde57..609d940b70 100644 --- a/examples/worker/uses_workers.bzl +++ b/examples/worker/uses_workers.bzl @@ -22,6 +22,7 @@ def _work(ctx): execution_requirements = {"supports-workers": "1"}, # The user can explicitly set the execution strategy mnemonic = "DoWork", + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, ) return [DefaultInfo(files = depset([output]))] diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 2c0bfefde4..a2450c3532 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -174,13 +174,6 @@ def _nodejs_binary_impl(ctx): if k in ctx.var.keys(): env_vars += "export %s=\"%s\"\n" % (k, ctx.var[k]) - if "DEBUG" in ctx.var and ctx.var["COMPILATION_MODE"] != "dbg": - print(""" - WARNING: `--define DEBUG` no longer triggers a debugging build, use - `--compilation_mode=dbg` instead. - - """) - expected_exit_code = 0 if hasattr(ctx.attr, "expected_exit_code"): expected_exit_code = ctx.attr.expected_exit_code @@ -287,12 +280,10 @@ without losing the defaults that should be set in most cases. The set of default environment variables is: -- `COMPILATION_MODE`: rules use this environment variable to produce optimized (eg. mangled and minimized) or debugging output -- `VERBOSE_LOGS`: rules use this environment variable to turn on debug output in their logs -- `DEBUG`: used by some npm packages to print debugging logs +- `VERBOSE_LOGS`: use by some rules & tools to turn on debug output in their logs - `NODE_DEBUG`: used by node.js itself to print more logs """, - default = ["COMPILATION_MODE", "VERBOSE_LOGS", "DEBUG", "NODE_DEBUG"], + default = ["VERBOSE_LOGS", "NODE_DEBUG"], ), "entry_point": attr.label( doc = """The script which should be executed first, usually containing a main function. diff --git a/internal/pkg_web/pkg_web.bzl b/internal/pkg_web/pkg_web.bzl index 9768b3bf83..41b72a314b 100644 --- a/internal/pkg_web/pkg_web.bzl +++ b/internal/pkg_web/pkg_web.bzl @@ -32,7 +32,7 @@ _ATTRS = { ), } -def move_files(output_name, files, action_factory, assembler, root_paths): +def move_files(output_name, files, action_factory, var, assembler, root_paths): """Moves files into an output directory Args: @@ -58,6 +58,7 @@ def move_files(output_name, files, action_factory, assembler, root_paths): executable = assembler, arguments = [args], execution_requirements = {"local": "1"}, + env = {"COMPILATION_MODE": var["COMPILATION_MODE"]}, ) return depset([www_dir]) @@ -90,6 +91,7 @@ def _impl(ctx): ctx.label.name, ctx.files.srcs, ctx.actions, + ctx.var, ctx.executable._assembler, root_paths, ) diff --git a/internal/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl index c8d358bfc1..d9cd60a590 100644 --- a/internal/providers/node_runtime_deps_info.bzl +++ b/internal/providers/node_runtime_deps_info.bzl @@ -63,9 +63,15 @@ def run_node(ctx, inputs, arguments, executable, **kwargs): # To access runfiles, you must use a runfiles helper in the program instead add_arg(arguments, "--nobazel_patch_module_resolver") + # Forward the COMPILATION_MODE to node process as an environment variable + env = kwargs.pop("env", {}) + if "COMPILATION_MODE" not in env.keys(): + env["COMPILATION_MODE"] = ctx.var["COMPILATION_MODE"] + ctx.actions.run( inputs = inputs + extra_inputs, arguments = arguments, executable = exec_exec, + env = env, **kwargs ) diff --git a/packages/labs/src/protobufjs/ts_proto_library.bzl b/packages/labs/src/protobufjs/ts_proto_library.bzl index 2c825e0116..8be4d76fd1 100644 --- a/packages/labs/src/protobufjs/ts_proto_library.bzl +++ b/packages/labs/src/protobufjs/ts_proto_library.bzl @@ -15,7 +15,7 @@ load("@build_bazel_rules_nodejs//:providers.bzl", "DeclarationInfo", "JSEcmaScriptModuleInfo", "JSNamedModuleInfo") -def _run_pbjs(actions, executable, output_name, proto_files, suffix = ".js", wrap = "amd", amd_name = ""): +def _run_pbjs(actions, executable, var, output_name, proto_files, suffix = ".js", wrap = "amd", amd_name = ""): js_file = actions.declare_file(output_name + suffix) # Create an intermediate file so that we can do some manipulation of the @@ -36,6 +36,7 @@ def _run_pbjs(actions, executable, output_name, proto_files, suffix = ".js", wra inputs = proto_files, outputs = [js_tmpl_file], arguments = [args], + env = {"COMPILATION_MODE": var["COMPILATION_MODE"]}, ) actions.expand_template( @@ -51,7 +52,7 @@ def _run_pbjs(actions, executable, output_name, proto_files, suffix = ".js", wra ) return js_file -def _run_pbts(actions, executable, js_file): +def _run_pbts(actions, executable, var, js_file): ts_file = actions.declare_file(js_file.basename[:-len(".js")] + ".d.ts") # Reference of arguments: @@ -66,6 +67,7 @@ def _run_pbts(actions, executable, js_file): inputs = [js_file], outputs = [ts_file], arguments = [args], + env = {"COMPILATION_MODE": var["COMPILATION_MODE"]}, ) return ts_file @@ -88,6 +90,7 @@ def _ts_proto_library(ctx): js_es5 = _run_pbjs( ctx.actions, ctx.executable, + ctx.var, output_name, sources, amd_name = "/".join([p for p in [ @@ -98,6 +101,7 @@ def _ts_proto_library(ctx): js_es6 = _run_pbjs( ctx.actions, ctx.executable, + ctx.var, output_name, sources, suffix = ".mjs", @@ -105,7 +109,7 @@ def _ts_proto_library(ctx): ) # pbts doesn't understand '.mjs' extension so give it the es5 file - dts = _run_pbts(ctx.actions, ctx.executable, js_es5) + dts = _run_pbts(ctx.actions, ctx.executable, ctx.var, js_es5) # Return a structure that is compatible with the deps[] of a ts_library. declarations = depset([dts]) diff --git a/packages/labs/src/webpack/webpack_bundle.bzl b/packages/labs/src/webpack/webpack_bundle.bzl index 6b2184da8a..6de1c19b8d 100644 --- a/packages/labs/src/webpack/webpack_bundle.bzl +++ b/packages/labs/src/webpack/webpack_bundle.bzl @@ -39,6 +39,7 @@ def _webpack_bundle(ctx): outputs = [ctx.outputs.bundle, ctx.outputs.sourcemap], arguments = [args], progress_message = "Bundling JavaScript %s [webpack]" % ctx.outputs.bundle.path, + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, ) return [DefaultInfo()] diff --git a/packages/rollup/test/version_stamp/BUILD.bazel b/packages/rollup/test/version_stamp/BUILD.bazel index 7c3f4cf1c3..4398c6ca30 100644 --- a/packages/rollup/test/version_stamp/BUILD.bazel +++ b/packages/rollup/test/version_stamp/BUILD.bazel @@ -22,4 +22,5 @@ golden_file_test( # Leave off the ".js" extension to test that it's the default output actual = "version_stamp", golden = "golden.js_", + golden_debug = "golden_debug.js_", ) diff --git a/packages/rollup/test/version_stamp/golden_debug.js_ b/packages/rollup/test/version_stamp/golden_debug.js_ new file mode 100644 index 0000000000..d336f784a4 --- /dev/null +++ b/packages/rollup/test/version_stamp/golden_debug.js_ @@ -0,0 +1,7 @@ +/** + * @license A dummy license banner that goes at the top of the file. + * This is version v1.2.3_debug + */ + +// License banner with version stamp will appear here +console.error('stamp'); diff --git a/packages/rollup/test/version_stamp/rollup.config.js b/packages/rollup/test/version_stamp/rollup.config.js index 6442d3f7c7..0f79697862 100644 --- a/packages/rollup/test/version_stamp/rollup.config.js +++ b/packages/rollup/test/version_stamp/rollup.config.js @@ -1,3 +1,5 @@ +const DEBUG = process.env['COMPILATION_MODE'] === 'dbg'; + // Parse the stamp file produced by Bazel from the version control system let version = ''; if (bazel_stamp_file) { @@ -8,6 +10,9 @@ if (bazel_stamp_file) { // Don't assume BUILD_SCM_VERSION exists if (versionTag) { version = 'v' + versionTag.split(' ')[1].trim(); + if (DEBUG) { + version += '_debug'; + } } } diff --git a/packages/terser/src/terser_minified.bzl b/packages/terser/src/terser_minified.bzl index 8b3e7609dd..475ec06160 100644 --- a/packages/terser/src/terser_minified.bzl +++ b/packages/terser/src/terser_minified.bzl @@ -170,6 +170,7 @@ def _terser(ctx): outputs = outputs, executable = ctx.executable.terser_bin, arguments = [args], + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, progress_message = "Minifying JavaScript %s [terser]" % (outputs[0].short_path), ) diff --git a/packages/typescript/src/internal/build_defs.bzl b/packages/typescript/src/internal/build_defs.bzl index e917f199d3..0b15f19d65 100644 --- a/packages/typescript/src/internal/build_defs.bzl +++ b/packages/typescript/src/internal/build_defs.bzl @@ -156,6 +156,7 @@ def _compile_action(ctx, inputs, outputs, tsconfig_file, node_opts, description execution_requirements = { "supports-workers": str(int(ctx.attr.supports_workers)), }, + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, ) # Enable the replay_params in case an aspect needs to re-build this library.