Continue to refactor run_remote_tests script.

Refactor run_remote_tests script. 

What I did:
1. enter_chroot once and only once if necessary. The previous version would enter/exit chroot 3 times in worst case. And the entire logic happens inside chroot only, this has greatly simplified the overall workflow, and reduced the number of variables used. Shell script variables could be tricky.

2. Change variable type to test_type since type is a bash builtin. 

3. get rid of {$TMP}/run_test.sh script, since it is not necessary any more.

4. get rid of rsync step from third_party/autotest/files to ${BUILD_DIR}, since it is not neccessary either.

overall, reduced ~40 lines of code.

All the change should be transparent to end users and there should be no regression changes at all.

w/wo emerge autotest 
w/wo cros_workon 
in/outside of chroot. 

and all its combinations. 

Change-Id: I9c1532e9cb6cc0e724d4b6d870723df3e2a147ec

BUG=9291
TEST=Run storageFio test since it need prebuild test and deps.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=6fbd3d1

[ericli, 2010-11-22 18:32:15]

Ken,

I would really appreciated if you could patch in this CL and test it locally
before LGTM. I do not want to break anyone's test workflow. Thanks a lot.

Eric

[jrbarnette, 2010-11-23 00:11:28]

http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh#newcode93
run_remote_tests.sh:93: function autodetect_build() {
In this function, if FLAGS_use_emerged is false, and FLAGS_build is
also false, the function will exit with AUTOTEST_DIR unset.  WIth this
variable not set, the 'pushd' at line 169 fails, causing the script to fail.

(Discovered while testing the patch...)

[ericli, 2010-11-23 00:49:10]

Richard,

Thanks for the catch. Yes, it should handle such scenario better.

But if both use_emerged and build flags are false, where is your autotest
src code coming from?

--build=false also implied you were not cros_workon autotest, otherwise, it
should be reset to true inside the script.

Eric

On Mon, Nov 22, 2010 at 4:11 PM, <jrbarnette@chromium.org> wrote:

>
> http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh
> File run_remote_tests.sh (right):
>
>
> http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh#newcode93
> run_remote_tests.sh:93: function autodetect_build() {
> In this function, if FLAGS_use_emerged is false, and FLAGS_build is
> also false, the function will exit with AUTOTEST_DIR unset.  WIth this
> variable not set, the 'pushd' at line 169 fails, causing the script to
> fail.
>
> (Discovered while testing the patch...)
>
>
> http://codereview.chromium.org/5176009/
>



-- 
Eric Li
李咏竹
Google Kirkland

[jrbarnette, 2010-11-23 02:22:39]

On Nov 22, 2010, at 4:49 PM, Eric Li(李咏竹) wrote:

> Richard,
>
> Thanks for the catch. Yes, it should handle such scenario better.
>
> But if both use_emerged and build flags are false, where is your  
> autotest src code coming from?
>
Both flags false is the default condition if no options are specified.
This case is treated as equivalent to use_emerged: the previously
emerged package is used, and source is ignored.


> --build=false also implied you were not cros_workon autotest,  
> otherwise, it should be reset to true inside the script.
>
Yes.  The expectation should be that most users of run_remote_tests
won't be working on the autotest package.

Thanks!


> Eric
>
> On Mon, Nov 22, 2010 at 4:11 PM, <jrbarnette@chromium.org> wrote:
>
> http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh
> File run_remote_tests.sh (right):
>
> http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh#newcode93
> run_remote_tests.sh:93: function autodetect_build() {
> In this function, if FLAGS_use_emerged is false, and FLAGS_build is
> also false, the function will exit with AUTOTEST_DIR unset.  WIth this
> variable not set, the 'pushd' at line 169 fails, causing the script to
> fail.
>
> (Discovered while testing the patch...)
>
>
> http://codereview.chromium.org/5176009/
>
>
>
> -- 
> Eric Li
> 李咏竹
> Google Kirkland
>
>

-- jrb

[ericli, 2010-11-23 18:49:29]

PTAL.

http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh#newcode93
run_remote_tests.sh:93: function autodetect_build() {
Hi Richard,
Thanks to push me think more about the logic inside here. Now I believe its more
clear to follow.

On 2010/11/23 00:11:29, jrbarnette wrote:
> In this function, if FLAGS_use_emerged is false, and FLAGS_build is
> also false, the function will exit with AUTOTEST_DIR unset.  WIth this
> variable not set, the 'pushd' at line 169 fails, causing the script to fail.
> 
> (Discovered while testing the patch...)

[kmixter1, 2010-11-23 22:21:19]

I just have high level comments first:

Not copying files into the chroot means you're not setting all files with
root permissions, like they would be if they were emerged.  This caused
problems in logging_UserCrash where I try to run an executable I build as
chronos.  I've since worked around that problem (by fixing the permissions
in the test itself) but I have to wonder if that might mess up other tests.
 I don't know how to test this except by running all the main suites
(regression, nightly, smoke) with your version and verifying it works.
 Also, might this cause problems with prebuild if you're building for ARM
sometimes and for x86 others - does every test's setup step correctly blow
away all files before building the test?

I do get a permissions errors when I run:

14:16:20 ERROR| Package tarball creation failed, continuing anyway
Traceback (most recent call last):
  File
"/home/kmixter/trunk/src/third_party/autotest/files/server/autotest.py",
line 977, in _process_line
    self._send_tarball(pkg_name, dest_path)
  File
"/home/kmixter/trunk/src/third_party/autotest/files/server/autotest.py",
line 999, in _send_tarball
    prebuild.setup(self.job.clientdir, src_dir)
  File
"/home/kmixter/trunk/src/third_party/autotest/files/server/prebuild.py",
line 62, in setup
    client_test = init_test(client_test_dir)
  File
"/home/kmixter/trunk/src/third_party/autotest/files/server/prebuild.py",
line 47, in init_test
    return client_setup_job.init_test(options, testdir)
  File
"/home/kmixter/trunk/src/third_party/autotest/files/client/bin/setup_job.py",
line 54, in init_test
    job = setup_job(options=options)
  File
"/home/kmixter/trunk/src/third_party/autotest/files/client/bin/setup_job.py",
line 34, in __init__
    self._cleanup_results_dir()
  File
"/home/kmixter/trunk/src/third_party/autotest/files/client/bin/job.py", line
315, in _cleanup_results_dir
    shutil.rmtree(f)
  File "/usr/lib64/python2.6/shutil.py", line 216, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "/usr/lib64/python2.6/shutil.py", line 225, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/usr/lib64/python2.6/shutil.py", line 223, in rmtree
    os.rmdir(path)
OSError: [Errno 13] Permission denied:
'/home/kmixter/trunk/src/third_party/autotest/files/client/results/logging_UserCrash/results'

Not sure why it's trying to create results directories.'

I do like that it's running faster without having to reenter the chroot.  My
concern with folks with TTY problems remains, but just checking it in, being
ready to revert, and being ready to field complaints if it causes troubles
is probably the only way to test that.


On Tue, Nov 23, 2010 at 10:49 AM, <ericli@chromium.org> wrote:

> PTAL.
>
>
>
> http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh
> File run_remote_tests.sh (right):
>
>
> http://codereview.chromium.org/5176009/diff/2001/run_remote_tests.sh#newcode93
> run_remote_tests.sh:93: function autodetect_build() {
> Hi Richard,
> Thanks to push me think more about the logic inside here. Now I believe
> its more clear to follow.
>
>
> On 2010/11/23 00:11:29, jrbarnette wrote:
>
>> In this function, if FLAGS_use_emerged is false, and FLAGS_build is
>> also false, the function will exit with AUTOTEST_DIR unset.  WIth this
>> variable not set, the 'pushd' at line 169 fails, causing the script to
>>
> fail.
>
>  (Discovered while testing the patch...)
>>
>
> http://codereview.chromium.org/5176009/
>

[jrbarnette, 2010-11-23 23:16:04]

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode90
run_remote_tests.sh:90: tc-export CC CXX PKG_CONFIG
This sequence here seems a bit risky:  It pulls heaven knows what
variable settings into the current script with no guarantee that there
aren't conflicts that break the script.  It would probably be good to
delay this until just before invoking autoserv, and then put it in a
subshell for protection.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode96
run_remote_tests.sh:96: elif $(dirname $0)/cros_workon --board=${FLAGS_board}
list | \
nit on the pre-existing code:  the '\' here isn't needed, and the style
guidelines discourage it.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode101
run_remote_tests.sh:101: autotest. To use installed autotest, pass
--use_emerged."
This informational message should only be printed if the script
overrides the default setting of FLAGS_build.  As is, it will print even
if the user already specified --build.

I think the logic needed is this:
  if not --build and not --use_emerged and "cros_workon autotest"
    info "Detected cros_workon ..."
    FLAGS_build=TRUE

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode117
run_remote_tests.sh:117: "As requested, using emerged autotests already
installed at ${AUTOTEST_DIR}."
The text of message originally depended on whether the user specified
--use_emerged.  I think that that earlier distinction of the messages in the
two cases was useful.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode121
run_remote_tests.sh:121: start autotest first."
The code guidelines and I would both prefer that this test be written
  elif [ ${FLAGS_BUILD} -eq ... ]; then
    die ...
  else
    die ...
  ...

[kmixter1, 2010-11-23 23:30:41]

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode101
run_remote_tests.sh:101: autotest. To use installed autotest, pass
--use_emerged."
On 2010/11/23 23:16:04, jrbarnette wrote:
> This informational message should only be printed if the script
> overrides the default setting of FLAGS_build.  As is, it will print even
> if the user already specified --build.
> 
> I think the logic needed is this:
>   if not --build and not --use_emerged and "cros_workon autotest"
>     info "Detected cros_workon ..."
>     FLAGS_build=TRUE

Or more simply, add a elif case ${FLAGS_build} -eq ${FLAGS_TRUE} which does
nothing but avoid showing the message.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode110
run_remote_tests.sh:110: if [ -d "${AUTOTEST_DIR}" ]; then
I would swap the order, if [ ! -d "${...}" ]; then
  print message
  die.

and then do the normal case outside the if afterwards.  Agree with Richard that
the emerged autotest command should only be shown if FLAGS_use_emerged was set.

[ericli, 2010-11-24 19:20:50]

Richard and Ken,

I had addressed all your comments and please take another look.

Ken,
I could not reproduce your failure stack trace. Would you try my patch on a new
client?

Eric

On 2010/11/23 23:30:41, kmixter1 wrote:
> http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh
> File run_remote_tests.sh (right):
> 
>
http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode101
> run_remote_tests.sh:101: autotest. To use installed autotest, pass
> --use_emerged."
> On 2010/11/23 23:16:04, jrbarnette wrote:
> > This informational message should only be printed if the script
> > overrides the default setting of FLAGS_build.  As is, it will print even
> > if the user already specified --build.
> > 
> > I think the logic needed is this:
> >   if not --build and not --use_emerged and "cros_workon autotest"
> >     info "Detected cros_workon ..."
> >     FLAGS_build=TRUE
> 
> Or more simply, add a elif case ${FLAGS_build} -eq ${FLAGS_TRUE} which does
> nothing but avoid showing the message.
> 
>
http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode110
> run_remote_tests.sh:110: if [ -d "${AUTOTEST_DIR}" ]; then
> I would swap the order, if [ ! -d "${...}" ]; then
>   print message
>   die.
> 
> and then do the normal case outside the if afterwards.  Agree with Richard
that
> the emerged autotest command should only be shown if FLAGS_use_emerged was
set.

[ericli, 2010-11-24 19:20:57]

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode90
run_remote_tests.sh:90: tc-export CC CXX PKG_CONFIG
On 2010/11/23 23:16:04, jrbarnette wrote:
> This sequence here seems a bit risky:  It pulls heaven knows what
> variable settings into the current script with no guarantee that there
> aren't conflicts that break the script.  It would probably be good to
> delay this until just before invoking autoserv, and then put it in a
> subshell for protection.

Done.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode96
run_remote_tests.sh:96: elif $(dirname $0)/cros_workon --board=${FLAGS_board}
list | \
On 2010/11/23 23:16:04, jrbarnette wrote:
> nit on the pre-existing code:  the '\' here isn't needed, and the style
> guidelines discourage it.

Done.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode101
run_remote_tests.sh:101: autotest. To use installed autotest, pass
--use_emerged."
On 2010/11/23 23:16:04, jrbarnette wrote:
> This informational message should only be printed if the script
> overrides the default setting of FLAGS_build.  As is, it will print even
> if the user already specified --build.
> 
> I think the logic needed is this:
>   if not --build and not --use_emerged and "cros_workon autotest"
>     info "Detected cros_workon ..."
>     FLAGS_build=TRUE

Done.

http://codereview.chromium.org/5176009/diff/10001/run_remote_tests.sh#newcode117
run_remote_tests.sh:117: "As requested, using emerged autotests already
installed at ${AUTOTEST_DIR}."
On 2010/11/23 23:16:04, jrbarnette wrote:
> The text of message originally depended on whether the user specified
> --use_emerged.  I think that that earlier distinction of the messages in the
> two cases was useful.

Done.

[kmixter1, 2010-11-24 19:50:13]

LGTM

The problem with directories was specific to my checkout.

I do worry that the autodetect is getting too complex to maintain, but there is
some value in telling the user exactly what they did and what prerequisites they
may have missed.

Please watch PFQ closely today if you push this today.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode116
run_remote_tests.sh:116: "Detected cros_workon autotests, Build and run
autotests from ${AUTOTEST_DIR}. \
Some clean up:

Detected cros_workon autotests.  Building and running your autotests from
${AUTOTEST_DIR}.  To use installed autotest, pass --use_emerged.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode122
run_remote_tests.sh:122: # flag use_emerged should be false once the code
reaches here.
I'm not seeing how that's correct when it enters this function as true.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode127
run_remote_tests.sh:127: "build flag was turned on but ${AUTOTEST_DIR} is not
found. Run cros_workon \
Capitalize

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode283
run_remote_tests.sh:283: '. ${BUILD_ENV};tc-export CC CXX
PKG_CONFIG;./server/autoserv ${autoserv_args}'
nit: Amperands may make this easier to diagnose if something fails

[ericli, 2010-11-24 20:47:54]

PTAL.

My response might worth another look from you.

Thanks.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode116
run_remote_tests.sh:116: "Detected cros_workon autotests, Build and run
autotests from ${AUTOTEST_DIR}. \
changed, but I kept emerged autotest instead of installed autotest.

On 2010/11/24 19:50:13, kmixter1 wrote:
> Some clean up:
> 
> Detected cros_workon autotests.  Building and running your autotests from
> ${AUTOTEST_DIR}.  To use installed autotest, pass --use_emerged.

Done.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode122
run_remote_tests.sh:122: # flag use_emerged should be false once the code
reaches here.
if you enter this function with flag used_emerged to TRUE, it will fall into the
code block between line 94~104, and it  will either die or return, so never
reach here.

On 2010/11/24 19:50:13, kmixter1 wrote:
> I'm not seeing how that's correct when it enters this function as true.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode283
run_remote_tests.sh:283: '. ${BUILD_ENV};tc-export CC CXX
PKG_CONFIG;./server/autoserv ${autoserv_args}'
On 2010/11/24 19:50:13, kmixter1 wrote:
> nit: Amperands may make this easier to diagnose if something fails

Done.

[kmixter1, 2010-11-24 23:31:21]

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode100
run_remote_tests.sh:100: else
please switch to fi.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode105
run_remote_tests.sh:105: elif [ ${FLAGS_build} -eq ${FLAGS_FALSE} ] && \
Please switch to:
  ...
  return
fi

if [ ${FLAGS_build} ] ....

This makes the control clearer - specifically that the use_emerged if block
always returns.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode114
run_remote_tests.sh:114: else
Please switch to fi as it shows the code following is the common case.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode116
run_remote_tests.sh:116: "Detected cros_workon autotests, Build and run
autotests from ${AUTOTEST_DIR}. \
On 2010/11/24 20:47:54, ericli wrote:
> changed, but I kept emerged autotest instead of installed autotest.
> 
> On 2010/11/24 19:50:13, kmixter1 wrote:
> > Some clean up:
> > 
> > Detected cros_workon autotests.  Building and running your autotests from
> > ${AUTOTEST_DIR}.  To use installed autotest, pass --use_emerged.
> 
> Done.

OK.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode122
run_remote_tests.sh:122: # flag use_emerged should be false once the code
reaches here.
On 2010/11/24 20:47:54, ericli wrote:
> if you enter this function with flag used_emerged to TRUE, it will fall into
the
> code block between line 94~104, and it  will either die or return, so never
> reach here.
> 
> On 2010/11/24 19:50:13, kmixter1 wrote:
> > I'm not seeing how that's correct when it enters this function as true.
> 

I see that now, but it's not clear.  I think some of the above simplifications
(fewer indented lines == fewer paths to consider) will make it a little clearer.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode127
run_remote_tests.sh:127: "build flag was turned on but ${AUTOTEST_DIR} is not
found. Run cros_workon \
On 2010/11/24 19:50:13, kmixter1 wrote:
> Capitalize

Sorry if it wasn't clear: I'm requesting to capitalize "Build".

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode129
run_remote_tests.sh:129: else
Please switch to fi as above.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode132
run_remote_tests.sh:132: else
Suggest switching to fi, and returning above this.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode138
run_remote_tests.sh:138: else
Please switch to fi, as above.

[ericli, 2010-11-25 02:23:07]

PTAL.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode100
run_remote_tests.sh:100: else
On 2010/11/24 23:31:21, kmixter1 wrote:
> please switch to fi.

Done.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode105
run_remote_tests.sh:105: elif [ ${FLAGS_build} -eq ${FLAGS_FALSE} ] && \
On 2010/11/24 23:31:21, kmixter1 wrote:
> Please switch to:
>   ...
>   return
> fi
> 
> if [ ${FLAGS_build} ] ....
> 
> This makes the control clearer - specifically that the use_emerged if block
> always returns.

Done.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode105
run_remote_tests.sh:105: elif [ ${FLAGS_build} -eq ${FLAGS_FALSE} ] && \
On 2010/11/24 23:31:21, kmixter1 wrote:
> Please switch to:
>   ...
>   return
> fi
> 
> if [ ${FLAGS_build} ] ....
> 
> This makes the control clearer - specifically that the use_emerged if block
> always returns.

Done.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode127
run_remote_tests.sh:127: "build flag was turned on but ${AUTOTEST_DIR} is not
found. Run cros_workon \
I dont fully understand, why?

On 2010/11/24 23:31:21, kmixter1 wrote:
> On 2010/11/24 19:50:13, kmixter1 wrote:
> > Capitalize
> 
> Sorry if it wasn't clear: I'm requesting to capitalize "Build".

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode129
run_remote_tests.sh:129: else
On 2010/11/24 23:31:21, kmixter1 wrote:
> Please switch to fi as above.

Done.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode132
run_remote_tests.sh:132: else
On 2010/11/24 23:31:21, kmixter1 wrote:
> Suggest switching to fi, and returning above this.

Done.

http://codereview.chromium.org/5176009/diff/17001/run_remote_tests.sh#newcode138
run_remote_tests.sh:138: else
On 2010/11/24 23:31:21, kmixter1 wrote:
> Please switch to fi, as above.

Done.

[jrbarnette, 2010-11-25 18:53:53]

The new logic in autodetect_build is much easier to follow, thanks!

I have only a few nits left.

http://codereview.chromium.org/5176009/diff/26001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/26001/run_remote_tests.sh#newcode106
run_remote_tests.sh:106: if [ ${FLAGS_build} -eq ${FLAGS_FALSE} ] && \
The trailing \ isn't needed (and I find it distracting).

http://codereview.chromium.org/5176009/diff/26001/run_remote_tests.sh#newcode282
run_remote_tests.sh:282: ./server/autoserv ${autoserv_args}'
This doesn't need "bash -c"; for a subshell, the following is sufficient:
  ( . ${BUILD_ENV} ... ; ./server/autoserve ... )
Using '()' for the subshell also means you don't need to export
variables (e.g. "export autoserv_args") to make them visible.

[ericli, 2010-11-29 21:51:12]

Richard's comments had been addressed in the last upload. Ken, do you have more
comments?

[kmixter1, 2010-11-29 22:04:36]

LGTM++ with minor comments.  Hopefully this avoids the tty problem because it is
a nice simplification.

http://codereview.chromium.org/5176009/diff/30001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/30001/run_remote_tests.sh#newcode128
run_remote_tests.sh:128: start autotest to continue."
Isn't it also necessary to repo sync in this case?

http://codereview.chromium.org/5176009/diff/30001/run_remote_tests.sh#newcode136
run_remote_tests.sh:136: autotest-tests or cros_workon start autotest to
continue."
If you cros_workon you probably need to repo sync as well.  The 'or' case seems
like it might confuse people.  I think it'd be ok to just only suggest the
emerge-board autotest autotest-tests solution.

[ericli, 2010-11-29 22:12:37]

Ken, 
Last change made based on your suggestion.

I am pushing now. And I will keep an eye on PFQ.

http://codereview.chromium.org/5176009/diff/30001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5176009/diff/30001/run_remote_tests.sh#newcode128
run_remote_tests.sh:128: start autotest to continue."
Yes, true. But this reflect what it had before this refactoring CL.

I will change it to:

Run cros_workon start autotest and repo sync to continue.
On 2010/11/29 22:04:36, kmixter1 wrote:
> Isn't it also necessary to repo sync in this case?

http://codereview.chromium.org/5176009/diff/30001/run_remote_tests.sh#newcode136
run_remote_tests.sh:136: autotest-tests or cros_workon start autotest to
continue."
OK.

On 2010/11/29 22:04:36, kmixter1 wrote:
> If you cros_workon you probably need to repo sync as well.  The 'or' case
seems
> like it might confuse people.  I think it'd be ok to just only suggest the
> emerge-board autotest autotest-tests solution.