Merge pull request #3337 from mwichmann/tool-add

[#3336] do not add not-found tool paths

7 files changed, 148 insertions, 98 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index c80af4b..4d095dc 100755
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -20,6 +20,8 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Use importlib to dynamically load tool and platform modules instead of imp module
     - sconsign: default to .sconsign.dblite if no filename is specified.
       Be more informative in case of unsupported pickle protocol (py2 only).
+    - Fix issue #3336 - on Windows, paths were being added to PATH even if
+      tools were not found in those paths.
     - More fixes for newer Java versions (since 9): handle new jdk directory
       naming (jdk-X.Y instead of jdkX.Y) on Windows; handle two-digit major
       version. Docstrings improved.
diff --git a/src/engine/SCons/Tool/ToolTests.py b/src/engine/SCons/Tool/ToolTests.py
index f005143..74524f1 100644
--- a/src/engine/SCons/Tool/ToolTests.py
+++ b/src/engine/SCons/Tool/ToolTests.py
@@ -23,6 +23,7 @@
 
 __revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
+import os
 import sys
 import unittest
 
@@ -31,29 +32,49 @@ import TestUnit
 import SCons.Errors
 import SCons.Tool
 
+
+class DummyEnvironment(object):
+    def __init__(self):
+        self.dict = {}
+    def Detect(self, progs):
+        if not SCons.Util.is_List(progs):
+            progs = [ progs ]
+        return progs[0]
+    def Append(self, **kw):
+        self.dict.update(kw)
+    def __getitem__(self, key):
+        return self.dict[key]
+    def __setitem__(self, key, val):
+        self.dict[key] = val
+    def __contains__(self, key):
+        return self.dict.__contains__(key)
+    def has_key(self, key):
+        return key in self.dict
+    def subst(self, string, *args, **kwargs):
+        return string
+
+    PHONY_PATH = "/usr/phony/bin"
+    def WhereIs(self, key_program):
+        # for pathfind test for Issue #3336:
+        # need to fake the case where extra paths are searched, and
+        # if one has a "hit" after some fails, the fails are left in
+        # the environment's PATH. So construct a positive answer if
+        # we see a magic known path component in PATH; answer in
+        # the negative otherwise.
+        paths = self['ENV']['PATH']
+        if self.PHONY_PATH in paths:
+            return os.path.join(self.PHONY_PATH, key_program)
+        return None
+    def AppendENVPath(self, pathvar, path):
+        # signature matches how called from find_program_path()
+        self['ENV'][pathvar] = self['ENV'][pathvar] + os.pathsep + path
+
+
 class ToolTestCase(unittest.TestCase):
     def test_Tool(self):
         """Test the Tool() function"""
-        class Environment(object):
-            def __init__(self):
-                self.dict = {}
-            def Detect(self, progs):
-                if not SCons.Util.is_List(progs):
-                    progs = [ progs ]
-                return progs[0]
-            def Append(self, **kw):
-                self.dict.update(kw)
-            def __getitem__(self, key):
-                return self.dict[key]
-            def __setitem__(self, key, val):
-                self.dict[key] = val
-            def __contains__(self, key):
-                return self.dict.__contains__(key)
-            def has_key(self, key):
-                return key in self.dict
-            def subst(self, string, *args, **kwargs):
-                return string
-        env = Environment()
+
+        env = DummyEnvironment()
         env['BUILDERS'] = {}
         env['ENV'] = {}
         env['PLATFORM'] = 'test'
@@ -78,6 +99,23 @@ class ToolTestCase(unittest.TestCase):
             raise
 
 
+    def test_pathfind(self):
+        """Test that find_program_path() does not alter PATH"""
+
+        env = DummyEnvironment()
+        PHONY_PATHS = [
+            r'C:\cygwin64\bin',
+            r'C:\cygwin\bin',
+            '/usr/local/dummy/bin',
+            env.PHONY_PATH,     # will be recognized by dummy WhereIs
+        ]
+        env['ENV'] = {}
+        env['ENV']['PATH'] = '/usr/local/bin:/opt/bin:/bin:/usr/bin'
+        pre_path = env['ENV']['PATH']
+        _ = SCons.Tool.find_program_path(env, 'no_tool', default_paths=PHONY_PATHS)
+        assert env['ENV']['PATH'] == pre_path, env['ENV']['PATH']
+
+
 if __name__ == "__main__":
     suite = unittest.makeSuite(ToolTestCase, 'test_')
     TestUnit.run(suite)
diff --git a/src/engine/SCons/Tool/__init__.py b/src/engine/SCons/Tool/__init__.py
index c0aa634..cc935d1 100644
--- a/src/engine/SCons/Tool/__init__.py
+++ b/src/engine/SCons/Tool/__init__.py
@@ -104,7 +104,9 @@ TOOL_ALIASES = {
 
 
 class Tool(object):
-    def __init__(self, name, toolpath=[], **kw):
+    def __init__(self, name, toolpath=None, **kw):
+        if toolpath is None:
+            toolpath = []
 
         # Rename if there's a TOOL_ALIAS for this tool
         self.name = TOOL_ALIASES.get(name, name)
@@ -394,7 +396,8 @@ def _call_env_subst(env, string, *args, **kw):
 
 
 class _ShLibInfoSupport(object):
-    def get_libtype(self):
+    @property
+    def libtype(self):
         return 'ShLib'
 
     def get_lib_prefix(self, env, *args, **kw):
@@ -411,7 +414,8 @@ class _ShLibInfoSupport(object):
 
 
 class _LdModInfoSupport(object):
-    def get_libtype(self):
+    @property
+    def libtype(self):
         return 'LdMod'
 
     def get_lib_prefix(self, env, *args, **kw):
@@ -428,7 +432,8 @@ class _LdModInfoSupport(object):
 
 
 class _ImpLibInfoSupport(object):
-    def get_libtype(self):
+    @property
+    def libtype(self):
         return 'ImpLib'
 
     def get_lib_prefix(self, env, *args, **kw):
@@ -480,25 +485,21 @@ class _LibInfoGeneratorBase(object):
                         'ImpLib': _ImpLibInfoSupport}
 
     def __init__(self, libtype, infoname):
-        self.set_libtype(libtype)
-        self.set_infoname(infoname)
+        self.libtype = libtype
+        self.infoname = infoname
+
+    @property
+    def libtype(self):
+        return self._support.libtype
 
-    def set_libtype(self, libtype):
+    @libtype.setter
+    def libtype(self, libtype):
         try:
             support_class = self._support_classes[libtype]
         except KeyError:
             raise ValueError('unsupported libtype %r' % libtype)
         self._support = support_class()
 
-    def get_libtype(self):
-        return self._support.get_libtype()
-
-    def set_infoname(self, infoname):
-        self.infoname = infoname
-
-    def get_infoname(self):
-        return self.infoname
-
     def get_lib_prefix(self, env, *args, **kw):
         return self._support.get_lib_prefix(env, *args, **kw)
 
@@ -518,9 +519,8 @@ class _LibInfoGeneratorBase(object):
         try:
             libtype = kw['generator_libtype']
         except KeyError:
-            libtype = self.get_libtype()
-        infoname = self.get_infoname()
-        return 'Versioned%s%s' % (libtype, infoname)
+            libtype = self.libtype
+        return 'Versioned%s%s' % (libtype, self.infoname)
 
     def generate_versioned_lib_info(self, env, args, result=None, **kw):
         callback = self.get_versioned_lib_info_generator(**kw)
@@ -730,7 +730,7 @@ class _LibSonameGenerator(_LibInfoGeneratorBase):
 
         if not soname:
             # fallback to library name (as returned by appropriate _LibNameGenerator)
-            soname = _LibNameGenerator(self.get_libtype())(env, libnode)
+            soname = _LibNameGenerator(self.libtype)(env, libnode)
             if Verbose:
                 print("_LibSonameGenerator: FALLBACK: soname=%r" % soname)
 
@@ -1316,30 +1316,35 @@ def tool_list(platform, env):
     return [x for x in tools if x]
 
 
-def find_program_path(env, key_program, default_paths=[]):
+def find_program_path(env, key_program, default_paths=None):
     """
-    Find the location of key_program and then return the path it was located at.
-    Checking the default install locations.
-    Mainly for windows where tools aren't all installed in /usr/bin,etc
-    :param env: Current Environment()
-    :param key_program: Program we're using to locate the directory to add to PATH.
+    Find the location of a tool using various means.
+
+    Mainly for windows where tools aren't all installed in /usr/bin, etc.
+
+    :param env: Current Construction Environment.
+    :param key_program: Tool to locate.
+    :param default_paths: List of additional paths this tool might be found in.
     """
     # First search in the SCons path
     path = env.WhereIs(key_program)
-    if (path):
+    if path:
         return path
-    # then the OS path:
+
+    # Then in the OS path
     path = SCons.Util.WhereIs(key_program)
-    if (path):
+    if path:
         return path
 
-    # If that doesn't work try default location for mingw
+    # Finally, add the defaults and check again. Do not change
+    # ['ENV']['PATH'] permananetly, the caller can do that if needed.
+    if default_paths is None:
+        return path
     save_path = env['ENV']['PATH']
     for p in default_paths:
         env.AppendENVPath('PATH', p)
     path = env.WhereIs(key_program)
-    if not path:
-        env['ENV']['PATH'] = save_path
+    env['ENV']['PATH'] = save_path
     return path
 
 # Local Variables:
diff --git a/src/engine/SCons/Tool/lex.py b/src/engine/SCons/Tool/lex.py
index a63ddc9..6b83aa9 100644
--- a/src/engine/SCons/Tool/lex.py
+++ b/src/engine/SCons/Tool/lex.py
@@ -45,6 +45,11 @@ from SCons.Platform.win32 import CHOCO_DEFAULT_PATH
 
 LexAction = SCons.Action.Action("$LEXCOM", "$LEXCOMSTR")
 
+if sys.platform == 'win32':
+    BINS = ['flex', 'lex', 'win_flex']
+else:
+    BINS = ["flex", "lex"]
+
 def lexEmitter(target, source, env):
     sourceBase, sourceExt = os.path.splitext(SCons.Util.to_String(source[0]))
 
@@ -70,22 +75,22 @@ def lexEmitter(target, source, env):
 
 def get_lex_path(env, append_paths=False):
     """
-    Find the a path containing the lex or flex binaries. If a construction 
-    environment is passed in then append the path to the ENV PATH.
-    """
-    # save existing path to reset if we don't want to append any paths
-    envPath = env['ENV']['PATH']
-    bins = ['flex', 'lex', 'win_flex']
+    Find the path to the lex tool, searching several possible names
 
-    for prog in bins:
+    Only called in the Windows case, so the default_path
+    can be Windows-specific
+
+    :param env: current construction environment
+    :param append_paths: if set, add the path to the tool to PATH
+    :return: path to lex tool, if found
+    """
+    for prog in BINS:
         bin_path = SCons.Tool.find_program_path(
-            env, 
-            prog, 
+            env,
+            prog,
             default_paths=CHOCO_DEFAULT_PATH + MINGW_DEFAULT_PATHS + CYGWIN_DEFAULT_PATHS )
         if bin_path:
-            if not append_paths:
-                env['ENV']['PATH'] = envPath
-            else:
+            if append_paths:
                 env.AppendENVPath('PATH', os.path.dirname(bin_path))
             return bin_path
     SCons.Warnings.Warning('lex tool requested, but lex or flex binary not found in ENV PATH')
@@ -113,20 +118,21 @@ def generate(env):
     env["LEXFLAGS"] = SCons.Util.CLVar("")
 
     if sys.platform == 'win32':
-        get_lex_path(env, append_paths=True)
-        env["LEX"] = env.Detect(['flex', 'lex', 'win_flex'])
+        # ignore the return - we do not need the full path here
+        _ = get_lex_path(env, append_paths=True)
+        env["LEX"] = env.Detect(BINS)
         if not env.get("LEXUNISTD"):
             env["LEXUNISTD"] = SCons.Util.CLVar("")
         env["LEXCOM"] = "$LEX $LEXUNISTD $LEXFLAGS -t $SOURCES > $TARGET"
     else:
-        env["LEX"] = env.Detect(["flex", "lex"])
+        env["LEX"] = env.Detect(BINS)
         env["LEXCOM"] = "$LEX $LEXFLAGS -t $SOURCES > $TARGET"
 
 def exists(env):
     if sys.platform == 'win32':
         return get_lex_path(env)
     else:
-        return env.Detect(["flex", "lex"])
+        return env.Detect(BINS)
 
 # Local Variables:
 # tab-width:4
diff --git a/src/engine/SCons/Tool/swig.py b/src/engine/SCons/Tool/swig.py
index c6abefb..6ed9d82 100644
--- a/src/engine/SCons/Tool/swig.py
+++ b/src/engine/SCons/Tool/swig.py
@@ -184,9 +184,10 @@ def generate(env):
 
     from SCons.Platform.mingw import MINGW_DEFAULT_PATHS
     from SCons.Platform.cygwin import CYGWIN_DEFAULT_PATHS
+    from SCons.Platform.win32 import CHOCO_DEFAULT_PATH
 
     if sys.platform == 'win32':
-        swig = SCons.Tool.find_program_path(env, 'swig', default_paths=MINGW_DEFAULT_PATHS + CYGWIN_DEFAULT_PATHS + [r'C:\ProgramData\chocolatey\bin'] )
+        swig = SCons.Tool.find_program_path(env, 'swig', default_paths=MINGW_DEFAULT_PATHS + CYGWIN_DEFAULT_PATHS + CHOCO_DEFAULT_PATH)
         if swig:
             swig_bin_dir = os.path.dirname(swig)
             env.AppendENVPath('PATH', swig_bin_dir)
diff --git a/src/engine/SCons/Tool/yacc.py b/src/engine/SCons/Tool/yacc.py
index 5db49d3..2a5ffd8 100644
--- a/src/engine/SCons/Tool/yacc.py
+++ b/src/engine/SCons/Tool/yacc.py
@@ -45,6 +45,11 @@ from SCons.Platform.win32 import CHOCO_DEFAULT_PATH
 
 YaccAction = SCons.Action.Action("$YACCCOM", "$YACCCOMSTR")
 
+if sys.platform == 'win32':
+    BINS = ['bison', 'yacc', 'win_bison']
+else:
+    BINS = ["bison", "yacc"]
+
 def _yaccEmitter(target, source, env, ysuf, hsuf):
     yaccflags = env.subst("$YACCFLAGS", target=target, source=source)
     flags = SCons.Util.CLVar(yaccflags)
@@ -100,25 +105,26 @@ def yyEmitter(target, source, env):
 
 def get_yacc_path(env, append_paths=False):
     """
-    Find the a path containing the lex or flex binaries. If a construction 
-    environment is passed in then append the path to the ENV PATH.
-    """
-    # save existing path to reset if we don't want to append any paths
-    envPath = env['ENV']['PATH']
-    bins = ['bison', 'yacc', 'win_bison']
+    Find the path to the yacc tool, searching several possible names
+
+    Only called in the Windows case, so the default_path
+    can be Windows-specific
 
-    for prog in bins:
+    :param env: current construction environment
+    :param append_paths: if set, add the path to the tool to PATH
+    :return: path to yacc tool, if found
+    """
+    for prog in BINS:
         bin_path = SCons.Tool.find_program_path(
-            env, 
-            prog, 
+            env,
+            prog,
             default_paths=CHOCO_DEFAULT_PATH + MINGW_DEFAULT_PATHS + CYGWIN_DEFAULT_PATHS )
         if bin_path:
-            if not append_paths:
-                env['ENV']['PATH'] = envPath
-            else:
+            if append_paths:
                 env.AppendENVPath('PATH', os.path.dirname(bin_path))
             return bin_path
-    SCons.Warnings.Warning('lex tool requested, but lex or flex binary not found in ENV PATH')
+    SCons.Warnings.Warning('yacc tool requested, but yacc or bison binary not found in ENV PATH')
+
 
 def generate(env):
     """Add Builders and construction variables for yacc to an Environment."""
@@ -140,29 +146,21 @@ def generate(env):
     cxx_file.add_emitter('.yy', yyEmitter)
 
     if sys.platform == 'win32':
-        bison = SCons.Tool.find_program_path(env, 'bison', default_paths=MINGW_DEFAULT_PATHS + CYGWIN_DEFAULT_PATHS )
-        if bison:
-            bison_bin_dir = os.path.dirname(bison)
-            env.AppendENVPath('PATH', bison_bin_dir)
-        else:
-            SCons.Warnings.Warning('yacc tool requested, but bison binary not found in ENV PATH')
+        # ignore the return, all we need is for the path to be added
+        _ = get_yacc_path(env, append_paths=True)
 
-    if sys.platform == 'win32':
-        get_yacc_path(env, append_paths=True)
-        env["YACC"] = env.Detect(['bison', 'yacc', 'win_bison'])
-    else:
-        env["YACC"] = env.Detect(["bison", "yacc"])
-    
+    env["YACC"] = env.Detect(BINS)
     env['YACCFLAGS'] = SCons.Util.CLVar('')
     env['YACCCOM']   = '$YACC $YACCFLAGS -o $TARGET $SOURCES'
     env['YACCHFILESUFFIX'] = '.h'
-
     env['YACCHXXFILESUFFIX'] = '.hpp'
-
     env['YACCVCGFILESUFFIX'] = '.vcg'
 
 def exists(env):
-    return env.Detect(['bison', 'yacc'])
+    if sys.platform == 'win32':
+        return get_yacc_path(env)
+    else:
+        return env.Detect(BINS)
 
 # Local Variables:
 # tab-width:4
diff --git a/test/LEX/live_mingw.py b/test/LEX/live_mingw.py
index 13e2342..d535065 100644
--- a/test/LEX/live_mingw.py
+++ b/test/LEX/live_mingw.py
@@ -41,7 +41,7 @@ if sys.platform != 'win32':
     test.skip_test('Not windows environment; skipping test.\n')
 
 if not test.where_is('gcc'):
-        test.skip_test('No mingw or cygwin on windows; skipping test.\n')
+        test.skip_test('No mingw or cygwin build environment found; skipping test.\n')
 
 lex = test.where_is('lex') or test.where_is('flex')