Make enter_chroot not be chatty unless you use --verbose

enter_chroot.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 enter the chroot environment
 
 # --- BEGIN COMMON.SH BOILERPLATE ---
 # Load common CrOS utilities.  Inside the chroot this file is installed in
@@ skipping 33 matching lines @@
 DEFINE_string chrome_root_mount "/home/$USER/chrome_root" \
   "The mount point of the chrome broswer source in the chroot."
 
 DEFINE_boolean git_config $FLAGS_TRUE \
   "Config git to work with your user/pass in the chroot."
 DEFINE_boolean official_build $FLAGS_FALSE \
   "Set CHROMEOS_OFFICIAL=1 for release builds."
 DEFINE_boolean mount $FLAGS_FALSE "Only set up mounts."
 DEFINE_boolean unmount $FLAGS_FALSE "Only tear down mounts."
 DEFINE_boolean ssh_agent $FLAGS_TRUE "Import ssh agent."
+DEFINE_boolean verbose $FLAGS_FALSE "Print out actions taken"
 
 # More useful help
 FLAGS_HELP="USAGE: $0 [flags] [VAR=value] [-- command [arg1] [arg2] ...]
 
 One or more VAR=value pairs can be specified to export variables into
 the chroot environment.  For example:
 
    $0 FOO=bar BAZ=bel
 
 If [-- command] is present, runs the command inside the chroot,
 after changing directory to /$USER/trunk/src/scripts.  Note that neither
 the command nor args should include single quotes.  For example:
 
     $0 -- ./build_platform_packages.sh
 
 Otherwise, provides an interactive shell.
 "
 
+# Version of info from common.sh that only echos if --verbose is set.
+function v_info {

[kliegs, 2011/02/04 22:46:50]

Would it be better to just shift the output levels used? So things that are info
but should stay get promoted to warning?

And things that should be suppressed are either left at INFO or moved to
DEBUG/NOTICE?

There are some (mostly) standard names for output messages (ERROR, WARN, INFO,
DEBUG, NOTICE).  Adding a new one (V_INFO) seems unnecessary.

[dgarrett, 2011/02/04 23:57:43]

I just checked, and DEBUG doesn't seem to exist for shell scripts. There also
doesn't seem to be any way to adjust which levels are displayed. I'd checked for
that before submitting this.

I think created a new 'level' for this script is the only way to go.

On 2011/02/04 22:46:50, kliegs wrote:

[kliegs, 2011/02/05 07:55:54]

Even if DEBUG doesn't exist I'd rather see you call your new function DEBUG than
VINFO.  VINFO has no meaning to anyone but people all understand what DEBUG
means.  Having the flag that enables it called verbose or similar is fine - a
--verbose or -v flag is a common mechanism for shell scripts.  

Creating a new level just strikes me as a really bad idea.  Every other project
I've been part of fights really hard to not add new log levels and there's a
reason for it.  I'd like to see us do the same and use the standard levels that
the industry is familiar with.  It should at least be done with some real
thought - breaking convention shouldn't be done lightly.

On 2011/02/04 23:57:43, dgarrett wrote:

+  if [ $FLAGS_verbose -eq $FLAGS_TRUE ]; then
+    info "$1"
+  fi
+}
+
 # Double up on the first '--' argument.  Why?  For enter_chroot, we want to
 # emulate the behavior of sudo for setting environment vars.  That is, we want:
 #   ./enter_chroot [flags] [VAR=val] [-- command]
 # ...but shflags ends up eating the '--' out of the command line and gives
 # us back "VAR=val" and "command" together in one chunk.  By doubling up, we
 # end up getting what we want back from shflags.
 #
 # Examples of how people might be using enter_chroot:
 #  1. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 -- cmd arg1 arg2
 #     Set env vars and run cmd w/ args
@@ skipping 45 matching lines @@
 function setup_env {
   # Validate sudo timestamp before entering the critical section so that we
   # don't stall for a password while we have the lockfile.
   # Don't use sudo -v since that has issues on machines w/ no password.
   sudo echo "" > /dev/null
 
   (
     flock 200
     echo $$ >> "$LOCKFILE"
 
-    info "Mounting chroot environment."
+    v_info "Mounting chroot environment."
 
     # Mount only if not already mounted
     MOUNTED_PATH="$(readlink -f "$FLAGS_chroot/proc")"
     if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]; then
       sudo mount none -t proc "$MOUNTED_PATH" || \
           die "Could not mount $MOUNTED_PATH"
     fi
 
     MOUNTED_PATH="$(readlink -f "$FLAGS_chroot/sys")"
     if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]; then
