Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1372303003:
Make create_installer_archive.py operate silently by default. (Closed)
DescriptionMake create_installer_archive.py operate silently by default.
BUG=537215
TEST=No info logs when building mini_installer target (info logs are back if adding --verbose)
Committed: https://crrev.com/2ca132b35c9f42216c24af3b19dc518a197c4194
Cr-Commit-Position: refs/heads/master@{#352367}
Patch Set 1 #Patch Set 2 : Document optional argument in gyp/GN #
Total comments: 9
Patch Set 3 : reorder params (verbose param last) #Patch Set 4 : fix incorrect param ordering (I hate refactoring Python!) #
Total comments: 6
Patch Set 5 : Better stdout/stderr capture #
Messages
Total messages: 24 (7 generated)
gab@chromium.org changed reviewers: + csharp@chromium.org, grt@chromium.org
Greg PTAL. +csharp for python readability. Thanks, Gab
https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:69: def CompressUsingLZMA(verbose, build_dir, compressed_file, input_file): verbose should probably be the last argument https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:187: def RunSystemCommand(verbose, cmd, **kw): The order should probably be "cmd, verbose, **kw". Also, **kw doesn't seem to be used at all, what would you think about removing it? https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:194: kw['stdout'] = open(os.devnull, "w") nit:kw['stdout'] -> devnull https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:195: exit_code = subprocess.call(cmd, **kw) add stdout=devnull
Thanks, re-ordered params, other replies below. https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:69: def CompressUsingLZMA(verbose, build_dir, compressed_file, input_file): On 2015/09/29 17:01:37, csharp wrote: > verbose should probably be the last argument Done. https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:187: def RunSystemCommand(verbose, cmd, **kw): On 2015/09/29 17:01:37, csharp wrote: > The order should probably be "cmd, verbose, **kw". Done. > > Also, **kw doesn't seem to be used at all, what would you think about removing > it? It's used on line 318 (to also force stdout to devnull -- but setting verbose to False explicitly there isn't quite the same as as-is it still prints the command being ran in verbose mode) https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:194: kw['stdout'] = open(os.devnull, "w") On 2015/09/29 17:01:36, csharp wrote: > nit:kw['stdout'] -> devnull You mean s/kw['stdout']/devnull/ as a variable to be used below? That only works if we get rid of |kw| altogether which is being used on line 318 so I don't think we can do that. https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:195: exit_code = subprocess.call(cmd, **kw) On 2015/09/29 17:01:37, csharp wrote: > add stdout=devnull IIUC you want me to use the above debated variable here? That seems weird to me (as |devnull| would sometimes be None instead of os.devnull).
LGTM if you don't want to drop **kw https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:194: kw['stdout'] = open(os.devnull, "w") On 2015/09/29 17:32:28, gab wrote: > On 2015/09/29 17:01:36, csharp wrote: > > nit:kw['stdout'] -> devnull > > You mean s/kw['stdout']/devnull/ as a variable to be used below? > > That only works if we get rid of |kw| altogether which is being used on line 318 > so I don't think we can do that. Ah, I hadn't realized it was still used. https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:318: RunSystemCommand(cmd, options.verbose, stdout=open(os.devnull, "w")) What about removing the stdout setting here and just always pass in false for verbose? You don't get this command printed, but then you can drop **kw
https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:318: RunSystemCommand(cmd, options.verbose, stdout=open(os.devnull, "w")) On 2015/09/29 17:38:00, csharp wrote: > What about removing the stdout setting here and just always pass in false for > verbose? You don't get this command printed, but then you can drop **kw Feels knowing the actual command could be useful. @grt?
this is cool! thanks for making builds quieter. https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:197: raise Exception("Error while running cmd: %s, exit_code: %s" % is it possible to capture the command's stdout in the non-verbose case and emit it here? i imagine it would make diagnosing failures in official builds easier if the output was included in case of error.
Patchset #5 (id:80001) has been deleted
Think I found a much better way to do stdout/stderr capture here. grt/csharp PTAL. Thanks, Gab https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:197: raise Exception("Error while running cmd: %s, exit_code: %s" % On 2015/09/30 18:25:08, grt wrote: > is it possible to capture the command's stdout in the non-verbose case and emit > it here? i imagine it would make diagnosing failures in official builds easier > if the output was included in case of error. Improved the way stdout/stderr capture is done by this method, PTAL. https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:318: RunSystemCommand(cmd, options.verbose, stdout=open(os.devnull, "w")) On 2015/09/29 17:44:02, gab wrote: > On 2015/09/29 17:38:00, csharp wrote: > > What about removing the stdout setting here and just always pass in false for > > verbose? You don't get this command printed, but then you can drop **kw > > Feels knowing the actual command could be useful. > > @grt? Revised this as it made things more complicated for the change I just made to RunSystemCommand(). The default output of makecab really isn't that bad (2 lines by default looks like..) plus now that we don't emit it by default unless it fails it's really irrelevant to specifically omit it.
if this does what i think it does, then this is an enthusiastic lgtm! https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/1372303003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:197: raise Exception("Error while running cmd: %s, exit_code: %s" % On 2015/10/01 14:21:18, gab wrote: > On 2015/09/30 18:25:08, grt wrote: > > is it possible to capture the command's stdout in the non-verbose case and > emit > > it here? i imagine it would make diagnosing failures in official builds easier > > if the output was included in case of error. > > Improved the way stdout/stderr capture is done by this method, PTAL. awesome!
On 2015/10/01 14:31:57, grt wrote: > if this does what i think it does, then this is an enthusiastic lgtm! I tested locally and can confirm that it does what I think you think it does ;-)
lgtm
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372303003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372303003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372303003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372303003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372303003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372303003/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2ca132b35c9f42216c24af3b19dc518a197c4194 Cr-Commit-Position: refs/heads/master@{#352367} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
