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

run_remote_tests.sh

 #!/bin/bash
 
 # Copyright (c) 2009 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Script to run client or server tests on a live remote image.
 
 # Load common constants.  This should be the first executable line.
 # The path to common.sh should be relative to your script's location.
@@ skipping 46 matching lines @@
   if [[ "${type}" != "client" ]] && [[ "${type}" != "server" ]]; then
     die "Unknown type of test (${type}) in ${control_file}"
   fi
   echo ${type}
 }
 
 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}
     else
       TMP=$(mktemp -d ${FLAGS_chroot}/tmp/run_remote_tests.XXXX)
     fi
     TMP_INSIDE_CHROOT=$(echo ${TMP#${FLAGS_chroot}})
   else
     if [[ -n "${FLAGS_results_dir_root}" ]]; then
       TMP=${FLAGS_results_dir_root}
@@ skipping 170 matching lines @@
     # testName/some_control --> testName.some_control
     if [[ "${control_file_name}" != control ]]; then
       if [[ "${control_file_name}" == control.* ]]; then
         short_name=${short_name}.${control_file_name/control./}
       else
         short_name=${short_name}.${control_file_name}
       fi
     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"
     fi
 
     RAN_ANY_TESTS=${FLAGS_TRUE}
 
     # Remove chrome autotest location prefix from control_file if needed
     if [[ ${control_file:0:${#chrome_autotests}} == \
           "${chrome_autotests}" ]]; then
       control_file="${control_file:${#chrome_autotests}+1}"
       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}
 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"
       ${ENTER_CHROOT} sudo bash -c "${INSIDE_BUILD_DIR}/run_test.sh" >&2
     fi
   done
 
   echo ""
   info "Test results:"
   ./generate_test_report "${TMP}" --strip="${TMP}/"
 
   print_time_elapsed
 }
 
 main "$@"