From 9fa4343aaa80e4f6e8d27774029739abaf6c7c5b Mon Sep 17 00:00:00 2001 From: Xiaoyi Shi Date: Mon, 4 Nov 2019 16:02:55 +0800 Subject: [PATCH] feat: support --compilation_mode flag --- .bazelci/presubmit.yml | 2 +- common.bazelrc | 9 +++++---- docs/Built-ins.md | 10 ++++++++-- docs/Terser.md | 8 ++++---- internal/bazel_integration_test/test_runner.js | 2 +- internal/golden_file_test/bin.js | 8 ++++---- internal/node/node.bzl | 12 ++++++++++-- internal/providers/js_providers.bzl | 4 ++-- package.json | 2 +- packages/create/index.js | 6 +++--- packages/terser/src/terser_minified.bzl | 10 +++++----- packages/terser/test/debug/BUILD.bazel | 2 +- packages/terser/test/inline_sourcemap/spec.js | 8 ++++---- packages/terser/test/sourcemap/directory_spec.js | 5 +++-- packages/terser/test/sourcemap/terser_spec.js | 3 ++- 15 files changed, 54 insertions(+), 37 deletions(-) diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index de7d4e1475..99f85bc0be 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -106,8 +106,8 @@ tasks: name: ubuntu1804_debug platform: ubuntu1804 test_flags: + - "--compilation_mode=dbg" - "--define=VERBOSE_LOGS=1" - - "--define=DEBUG=1" - "--test_tag_filters=-e2e,-examples,-no-bazelci,-no-bazelci-ubuntu,-manual" test_targets: - "//..." diff --git a/common.bazelrc b/common.bazelrc index c92fa59b1c..87363032e9 100644 --- a/common.bazelrc +++ b/common.bazelrc @@ -42,16 +42,17 @@ test --test_output=errors # --define=VERBOSE_LOGS=1 # Rules will output verbose logs if the VERBOSE_LOGS environment variable is set. `VERBOSE_LOGS` will be passed to # `nodejs_binary` and `nodejs_test` via the default value of the `default_env_vars` attribute of those rules. -# --define=DEBUG=1 -# Rules may change their build outputs if the DEBUG environment variable is set. For example, -# mininfiers such as terser may make their output more human readable when this is set. `DEBUG` will be passed to +# --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. +# 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. # The node process will break before user code starts and wait for the debugger to connect. run:debug --define=VERBOSE_LOGS=1 -- --node_options=--inspect-brk # The following option will change the build output of certain rules such as terser and may not be desirable in all cases -build:debug --define=DEBUG=1 +build:debug --compilation_mode=dbg # Turn off legacy external runfiles # This prevents accidentally depending on this feature, which Bazel will remove. diff --git a/docs/Built-ins.md b/docs/Built-ins.md index 401d9db4e2..b705f5ab77 100755 --- a/docs/Built-ins.md +++ b/docs/Built-ins.md @@ -226,10 +226,13 @@ without losing the defaults that should be set in most cases. The set of default environment variables is: -- `DEBUG`: rules use this environment variable to turn on debug information in their output artifacts +- `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 - `NODE_DEBUG`: used by node.js itself to print more logs +Note that, `DEBUG` is derived from bazel compilation mode if not present in --define. + #### `entry_point` (*[label], mandatory*): The script which should be executed first, usually containing a main function. @@ -438,10 +441,13 @@ without losing the defaults that should be set in most cases. The set of default environment variables is: -- `DEBUG`: rules use this environment variable to turn on debug information in their output artifacts +- `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 - `NODE_DEBUG`: used by node.js itself to print more logs +Note that, `DEBUG` is derived from bazel compilation mode if not present in --define. + #### `entry_point` (*[label], mandatory*): The script which should be executed first, usually containing a main function. diff --git a/docs/Terser.md b/docs/Terser.md index 6d5a29ad86..d8dd4c18eb 100755 --- a/docs/Terser.md +++ b/docs/Terser.md @@ -96,7 +96,7 @@ Bazel will make a copy of your config file, treating it as a template. If you use the magic strings `"bazel_debug"` or `"bazel_no_debug"`, these will be replaced with `true` and `false` respecting the value of the `debug` attribute -or the `--define=DEBUG=1` bazel flag. +or the `--compilation_mode=dbg` bazel flag. For example, @@ -115,8 +115,8 @@ If `config_file` isn't supplied, Bazel will use a default config file. #### `debug` (*Boolean*): Configure terser to produce more readable output. -Instead of setting this attribute, consider setting the DEBUG variable instead -bazel build --define=DEBUG=1 //my/terser:target +Instead of setting this attribute, consider using debugging compilation mode instead +bazel build --compilation_mode=dbg //my/terser:target so that it only affects the current build. @@ -126,7 +126,7 @@ so that it only affects the current build. #### `src` (*[label], mandatory*): File(s) to minify. - + Can be a .js file, a rule producing .js files as its default output, or a rule producing a directory of .js files. Note that you can pass multiple files to terser, which it will bundle together. diff --git a/internal/bazel_integration_test/test_runner.js b/internal/bazel_integration_test/test_runner.js index 71798a08bc..0a4b60e404 100644 --- a/internal/bazel_integration_test/test_runner.js +++ b/internal/bazel_integration_test/test_runner.js @@ -24,7 +24,7 @@ const fs = require('fs'); const path = require('path'); const tmp = require('tmp'); -const DEBUG = !!process.env['DEBUG']; +const DEBUG = process.env['COMPILATION_MODE'] === 'dbg'; const VERBOSE_LOGS = !!process.env['VERBOSE_LOGS']; function log(...m) { diff --git a/internal/golden_file_test/bin.js b/internal/golden_file_test/bin.js index 1f37afc717..15519d8163 100644 --- a/internal/golden_file_test/bin.js +++ b/internal/golden_file_test/bin.js @@ -3,7 +3,7 @@ const path = require('path'); function main(args) { const [mode, golden_no_debug, golden_debug, actual] = args; - const debug = !!process.env['DEBUG']; + const debug = process.env['COMPILATION_MODE'] === 'dbg'; const golden = debug ? golden_debug : golden_no_debug; const actualContents = fs.readFileSync(require.resolve(actual), 'utf-8').replace(/\r\n/g, '\n'); const goldenContents = fs.readFileSync(require.resolve(golden), 'utf-8').replace(/\r\n/g, '\n'); @@ -22,12 +22,12 @@ function main(args) { prettyDiff = prettyDiff.substr(0, 5000) + '/n...elided...'; } throw new Error(`Actual output doesn't match golden file: - + ${prettyDiff} - + Update the golden file: - bazel run ${debug ? '--define=DEBUG=1 ' : ''}${ + bazel run ${debug ? '--compilation_mode=dbg ' : ''}${ process.env['BAZEL_TARGET'].replace(/_bin$/, '')}.accept `); } else { diff --git a/internal/node/node.bzl b/internal/node/node.bzl index dba72ae965..921b757772 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -171,6 +171,13 @@ 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 @@ -303,11 +310,12 @@ without losing the defaults that should be set in most cases. The set of default environment variables is: -- `DEBUG`: rules use this environment variable to turn on debug information in their output artifacts +- `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 - `NODE_DEBUG`: used by node.js itself to print more logs """, - default = ["DEBUG", "VERBOSE_LOGS", "NODE_DEBUG"], + default = ["COMPILATION_MODE", "VERBOSE_LOGS", "DEBUG", "NODE_DEBUG"], ), "entry_point": attr.label( doc = """The script which should be executed first, usually containing a main function. diff --git a/internal/providers/js_providers.bzl b/internal/providers/js_providers.bzl index 25646afdc9..4c52b13451 100644 --- a/internal/providers/js_providers.bzl +++ b/internal/providers/js_providers.bzl @@ -29,8 +29,8 @@ subscribes to these by having a (possibly transitive) dependency on the publishe Debug output is considered orthogonal to these providers. Any output may or may not have user debugging affordances provided, such as readable minification. -We expect that rules will have a boolean `debug` attribute, and/or accept the `DEBUG` -environment variable. +We expect that rules will have a boolean `debug` attribute, and/or accept the +`COMPILATION_MODE` environment variable. Note that this means a given build either produces debug or non-debug output. If users really need to produce both in a single build, they'll need two rules with differing 'debug' attributes. diff --git a/package.json b/package.json index dcb93658ec..0d56e18e03 100644 --- a/package.json +++ b/package.json @@ -83,7 +83,7 @@ "test_e2e": "bazel --host_jvm_args=-Xms256m --host_jvm_args=-Xmx1280m test --test_tag_filters=e2e --local_resources=792,1.0,1.0 --test_arg=--local_resources=13288,1.0,1.0 ...", "test_examples": "bazel --host_jvm_args=-Xms256m --host_jvm_args=-Xmx1280m test --test_tag_filters=examples --local_resources=792,1.0,1.0 --test_arg=--local_resources=13288,1.0,1.0 ...", "run_integration_test": "bazel --host_jvm_args=-Xms256m --host_jvm_args=-Xmx1280m run --local_resources=792,1.0,1.0 --test_arg=--local_resources=13288,1.0,1.0", - "run_integration_test_debug": "yarn run_integration_test --define=DEBUG=1", + "run_integration_test_debug": "yarn run_integration_test --compilation_mode=dbg", "test_all": "./scripts/test_all.sh", "clean_all": "./scripts/clean_all.sh", "// Unchecked warnings": "The following warnings are not checked as disabling them locally is broken", diff --git a/packages/create/index.js b/packages/create/index.js index f4bb573b5c..d5cecb9c49 100644 --- a/packages/create/index.js +++ b/packages/create/index.js @@ -2,7 +2,7 @@ const fs = require('fs'); const path = require('path'); -const DEBUG = !!process.env['DEBUG']; +const DEBUG = process.env['COMPILATION_MODE'] === 'dbg'; /** * Detect if the user ran `yarn create @bazel` so we can default @@ -26,7 +26,7 @@ function validateWorkspaceName(name, error) { return true; } error(`ERROR: ${name} is not a valid Bazel workspace name. - + A workspace name must start with a letter and can contain letters, numbers, and underscores (this is to maximize the number of languages for which this string can be a valid package/module name). It should describe the project in reverse-DNS form, with elements separated by underscores. @@ -167,7 +167,7 @@ install_bazel_dependencies()`; if (args['typescript']) { workspaceContent += ` -# Setup TypeScript toolchain +# Setup TypeScript toolchain load("@npm_bazel_typescript//:index.bzl", "ts_setup_workspace") ts_setup_workspace()`; } diff --git a/packages/terser/src/terser_minified.bzl b/packages/terser/src/terser_minified.bzl index 8c3e5aaab1..8b3e7609dd 100644 --- a/packages/terser/src/terser_minified.bzl +++ b/packages/terser/src/terser_minified.bzl @@ -35,7 +35,7 @@ If the input is a directory, then the output will also be a directory, named aft _TERSER_ATTRS = { "src": attr.label( doc = """File(s) to minify. - + Can be a .js file, a rule producing .js files as its default output, or a rule producing a directory of .js files. Note that you can pass multiple files to terser, which it will bundle together. @@ -62,7 +62,7 @@ Bazel will make a copy of your config file, treating it as a template. If you use the magic strings `"bazel_debug"` or `"bazel_no_debug"`, these will be replaced with `true` and `false` respecting the value of the `debug` attribute -or the `--define=DEBUG=1` bazel flag. +or the `--compilation_mode=dbg` bazel flag. For example, @@ -85,8 +85,8 @@ If `config_file` isn't supplied, Bazel will use a default config file. "debug": attr.bool( doc = """Configure terser to produce more readable output. -Instead of setting this attribute, consider setting the DEBUG variable instead -bazel build --define=DEBUG=1 //my/terser:target +Instead of setting this attribute, consider using debugging compilation mode instead +bazel build --compilation_mode=dbg //my/terser:target so that it only affects the current build. """, ), @@ -128,7 +128,7 @@ def _terser(ctx): args.add_all([s.path for s in sources]) args.add_all(["--output", outputs[0].path]) - debug = ctx.attr.debug or "DEBUG" in ctx.var.keys() + debug = ctx.attr.debug or ctx.var["COMPILATION_MODE"] == "dbg" if debug: args.add("--debug") args.add("--beautify") diff --git a/packages/terser/test/debug/BUILD.bazel b/packages/terser/test/debug/BUILD.bazel index c1f50a4b0c..09ddcdc437 100644 --- a/packages/terser/test/debug/BUILD.bazel +++ b/packages/terser/test/debug/BUILD.bazel @@ -20,7 +20,7 @@ terser_minified( src = "input.js", sourcemap = False, # Don't specify debug = True - # Instead we'll run the test with --define=DEBUG=1 + # Instead we'll run the test with --compilation_mode=dbg ) golden_file_test( diff --git a/packages/terser/test/inline_sourcemap/spec.js b/packages/terser/test/inline_sourcemap/spec.js index 9d8b2ec076..77dc3b6246 100644 --- a/packages/terser/test/inline_sourcemap/spec.js +++ b/packages/terser/test/inline_sourcemap/spec.js @@ -2,16 +2,16 @@ const fs = require('fs'); const path = require('path'); const sm = require('source-map'); const {runfiles} = require('build_bazel_rules_nodejs/internal/linker'); -const os = require('os'); +const DEBUG = process.env['COMPILATION_MODE'] === 'dbg'; describe('terser sourcemap handling', () => { it('should produce a sourcemap output', async () => { const file = runfiles.resolvePackageRelative('out.min.js.map'); const rawSourceMap = JSON.parse(fs.readFileSync(file, 'utf-8')); await sm.SourceMapConsumer.with(rawSourceMap, null, consumer => { - // terser will produce different output if the DEBUG environment variable is set - const pos = consumer.originalPositionFor( - !process.env['DEBUG'] ? {line: 1, column: 89} : {line: 6, column: 22}); + // terser will produce different output based on DEBUG flag + const pos = + consumer.originalPositionFor(!DEBUG ? {line: 1, column: 89} : {line: 6, column: 22}); expect(pos.name).toBe('something'); expect(pos.line).toBe(3); expect(pos.column).toBe(14); diff --git a/packages/terser/test/sourcemap/directory_spec.js b/packages/terser/test/sourcemap/directory_spec.js index 699031b6ba..a91b460ea4 100644 --- a/packages/terser/test/sourcemap/directory_spec.js +++ b/packages/terser/test/sourcemap/directory_spec.js @@ -2,6 +2,7 @@ const fs = require('fs'); const sm = require('source-map'); const path = require('path'); const {runfiles} = require('build_bazel_rules_nodejs/internal/linker'); +const DEBUG = process.env['COMPILATION_MODE'] === 'dbg'; describe('terser on a directory with map files', () => { it('should produce an output for each input', () => { @@ -16,8 +17,8 @@ describe('terser on a directory with map files', () => { await sm.SourceMapConsumer.with(rawSourceMap, null, consumer => { const pos = consumer.originalPositionFor( // position of MyClass in terser_minified output src1.min.js - // depends on DEBUG flag - !process.env['DEBUG'] ? {line: 1, column: 18} : {line: 3, column: 5}); + // depends on COMPILATION_MODE + !DEBUG ? {line: 1, column: 18} : {line: 3, column: 5}); expect(pos.source).toBe('src1.ts'); expect(pos.line).toBe(2); expect(pos.column).toBe(14); diff --git a/packages/terser/test/sourcemap/terser_spec.js b/packages/terser/test/sourcemap/terser_spec.js index 740025c47c..9d85906c02 100644 --- a/packages/terser/test/sourcemap/terser_spec.js +++ b/packages/terser/test/sourcemap/terser_spec.js @@ -1,6 +1,7 @@ const fs = require('fs'); const sm = require('source-map'); const DIR = 'build_bazel_rules_nodejs/packages/terser/test/sourcemap'; +const DEBUG = process.env['COMPILATION_MODE'] === 'dbg'; describe('terser sourcemap handling', () => { it('should produce a sourcemap output', async () => { @@ -10,7 +11,7 @@ describe('terser sourcemap handling', () => { const pos = consumer.originalPositionFor( // position of MyClass in terser_minified output src1.min.js // depends on DEBUG flag - !process.env['DEBUG'] ? {line: 1, column: 18} : {line: 3, column: 5}); + !DEBUG ? {line: 1, column: 18} : {line: 3, column: 5}); expect(pos.source).toBe('src1.ts'); expect(pos.line).toBe(2); expect(pos.column).toBe(14);