bazel,dev: in dev, handle different --compilation_modes correctly...
#106944
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
rail
approved these changes
Jul 18, 2023
| } | ||
| if arg == "--config" || arg == "--compilation_mode" || arg == "-c" { | ||
| ret = append(ret, arg) | ||
| addNext = true |
There was a problem hiding this comment.
A nit. You can add continue here, so you don't even try to evaluate the next if (and potential future ifs).
There was a problem hiding this comment.
I used if .. else if for this to accomplish the same thing without using continue.
... and switch our default compilation mode to `dbg`. Under `fastbuild`, built binaries are stripped. This is not the case with `dbg`. Have `dev doctor` recommend using `dbg`, and either way make `dev` resilient to whatever kind of `--compilation_mode` you're using. In principle or theory `dbg` is slower than `fastbuild`, but for the Go compiler it generates debuggable binaries by default and you have to opt-in to stripping, so it shoould make no real difference. Now, we think of the `--compilation_mode` simply as follows: `dbg` is the default that everyone is opted into by default unless they explicitly set `-c opt` (we use this for release). For now I don't see a reason anyone would need `fastbuild`. Epic: CRDB-17171 Release note: None Closes: cockroachdb#106820
|
bors r=rail |
|
Build failed (retrying...): |
|
Build succeeded: |
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Jul 19, 2023
In cockroachdb#106944 the `dbg` compilation mode became the default for all non-release builds. This caused stack overflows in the memo cycle detection tests, which excessive invoke recursive functions, likely because stack frames are larger in `dbg` mode than in `opt` mode or because certain optimizations aren't performed in `dbg` mode that reduce stack depth due to recursion, like inlining. This commit fixes the test by lowering the cycle detection limit, making it so that a cycle is detected before a stack overflow occurs. Epic: None Release note: None
|
Follow-up: #107207 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
... and switch our default compilation mode to
dbg.Under
fastbuild, built binaries are stripped. This is not the case withdbg. Havedev doctorrecommend usingdbg, and either way makedevresilient to whatever kind of--compilation_modeyou're using.In principle or theory
dbgis slower thanfastbuild, but for the Go compiler it generates debuggable binaries by default and you have to opt-in to stripping, so it shoould make no real difference.Now, we think of the
--compilation_modesimply as follows:dbgis the default that everyone is opted into by default unless they explicitly set-c opt(we use this for release). For now I don't see a reason anyone would needfastbuild.Epic: CRDB-17171
Release note: None
Closes: #106820