Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1798093002:
[Chromecast] Set symbol_level=2 for Chromecast builds by default. (Closed)
Description[Chromecast] Set symbol_level=2 for Chromecast builds by default.
Like official builds, Chromecast builds should enable full symbols by
default. This is useful for debugging engineering builds and those from
the field. Enable these by default so a GN arg need not be set
explicitly.
BUG= internal b/27602924
Committed: https://crrev.com/505f4c7a0e71f32e02f9939bfce20f8398180785
Cr-Commit-Position: refs/heads/master@{#381066}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 21 (6 generated)
slan@chromium.org changed reviewers: + alokp@chromium.org, halliwell@chromium.org, kmackay@chromium.org
Cast guys, This enables full symbols on Release builds by default. This way, we don't need to set this explicitly in our setup scripts. PTAL!
On 2016/03/14 16:41:42, slan wrote: > Cast guys, > > This enables full symbols on Release builds by default. This way, we don't need > to set this explicitly in our setup scripts. > > PTAL! P.S. Will send to Dirk for OWNERS review on Cast lgtm
The CQ bit was checked by slan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1798093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798093002/1
https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:31: } else if (!is_linux || is_debug || is_official_build || is_chromecast) { This is OK, but can you explain what is wrong with setting this flag explicitly in our makefile?
lgtm
https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:31: } else if (!is_linux || is_debug || is_official_build || is_chromecast) { On 2016/03/14 16:48:23, alokp wrote: > This is OK, but can you explain what is wrong with setting this flag explicitly > in our makefile? This solution also ensures this behavior on Android and desktop Cast builds, which the Makefile solution does not. Without this, we have to set this flag explicitly to get stack traces on Eng and Release builds.
On 2016/03/14 17:00:37, slan wrote: > https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... > File build/config/compiler/compiler.gni (right): > > https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... > build/config/compiler/compiler.gni:31: } else if (!is_linux || is_debug || > is_official_build || is_chromecast) { > On 2016/03/14 16:48:23, alokp wrote: > > This is OK, but can you explain what is wrong with setting this flag > explicitly > > in our makefile? > > This solution also ensures this behavior on Android and desktop Cast builds, > which the Makefile solution does not. Without this, we have to set this flag > explicitly to get stack traces on Eng and Release builds. lgtm
https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:31: } else if (!is_linux || is_debug || is_official_build || is_chromecast) { On 2016/03/14 17:00:36, slan wrote: > On 2016/03/14 16:48:23, alokp wrote: > > This is OK, but can you explain what is wrong with setting this flag > explicitly > > in our makefile? > > This solution also ensures this behavior on Android and desktop Cast builds, > which the Makefile solution does not. Without this, we have to set this flag > explicitly to get stack traces on Eng and Release builds. I am assuming that this only affects Eng builds, right? Because both Debug and User builds should be handled by is_debug and is_official_build, respectively? Not sure how important it is to set it for desktop release builds. What is the issue with Android? Note that I am not opposed to this patch, just trying to understand the need before adding chromecast bits in a chromium-wide file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/14 17:13:53, alokp wrote: > https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... > File build/config/compiler/compiler.gni (right): > > https://codereview.chromium.org/1798093002/diff/1/build/config/compiler/compi... > build/config/compiler/compiler.gni:31: } else if (!is_linux || is_debug || > is_official_build || is_chromecast) { > On 2016/03/14 17:00:36, slan wrote: > > On 2016/03/14 16:48:23, alokp wrote: > > > This is OK, but can you explain what is wrong with setting this flag > > explicitly > > > in our makefile? > > > > This solution also ensures this behavior on Android and desktop Cast builds, > > which the Makefile solution does not. Without this, we have to set this flag > > explicitly to get stack traces on Eng and Release builds. > > I am assuming that this only affects Eng builds, right? Because both Debug and > User builds should be handled by is_debug and is_official_build, respectively? This is true to some extent. Debug builds are covered, but as discussed offline, user builds are not covered by is_official_build. If we decide to set is_official_build for user builds, this won't be needed. Regardless, Eng builds need a solution. > Not sure how important it is to set it for desktop release builds. Agreed, but it would be convenient for engineers to simply pass is_chromecast=true and have it work. > > What is the issue with Android? We want symbols for Android Eng/Release builds too. > > Note that I am not opposed to this patch, just trying to understand the need > before adding chromecast bits in a chromium-wide file. Ack. I agree with you, this is definitely one such time where we must make a tradeoff between modularity and convenience. My feeling is that since we are importing GN args from chrome_build.gni, it is not significantly worse to do the same with chromecast_build.gni.
slan@chromium.org changed reviewers: + dpranke@chromium.org
Hey Dirk,
This change will make sure that all Chromecast builds have symbols by default.
For context, we have {Debug, Eng, Release} builds... Eng is a Release build with
DCHECKs enabled.
Please take a look!
I think this is fine, and if you really do what this to be the default in a release+dcheck cast build, we should embed that knowledge in the build files rather than relying on bot configuration, etc. to make it so. So, lgtm if that's the case.
The CQ bit was checked by slan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1798093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798093002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Set symbol_level=2 for Chromecast builds by default. Like official builds, Chromecast builds should enable full symbols by default. This is useful for debugging engineering builds and those from the field. Enable these by default so a GN arg need not be set explicitly. BUG= internal b/27602924 ========== to ========== [Chromecast] Set symbol_level=2 for Chromecast builds by default. Like official builds, Chromecast builds should enable full symbols by default. This is useful for debugging engineering builds and those from the field. Enable these by default so a GN arg need not be set explicitly. BUG= internal b/27602924 Committed: https://crrev.com/505f4c7a0e71f32e02f9939bfce20f8398180785 Cr-Commit-Position: refs/heads/master@{#381066} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/505f4c7a0e71f32e02f9939bfce20f8398180785 Cr-Commit-Position: refs/heads/master@{#381066} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
