Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183063002:
[build] Conditionally print to stdout on Android (Closed)
|
Created:
4 years, 4 months ago by Michael Achenbach Modified:
4 years, 3 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[build] Conditionally print to stdout on Android
This adds a gyp/gn variable to control printing to stdout
on Android. This is false by default and true for all
v8 stand-alone android bots.
BUG=chromium:629806
Committed: https://crrev.com/4289c28fb9291f95d1372059bf8cfef60059bf7d
Cr-Commit-Position: refs/heads/master@{#38253}
Patch Set 1 #Patch Set 2 : [gn] Set defines for Android #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Define behind variable #
Messages
Total messages: 40 (14 generated)
Description was changed from ========== [gn] Set defines for Android BUG= ========== to ========== [gn] Set defines for Android BUG=chromium:629806 ==========
machenbach@chromium.org changed reviewers: + jochen@chromium.org, vogelheim@chromium.org
PTAL https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn#newcode336 BUILD.gn:336: defines += [ "V8_ANDROID_LOG_STDOUT" ] Porting https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=%22ANDROID... See differences here: https://build.chromium.org/p/client.v8.ports/builders/V8%20Android%20Arm%20-%... The ANDROID define seems to come from chromium now, except for mksnapshot.cc
hum, I'm not really happy about a v8_standalone setting. Why can't we define this flag always?
lgtm, but not really happy. https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn#newcode336 BUILD.gn:336: defines += [ "V8_ANDROID_LOG_STDOUT" ] On 2016/07/26 15:06:34, Michael Achenbach (slow) wrote: > Porting > https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=%22ANDROID... > > See differences here: > https://build.chromium.org/p/client.v8.ports/builders/V8%20Android%20Arm%20-%... > > The ANDROID define seems to come from chromium now, except for mksnapshot.cc Are there any sort of asserts or unit tests for BUILD.gn files? If this was in regular v8 code, this sort of built-in assumption that elsewhere surely a flag had already been set and somewhere else entirely we intend rely on its presence would make me super nervous, and I'd demand a rewrite or at least a shitload of asserts. Does gn even give us the means to do that?
On 2016/07/27 07:39:20, jochen wrote: > hum, I'm not really happy about a v8_standalone setting. > > Why can't we define this flag always? Maybe we can? I always try to keep things as they were in gyp to avoid new side-effects in chromium, e.g. larger logs. IIUC, this flag makes log printing go to stdout, which it otherwise wouldn't (where does it go?).
On 2016/07/27 07:39:20, jochen wrote: > hum, I'm not really happy about a v8_standalone setting. Furthermore: We had a separate standalone config before and it might be prudent to have something similar. It doesn't work file-based anymore like before but I'd be happy to find a better solution.
On 2016/07/27 07:43:41, vogelheim wrote: > lgtm, but not really happy. > > https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn#newcode336 > BUILD.gn:336: defines += [ "V8_ANDROID_LOG_STDOUT" ] > On 2016/07/26 15:06:34, Michael Achenbach (slow) wrote: > > Porting > > > https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=%22ANDROID... > > > > See differences here: > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Android%20Arm%20-%... > > > > The ANDROID define seems to come from chromium now, except for mksnapshot.cc > > Are there any sort of asserts or unit tests for BUILD.gn files? I'll try adding more assertions in a separate CL. But we need to draw a line. I could theoretically assert everything that comes from "build".
On 2016/07/27 at 08:02:06, machenbach wrote: > On 2016/07/27 07:43:41, vogelheim wrote: > > lgtm, but not really happy. > > > > https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn > > File BUILD.gn (right): > > > > https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn#newcode336 > > BUILD.gn:336: defines += [ "V8_ANDROID_LOG_STDOUT" ] > > On 2016/07/26 15:06:34, Michael Achenbach (slow) wrote: > > > Porting > > > > > https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=%22ANDROID... > > > > > > See differences here: > > > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Android%20Arm%20-%... > > > > > > The ANDROID define seems to come from chromium now, except for mksnapshot.cc > > > > Are there any sort of asserts or unit tests for BUILD.gn files? > > I'll try adding more assertions in a separate CL. But we need to draw a line. I could theoretically assert everything that comes from "build". The fact that we had a different standalone configuration, however, has bitten us several times in the past :-/
On 2016/07/27 08:10:57, jochen wrote: > On 2016/07/27 at 08:02:06, machenbach wrote: > > On 2016/07/27 07:43:41, vogelheim wrote: > > > lgtm, but not really happy. > > > > > > https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn > > > File BUILD.gn (right): > > > > > > https://codereview.chromium.org/2183063002/diff/20001/BUILD.gn#newcode336 > > > BUILD.gn:336: defines += [ "V8_ANDROID_LOG_STDOUT" ] > > > On 2016/07/26 15:06:34, Michael Achenbach (slow) wrote: > > > > Porting > > > > > > > > https://cs.chromium.org/chromium/src/v8/gypfiles/standalone.gypi?q=%22ANDROID... > > > > > > > > See differences here: > > > > > > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Android%20Arm%20-%... > > > > > > > > The ANDROID define seems to come from chromium now, except for > mksnapshot.cc > > > > > > Are there any sort of asserts or unit tests for BUILD.gn files? > > > > I'll try adding more assertions in a separate CL. But we need to draw a line. > I could theoretically assert everything that comes from "build". > > The fact that we had a different standalone configuration, however, has bitten > us several times in the past :-/ True and I'd be happy to get rid of it. We can also get rid of it the other way around here. I.e. not setting the flag at all neither stand-alone nor chromium. I want to avoid the case where: - I add this so that it gets active in chromium - It lands and rolls - Some obscure bot flakily dies because of huge log files and sheriffs spend hours to trace down the cause - Several rolls are reverted...
is there a particular reason we log to stdout in standalone builds instead of just using the logging facilities which we link against anyways?
This comes from https://codereview.chromium.org/9179009 - like so many things, maybe it was the standard, copied from common.gypi at that time, but later changed in chromium without us noticing.
Not writing to stdout might also require changes in perf measurement infrastructure.
how do we even get the stdout on android?
On 2016/08/01 12:10:53, jochen wrote: > how do we even get the stdout on android? Not sure what you mean. Technically we use the device utils which wrap adb: https://cs.chromium.org/chromium/src/v8/tools/run_perf.py?q=run_perf&sq=packa...
On 2016/08/01 12:33:42, Michael Achenbach (slow) wrote: > On 2016/08/01 12:10:53, jochen wrote: > > how do we even get the stdout on android? > > Not sure what you mean. Technically we use the device utils which wrap adb: > https://cs.chromium.org/chromium/src/v8/tools/run_perf.py?q=run_perf&sq=packa... https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android...
what about adding a variable v8_android_log_stdout or similar, and have the bots that do perf tests on android set that?
On 2016/08/01 12:45:16, jochen wrote: > what about adding a variable v8_android_log_stdout or similar, and have the bots > that do perf tests on android set that? Yes - we can make it explicit. I'll repurpose this CL then...
PTAL at patch 4. The flag is now behind a variable, which is set for all v8 stand-alone android bots.
The CQ bit was checked by machenbach@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2183063002/#ps60001 (title: "Define behind variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by machenbach@chromium.org
Description was changed from ========== [gn] Set defines for Android BUG=chromium:629806 ========== to ========== [build] Conditionally print to stdout on Android BUG=chromium:629806 ==========
Description was changed from ========== [build] Conditionally print to stdout on Android BUG=chromium:629806 ========== to ========== [build] Conditionally print to stdout on Android This adds a gyp/gn variable to control printing to stdout on Android. This is false by default and true for all v8 stand-alone android bots. BUG=chromium:629806 ==========
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [build] Conditionally print to stdout on Android This adds a gyp/gn variable to control printing to stdout on Android. This is false by default and true for all v8 stand-alone android bots. BUG=chromium:629806 ========== to ========== [build] Conditionally print to stdout on Android This adds a gyp/gn variable to control printing to stdout on Android. This is false by default and true for all v8 stand-alone android bots. BUG=chromium:629806 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [build] Conditionally print to stdout on Android This adds a gyp/gn variable to control printing to stdout on Android. This is false by default and true for all v8 stand-alone android bots. BUG=chromium:629806 ========== to ========== [build] Conditionally print to stdout on Android This adds a gyp/gn variable to control printing to stdout on Android. This is false by default and true for all v8 stand-alone android bots. BUG=chromium:629806 Committed: https://crrev.com/4289c28fb9291f95d1372059bf8cfef60059bf7d Cr-Commit-Position: refs/heads/master@{#38253} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4289c28fb9291f95d1372059bf8cfef60059bf7d Cr-Commit-Position: refs/heads/master@{#38253}
Message was sent while issue was closed.
On 2016/08/02 15:38:15, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/4289c28fb9291f95d1372059bf8cfef60059bf7d > Cr-Commit-Position: refs/heads/master@{#38253} Android build will be broken by this CL. the error message is: ../src/base/platform/platform-posix.cc:34:10: fatal error: 'android/log.h' file not found Anyone have time to take a look at this issue? Thanks!
Message was sent while issue was closed.
On 2016/08/24 03:05:25, zhengxing.li wrote: > On 2016/08/02 15:38:15, commit-bot: I haz the power wrote: > > Patchset 4 (id:??) landed as > > https://crrev.com/4289c28fb9291f95d1372059bf8cfef60059bf7d > > Cr-Commit-Position: refs/heads/master@{#38253} > > Android build will be broken by this CL. the error message is: > ../src/base/platform/platform-posix.cc:34:10: fatal error: 'android/log.h' file > not found > > Anyone have time to take a look at this issue? > > Thanks! What do you mean by "will be"? Could you please file a bug if any action needs to be taken?
Message was sent while issue was closed.
On 2016/08/28 19:57:46, machenbach (OOO until Aug 28) wrote: > On 2016/08/24 03:05:25, zhengxing.li wrote: > > Android build will be broken by this CL. the error message is: > > ../src/base/platform/platform-posix.cc:34:10: fatal error: 'android/log.h' > file > > not found > > > > Anyone have time to take a look at this issue? > > > > Thanks! > > What do you mean by "will be"? Could you please file a bug if any action needs > to be taken? "will be" means "has been", in this case. I've fixed it in https://codereview.chromium.org/2264283007 .
Message was sent while issue was closed.
On 2016/08/29 09:26:31, Jakob wrote: > On 2016/08/28 19:57:46, machenbach (OOO until Aug 28) wrote: > > On 2016/08/24 03:05:25, zhengxing.li wrote: > > > Android build will be broken by this CL. the error message is: > > > ../src/base/platform/platform-posix.cc:34:10: fatal error: 'android/log.h' > > file > > > not found > > > > > > Anyone have time to take a look at this issue? > > > > > > Thanks! > > > > What do you mean by "will be"? Could you please file a bug if any action needs > > to be taken? > > "will be" means "has been", in this case. I've fixed it in > https://codereview.chromium.org/2264283007 . Alright, thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
