Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886803002:
Enable DCHECKs by default in non-official builds. (Closed)
DescriptionEnable DCHECKs by default in non-official builds.
BUG=722827
Review-Url: https://codereview.chromium.org/2886803002
Cr-Commit-Position: refs/heads/master@{#472195}
Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5adb357392434ad
Patch Set 1 #Patch Set 2 : fix iOS DCHECK build #
Total comments: 2
Patch Set 3 : DCHECK_IS_ON() #
Messages
Total messages: 46 (20 generated)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + brettw@chromium.org
Brett, as discussed on chromium-dev, PTaL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thakis@chromium.org changed reviewers: + thakis@chromium.org
lgtm, good luck!
Description was changed from ========== Enable DCHECKs by default in non-official builds. BUG=722827 ========== to ========== Enable DCHECKs by default in non-official builds. BUG=722827 TBR=sdefresne@chromium.org for ios/ ==========
gab@chromium.org changed reviewers: + sdefresne@chromium.org
TBR sdefresne for ios/ (has no one on iOS team ran with DCHECKs on in that long for realz :-O?!)
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/16 15:29:34, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Everybody build/run Debug on iOS.
https://codereview.chromium.org/2886803002/diff/20001/ios/web/web_state/ui/wk... File ios/web/web_state/ui/wk_web_view_configuration_provider.mm (left): https://codereview.chromium.org/2886803002/diff/20001/ios/web/web_state/ui/wk... ios/web/web_state/ui/wk_web_view_configuration_provider.mm:90: #if !defined(NDEBUG) || !defined(DCHECK_ALWAYS_ON) // Matches DCHECK_IS_ON. I think this should instead use DCHECK_IS_ON() (see base/logging.h line 762-766) so that this code is enabled when "is_debug = true" even if "dcheck_always_on = false". #if DCHECK_IS_ON() ... #endif // DCHECK_IS_ON()
lgtm with suggested change
On 2017/05/16 15:29:18, gab wrote: > TBR sdefresne for ios/ (has no one on iOS team ran with DCHECKs on in that long > for realz :-O?!) On the contrary, nobody has build Release with DCHECK. Everybody run and build Debug with DCHECK.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2886803002/#ps40001 (title: "DCHECK_IS_ON()")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2886803002/diff/20001/ios/web/web_state/ui/wk... File ios/web/web_state/ui/wk_web_view_configuration_provider.mm (left): https://codereview.chromium.org/2886803002/diff/20001/ios/web/web_state/ui/wk... ios/web/web_state/ui/wk_web_view_configuration_provider.mm:90: #if !defined(NDEBUG) || !defined(DCHECK_ALWAYS_ON) // Matches DCHECK_IS_ON. On 2017/05/16 16:03:52, sdefresne wrote: > I think this should instead use DCHECK_IS_ON() (see base/logging.h line 762-766) > so that this code is enabled when "is_debug = true" even if "dcheck_always_on = > false". > > #if DCHECK_IS_ON() > ... > #endif // DCHECK_IS_ON() Ah right, that's what I intended to use, somehow got side-tracked by previous code. Thanks, done!
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494952241231110,
"parent_rev": "940082f0cb8fa26ab9d92603b9810a1de3ae4eac", "commit_rev":
"875a2fa43caab0ee82efe68fb5adb357392434ad"}
Message was sent while issue was closed.
Description was changed from ========== Enable DCHECKs by default in non-official builds. BUG=722827 TBR=sdefresne@chromium.org for ios/ ========== to ========== Enable DCHECKs by default in non-official builds. BUG=722827 TBR=sdefresne@chromium.org for ios/ Review-Url: https://codereview.chromium.org/2886803002 Cr-Commit-Position: refs/heads/master@{#472195} Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5ad... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5ad...
Message was sent while issue was closed.
It takes some time to figure it out. Do we need any kind of announcement messaging associated with it? Did I miss anything?
Message was sent while issue was closed.
See "[chromium-dev] Intent to force dcheck_always_on in component builds". Sorry, I should've caught that this is only mentioned on the review, not in the Cl description. On Tue, May 16, 2017 at 5:29 PM, <pfeldman@chromium.org> wrote: > It takes some time to figure it out. Do we need any kind of announcement > messaging associated with it? Did I miss anything? > > https://codereview.chromium.org/2886803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2892473002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 472195 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
Urk, I wish I had been cc'ed on this but I was perhaps too late in my reply to the thread. As I tried to outline in my response, there are a number of tradeoffs here. In particular, I'm a bit concerned that this might slow things down and/or add more load to the system, and given that the system is apparently quite close to capacity on windows and linux at the moment, we might not have wanted to land this right now. It looks like we reverted the CL, but let's wait until the tree is nice and green for a day or two before trying to land it again.
Message was sent while issue was closed.
On 2017/05/16 23:28:46, Dirk Pranke (slow) wrote: > Urk, I wish I had been cc'ed on this but I was perhaps too late in my reply to > the thread. As I tried to outline in my response, there are a number of > tradeoffs here. In particular, I'm a bit concerned that this might slow things > down and/or add more load to the system, and given that the system is apparently > quite close to capacity on windows and linux at the moment, we might not have > wanted to land this right now. > > It looks like we reverted the CL, but let's wait until the tree is nice and > green for a day or two before trying to land it again. Yeah, figured I'd give it a shot, see what pops, reverted now till next attempt. I don't think many bot configs will be affected by this as the vast majority (all but iOS?) explicitly use dcheck_always_on already.
Message was sent while issue was closed.
Wait, don't all the bots already build release with this forced on? I expected this to be a noop for the bots. On May 16, 2017 7:28 PM, <dpranke@chromium.org> wrote: > Urk, I wish I had been cc'ed on this but I was perhaps too late in my > reply to > the thread. As I tried to outline in my response, there are a number of > tradeoffs here. In particular, I'm a bit concerned that this might slow > things > down and/or add more load to the system, and given that the system is > apparently > quite close to capacity on windows and linux at the moment, we might not > have > wanted to land this right now. > > It looks like we reverted the CL, but let's wait until the tree is nice and > green for a day or two before trying to land it again. > > https://codereview.chromium.org/2886803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/05/16 23:33:54, Nico wrote: > Wait, don't all the bots already build release with this forced on? I > expected this to be a noop for the bots. Me too =O! Discovered that at least some iOS and ChromeOS bots appear not to (compile failures..!)... probably an oversight as all bots really *should* run tests with DCHECKs enabled IMO. > > On May 16, 2017 7:28 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dpranke@chromium.org> wrote: > > > Urk, I wish I had been cc'ed on this but I was perhaps too late in my > > reply to > > the thread. As I tried to outline in my response, there are a number of > > tradeoffs here. In particular, I'm a bit concerned that this might slow > > things > > down and/or add more load to the system, and given that the system is > > apparently > > quite close to capacity on windows and linux at the moment, we might not > > have > > wanted to land this right now. > > > > It looks like we reverted the CL, but let's wait until the tree is nice and > > green for a day or two before trying to land it again. > > > > https://codereview.chromium.org/2886803002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
Message was sent while issue was closed.
On 2017/05/16 23:45:02, gab wrote: > On 2017/05/16 23:33:54, Nico wrote: > > Wait, don't all the bots already build release with this forced on? I > > expected this to be a noop for the bots. > > Me too =O! Discovered that at least some iOS and ChromeOS bots appear not to > (compile failures..!)... probably an oversight as all bots really *should* run > tests with DCHECKs enabled IMO. As I wrote on the chromium-dev thread, (most of) the release waterfall bots intentionally do *not* run w/ dcheck enabled.
Message was sent while issue was closed.
On Tue, May 16, 2017 at 8:23 PM, <dpranke@chromium.org> wrote: > On 2017/05/16 23:45:02, gab wrote: > > On 2017/05/16 23:33:54, Nico wrote: > > > Wait, don't all the bots already build release with this forced on? I > > > expected this to be a noop for the bots. > > > > Me too =O! Discovered that at least some iOS and ChromeOS bots appear > not to > > (compile failures..!)... probably an oversight as all bots really > *should* run > > tests with DCHECKs enabled IMO. > > As I wrote on the chromium-dev thread, (most of) the release waterfall > bots > intentionally do *not* run w/ dcheck enabled. > Main waterfall, not try waterfall, right? And most load is from the try waterfall (?) > > https://codereview.chromium.org/2886803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/05/17 00:29:34, Nico wrote: > On Tue, May 16, 2017 at 8:23 PM, <mailto:dpranke@chromium.org> wrote: > > > On 2017/05/16 23:45:02, gab wrote: > > > On 2017/05/16 23:33:54, Nico wrote: > > > > Wait, don't all the bots already build release with this forced on? I > > > > expected this to be a noop for the bots. > > > > > > Me too =O! Discovered that at least some iOS and ChromeOS bots appear > > not to > > > (compile failures..!)... probably an oversight as all bots really > > *should* run > > > tests with DCHECKs enabled IMO. > > > > As I wrote on the chromium-dev thread, (most of) the release waterfall > > bots > > intentionally do *not* run w/ dcheck enabled. > > > > Main waterfall, not try waterfall, right? And most load is from the try > waterfall (?) Correct. Most of the load should be coming from builders w/ dchecks enabled. Hence my original question on that thread: do we know how the dcheck in question got landed, because I would've expected it to be caught.
Message was sent while issue was closed.
On 2017/05/17 00:36:18, Dirk Pranke (slow) wrote: > On 2017/05/17 00:29:34, Nico wrote: > > On Tue, May 16, 2017 at 8:23 PM, <mailto:dpranke@chromium.org> wrote: > > > > > On 2017/05/16 23:45:02, gab wrote: > > > > On 2017/05/16 23:33:54, Nico wrote: > > > > > Wait, don't all the bots already build release with this forced on? I > > > > > expected this to be a noop for the bots. > > > > > > > > Me too =O! Discovered that at least some iOS and ChromeOS bots appear > > > not to > > > > (compile failures..!)... probably an oversight as all bots really > > > *should* run > > > > tests with DCHECKs enabled IMO. > > > > > > As I wrote on the chromium-dev thread, (most of) the release waterfall > > > bots > > > intentionally do *not* run w/ dcheck enabled. > > > > > > > Main waterfall, not try waterfall, right? And most load is from the try > > waterfall (?) > > Correct. Most of the load should be coming from builders w/ dchecks > enabled. > > Hence my original question on that thread: do we know how the dcheck > in question got landed, because I would've expected it to be caught. "That DCHECK" being the iOS dcheck that's being fixed in this CL? If so, I'm guessing the answer is that iOS bots are either debug or release without dchecks atm.
Message was sent while issue was closed.
On 2017/05/17 00:50:16, Nico wrote: > On 2017/05/17 00:36:18, Dirk Pranke (slow) wrote: > > On 2017/05/17 00:29:34, Nico wrote: > > > On Tue, May 16, 2017 at 8:23 PM, <mailto:dpranke@chromium.org> wrote: > > > > > > > On 2017/05/16 23:45:02, gab wrote: > > > > > On 2017/05/16 23:33:54, Nico wrote: > > > > > > Wait, don't all the bots already build release with this forced on? I > > > > > > expected this to be a noop for the bots. > > > > > > > > > > Me too =O! Discovered that at least some iOS and ChromeOS bots appear > > > > not to > > > > > (compile failures..!)... probably an oversight as all bots really > > > > *should* run > > > > > tests with DCHECKs enabled IMO. > > > > > > > > As I wrote on the chromium-dev thread, (most of) the release waterfall > > > > bots > > > > intentionally do *not* run w/ dcheck enabled. > > > > > > > > > > Main waterfall, not try waterfall, right? And most load is from the try > > > waterfall (?) > > > > Correct. Most of the load should be coming from builders w/ dchecks > > enabled. > > > > Hence my original question on that thread: do we know how the dcheck > > in question got landed, because I would've expected it to be caught. > > "That DCHECK" being the iOS dcheck that's being fixed in this CL? If so, I'm > guessing the answer is that iOS bots are either debug or release without dchecks > atm. "That DCHECK" == "a DCHECK being hit on browser startup" from gab@'s original chromium-dev thread. If that's the DCHECK being fixed here, then, yes, the answer is that I believe the iOS bots in the CQ do *not* have DCHECK enabled and so wouldn't have caught this. (And they probably should have it enabled).
Message was sent while issue was closed.
@DCHECK that was hit on startup, I didn't explicitly hit this myself, it's mostly anecdata: I tend to be the point of contact whenever a dev in MON hits a random DCHECK in base unrelated to their CL (e.g. thread-checking from a CL on trunk that did it wrong and doesn't hit that codepath in tests...) . On Tue, May 16, 2017 at 8:57 PM <dpranke@chromium.org> wrote: > On 2017/05/17 00:50:16, Nico wrote: > > On 2017/05/17 00:36:18, Dirk Pranke (slow) wrote: > > > On 2017/05/17 00:29:34, Nico wrote: > > > > On Tue, May 16, 2017 at 8:23 PM, <mailto:dpranke@chromium.org> > wrote: > > > > > > > > > On 2017/05/16 23:45:02, gab wrote: > > > > > > On 2017/05/16 23:33:54, Nico wrote: > > > > > > > Wait, don't all the bots already build release with this > forced on? > I > > > > > > > expected this to be a noop for the bots. > > > > > > > > > > > > Me too =O! Discovered that at least some iOS and ChromeOS bots > appear > > > > > not to > > > > > > (compile failures..!)... probably an oversight as all bots really > > > > > *should* run > > > > > > tests with DCHECKs enabled IMO. > > > > > > > > > > As I wrote on the chromium-dev thread, (most of) the release > waterfall > > > > > bots > > > > > intentionally do *not* run w/ dcheck enabled. > > > > > > > > > > > > > Main waterfall, not try waterfall, right? And most load is from the > try > > > > waterfall (?) > > > > > > Correct. Most of the load should be coming from builders w/ dchecks > > > enabled. > > > > > > Hence my original question on that thread: do we know how the dcheck > > > in question got landed, because I would've expected it to be caught. > > > > "That DCHECK" being the iOS dcheck that's being fixed in this CL? If so, > I'm > > guessing the answer is that iOS bots are either debug or release without > dchecks > > atm. > > "That DCHECK" == "a DCHECK being hit on browser startup" from gab@'s > original > chromium-dev thread. If that's the DCHECK being fixed here, then, yes, the > answer > is that I believe the iOS bots in the CQ do *not* have DCHECK enabled and > so > wouldn't > have caught this. (And they probably should have it enabled). > > https://codereview.chromium.org/2886803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
*snip* > It looks like we reverted the CL FWIW the findit revert didn't land, dmazzoni fixed the cros builder compile instead afaik.
Message was sent while issue was closed.
On Tue, May 16, 2017 at 9:20 PM, <thakis@chromium.org> wrote: > *snip* > > > It looks like we reverted the CL > > FWIW the findit revert didn't land, dmazzoni fixed the cros builder compile > instead afaik. > Sorry, this is wrong, the revert did land after all: https://codereview.chromium.org/2892473002 -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Description was changed from ========== Enable DCHECKs by default in non-official builds. BUG=722827 TBR=sdefresne@chromium.org for ios/ Review-Url: https://codereview.chromium.org/2886803002 Cr-Commit-Position: refs/heads/master@{#472195} Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5ad... ========== to ========== Enable DCHECKs by default in non-official builds. BUG=722827 TBR=sdefresne@chromium.org for ios/ Review-Url: https://codereview.chromium.org/2886803002 Cr-Commit-Position: refs/heads/master@{#472195} Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5ad... ==========
Description was changed from ========== Enable DCHECKs by default in non-official builds. BUG=722827 TBR=sdefresne@chromium.org for ios/ Review-Url: https://codereview.chromium.org/2886803002 Cr-Commit-Position: refs/heads/master@{#472195} Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5ad... ========== to ========== Enable DCHECKs by default in non-official builds. BUG=722827 Review-Url: https://codereview.chromium.org/2886803002 Cr-Commit-Position: refs/heads/master@{#472195} Committed: https://chromium.googlesource.com/chromium/src/+/875a2fa43caab0ee82efe68fb5ad... ==========
gab@chromium.org changed reviewers: - brettw@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
