Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add default DEBUG and VERBOSE_LOGS configuration_env_vars to nodejs_binary #1080

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Sep 2, 2019

  • all rules updated to use DEBUG and VERBOSE_LOGS environment variables
  • added golden_debug attribute golden_file_test to support the case where a rule has different output if DEBUG is set; for example
golden_file_test(
name = "test",
actual = "out.min.js",
golden = "output.golden.js_",
golden_debug = "output.debug.golden.js_",
)

@gregmagolan gregmagolan requested a review from soldair as a code owner September 2, 2019 20:00
@gregmagolan gregmagolan force-pushed the debug_env branch 4 times, most recently from 4313341 to 37ef4e6 Compare September 2, 2019 21:09
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should figure out how --config=debug relates to --define=DEBUG=1
Maybe they can be the same meaning, and we update the .bazelrc to include the --define flag in --config=debug - that's how I imagined this working. Then users have only one thing to learn.
OTOH it means that you get breakpoints in your node programs and also get different rule outputs, but maybe you only wanted one of those things to happen.

Needs some documentation somewhere - ideally there should be "if something is wrong with your Bazel build, include --config=debug in your command line, find the log file at /some/path and check it for confidential info, then send to us to help diagnose your problem" (not that users read all docs before filing but at least we can point them to that when we have trouble responding to an issue)

internal/rollup/terser-wrapped.js Outdated Show resolved Hide resolved
@gregmagolan
Copy link
Collaborator Author

I'll add some docs to the main readme for these settings. I think we can recommend that user's add

test:debug --test_arg=--node_options=--inspect-brk --test_output=streamed --test_strategy=exclusive --test_timeout=9999 --nocache_test_results --define=VERBOSE_LOGS=1
run:debug --define=VERBOSE_LOGS=1
build:debug --define=DEBUG=1

to the bazelrc in the docs and explain what each flag does including the two define flags. User's can then choose to exclude certain flags such as build:debug --define=DEBUG=1 if they don't want --config=debug to affect their build.

Also, I think VERBOSE_LOGS may be better than DEBUG_LOGS as per your slack comment.

@gregmagolan gregmagolan force-pushed the debug_env branch 2 times, most recently from e0075d0 to b966d5a Compare September 3, 2019 16:48
@gregmagolan gregmagolan changed the title feat: add default DEBUG and DEBUG_LOGS configuration_env_vars to nodejs_binary feat: add default DEBUG and VERBOSE_LOGS configuration_env_vars to nodejs_binary Sep 3, 2019
@gregmagolan gregmagolan force-pushed the debug_env branch 2 times, most recently from 4eec8f3 to eccbc42 Compare September 3, 2019 17:50
name: ubuntu1804_debug
platform: ubuntu1804
test_flags:
- "--define=VERBOSE_LOGS=1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I'm not sure it's worth adding CI load for this. let's just turn on these options in all our builds? I think spammy is probably good so we can understand CI failures for all jobs?

Copy link
Collaborator Author

@gregmagolan gregmagolan Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since - "--define=DEBUG=1" changes build output I think we need to run all tests without it set and with it set. No other way to know if the rules that change their output based on this break things.

- "--test_tag_filters=-e2e,-examples,-fix-bazelci-ubuntu"
test_targets:
- "//..."
- "//packages/terser/test/debug:test_define_DEBUG"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the one bit of coverage we miss if we don't have a mix of debug/non-debug jobs. Not convinced it's needed since the debug attribute covers like 80% of the feature

.circleci/config.yml Outdated Show resolved Hide resolved
DEVELOPING.md Outdated Show resolved Hide resolved
@@ -22,7 +24,8 @@ ${prettyDiff}

Update the golden file:

bazel run ${process.env['BAZEL_TARGET'].replace(/_bin$/, '')}.accept
bazel run ${debug ? '--define=DEBUG=1 ' : ''}${
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I guess that's the principled thing... kinda annoying though

internal/node/node.bzl Outdated Show resolved Hide resolved
@gregmagolan gregmagolan force-pushed the debug_env branch 3 times, most recently from 5c7353c to 60528d6 Compare September 3, 2019 18:12
common.bazelrc Outdated Show resolved Hide resolved
…dejs_binary

- all rules updated to use DEBUG and VERBOSE_LOGS environment variables
- added golden_debug attribute golden_file_test to support the case where a rule has different output if DEBUG is set; for example
```
golden_file_test(
    name = "test",
    actual = "out.min.js",
    golden = "output.golden.js_",
    golden_debug = "output.debug.golden.js_",
)
```
@gregmagolan gregmagolan merged commit df37fca into bazel-contrib:master Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants