Turn off logging by default

Fix race condition in hook loader by disabling logging
by default.

Fixes bug #152

--HG--
rename : tests/test_log_dir.sh => tests/test_log_file.sh

Author: dhellmann

docs/source/history.rst

@@ -8,6 +8,13 @@ dev
     mode. (:bbuser:`upsuper`)
   - Add ``--help`` option to ``mkproject``.
   - Add ``--help`` option to ``workon``.
+  - Turn off logging from the hook loader by default, and replace
+    ``VIRTUALENVWRAPPER_LOG_DIR`` environment variable with
+    ``VIRTUALENVWRAPPER_LOG_FILE``. The rotating log behavior remains
+    the same. The motivation for this change is the race condition
+    caused by that rotating behavior, especially when the wrappers are
+    being used by users with different permissions and
+    umasks. (:bbissue:`152`)
 
 3.6.1
 

docs/source/install.rst

@@ -220,14 +220,14 @@ default is ``$WORKON_HOME``.
 
    * :ref:`scripts`
 
-.. _variable-VIRTUALENVWRAPPER_LOG_DIR:
+.. _variable-VIRTUALENVWRAPPER_LOG_FILE:
 
 Location of Hook Logs
 ---------------------
 
-The variable ``VIRTUALENVWRAPPER_LOG_DIR`` tells virtualenvwrapper
+The variable ``VIRTUALENVWRAPPER_LOG_FILE`` tells virtualenvwrapper
 where the logs for the hook loader should be written. The default is
-``$WORKON_HOME``.
+to not log from the hooks.
 
 .. _variable-VIRTUALENVWRAPPER_VIRTUALENV:
 

tests/test.sh

@@ -26,7 +26,6 @@ test_virtualenvwrapper_initialize() {
         assertTrue "Global $WORKON_HOME/$hook was not created" "[ -f $WORKON_HOME/$hook ]"
         assertTrue "Global $WORKON_HOME/$hook is not executable" "[ -x $WORKON_HOME/$hook ]"
     done
-    assertTrue "Log file was not created" "[ -f $WORKON_HOME/hook.log ]"
     export pre_test_dir=$(cd "$test_dir"; pwd)
     echo "echo GLOBAL initialize >> \"$pre_test_dir/catch_output\"" >> "$WORKON_HOME/initialize"
     virtualenvwrapper_initialize

tests/test_log_dir.sh renamed to tests/test_log_file.sh

@@ -8,27 +8,25 @@ setUp () {
 }
 
 test_set_by_user() {
-    export VIRTUALENVWRAPPER_LOG_DIR="$WORKON_HOME/logs"
-    mkdir -p "$VIRTUALENVWRAPPER_LOG_DIR"
+    export VIRTUALENVWRAPPER_LOG_FILE="$WORKON_HOME/hooks.log"
     source "$test_dir/../virtualenvwrapper.sh"
-    assertTrue "Log file was not created" "[ -f $WORKON_HOME/logs/hook.log ]"
+    assertTrue "Log file was not created" "[ -f $VIRTUALENVWRAPPER_LOG_FILE ]"
 }
 
 test_file_permissions() {
-    export VIRTUALENVWRAPPER_LOG_DIR="$WORKON_HOME/logs"
-    mkdir -p "$VIRTUALENVWRAPPER_LOG_DIR"
+    export VIRTUALENVWRAPPER_LOG_FILE="$WORKON_HOME/hooks.log"
     source "$test_dir/../virtualenvwrapper.sh"
-    perms=$(ls -l "$WORKON_HOME/logs/hook.log" | cut -f1 -d' ')
+    perms=$(ls -l "$VIRTUALENVWRAPPER_LOG_FILE" | cut -f1 -d' ')
     #echo $perms
     assertTrue "Log file permissions are wrong: $perms" "echo $perms | grep '^-rw-rw'"
 }
 
 test_not_set_by_user() {
     unset WORKON_HOME
-    unset VIRTUALENVWRAPPER_LOG_DIR
+    unset VIRTUALENVWRAPPER_LOG_FILE
     unset VIRTUALENVWRAPPER_HOOK_DIR
     source "$test_dir/../virtualenvwrapper.sh"
-    assertSame "$WORKON_HOME" "$VIRTUALENVWRAPPER_LOG_DIR"
+    assertSame "" "$VIRTUALENVWRAPPER_LOG_FILE"
 }
 
 . "$test_dir/shunit2"

tests/test_run_hook.sh

@@ -64,11 +64,4 @@ test_virtualenvwrapper_run_hook_permissions() {
     assertSame "Errno 13] Permission denied" "$error"
 }
 
-test_virtualenvwrapper_run_hook_without_log_dir() {
-    old_log_dir="$VIRTUALENVWRAPPER_LOG_DIR"
-    unset VIRTUALENVWRAPPER_LOG_DIR
-    assertFalse "virtualenvwrapper_run_hook initialize"
-    export VIRTUALENVWRAPPER_LOG_DIR="$old_log_dir"
-}
-
 . "$test_dir/shunit2"

virtualenvwrapper.sh

@@ -202,12 +202,6 @@ function virtualenvwrapper_run_hook {
 
     hook_script="$(virtualenvwrapper_tempfile ${1}-hook)" || return 1
 
-    if [ -z "$VIRTUALENVWRAPPER_LOG_DIR" ]
-    then
-        echo "ERROR: VIRTUALENVWRAPPER_LOG_DIR is not set." 1>&2
-        command \rm -f "$hook_script"
-        return 1
-    fi
     "$VIRTUALENVWRAPPER_PYTHON" -c 'from virtualenvwrapper.hook_loader import main; main()' $HOOK_VERBOSE_OPTION --script "$hook_script" "$@"
     result=$?
 
@@ -286,12 +280,6 @@ function virtualenvwrapper_initialize {
         export VIRTUALENVWRAPPER_HOOK_DIR="$WORKON_HOME"
     fi
 
-    # Set the location of the hook script logs
-    if [ "$VIRTUALENVWRAPPER_LOG_DIR" = "" ]
-    then
-        export VIRTUALENVWRAPPER_LOG_DIR="$WORKON_HOME"
-    fi
-
     virtualenvwrapper_run_hook "initialize"
 
     virtualenvwrapper_setup_tab_completion

virtualenvwrapper/hook_loader.py

@@ -85,16 +85,17 @@ def main():
     root_logger = logging.getLogger('')
 
     # Set up logging to a file
-    root_logger.setLevel(logging.DEBUG)
-    file_handler = GroupWriteRotatingFileHandler(
-        os.path.expandvars(os.path.join('$VIRTUALENVWRAPPER_LOG_DIR',
-                                        'hook.log')),
-        maxBytes=10240,
-        backupCount=1,
-        )
-    formatter = logging.Formatter(LOG_FORMAT)
-    file_handler.setFormatter(formatter)
-    root_logger.addHandler(file_handler)
+    logfile = os.environ.get('VIRTUALENVWRAPPER_LOG_FILE')
+    if logfile:
+        root_logger.setLevel(logging.DEBUG)
+        file_handler = GroupWriteRotatingFileHandler(
+            logfile,
+            maxBytes=10240,
+            backupCount=1,
+            )
+        formatter = logging.Formatter(LOG_FORMAT)
+        file_handler.setFormatter(formatter)
+        root_logger.addHandler(file_handler)
 
     # Send higher-level messages to the console, too
     console = logging.StreamHandler()