feat: support --compilation_mode flag

ashi009 · alexeagle · commit 9fa4343aaa80 · 2019-11-14T15:18:00.000-08:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
