Various improvements to the change

7 files changed, 79 insertions, 43 deletions

diff --git a/src/engine/SCons/Action.py b/src/engine/SCons/Action.py
index ccd5c17..2dea8cc 100644
--- a/src/engine/SCons/Action.py
+++ b/src/engine/SCons/Action.py
@@ -853,16 +853,6 @@ class CommandAction(_ActionAction):
                                 "a single command")
         self.cmd_list = cmd
 
-        # Regex meant to parse a file path in the following cases:
-        #   /path/to/file
-        #   '/path/to/file'
-        #   "C:\path\to\file"
-        # Does not allow for stripping off an optional @ before the parameter.
-        # We don't need to support detecting temp files generated by
-        # TempFileMunge because in signature mode (which is what is used during
-        # action scanning) TempFileMunge doesn't modify the command string.
-        self.command_strip = re.compile('^[\'"]?([^\'"]*)[\'"]?$')
-
     def __str__(self):
         if is_List(self.cmd_list):
             return ' '.join(map(str, self.cmd_list))
@@ -992,17 +982,12 @@ class CommandAction(_ActionAction):
         if not icd or icd in ('0', 'None'):
             return []
         from SCons.Subst import SUBST_SIG
-        if executor:
-            cmd_list = env.subst_list(self.cmd_list, SUBST_SIG, executor=executor)
-        else:
-            cmd_list = env.subst_list(self.cmd_list, SUBST_SIG, target, source)
-        res = []
 
-        # Avoid circular dependencies by ignoring any paths to targets.
-        targets_norm = set()
-        for t in target:
-            targets_norm.add(os.path.normcase(t.abspath))
-            targets_norm.add(os.path.normcase(str(t)))
+        # Avoid circular and duplicate dependencies by not provide source and
+        # target to subst_list. This causes references to $SOURCES, $TARGETS,
+        # and all related variables to disappear.
+        cmd_list = env.subst_list(self.cmd_list, SUBST_SIG, [], [])
+        res = []
 
         for cmd_line in cmd_list:
             if cmd_line:
@@ -1010,13 +995,28 @@ class CommandAction(_ActionAction):
                     d = str(entry)
                     if not d.startswith(('&', '-', '/') if os.name == 'nt'
                                         else ('&', '-')):
-                        m = self.command_strip.match(d)
+                        m = strip_quotes.match(d)
                         if m:
                             d = m.group(1)
-                            # For now, only match files, not directories.
-                            d = d if os.path.isfile(d) else env.WhereIs(d)
-                            if d and os.path.normcase(d) not in targets_norm:
-                                res.append(env.fs.File(d))
+
+                        # For now, only match files, not directories.
+                        p = os.path.abspath(d) if os.path.isfile(d) else \
+                            env.WhereIs(d)
+                        if p:
+                            res.append(env.fs.File(p))
+                        else:
+                            # Try to find the dependency in any source
+                            # repositories.
+                            #
+                            # TODO: I want to enumerate get_all_rdirs() and am
+                            # not sure whether I want to enumerate
+                            # srcdir_list(). srcdir_find_file() does both. Is
+                            # that bad? Should we instead change env.WhereIs()
+                            # to search in repositories too?
+                            n, _ = env.fs.Top.srcdir_find_file(d)
+                            if n and n not in target and n not in source:
+                                res.append(n)
+
         return res
 
 
diff --git a/test/Batch/CHANGED_SOURCES.py b/test/Batch/CHANGED_SOURCES.py
index 477869f..b54bd2e 100644
--- a/test/Batch/CHANGED_SOURCES.py
+++ b/test/Batch/CHANGED_SOURCES.py
@@ -48,8 +48,11 @@ for infile in sys.argv[2:]:
 sys.exit(0)
 """)
 
+# Disable IMPLICIT_COMMAND_DEPENDENCIES because otherwise it renders them less
+# effective. They count on paths provided at the end of the command string only
+# being counted as dependencies if Depends() is used.
 test.write('SConstruct', """
-env = Environment()
+env = Environment(IMPLICIT_COMMAND_DEPENDENCIES=False)
 env['BATCH_BUILD'] = 'batch_build.py'
 env['BATCHCOM'] = r'%(_python_)s $BATCH_BUILD ${TARGET.dir} $CHANGED_SOURCES'
 bb = Action('$BATCHCOM', batch_key=True, targets='CHANGED_TARGETS')
