Commits

Doug Hellmann committed 71aa555

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

Comments (0)

Files changed (8)

docs/source/history.rst

     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

 
    * :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:
 
         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

-#!/bin/sh
-
-test_dir=$(cd $(dirname $0) && pwd)
-source "$test_dir/setup.sh"
-
-setUp () {
-    echo
-}
-
-test_set_by_user() {
-    export VIRTUALENVWRAPPER_LOG_DIR="$WORKON_HOME/logs"
-    mkdir -p "$VIRTUALENVWRAPPER_LOG_DIR"
-    source "$test_dir/../virtualenvwrapper.sh"
-    assertTrue "Log file was not created" "[ -f $WORKON_HOME/logs/hook.log ]"
-}
-
-test_file_permissions() {
-    export VIRTUALENVWRAPPER_LOG_DIR="$WORKON_HOME/logs"
-    mkdir -p "$VIRTUALENVWRAPPER_LOG_DIR"
-    source "$test_dir/../virtualenvwrapper.sh"
-    perms=$(ls -l "$WORKON_HOME/logs/hook.log" | 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_HOOK_DIR
-    source "$test_dir/../virtualenvwrapper.sh"
-    assertSame "$WORKON_HOME" "$VIRTUALENVWRAPPER_LOG_DIR"
-}
-
-. "$test_dir/shunit2"

tests/test_log_file.sh

+#!/bin/sh
+
+test_dir=$(cd $(dirname $0) && pwd)
+source "$test_dir/setup.sh"
+
+setUp () {
+    echo
+}
+
+test_set_by_user() {
+    export VIRTUALENVWRAPPER_LOG_FILE="$WORKON_HOME/hooks.log"
+    source "$test_dir/../virtualenvwrapper.sh"
+    assertTrue "Log file was not created" "[ -f $VIRTUALENVWRAPPER_LOG_FILE ]"
+}
+
+test_file_permissions() {
+    export VIRTUALENVWRAPPER_LOG_FILE="$WORKON_HOME/hooks.log"
+    source "$test_dir/../virtualenvwrapper.sh"
+    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_FILE
+    unset VIRTUALENVWRAPPER_HOOK_DIR
+    source "$test_dir/../virtualenvwrapper.sh"
+    assertSame "" "$VIRTUALENVWRAPPER_LOG_FILE"
+}
+
+. "$test_dir/shunit2"

tests/test_run_hook.sh

     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

 
     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=$?
 
         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

     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()