@@ skipping 33 matching lines @@
     fi
 
     MOUNTED_PATH="$(readlink -f "${FLAGS_chroot}${INNER_CHROME_ROOT}")"
     if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]; then
       ! CHROME_ROOT="$(readlink -f "$FLAGS_chrome_root")"
       if [ -z "$CHROME_ROOT" ]; then
         ! CHROME_ROOT="$(cat "${FLAGS_chroot}${CHROME_ROOT_CONFIG}" \
           2>/dev/null)"
       fi
       if [[ ( -z "$CHROME_ROOT" ) || ( ! -d "${CHROME_ROOT}/src" ) ]]; then
-        info "Not mounting chrome source"
+        v_info "Not mounting chrome source"

[kliegs, 2011/02/04 22:46:50]

This can actually be an error if CHROME_ROOT is set but doesn't exist. If you're
going to suppress it in the case where CHROME_ROOT is not set you should add it
as an error for when it is

[dgarrett, 2011/02/04 23:57:43]

Currently, this error is displayed every time you enter without valid Chrome
source. That's just noise.

On 2011/02/04 22:46:50, kliegs wrote:

[kliegs, 2011/02/05 07:55:54]

We're not in the chromite world yet.  Our entire team mounts chrome outside the
chroot and typos on the path are fairly common.  I know most teams ignore Chrome
and only use whatever the ebuild gets them but losing this warning is bad.

What I was suggesting wasn't to make it always say that.  But instead of
removing it fully, only remove it when -z is true. So it'd become

if [[ -z "$CHROME_ROOT" ]]; then
  v_info "Not mounting chrome source"
  sudo rm -rf "${FLAGS_chroot}${CHROME_ROOT_CONFIG}"
elif  [[ ! -d "${CHROME_ROOT}/src" ]]; then
  error "Not mounting chrome source"
else 
  v_info "Mounting chrome source at: $INNER_CHROME_ROOT"
   ...
  
On 2011/02/04 23:57:43, dgarrett wrote:

         sudo rm -f "${FLAGS_chroot}${CHROME_ROOT_CONFIG}"
       else
-        info "Mounting chrome source at: $INNER_CHROME_ROOT"
+        v_info "Mounting chrome source at: $INNER_CHROME_ROOT"
         echo "$CHROME_ROOT" | \
           sudo dd of="${FLAGS_chroot}${CHROME_ROOT_CONFIG}"
         mkdir -p "$MOUNTED_PATH"
         sudo mount --bind "$CHROME_ROOT" "$MOUNTED_PATH" || \
           die "Could not mount $MOUNTED_PATH"
       fi
     fi
 
     MOUNTED_PATH="$(readlink -f "${FLAGS_chroot}${INNER_DEPOT_TOOLS_ROOT}")"
     if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]; then
       if [ $(which gclient 2>/dev/null) ]; then
-        info "Mounting depot_tools"
+        v_info "Mounting depot_tools"
         DEPOT_TOOLS=$(dirname "$(which gclient)")
         mkdir -p "$MOUNTED_PATH"
         if ! sudo mount --bind "$DEPOT_TOOLS" "$MOUNTED_PATH"; then
           warn "depot_tools failed to mount; perhaps it's on NFS?"
           warn "This may impact chromium build."
         fi
       fi
     fi
 
     # Install fuse module.
@@ skipping 49 matching lines @@
     # Remove any dups from lock file while installing new one
     sort -n "$TMP_LOCKFILE" | uniq > "$LOCKFILE"
 
     if [ $(which gconftool-2 2>/dev/null) ]; then
       SAVED_PREF=$(cat "${FLAGS_chroot}${SAVED_AUTOMOUNT_PREF_FILE}")
       gconftool-2 -s --type=boolean ${AUTOMOUNT_PREF} ${SAVED_PREF} || \
         warn "could not re-set your automount preference."
     fi
 
     if [ -s "$LOCKFILE" ]; then
-      info "At least one other pid is running in the chroot, so not"
-      info "tearing down env."
+      v_info "At least one other pid is running in the chroot, so not"
+      v_info "tearing down env."

[kliegs, 2011/02/04 22:46:50]

This feels like a warning to me, not an error.  Depending on the situation it
can often mean something is wrong and not just be an advisory info.

