Update Cachedir file copy error handling to handle the expected exceptions shutil.SameFileError, IOError for the default copy shutil.copy(). Move notice that file has been retrieved until after the file has actually been copied and didn't thrown exception

Author: bdbaddog

CHANGES.txt

@@ -36,6 +36,10 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Fix occasional test failures caused by not being able to find a file or directory fixture
       when running multiple tests with multiple jobs.
 
+  From Raven Kopelman (@ravenAtSafe on GitHub):
+    - Prevent files from being partially read from the cache. If there's an error while copying the file
+      is now removed.
+
   From Joachim Kuebart:
     - Suppress missing SConscript deprecation warning if `must_exist=False`
       is used.
@@ -48,6 +52,7 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
   From Daniel Moody:
     - Fix issue where java parsed a class incorrectly from lambdas used after a new.
 
+
   From Mats Wichmann:
     - Complete tests for Dictionary, env.keys() and env.values() for
       OverrideEnvironment. Enable env.setdefault() method, add tests.

SCons/CacheDir.py

@@ -29,6 +29,7 @@
 import os
 import stat
 import sys
+import shutil
 
 import SCons.Action
 import SCons.Errors
@@ -50,31 +51,28 @@ def CacheRetrieveFunc(target, source, env):
         cd.CacheDebug('CacheRetrieve(%s):  %s not in cache\n', t, cachefile)
         return 1
     cd.hits += 1
-    cd.CacheDebug('CacheRetrieve(%s):  retrieving from %s\n', t, cachefile)
     if SCons.Action.execute_actions:
         if fs.islink(cachefile):
             fs.symlink(fs.readlink(cachefile), t.get_internal_path())
         else:
             try:
                 env.copy_from_cache(cachefile, t.get_internal_path())
-            except:
-                try:
+            except (shutil.SameFileError, IOError) as e:
                     # In case file was partially retrieved (and now corrupt)
                     # delete it to avoid poisoning commands like 'ar' that
                     # read from the initial state of the file they are writing
                     # to.
                     t.fs.unlink(t.get_internal_path())
-                except:
-                    pass
-
-                raise
+                    cd.CacheDebug('CacheRetrieve(%s):  Error while retrieving from %s deleting %s\n', t, cachefile)
+                    raise e
 
             try:
                 os.utime(cachefile, None)
             except OSError:
                 pass
         st = fs.stat(cachefile)
         fs.chmod(t.get_internal_path(), stat.S_IMODE(st[stat.ST_MODE]) | stat.S_IWRITE)
+    cd.CacheDebug('CacheRetrieve(%s):  retrieved from %s\n', t, cachefile)
     return 0
 
 def CacheRetrieveString(target, source, env):
@@ -83,7 +81,7 @@ def CacheRetrieveString(target, source, env):
     cd = env.get_CacheDir()
     cachedir, cachefile = cd.cachepath(t)
     if t.fs.exists(cachefile):
-        return "Retrieving `%s' from cache" % t.get_internal_path()
+        return "Retrieved `%s' from cache" % t.get_internal_path()
     return None
 
 CacheRetrieve = SCons.Action.Action(CacheRetrieveFunc, CacheRetrieveString)

test/CacheDir/debug.py

@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# __COPYRIGHT__
+# Copyright The SCons Foundation
 #
 # Permission is hereby granted, free of charge, to any person obtaining
 # a copy of this software and associated documentation files (the
@@ -22,7 +22,6 @@
 # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 #
 
-__revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
 """
 Test the --cache-debug option to see if it prints the expected messages.
@@ -145,16 +144,16 @@ def cat(env, source, target):
 
 expect = \
 r"""Retrieved `aaa.out' from cache
-CacheRetrieve\(aaa.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(aaa.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 Retrieved `bbb.out' from cache
-CacheRetrieve\(bbb.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(bbb.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 Retrieved `ccc.out' from cache
-CacheRetrieve\(ccc.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(ccc.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 Retrieved `all' from cache
-CacheRetrieve\(all\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(all\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 """
 
@@ -179,13 +178,13 @@ def cat(env, source, target):
          stdout=expect)
 
 expect = \
-r"""CacheRetrieve\(aaa.out\):  retrieving from [0-9a-fA-F]+
+r"""CacheRetrieve\(aaa.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
-CacheRetrieve\(bbb.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(bbb.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
-CacheRetrieve\(ccc.out\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(ccc.out\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
-CacheRetrieve\(all\):  retrieving from [0-9a-fA-F]+
+CacheRetrieve\(all\):  retrieved from [0-9a-fA-F]+
 requests: [0-9]+, hits: [0-9]+, misses: [0-9]+, hit rate: [0-9]+\.[0-9]{2,}%
 """