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

run_remote_tests.sh

Index: run_remote_tests.sh
diff --git a/run_remote_tests.sh b/run_remote_tests.sh
index babbbdceb045f371810f96c7da1063de67f90a65..c5f7ad7c39bc034d379fd1b12c54bc7cf26f783d 100755
--- a/run_remote_tests.sh
+++ b/run_remote_tests.sh
@@ -64,7 +64,7 @@ function create_tmp() {
   # Set global TMP for remote_access.sh's sake
   # and if --results_dir_root is specified,
   # set TMP and create dir appropriately
-  if [[ ${INSIDE_CHROOT} -eq 0 ]]; then
+  if [[ ${INSIDE_CHROOT} -eq ${FLAGS_TRUE} ]]; then

[kmixter1, 2010/11/18 22:33:38]

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.

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

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:

[kmixter1, 2010/11/18 23:59:39]

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.

     if [[ -n "${FLAGS_results_dir_root}" ]]; then
       TMP=${FLAGS_chroot}${FLAGS_results_dir_root}
       mkdir -p -m 777 ${TMP}
@@ -255,8 +255,13 @@ function main() {
     fi
 
     local results_dir_name="${short_name}"
-    local results_dir="${TMP_INSIDE_CHROOT}/${results_dir_name}"
-    rm -rf "${results_dir}"
+    local results_dir
+    if [[ ${FLAGS_build} -eq ${FLAGS_TRUE} ]]; then
+      results_dir="${TMP_INSIDE_CHROOT}/${results_dir_name}"

[kmixter1, 2010/11/18 22:33:38]

I find this more confusing.  Why not just keep the original logic to
${ENTER_CHROOT} in build and non-build cases?

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

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:

[kmixter1, 2010/11/18 23:59:39]

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.

+    else
+      results_dir="${TMP}/${results_dir_name}"
+    fi
+    sudo rm -rf "${results_dir}"
     local verbose=""
     if [[ ${FLAGS_verbose} -eq $FLAGS_TRUE ]]; then
       verbose="--verbose"
@@ -271,25 +276,21 @@ function main() {
       info "Running chrome autotest ${control_file}"
     fi
 
-    export AUTOSERV_TEST_ARGS="${FLAGS_args}"
-    export AUTOSERV_ARGS="-m ${FLAGS_remote} \
-        --ssh-port ${FLAGS_ssh_port} \
+    local autoserv_test_args="${FLAGS_args}"
+    if [ -n "${autoserv_test_args}" ]; then
+      autoserv_test_args="-a \"${autoserv_test_args}\""
+    fi
+    local autoserv_args="-m ${FLAGS_remote} --ssh-port ${FLAGS_ssh_port} \
         ${option} ${control_file} -r ${results_dir} ${verbose}"
     if [ ${FLAGS_build} -eq ${FLAGS_FALSE} ]; then
-      cat > "${TMP}/run_test.sh" <<EOF
-export AUTOSERV_TEST_ARGS="${AUTOSERV_TEST_ARGS}"
-export AUTOSERV_ARGS="${AUTOSERV_ARGS}"
-cd /home/${USER}/trunk/src/scripts
-./autotest_run.sh --board "${FLAGS_board}"
-EOF
-      chmod a+rx "${TMP}/run_test.sh"
-      ${ENTER_CHROOT} ${TMP_INSIDE_CHROOT}/run_test.sh >&2
+      pushd ${autotest_dir} >/dev/null
+      sudo chmod a+w ./server/{tests,site_tests}
+      echo ./server/autoserv ${autoserv_args} ${autoserv_test_args}
+      ./server/autoserv ${autoserv_args} ${autoserv_test_args}

[kmixter1, 2010/11/18 22:33:38]

It seems better to always run autoserv inside the chroot.  That would also
remove the need to change results_dir conditionally.

+      popd >/dev/null
     else
       cp "${BUILD_DIR}/environment" "${TMP}/run_test.sh"
       GRAPHICS_BACKEND=${GRAPHICS_BACKEND:-OPENGL}
-      if [ -n "${AUTOSERV_TEST_ARGS}" ]; then
-        AUTOSERV_TEST_ARGS="-a \"${AUTOSERV_TEST_ARGS}\""
-      fi
       cat >> "${TMP}/run_test.sh" <<EOF
 export GCLIENT_ROOT=/home/${USER}/trunk
 export GRAPHICS_BACKEND=${GRAPHICS_BACKEND}
@@ -297,7 +298,8 @@ export SSH_AUTH_SOCK=${SSH_AUTH_SOCK} TMPDIR=/tmp SSH_AGENT_PID=${SSH_AGENT_PID}
 export SYSROOT=/build/${FLAGS_board}
 tc-export CC CXX PKG_CONFIG
 cd ${INSIDE_BUILD_DIR}
-./server/autoserv ${AUTOSERV_ARGS} ${AUTOSERV_TEST_ARGS}
+echo ./server/autoserv ${autoserv_args} ${autoserv_test_args}
+./server/autoserv ${autoserv_args} ${autoserv_test_args}
 EOF
       sudo cp "${TMP}/run_test.sh" "${BUILD_DIR}"
       sudo chmod a+rx "${BUILD_DIR}/run_test.sh"