[dgarrett, 2011/02/04 23:57:43]

It's an awfully normal case though.

On 2011/02/04 22:46:50, kliegs wrote:

[kliegs, 2011/02/05 07:55:54]

Its a very rare case for me.  I almost never see this unless something is wrong.
 Its also been instrumental in finding some problems with the build bots (will
they be running with verbose?)  How sure are you this is a common case?
On 2011/02/04 23:57:43, dgarrett wrote:

     else
       MOUNTED_PATH=$(readlink -f "$FLAGS_chroot")
-      info "Unmounting chroot environment."
+      v_info "Unmounting chroot environment."
       # sort the list of mounts in reverse order, to ensure umount of
       # cascading mounts in proper order
       for i in \
         $(mount | grep -F "on $MOUNTED_PATH/" | sort -r | awk '{print $3}'); do
         safe_umount "$i"
       done
     fi
   ) 200>>"$LOCKFILE" || die "teardown_env failed"
 }
 
 if [ $FLAGS_mount -eq $FLAGS_TRUE ]; then
   setup_env
-  info "Make sure you run"
-  info "    $0 --unmount"
-  info "before deleting $FLAGS_chroot"
-  info "or you'll end up deleting $FLAGS_trunk too!"
+  v_info "Make sure you run"
+  v_info "    $0 --unmount"
+  v_info "before deleting $FLAGS_chroot"
+  v_info "or you'll end up deleting $FLAGS_trunk too!"
   exit 0
 fi
 
 if [ $FLAGS_unmount -eq $FLAGS_TRUE ]; then
   teardown_env
   exit 0
 fi
 
 # Apply any hacks needed to update the chroot.
 chroot_hacks_from_outside "${FLAGS_chroot}"
@@ skipping 19 matching lines @@
 # Use git:8 chars of sha1
 REVISION=$(cd ${FLAGS_trunk}/src/scripts ; git rev-parse --short=8 HEAD)
 CHROOT_PASSTHRU="CHROMEOS_REVISION=$REVISION BUILDBOT_BUILD=$FLAGS_build_number CHROMEOS_OFFICIAL=$CHROMEOS_OFFICIAL"
 CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \
 CHROMEOS_RELEASE_APPID=${CHROMEOS_RELEASE_APPID:-"{DEV-BUILD}"}"
 CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \
 CHROMEOS_VERSION_TRACK=$CHROMEOS_VERSION_TRACK CHROMEOS_VERSION_AUSERVER=$CHROMEOS_VERSION_AUSERVER CHROMEOS_VERSION_DEVSERVER=$CHROMEOS_VERSION_DEVSERVER"
 
 if [ -d "$HOME/.subversion" ]; then
   # Bind mounting .subversion into chroot
-  info "mounting ~/.subversion into chroot"
+  v_info "mounting ~/.subversion into chroot"
   MOUNTED_PATH="$(readlink -f "${FLAGS_chroot}/home/${USER}/.subversion")"
   if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]; then
     mkdir -p "$MOUNTED_PATH"
     sudo mount --bind "$HOME/.subversion" "$MOUNTED_PATH" || \
       die "Could not mount $MOUNTED_PATH"
   fi
 fi
 
 # Configure committer username and email in chroot .gitconfig
 if [ $FLAGS_git_config -eq $FLAGS_TRUE ]; then
   git config -f ${FLAGS_chroot}/home/${USER}/.gitconfig --replace-all \
     user.name "$(cd /tmp; git var GIT_COMMITTER_IDENT | sed -e 's/ *<.*//')"
   git config -f ${FLAGS_chroot}/home/${USER}/.gitconfig --replace-all \
     user.email "$(cd /tmp; git var GIT_COMMITTER_IDENT | \
       sed -e 's/.*<\([^>]*\)>.*/\1/')"
 fi
 
 # Run command or interactive shell.  Also include the non-chrooted path to
 # the source trunk for scripts that may need to print it (e.g.
 # build_image.sh).
 sudo -- chroot "$FLAGS_chroot" sudo -i -u $USER $CHROOT_PASSTHRU \
   EXTERNAL_TRUNK_PATH="${FLAGS_trunk}" LANG=C SSH_AGENT_PID="${SSH_AGENT_PID}" \
   SSH_AUTH_SOCK="${SSH_AUTH_SOCK}" "$@"
 
 # Remove trap and explicitly unmount
 trap - EXIT
 teardown_env