diff --git a/test/Batch/action-changed.py b/test/Batch/action-changed.py
index b2b1673..33c026e 100644
--- a/test/Batch/action-changed.py
+++ b/test/Batch/action-changed.py
@@ -33,7 +33,11 @@ import os
 
 import TestSCons
 
-python = TestSCons.python
+# swap slashes because on py3 on win32 inside the generated build.py
+# the backslashes are getting interpretted as unicode which is
+# invalid.
+python = TestSCons.python.replace('\\','//')
+_python_ = TestSCons._python_
 
 test = TestSCons.TestSCons()
 
@@ -53,17 +57,19 @@ sys.exit(0)
 test.write('build.py', build_py_contents % (python, 'one'))
 os.chmod(test.workpath('build.py'), 0o755)
 
+build_py_workpath = test.workpath('build.py')
+
 test.write('SConstruct', """
 env = Environment()
 env.PrependENVPath('PATHEXT', '.PY')
-bb = Action(r'"%s" $CHANGED_TARGETS -- $CHANGED_SOURCES',
+bb = Action(r'%(_python_)s "%(build_py_workpath)s" $CHANGED_TARGETS -- $CHANGED_SOURCES',
             batch_key=True,
             targets='CHANGED_TARGETS')
 env['BUILDERS']['Batch'] = Builder(action=bb)
 env.Batch('f1.out', 'f1.in')
 env.Batch('f2.out', 'f2.in')
 env.Batch('f3.out', 'f3.in')
-""" % test.workpath('build.py'))
+""" % locals())
 
 test.write('f1.in', "f1.in\n")
 test.write('f2.in', "f2.in\n")
diff --git a/test/Depends/Depends.py b/test/Depends/Depends.py
index 3ed9e12..51737a7 100644
--- a/test/Depends/Depends.py
+++ b/test/Depends/Depends.py
@@ -50,11 +50,15 @@ sys.exit(0)
 SUBDIR_foo_dep = os.path.join('$SUBDIR', 'foo.dep')
 SUBDIR_f3_out = os.path.join('$SUBDIR', 'f3.out')
 
+# Disable IMPLICIT_COMMAND_DEPENDENCIES because otherwise it renders them less
+# effective. They count on paths provided at the end of the command string only
+# being counted as dependencies if Depends() is used.
 test.write('SConstruct', """
 DefaultEnvironment(tools=[])
 Foo = Builder(action = r'%(_python_)s build.py $TARGET $SOURCES subdir/foo.dep')
 Bar = Builder(action = r'%(_python_)s build.py $TARGET $SOURCES subdir/bar.dep')
-env = Environment(tools=[], BUILDERS = { 'Foo' : Foo, 'Bar' : Bar }, SUBDIR='subdir')
+env = Environment(tools=[], BUILDERS = { 'Foo' : Foo, 'Bar' : Bar }, SUBDIR='subdir',
+                  IMPLICIT_COMMAND_DEPENDENCIES=False)
 env.Depends(target = ['f1.out', 'f2.out'], dependency = r'%(SUBDIR_foo_dep)s')
 env.Depends(target = r'%(SUBDIR_f3_out)s', dependency = 'subdir/bar.dep')
 env.Foo(target = 'f1.out', source = 'f1.in')
