Remove the dependency to autotest_run.sh so I can get rid of it in next CL.

Remove the dependency to autotest_run.sh so I can get rid of it in next CL.

Change-Id: Ieaa2553612aefd6161785185350d495cbf0dad81

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

w/wo emerge autotest
w/wo cros_workon
in/outside of chroot.
w/wo --used_emerged
w/wo --results_dir_root

and all its combinations.

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

[ericli, 2010-11-18 21:48:32]

Please re-review.

[kmixter1, 2010-11-18 22:33:37]

A few comments.  Sorry, I've had it on my todo list to remove that script and
didn't get around to it yet.  I prepared a CL

http://codereview.chromium.org/5235001

(not tested) that seems like it should remove that script with minimal changes
if it's helpful.

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

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode67
run_remote_tests.sh:67: if [[ ${INSIDE_CHROOT} -eq ${FLAGS_TRUE} ]]; then
This if statement is saying if i-am-outside-the-chroot; then...  It happens that
${FLAGS_TRUE} == 0 so your change is logically the same, but it's backwards from
how you would read it.

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode260
run_remote_tests.sh:260: results_dir="${TMP_INSIDE_CHROOT}/${results_dir_name}"
I find this more confusing.  Why not just keep the original logic to
${ENTER_CHROOT} in build and non-build cases?

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode289
run_remote_tests.sh:289: ./server/autoserv ${autoserv_args}
${autoserv_test_args}
It seems better to always run autoserv inside the chroot.  That would also
remove the need to change results_dir conditionally.

[ericli, 2010-11-18 23:05:22]

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

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode67
run_remote_tests.sh:67: if [[ ${INSIDE_CHROOT} -eq ${FLAGS_TRUE} ]]; then
my interpretation of this statement is "if I am inside chroot" then ...

outside chroot logic is in the else block?

On 2010/11/18 22:33:38, kmixter1 wrote:
> This if statement is saying if i-am-outside-the-chroot; then...  It happens
that
> ${FLAGS_TRUE} == 0 so your change is logically the same, but it's backwards
from
> how you would read it.

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode260
run_remote_tests.sh:260: results_dir="${TMP_INSIDE_CHROOT}/${results_dir_name}"
Address both of your concerns: I agree. This is more like a workaround now.
Sorry about that.

As I discussed/showed with you before, my next CL will simplify the in/outside
chroot logic, significantly.

This is also partly because I followed your suggestion to split the changes into
two CLs in preparing for revert.

And once this CL passed PFQ, I will send in the next one, which will have a much
more clear logic, and much less if/then based on chroot.

So I will beg your pardon to stay with it for a while, if possible.


On 2010/11/18 22:33:38, kmixter1 wrote:
> I find this more confusing.  Why not just keep the original logic to
> ${ENTER_CHROOT} in build and non-build cases?

[kmixter1, 2010-11-18 23:59:38]

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

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode67
run_remote_tests.sh:67: if [[ ${INSIDE_CHROOT} -eq ${FLAGS_TRUE} ]]; then
On 2010/11/18 23:05:22, ericli wrote:
> my interpretation of this statement is "if I am inside chroot" then ...
> 
> outside chroot logic is in the else block?

The variable names may be confusing.  TMP_INSIDE_CHROOT is the path from inside
the chroot to the tmp directory in the chroot, it always starts with /tmp.  TMP
is that path this script has to use to access that directory (so it's the same
when we're running inside chroot, and prefixed with FLAGS_chroot outside).

The if block is outside.  If you're outside the chroot, we create TMP= a path
prefixed by the path to the chroot and TMP_INSIDE_CHROOT is that path with the
chroot path stripped.  and if you're inside (else clause) we create a tmp at
"/tmp" and TMP_INSIDE_CHROOT is same as TMP.

> 
> On 2010/11/18 22:33:38, kmixter1 wrote:
> > This if statement is saying if i-am-outside-the-chroot; then...  It happens
> that
> > ${FLAGS_TRUE} == 0 so your change is logically the same, but it's backwards
> from
> > how you would read it.
>

http://codereview.chromium.org/5212003/diff/10001/run_remote_tests.sh#newcode260
run_remote_tests.sh:260: results_dir="${TMP_INSIDE_CHROOT}/${results_dir_name}"
On 2010/11/18 23:05:22, ericli wrote:
> Address both of your concerns: I agree. This is more like a workaround now.
> Sorry about that.
> 
> As I discussed/showed with you before, my next CL will simplify the in/outside
> chroot logic, significantly.
> 
> This is also partly because I followed your suggestion to split the changes
into
> two CLs in preparing for revert.
> 
> And once this CL passed PFQ, I will send in the next one, which will have a
much
> more clear logic, and much less if/then based on chroot.
> 
> So I will beg your pardon to stay with it for a while, if possible.
> 
> 
> On 2010/11/18 22:33:38, kmixter1 wrote:
> > I find this more confusing.  Why not just keep the original logic to
> > ${ENTER_CHROOT} in build and non-build cases?
> 

Why not just change the code to run autoserv inside the chroot?  The goal of
creating two CLs was that each is good on its own.  This change now makes us
sometimes run autotest outside the chroot and with extra confusion above.  You
can check this in, but if we need to revert your later change because it causes
hangs in developers terminals, we'll need to fix added confusion up later.

[ericli, 2010-11-19 02:52:38]

PTAL.

Ken,

Your concern has been addressed now, and I had to fall back to your original
path to cat out an external file, kind of bad. But maximumly reserved your
original logic in this CL.

[kmixter1, 2010-11-19 18:34:08]

LGTM++ just a few nits

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh#newcode281
run_remote_tests.sh:281: cat > "${TMP}/run_test.sh" <<EOF
indent wrong

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh#newcode282
run_remote_tests.sh:282: cd /build/${FLAGS_board}/usr/local/autotest
use ${autotest_dir}?

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh#newcode288
run_remote_tests.sh:288: ${ENTER_CHROOT} ${TMP_INSIDE_CHROOT}/run_test.sh  >&2
extra space

[ericli, 2010-11-19 18:42:21]

Thanks. I will push now and watch PFQ, hopefully it will pass through this time.

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh
File run_remote_tests.sh (right):

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh#newcode281
run_remote_tests.sh:281: cat > "${TMP}/run_test.sh" <<EOF
On 2010/11/19 18:34:08, kmixter1 wrote:
> indent wrong

Done.

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh#newcode282
run_remote_tests.sh:282: cd /build/${FLAGS_board}/usr/local/autotest
when run outside of chroot, ${autotest_dir} is pointed to
/usr/local/google/home/ericli/chromiumos/cros3/chroot/build/x86-generic/usr/local/autotest

not a good path inside chroot.

On 2010/11/19 18:34:08, kmixter1 wrote:
> use ${autotest_dir}?

http://codereview.chromium.org/5212003/diff/20001/run_remote_tests.sh#newcode288
run_remote_tests.sh:288: ${ENTER_CHROOT} ${TMP_INSIDE_CHROOT}/run_test.sh  >&2
On 2010/11/19 18:34:08, kmixter1 wrote:
> extra space

Done.