Merge pull request #475 from jkloetzke/win32-robust-rename

Fix spurious file renaming errors on Windows

pym/bob/generators/common.py

@@ -4,7 +4,7 @@
 # SPDX-License-Identifier: GPL-3.0-or-later
 
 from ..errors import BuildError
-from ..utils import removePath, isWindows, summonMagic
+from ..utils import removePath, isWindows, summonMagic, replacePath
 import argparse
 import os
 import re
@@ -346,7 +346,7 @@ def updateFile(self, name, content, encoding=None, newline=None):
             oldContent = None
 
         if oldContent != newContent:
-            os.replace(newName, name)
+            replacePath(newName, name)
             with open(oldName, "wb") as f:
                 f.write(newContent)
         else:

pym/bob/input.py

@@ -13,7 +13,8 @@
 from .stringparser import checkGlobList, Env, DEFAULT_STRING_FUNS, IfExpression
 from .tty import InfoOnce, Warn, WarnOnce, setColorMode
 from .utils import asHexStr, joinScripts, compareVersion, binStat, \
-    updateDicRecursive, hashString, getPlatformTag, getPlatformString
+    updateDicRecursive, hashString, getPlatformTag, getPlatformString, \
+    replacePath
 from itertools import chain
 from os.path import expanduser
 from string import Template
@@ -3859,7 +3860,7 @@ def __generatePackages(self, nameFormatter, cacheKey, sandboxEnabled):
             with open(newCacheName, "wb") as f:
                 f.write(cacheKey)
                 PackagePickler(f, nameFormatter).dump(result)
-            os.replace(newCacheName, cacheName)
+            replacePath(newCacheName, cacheName)
         except OSError as e:
             print("Error saving internal state:", str(e), file=sys.stderr)
 

pym/bob/scm/url.py

@@ -6,7 +6,8 @@
 from .. import BOB_VERSION
 from ..errors import BuildError, ParseError
 from ..stringparser import IfExpression
-from ..utils import asHexStr, hashFile, isWindows, removeUserFromUrl, sslNoVerifyContext
+from ..utils import asHexStr, hashFile, isWindows, removeUserFromUrl, sslNoVerifyContext, \
+        replacePath
 from .scm import Scm, ScmAudit
 from http.client import HTTPException
 import asyncio
@@ -252,7 +253,7 @@ def _download(self, destination):
                 # Atomically move file to destination. Set explicit mode to
                 # retain Bob 0.15 behaviour.
                 os.chmod(tmpFileName, stat.S_IREAD|stat.S_IWRITE)
-                os.replace(tmpFileName, destination)
+                replacePath(tmpFileName, destination)
                 tmpFileName = None
 
         except urllib.error.HTTPError as e:

pym/bob/state.py

@@ -5,6 +5,7 @@
 
 from .errors import ParseError
 from .tty import colorize, WarnOnce, WARNING
+from .utils import replacePath
 import copy
 import errno
 import os
@@ -169,7 +170,7 @@ def __save(self):
                 with open(dirtyPath, "wb") as f:
                     with DigestAdder(f) as df:
                         pickle.dump(state, df)
-                os.replace(dirtyPath, self.__uncommittedPath)
+                replacePath(dirtyPath, self.__uncommittedPath)
             except OSError as e:
                 raise ParseError("Error saving workspace state: " + str(e))
         else:
@@ -188,7 +189,7 @@ def __commit(self, verify=True):
                     commit = (csum == data[-4:])
                 os.fsync(f.fileno())
             if commit:
-                os.replace(self.__uncommittedPath, self.__path)
+                replacePath(self.__uncommittedPath, self.__path)
                 return
             else:
                 print(colorize("Warning: discarding corrupted workspace state!", WARNING),

pym/bob/utils.py

@@ -211,10 +211,25 @@ def isWindows():
 else:
     __isWindows = True
 
+def _replacePathWin32(src, dst):
+    # Workaround for spurious PermissionError's on Windows.
+    i = 0
+    while True:
+        try:
+            os.replace(src, dst)
+            break
+        except PermissionError:
+            if i >= 10: raise
+            import time
+            time.sleep(0.1 * i)
+            i += 1
+
 if __isWindows:
     INVALID_CHAR_TRANS = str.maketrans(':*?<>"|', '_______')
+    replacePath = _replacePathWin32
 else:
     INVALID_CHAR_TRANS = str.maketrans('', '')
+    replacePath = os.replace
 
 
 __canSymlink = None
@@ -352,7 +367,7 @@ def close(self):
                     self.__inFile.close()
                 if self.__outFile:
                     self.__outFile.close()
-                    os.replace(self.__outFile.name, self.__cachePath)
+                    replacePath(self.__outFile.name, self.__cachePath)
             except OSError as e:
                 raise BuildError("Error closing hash cache: " + str(e))
 

test/test_utils.py

@@ -5,12 +5,13 @@
 
 from tempfile import NamedTemporaryFile, TemporaryDirectory
 from unittest import TestCase
-from unittest.mock import patch
+from unittest.mock import patch, MagicMock
 import os, stat
 import asyncio
 
 from bob.utils import joinScripts, removePath, emptyDirectory, compareVersion, \
-    getPlatformTag, run, check_output, removeUserFromUrl, runInEventLoop
+    getPlatformTag, run, check_output, removeUserFromUrl, runInEventLoop, \
+    _replacePathWin32
 from bob.errors import BuildError, ParseError
 
 class TestJoinScripts(TestCase):
@@ -295,3 +296,25 @@ def testWinUncFileUrl(self):
         self.assertEqual(removeUserFromUrl(r"file:///\\server\path"),
             r"file:///\\server\path")
 
+class TestReplacePath(TestCase):
+    def testWin32Ok(self):
+        m = MagicMock(return_value=None)
+        with patch('os.replace', m):
+            _replacePathWin32("foo", "bar")
+        m.assert_called_with("foo", "bar")
+
+    def testWin32Spurious(self):
+        """One PermissionError is discarded"""
+        m = MagicMock(side_effect=[PermissionError(42), None])
+        with patch('os.replace', m):
+            _replacePathWin32("foo", "bar")
+        m.assert_called_with("foo", "bar")
+
+    @patch('time.sleep', MagicMock())
+    def testWin32Fail(self):
+        """Sticky PermissionError is still raised"""
+        m = MagicMock(side_effect=PermissionError(42))
+        with self.assertRaises(PermissionError):
+            with patch('os.replace', m):
+                _replacePathWin32("foo", "bar")
+        m.assert_called_with("foo", "bar")