Make enter_chroot not be chatty unless you use --verbose

enter_chroot.sh

Index: enter_chroot.sh
diff --git a/enter_chroot.sh b/enter_chroot.sh
index aa6a45bee88669a6a57d84089aa0bf8618220059..f9975003e7994686b8447b4ef05c65b1a2925325 100755
--- a/enter_chroot.sh
+++ b/enter_chroot.sh
@@ -51,6 +51,7 @@ DEFINE_boolean official_build $FLAGS_FALSE \
 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] ...]
@@ -69,6 +70,13 @@ the command nor args should include single quotes.  For example:
 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]
@@ -134,7 +142,7 @@ function setup_env {
     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")"
@@ -188,10 +196,10 @@ function setup_env {
           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"
@@ -203,7 +211,7 @@ function setup_env {
     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
@@ -273,11 +281,11 @@ function teardown_env {
     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 \
@@ -290,10 +298,10 @@ function teardown_env {
 
 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
 
@@ -333,7 +341,7 @@ CHROMEOS_VERSION_TRACK=$CHROMEOS_VERSION_TRACK CHROMEOS_VERSION_AUSERVER=$CHROME
 
 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"