Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(400)

Issue 3266004: Move RunCommand, and Info/Warning/Die into common pylib (Closed)

Created:
10 years, 3 months ago by sosa
Modified:
10 years, 3 months ago
Reviewers:
petkov
CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa
Base URL:
ssh://git@chromiumos-git//crosutils.git
Visibility:
Public.

Description

Move RunCommand, and Info/Warning/Die into common pylib TEST=Ran all commands that used RunCommand or Die with their associated unittests.

Patch Set 1 #

Total comments: 7

Patch Set 2 : Address review comments #

Patch Set 3 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -166 lines) Patch
M archive_hwqual View 1 chunk +4 lines, -0 lines 0 comments Download
M bin/cbuildbot.py View 1 2 11 chunks +18 lines, -65 lines 0 comments Download
M bin/cbuildbot_comm.py View 1 5 chunks +10 lines, -10 lines 0 comments Download
M cros_mark_as_stable.py View 1 14 chunks +34 lines, -35 lines 0 comments Download
M cros_mark_as_stable_unittest.py View 9 chunks +16 lines, -15 lines 0 comments Download
M generate_test_report.py View 1 chunk +2 lines, -41 lines 0 comments Download
A lib/cros_build_lib.py View 1 1 chunk +120 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
sosa
10 years, 3 months ago (2010-08-28 00:18:12 UTC) #1
petkov
10 years, 3 months ago (2010-08-28 04:54:51 UTC) #2
LGTM. Would be nice to have unit tests :-)

http://codereview.chromium.org/3266004/diff/1/3
File bin/cbuildbot.py (right):

http://codereview.chromium.org/3266004/diff/1/3#newcode19
bin/cbuildbot.py:19: from cros_build_lib import Info, Warning, Die, RunCommand
sort?

http://codereview.chromium.org/3266004/diff/1/4
File bin/cbuildbot_comm.py (right):

http://codereview.chromium.org/3266004/diff/1/4#newcode7
bin/cbuildbot_comm.py:7: import os
ASCII sort would move os after SocketServer... Not sure what's right.

http://codereview.chromium.org/3266004/diff/1/5
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3266004/diff/1/5#newcode19
cros_mark_as_stable.py:19: from cros_build_lib import Info, Warning, Die,
RunCommand
You don't use RunCommand?

http://codereview.chromium.org/3266004/diff/1/8
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/3266004/diff/1/8#newcode29
lib/cros_build_lib.py:29: 
Remove blank line?

http://codereview.chromium.org/3266004/diff/1/8#newcode29
lib/cros_build_lib.py:29: 
Do you need a Raises section or something like that?

http://codereview.chromium.org/3266004/diff/1/8#newcode40
lib/cros_build_lib.py:40: if enter_chroot:  cmd = ['./enter_chroot.sh', '--'] +
cmd
./ ? This seems fragile... Maybe document somewhere...

http://codereview.chromium.org/3266004/diff/1/8#newcode52
lib/cros_build_lib.py:52: if not error_ok and proc.returncode != 0:
maybe remove != 0?

Powered by Google App Engine
This is Rietveld 408576698