corrections after bdbaddog's code review

9 files changed, 114 insertions, 102 deletions

diff --git a/src/engine/SCons/Platform/posix.py b/src/engine/SCons/Platform/posix.py
index fb57521..ad4e859 100644
--- a/src/engine/SCons/Platform/posix.py
+++ b/src/engine/SCons/Platform/posix.py
@@ -41,8 +41,8 @@ import select
 
 import SCons.Util
 from SCons.Platform import TempFileMunge
-from SCons.Platform.VE import ImportVirtualenv
-from SCons.Platform.VE import ignore_virtualenv, enable_virtualenv
+from SCons.Platform.virtualenv import ImportVirtualenv
+from SCons.Platform.virtualenv import ignore_virtualenv, enable_virtualenv
 
 exitvalmap = {
     2 : 127,
diff --git a/src/engine/SCons/Platform/VE.py b/src/engine/SCons/Platform/virtualenv.py
index f7aa80c..03fb486 100644
--- a/src/engine/SCons/Platform/VE.py
+++ b/src/engine/SCons/Platform/virtualenv.py
@@ -1,4 +1,4 @@
-"""SCons.Platform.VE
+"""SCons.Platform.virtualenv
 
 Support for virtualenv.
 """
@@ -33,31 +33,15 @@ import sys
 import SCons.Util
 
 
-def _get_bool_envvar(name, default=False):
-    try:
-        var = os.environ[name]
-    except KeyError:
-        return default
-    try:
-        return bool(int(var))
-    except ValueError:
-        if str(var).lower() in ('true', 'yes', 'y', 'on'):
-            return True
-        elif str(var).lower() in ('false', 'no', 'n', 'off'):
-            return False
-        else:
-            return default
-
-
 virtualenv_enabled_by_default = False
 
 
 def _enable_virtualenv_default():
-    return _get_bool_envvar('SCONS_ENABLE_VIRTUALENV', virtualenv_enabled_by_default)
+    return SCons.Util.get_bool_envvar('SCONS_ENABLE_VIRTUALENV', virtualenv_enabled_by_default)
 
 
 def _ignore_virtualenv_default():
-    return _get_bool_envvar('SCONS_IGNORE_VIRTUALENV', False)
+    return SCons.Util.get_bool_envvar('SCONS_IGNORE_VIRTUALENV', False)
 
 
 enable_virtualenv = _enable_virtualenv_default()
diff --git a/src/engine/SCons/Platform/VETests.py b/src/engine/SCons/Platform/virtualenvTests.py
index 8fd94af..02b37ab 100644
--- a/src/engine/SCons/Platform/VETests.py
+++ b/src/engine/SCons/Platform/virtualenvTests.py
@@ -30,7 +30,7 @@ import unittest
 import os
 import sys
 
-import SCons.Platform.VE
+import SCons.Platform.virtualenv
 import SCons.Util
 
 class Environment(collections.UserDict):
@@ -110,26 +110,6 @@ class SysPrefixes(object):
             sys.prefix = s['prefix']
         del self._stored
 
-class OsEnviron(object):
-    """Used to temporarily mock os.environ"""
-    def __init__(self, environ):
-        self._environ = environ
-
-    def start(self):
-        self._stored = os.environ
-        os.environ = self._environ
-
-    def stop(self):
-        os.environ = self._stored
-        del self._stored
-
-    def __enter__(self):
-        self.start()
-        return os.environ
-
-    def __exit__(self, *args):
-        self.stop()
-
 def _p(p):
     """Converts path string **p** from posix format to os-specific format."""
     drive = []
@@ -138,42 +118,6 @@ def _p(p):
     pieces = p.split('/')
     return os.path.sep.join(drive + pieces)
 
-class _get_bool_envvar_TestCase(unittest.TestCase):
-    def test_missing(self):
-        with OsEnviron(dict()):
-            var = SCons.Platform.VE._get_bool_envvar('FOO')
-            assert var is False, "var should be False, not %s" % repr(var)
-        with OsEnviron({'FOO': '1'}):
-            var = SCons.Platform.VE._get_bool_envvar('BAR')
-            assert var is False, "var should be False, not %s" % repr(var)
-
-    def test_true(self):
-        for foo in [ 'TRUE', 'True', 'true',
-                     'YES', 'Yes', 'yes',
-                     'Y', 'y',
-                     'ON', 'On', 'on',
-                     '1', '20', '-1']:
-            with OsEnviron({'FOO': foo}):
-                var = SCons.Platform.VE._get_bool_envvar('FOO')
-                assert var is True, 'var should be True, not %s' % repr(var)
-
-    def test_false(self):
-        for foo in [ 'FALSE', 'False', 'false',
-                     'NO', 'No', 'no',
-                     'N', 'n',
-                     'OFF', 'Off', 'off',
-                     '0']:
-            with OsEnviron({'FOO': foo}):
-                var = SCons.Platform.VE._get_bool_envvar('FOO', True)
-                assert var is False, 'var should be True, not %s' % repr(var)
-
-    def test_default(self):
-        with OsEnviron({'FOO': 'other'}):
-            var = SCons.Platform.VE._get_bool_envvar('FOO', True)
-            assert var is True, 'var should be True, not %s' % repr(var)
-            var = SCons.Platform.VE._get_bool_envvar('FOO', False)
-            assert var is False, 'var should be False, not %s' % repr(var)
-
 
 class _is_path_in_TestCase(unittest.TestCase):
     def test_false(self):
@@ -184,7 +128,7 @@ class _is_path_in_TestCase(unittest.TestCase):
                         (_p('/foo/bar'), _p('/foo/bar/geez')),
                         (_p('/'), _p('/foo')),
                         (_p('foo'), _p('foo/bar')) ]:
-            assert SCons.Platform.VE._is_path_in(*args) is False, "_is_path_in(%r, %r) should be False" % args
+            assert SCons.Platform.virtualenv._is_path_in(*args) is False, "_is_path_in(%r, %r) should be False" % args
 
     def test__true(self):
         for args in [   (_p('/foo'), _p('/')),
@@ -193,7 +137,7 @@ class _is_path_in_TestCase(unittest.TestCase):
                         (_p('/foo//bar//geez'), _p('/foo/bar')),
                         (_p('/foo/bar/geez'), _p('/foo//bar')),
                         (_p('/foo/bar/geez'), _p('//foo//bar')) ]:
-            assert SCons.Platform.VE._is_path_in(*args) is True, "_is_path_in(%r, %r) should be True" % args
+            assert SCons.Platform.virtualenv._is_path_in(*args) is True, "_is_path_in(%r, %r) should be True" % args
 
 class IsInVirtualenvTestCase(unittest.TestCase):
     def test_false(self):
@@ -203,7 +147,7 @@ class IsInVirtualenvTestCase(unittest.TestCase):
                         _p('/foo'),
                         _p('/prefix'),
                         _p('/prefix/foo') ]:
-                assert SCons.Platform.VE.IsInVirtualenv(p) is False, "IsInVirtualenv(%r) should be False" % p
+                assert SCons.Platform.virtualenv.IsInVirtualenv(p) is False, "IsInVirtualenv(%r) should be False" % p
 
         # "with virtualenv"
         with SysPrefixes(_p('/virtualenv/prefix'), real_prefix=_p('/real/prefix')):
@@ -213,7 +157,7 @@ class IsInVirtualenvTestCase(unittest.TestCase):
                         _p('/virtualenv/prefix/bar/..'),
                         _p('/virtualenv/prefix/bar/../../bleah'),
                         _p('/virtualenv/bleah') ]:
-                assert SCons.Platform.VE.IsInVirtualenv(p) is False, "IsInVirtualenv(%r) should be False" % p
+                assert SCons.Platform.virtualenv.IsInVirtualenv(p) is False, "IsInVirtualenv(%r) should be False" % p
 
         # "with venv"
         with SysPrefixes(_p('/virtualenv/prefix'), base_prefix=_p('/base/prefix')):
@@ -223,20 +167,20 @@ class IsInVirtualenvTestCase(unittest.TestCase):
                         _p('/virtualenv/prefix/bar/..'),
                         _p('/virtualenv/prefix/bar/../../bleah'),
                         _p('/virtualenv/bleah') ]:
-                assert SCons.Platform.VE.IsInVirtualenv(p) is False, "IsInVirtualenv(%r) should be False" % p
+                assert SCons.Platform.virtualenv.IsInVirtualenv(p) is False, "IsInVirtualenv(%r) should be False" % p
 
     def test_true(self):
         # "with virtualenv"
         with SysPrefixes(_p('/virtualenv/prefix'), real_prefix=_p('/real/prefix')):
             for p in [  _p('/virtualenv/prefix/foo'),
                         _p('/virtualenv/prefix/foo/bar') ]:
-                assert SCons.Platform.VE.IsInVirtualenv(p) is True, "IsInVirtualenv(%r) should be True" % p
+                assert SCons.Platform.virtualenv.IsInVirtualenv(p) is True, "IsInVirtualenv(%r) should be True" % p
 
         # "with venv"
         with SysPrefixes(_p('/virtualenv/prefix'), base_prefix=_p('/base/prefix')):
             for p in [  _p('/virtualenv/prefix/foo'),
                         _p('/virtualenv/prefix/foo/bar') ]:
-                assert SCons.Platform.VE.IsInVirtualenv(p) is True, "IsInVirtualenv(%r) should be True" % p
+                assert SCons.Platform.virtualenv.IsInVirtualenv(p) is True, "IsInVirtualenv(%r) should be True" % p
 
 class _inject_venv_pathTestCase(unittest.TestCase):
     def path_list(self):
@@ -252,13 +196,13 @@ class _inject_venv_pathTestCase(unittest.TestCase):
         env = Environment()
         path_string = os.path.pathsep.join(self.path_list())
         with SysPrefixes(_p('/virtualenv/prefix'), real_prefix=_p('/real/prefix')):
-            SCons.Platform.VE._inject_venv_path(env, path_string)
+            SCons.Platform.virtualenv._inject_venv_path(env, path_string)
             assert env['ENV']['PATH'] == _p('/virtualenv/prefix/bin'), env['ENV']['PATH']
 
     def test_with_path_list(self):
         env = Environment()
         with SysPrefixes(_p('/virtualenv/prefix'), real_prefix=_p('/real/prefix')):
-            SCons.Platform.VE._inject_venv_path(env, self.path_list())
+            SCons.Platform.virtualenv._inject_venv_path(env, self.path_list())
             assert env['ENV']['PATH'] == _p('/virtualenv/prefix/bin'), env['ENV']['PATH']
 
 class VirtualenvTestCase(unittest.TestCase):
@@ -267,10 +211,10 @@ class VirtualenvTestCase(unittest.TestCase):
             return "Virtualenv() should be None, not %s" % repr(given)
 
         with SysPrefixes(_p('/prefix')):
-            ve = SCons.Platform.VE.Virtualenv()
+            ve = SCons.Platform.virtualenv.Virtualenv()
             assert ve is None , _msg(ve)
         with SysPrefixes(_p('/base/prefix'), base_prefix=_p('/base/prefix')):
-            ve = SCons.Platform.VE.Virtualenv()
+            ve = SCons.Platform.virtualenv.Virtualenv()
             assert ve is None, _msg(ve)
 
     def test_not_none(self):
@@ -278,13 +222,13 @@ class VirtualenvTestCase(unittest.TestCase):
             return "Virtualenv() should == %r, not %s" % (_p(expected), repr(given))
 
         with SysPrefixes(_p('/virtualenv/prefix'), real_prefix=_p('/real/prefix')):
-            ve = SCons.Platform.VE.Virtualenv()
+            ve = SCons.Platform.virtualenv.Virtualenv()
             assert ve == _p('/virtualenv/prefix'), _msg('/virtualenv/prefix', ve)
         with SysPrefixes(_p('/same/prefix'), real_prefix=_p('/same/prefix')):
-            ve = SCons.Platform.VE.Virtualenv()
+            ve = SCons.Platform.virtualenv.Virtualenv()
             assert ve == _p('/same/prefix'),  _msg('/same/prefix', ve)
         with SysPrefixes(_p('/virtualenv/prefix'), base_prefix=_p('/base/prefix')):
-            ve = SCons.Platform.VE.Virtualenv()
+            ve = SCons.Platform.virtualenv.Virtualenv()
             assert ve == _p('/virtualenv/prefix'),  _msg('/virtualenv/prefix', ve)
 
 
diff --git a/src/engine/SCons/Platform/win32.py b/src/engine/SCons/Platform/win32.py
index 0cd8399..3b76208 100644
--- a/src/engine/SCons/Platform/win32.py
+++ b/src/engine/SCons/Platform/win32.py
@@ -39,8 +39,8 @@ import tempfile
 
 from SCons.Platform.posix import exitvalmap
 from SCons.Platform import TempFileMunge
-from SCons.Platform.VE import ImportVirtualenv
-from SCons.Platform.VE import ignore_virtualenv, enable_virtualenv
+from SCons.Platform.virtualenv import ImportVirtualenv
+from SCons.Platform.virtualenv import ignore_virtualenv, enable_virtualenv
 import SCons.Util
 
 try:
diff --git a/src/engine/SCons/Script/Main.py b/src/engine/SCons/Script/Main.py
index daa6be8..2c59808 100644
--- a/src/engine/SCons/Script/Main.py
+++ b/src/engine/SCons/Script/Main.py
@@ -59,7 +59,7 @@ import SCons.Job
 import SCons.Node
 import SCons.Node.FS
 import SCons.Platform
-import SCons.Platform.VE
+import SCons.Platform.virtualenv
 import SCons.SConf
 import SCons.Script
 import SCons.Taskmaster
@@ -864,12 +864,12 @@ def _main(parser):
     for warning_type, message in delayed_warnings:
         SCons.Warnings.warn(warning_type, message)
 
-    if not SCons.Platform.VE.virtualenv_enabled_by_default:
+    if not SCons.Platform.virtualenv.virtualenv_enabled_by_default:
         if options.enable_virtualenv:
-            SCons.Platform.VE.enable_virtualenv = True
+            SCons.Platform.virtualenv.enable_virtualenv = True
 
     if options.ignore_virtualenv:
-        SCons.Platform.VE.ignore_virtualenv = True
+        SCons.Platform.virtualenv.ignore_virtualenv = True
 
     if options.diskcheck:
         SCons.Node.FS.set_diskcheck(options.diskcheck)
diff --git a/src/engine/SCons/Script/SConsOptions.py b/src/engine/SCons/Script/SConsOptions.py
index a18c6b9..37dd644 100644
--- a/src/engine/SCons/Script/SConsOptions.py
+++ b/src/engine/SCons/Script/SConsOptions.py
@@ -38,7 +38,7 @@ except ImportError:
 _ = gettext
 
 import SCons.Node.FS
-import SCons.Platform.VE
+import SCons.Platform.virtualenv
 import SCons.Warnings
 
 OptionValueError        = optparse.OptionValueError
@@ -707,7 +707,7 @@ def Parser(version):
                   action="callback", callback=opt_duplicate,
                   help=opt_duplicate_help)
 
-    if not SCons.Platform.VE.virtualenv_enabled_by_default:
+    if not SCons.Platform.virtualenv.virtualenv_enabled_by_default:
         op.add_option('--enable-virtualenv',
                      dest="enable_virtualenv",
                      action="store_true",
diff --git a/src/engine/SCons/Script/__init__.py b/src/engine/SCons/Script/__init__.py
index ee3a191..d602507 100644
--- a/src/engine/SCons/Script/__init__.py
+++ b/src/engine/SCons/Script/__init__.py
@@ -82,7 +82,7 @@ import SCons.Builder
 import SCons.Environment
 import SCons.Node.FS
 import SCons.Platform
-import SCons.Platform.VE
+import SCons.Platform.virtualenv
 import SCons.Scanner
 import SCons.SConf
 import SCons.Subst
@@ -150,7 +150,7 @@ Environment             = SCons.Environment.Environment
 #OptParser               = SCons.SConsOptions.OptParser
 FindPathDirs            = SCons.Scanner.FindPathDirs
 Platform                = SCons.Platform.Platform
-Virtualenv              = SCons.Platform.VE.Virtualenv
+Virtualenv              = SCons.Platform.virtualenv.Virtualenv
 Return                  = _SConscript.Return
 Scanner                 = SCons.Scanner.Base
 Tool                    = SCons.Tool.Tool
diff --git a/src/engine/SCons/Util.py b/src/engine/SCons/Util.py
index 6643b72..4588955 100644
--- a/src/engine/SCons/Util.py
+++ b/src/engine/SCons/Util.py
@@ -1597,6 +1597,32 @@ def cmp(a, b):
     return (a > b) - (a < b)
 
 
+def get_bool_envvar(name, default=False):
+    """
+    Get a value of OS environment variable converting it to boolean.
+
+    - FOO=1, FOO=123, FOO=true, FOO=yes, FOO=y, FOO=on are examples of ``True``
+      values.
+    - FOO=0, FOO=false, FOO=no, FOO=n, FOO=off are examples of ``False``
+      values.
+
+    If a variable can't be converted to a boolean, default value is returned
+    (``False`` by default)
+    """
+    try:
+        var = os.environ[name]
+    except KeyError:
+        return default
+    try:
+        return bool(int(var))
+    except ValueError:
+        if str(var).lower() in ('true', 'yes', 'y', 'on'):
+            return True
+        elif str(var).lower() in ('false', 'no', 'n', 'off'):
+            return False
+        else:
+            return default
+
 # Local Variables:
 # tab-width:4
 # indent-tabs-mode:nil
diff --git a/src/engine/SCons/UtilTests.py b/src/engine/SCons/UtilTests.py
index 209c60f..6067c98 100644
--- a/src/engine/SCons/UtilTests.py
+++ b/src/engine/SCons/UtilTests.py
@@ -884,6 +884,64 @@ class flattenTestCase(unittest.TestCase):
         self.assertEqual(sorted(result),[1,2,3])
 
 
+class OsEnviron(object):
+    """Used to temporarily mock os.environ"""
+    def __init__(self, environ):
+        self._environ = environ
+
+    def start(self):
+        self._stored = os.environ
+        os.environ = self._environ
+
+    def stop(self):
+        os.environ = self._stored
+        del self._stored
+
+    def __enter__(self):
+        self.start()
+        return os.environ
+
+    def __exit__(self, *args):
+        self.stop()
+
+
+class get_bool_envvarTestCase(unittest.TestCase):
+    def test_missing(self):
+        with OsEnviron(dict()):
+            var = get_bool_envvar('FOO')
+            assert var is False, "var should be False, not %s" % repr(var)
+        with OsEnviron({'FOO': '1'}):
+            var = get_bool_envvar('BAR')
+            assert var is False, "var should be False, not %s" % repr(var)
+
+    def test_true(self):
+        for foo in [ 'TRUE', 'True', 'true',
+                     'YES', 'Yes', 'yes',
+                     'Y', 'y',
+                     'ON', 'On', 'on',
+                     '1', '20', '-1']:
+            with OsEnviron({'FOO': foo}):
+                var = get_bool_envvar('FOO')
+                assert var is True, 'var should be True, not %s' % repr(var)
+
+    def test_false(self):
+        for foo in [ 'FALSE', 'False', 'false',
+                     'NO', 'No', 'no',
+                     'N', 'n',
+                     'OFF', 'Off', 'off',
+                     '0']:
+            with OsEnviron({'FOO': foo}):
+                var = get_bool_envvar('FOO', True)
+                assert var is False, 'var should be True, not %s' % repr(var)
+
+    def test_default(self):
+        with OsEnviron({'FOO': 'other'}):
+            var = get_bool_envvar('FOO', True)
+            assert var is True, 'var should be True, not %s' % repr(var)
+            var = get_bool_envvar('FOO', False)
+            assert var is False, 'var should be False, not %s' % repr(var)
+
+
 if __name__ == "__main__":
     unittest.main()