diff --git a/test/ParseDepends.py b/test/ParseDepends.py
index 2de2105..aa15fc9 100644
--- a/test/ParseDepends.py
+++ b/test/ParseDepends.py
@@ -40,10 +40,13 @@ with open(sys.argv[1], 'wb') as f, open(sys.argv[2], 'rb') as afp2, open(sys.arg
     f.write(afp2.read() + afp3.read())
 """)
 
+# Pass IMPLICIT_COMMAND_DEPENDENCIES=False because the test depends on us not
+# taking a dependency on the last file in the action string.
 test.write('SConstruct', """
 Foo = Builder(action = r'%(_python_)s build.py $TARGET $SOURCES subdir/foo.dep')
 Bar = Builder(action = r'%(_python_)s build.py $TARGET $SOURCES subdir/bar.dep')
-env = Environment(BUILDERS = { 'Foo' : Foo, 'Bar' : Bar }, SUBDIR='subdir')
+env = Environment(BUILDERS = { 'Foo' : Foo, 'Bar' : Bar }, SUBDIR='subdir',
+                  IMPLICIT_COMMAND_DEPENDENCIES=False)
 env.ParseDepends('foo.d')
 env.ParseDepends('bar.d')
 env.Foo(target = 'f1.out', source = 'f1.in')
diff --git a/test/Repository/link-object.py b/test/Repository/link-object.py
index 78add90..f5bf783 100644
--- a/test/Repository/link-object.py
+++ b/test/Repository/link-object.py
@@ -44,7 +44,13 @@ workpath_repository = test.workpath('repository')
 repository_foo = test.workpath('repository', 'foo' + _exe)
 work_foo = test.workpath('work', 'foo' + _exe)
 
+# Don't take implicit command dependencies because otherwise the program will
+# be unexpectedly recompiled the first time we use chdir="work". The reason is
+# that we take .obj files as implicit dependencies but when we move the current
+# directory, those .obj files are no longer around.
 #
+# TODO: Should this test pass as-is without disabling implicit command
+# dependencies?
 test.write(['repository', 'SConstruct'], """
 Repository(r'%s')
 env = Environment()
diff --git a/test/explain/basic.py b/test/explain/basic.py
index 46ce6bc..2d57d35 100644
--- a/test/explain/basic.py
+++ b/test/explain/basic.py
@@ -335,6 +335,7 @@ scons: rebuilding `file3' because:
            ->Depends
            ->Implicit
            Old:%(_python_)s	New:%(_python_)s
+           Old:%(cat_py)s	New:%(cat_py)s
 %(_python_)s %(cat_py)s file3 zzz yyy xxx
 """ % locals())
 
@@ -354,12 +355,21 @@ env.AddPostAction(f3, r'%(_python_)s %(cat_py)s ${TARGET}.yyy $SOURCES yyy')
 env.AddPreAction(f3, r'%(_python_)s %(cat_py)s ${TARGET}.alt $SOURCES')
 """ % locals())
 
+# Because Action.get_implicit_deps() scans the entire action string, we do take
+# the additional "yyy" as a dependency. It already was a source, so it now is
+# considered to be a dependency ordering change.
 expect = test.wrap_stdout("""\
-scons: rebuilding `file3' because the build action changed:
-               old: %(python)s %(cat_py)s $TARGET $SOURCES
-               new: %(_python_)s %(cat_py)s ${TARGET}.alt $SOURCES
-                    %(python)s %(cat_py)s $TARGET $SOURCES
-                    %(_python_)s %(cat_py)s ${TARGET}.yyy $SOURCES yyy
+scons: rebuilding `file3' because:
+           the dependency order changed:
+           ->Sources
+           Old:zzz	New:zzz
+           Old:yyy	New:yyy
+           Old:xxx	New:xxx
+           ->Depends
+           ->Implicit
+           Old:%(_python_)s	New:%(_python_)s
+           Old:%(cat_py)s	New:%(cat_py)s
+           Old:None	New:yyy
 %(_python_)s %(cat_py)s file3.alt zzz yyy xxx
 %(_python_)s %(cat_py)s file3 zzz yyy xxx
 %(_python_)s %(cat_py)s file3.yyy zzz yyy xxx yyy
@@ -384,13 +394,17 @@ env.AddPreAction(f3, r'%(_python_)s %(cat_py)s ${TARGET}.alt $SOURCES')
 """ % locals())
 
 expect = test.wrap_stdout("""\
-scons: rebuilding `file3' because the build action changed:
-               old: %(_python_)s %(cat_py)s ${TARGET}.alt $SOURCES
-                    %(python)s %(cat_py)s $TARGET $SOURCES
-                    %(_python_)s %(cat_py)s ${TARGET}.yyy $SOURCES yyy
-               new: %(_python_)s %(cat_py)s ${TARGET}.alt $SOURCES
-                    %(python)s %(cat_py)s $TARGET $SOURCES
-                    %(_python_)s %(cat_py)s ${TARGET}.yyy $SOURCES xxx
+scons: rebuilding `file3' because:
+           the dependency order changed:
+           ->Sources
+           Old:zzz	New:zzz
+           Old:yyy	New:yyy
+           Old:xxx	New:xxx
+           ->Depends
+           ->Implicit
+           Old:%(_python_)s	New:%(_python_)s
+           Old:%(cat_py)s	New:%(cat_py)s
+           Old:yyy	New:xxx
 %(_python_)s %(cat_py)s file3.alt zzz yyy xxx
 %(_python_)s %(cat_py)s file3 zzz yyy xxx
 %(_python_)s %(cat_py)s file3.yyy zzz yyy xxx xxx