Skip to content

Commit

Permalink
fix: don't bake COMPILATION_MODE into launcher as exported environmen…
Browse files Browse the repository at this point in the history
…t 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.
  • Loading branch information
gregmagolan committed Jan 2, 2020
1 parent 0abc7f2 commit 8a931d8
Show file tree
Hide file tree
Showing 13 changed files with 38 additions and 17 deletions.
4 changes: 2 additions & 2 deletions common.bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions examples/parcel/parcel.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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()]
Expand Down
1 change: 1 addition & 0 deletions examples/worker/uses_workers.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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]))]
Expand Down
13 changes: 2 additions & 11 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion internal/pkg_web/pkg_web.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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])

Expand Down Expand Up @@ -90,6 +91,7 @@ def _impl(ctx):
ctx.label.name,
ctx.files.srcs,
ctx.actions,
ctx.var,
ctx.executable._assembler,
root_paths,
)
Expand Down
6 changes: 6 additions & 0 deletions internal/providers/node_runtime_deps_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
10 changes: 7 additions & 3 deletions packages/labs/src/protobufjs/ts_proto_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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:
Expand All @@ -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

Expand All @@ -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 [
Expand All @@ -98,14 +101,15 @@ def _ts_proto_library(ctx):
js_es6 = _run_pbjs(
ctx.actions,
ctx.executable,
ctx.var,
output_name,
sources,
suffix = ".mjs",
wrap = "es6",
)

# 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])
Expand Down
1 change: 1 addition & 0 deletions packages/labs/src/webpack/webpack_bundle.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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()]

Expand Down
1 change: 1 addition & 0 deletions packages/rollup/test/version_stamp/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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_",
)
7 changes: 7 additions & 0 deletions packages/rollup/test/version_stamp/golden_debug.js_
Original file line number Diff line number Diff line change
@@ -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');
5 changes: 5 additions & 0 deletions packages/rollup/test/version_stamp/rollup.config.js
Original file line number Diff line number Diff line change
@@ -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 = '<unknown>';
if (bazel_stamp_file) {
Expand All @@ -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';
}
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/terser/src/terser_minified.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)

Expand Down
1 change: 1 addition & 0 deletions packages/typescript/src/internal/build_defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 8a931d8

Please sign in to comment.