Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2614483002:
[android] Make profile_android_startup.py runnable on its own. (Closed)
Description[android] Make profile_android_startup.py runnable on its own.
A rewrite of the downstream orderfile recipe will call this script
rather than invoking AndroidProfileTool.CollectProfile directly.
BUG=639831
Committed: https://crrev.com/2684fb437214ba83d39455530a905d3a2a965d0a
Cr-Commit-Position: refs/heads/master@{#441400}
Patch Set 1 #Patch Set 2 : prereview: fixes #
Total comments: 7
Patch Set 3 : pasko comments #Patch Set 4 : pasko comments 2 #Messages
Total messages: 15 (7 generated)
jbudorick@chromium.org changed reviewers: + pasko@chromium.org
will the script be run directly by a new recipe? meta: having reasons for commit in commit messages greatly help speeding up reviews. https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... File tools/cygprofile/profile_android_startup.py (right): https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:210: def __init__(self, output_directory, host_cyglog_dir=None): why do we need this be optional? is it hard to make it always require host_cyglog_dir? https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:396: args = parser.parse_args(raw_args) why not just parser.parse_args()?
Description was changed from ========== [android] Make profile_android_startup.py runnable on its own. BUG=639831 ========== to ========== [android] Make profile_android_startup.py runnable on its own. A rewrite of the downstream orderfile recipe will call this script rather than invoking AndroidProfileTool.CollectProfile directly. BUG=639831 ==========
Updated the CL description. https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... File tools/cygprofile/profile_android_startup.py (right): https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:210: def __init__(self, output_directory, host_cyglog_dir=None): On 2017/01/04 14:17:24, pasko wrote: > why do we need this be optional? is it hard to make it always require > host_cyglog_dir? Doing so in this CL would break the downstream use of this. I'd need to land a matching CL shortly after this rolled. I'd prefer a three-sided change if we want this to be required. https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:396: args = parser.parse_args(raw_args) On 2017/01/04 14:17:24, pasko wrote: > why not just parser.parse_args()? This is a habit I've been getting into s.t. other things can call main if they'd like, though in practice I've yet to do so. Switched.
lgtm, thank you for doing this https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... File tools/cygprofile/profile_android_startup.py (right): https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:210: def __init__(self, output_directory, host_cyglog_dir=None): On 2017/01/04 15:11:52, jbudorick wrote: > On 2017/01/04 14:17:24, pasko wrote: > > why do we need this be optional? is it hard to make it always require > > host_cyglog_dir? > > Doing so in this CL would break the downstream use of this. I'd need to land a > matching CL shortly after this rolled. I'd prefer a three-sided change if we > want this to be required. OK, sure, please add a TODO to make this a mandatory argument after crbug/X is fixed or something. https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:396: args = parser.parse_args(raw_args) On 2017/01/04 15:11:52, jbudorick wrote: > On 2017/01/04 14:17:24, pasko wrote: > > why not just parser.parse_args()? > > This is a habit I've been getting into s.t. other things can call main if they'd > like, though in practice I've yet to do so. ah, ok. I'm probably from a different universe (C universe?), where calling main() is heavily discouraged, so I did not think about this possibility.
https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... File tools/cygprofile/profile_android_startup.py (right): https://codereview.chromium.org/2614483002/diff/20001/tools/cygprofile/profil... tools/cygprofile/profile_android_startup.py:210: def __init__(self, output_directory, host_cyglog_dir=None): On 2017/01/04 15:44:14, pasko wrote: > On 2017/01/04 15:11:52, jbudorick wrote: > > On 2017/01/04 14:17:24, pasko wrote: > > > why do we need this be optional? is it hard to make it always require > > > host_cyglog_dir? > > > > Doing so in this CL would break the downstream use of this. I'd need to land a > > matching CL shortly after this rolled. I'd prefer a three-sided change if we > > want this to be required. > > OK, sure, please add a TODO to make this a mandatory argument after crbug/X is > fixed or something. Done.
The CQ bit was checked by jbudorick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2614483002/#ps60001 (title: "pasko comments 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483549738951130,
"parent_rev": "82516c572dbb7eeaed2ad688205a10dfd9294e7f", "commit_rev":
"8699c193ae01c882dbc035ef4048537f4dc0775c"}
Message was sent while issue was closed.
Description was changed from ========== [android] Make profile_android_startup.py runnable on its own. A rewrite of the downstream orderfile recipe will call this script rather than invoking AndroidProfileTool.CollectProfile directly. BUG=639831 ========== to ========== [android] Make profile_android_startup.py runnable on its own. A rewrite of the downstream orderfile recipe will call this script rather than invoking AndroidProfileTool.CollectProfile directly. BUG=639831 Review-Url: https://codereview.chromium.org/2614483002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [android] Make profile_android_startup.py runnable on its own. A rewrite of the downstream orderfile recipe will call this script rather than invoking AndroidProfileTool.CollectProfile directly. BUG=639831 Review-Url: https://codereview.chromium.org/2614483002 ========== to ========== [android] Make profile_android_startup.py runnable on its own. A rewrite of the downstream orderfile recipe will call this script rather than invoking AndroidProfileTool.CollectProfile directly. BUG=639831 Committed: https://crrev.com/2684fb437214ba83d39455530a905d3a2a965d0a Cr-Commit-Position: refs/heads/master@{#441400} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2684fb437214ba83d39455530a905d3a2a965d0a Cr-Commit-Position: refs/heads/master@{#441400} |
