[PATCH v2 0/1] BaseTools:Introduce CopyFileOnChange() function to copy cache files

[PATCH v2 0/1] BaseTools:Introduce CopyFileOnChange() function to copy cache files

[Steven Shi]

V2:
set shallow=False in filecmp.cmp() to compare the src and dsc files content
use shutil.copy() replace the  shutil.copyfile() to do the file copy, which
is better to reserve the dsc file metadata

V1:
Initial fix

Steven Shi (1):
  BaseTools:Introduce CopyFileOnChange() function to copy cache files

 BaseTools/Source/Python/AutoGen/AutoGen.py    | 10 ++--
 .../Source/Python/Common/LongFilePathOs.py    |  4 ++
 BaseTools/Source/Python/Common/Misc.py        | 55 +++++++++++++++++++
 3 files changed, 64 insertions(+), 5 deletions(-)

-- 
2.17.1.windows.2

[PATCH v2 1/1] BaseTools:Introduce CopyFileOnChange() function to copy cache files

[Steven Shi]

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1894

Basetool need a CopyFileOnChange() function to avoid cache
file writing race in multi-thread build. Some platforms
build fail with file IO writing race issue when the
build cache is enabled to store cache files in multi-threads.
This is because common same library cache files (e.g. some
libs in MdePkg) can be stored by many different driver modules'
build threads at same time. Current build cache need a function
to check whether the same cache file already exist, and only
copy source file if it is different from the destination file.

This patch introduces an atomic copy function to avoid duplicated
cache files copy.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Christian Rodriguez <christian.rodriguez@intel.com>
Signed-off-by: Steven Shi <steven.shi@intel.com>
---
 BaseTools/Source/Python/AutoGen/AutoGen.py       | 10 +++++-----
 BaseTools/Source/Python/Common/LongFilePathOs.py |  4 ++++
 BaseTools/Source/Python/Common/Misc.py           | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 3f41fbb507..46c8fbe181 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -3901,11 +3901,11 @@ class ModuleAutoGen(AutoGen):
         CreateDirectory (FileDir)
         HashFile = path.join(self.BuildDir, self.Name + '.hash')
         if os.path.exists(HashFile):
-            shutil.copy2(HashFile, FileDir)
+            CopyFileOnChange(HashFile, FileDir)
         if not self.IsLibrary:
             ModuleFile = path.join(self.OutputDir, self.Name + '.inf')
             if os.path.exists(ModuleFile):
-                shutil.copy2(ModuleFile, FileDir)
+                CopyFileOnChange(ModuleFile, FileDir)
         else:
             OutputDir = self.OutputDir.replace('\\', '/').strip('/')
             DebugDir = self.DebugDir.replace('\\', '/').strip('/')
@@ -3925,7 +3925,7 @@ class ModuleAutoGen(AutoGen):
                     destination_file = os.path.join(FileDir, sub_dir)
                     destination_dir = os.path.dirname(destination_file)
                     CreateDirectory(destination_dir)
-                    shutil.copy2(File, destination_dir)
+                    CopyFileOnChange(File, destination_dir)
 
     def AttemptModuleCacheCopy(self):
         # If library or Module is binary do not skip by hash
@@ -3947,14 +3947,14 @@ class ModuleAutoGen(AutoGen):
                     for root, dir, files in os.walk(FileDir):
                         for f in files:
                             if self.Name + '.hash' in f:
-                                shutil.copy(HashFile, self.BuildDir)
+                                CopyFileOnChange(HashFile, self.BuildDir)
                             else:
                                 File = path.join(root, f)
                                 sub_dir = os.path.relpath(File, FileDir)
                                 destination_file = os.path.join(self.OutputDir, sub_dir)
                                 destination_dir = os.path.dirname(destination_file)
                                 CreateDirectory(destination_dir)
-                                shutil.copy(File, destination_dir)
+                                CopyFileOnChange(File, destination_dir)
                     if self.Name == "PcdPeim" or self.Name == "PcdDxe":
                         CreatePcdDatabaseCode(self, TemplateString(), TemplateString())
                     return True
diff --git a/BaseTools/Source/Python/Common/LongFilePathOs.py b/BaseTools/Source/Python/Common/LongFilePathOs.py
index ae8a68e4ad..190f36d7ec 100644
--- a/BaseTools/Source/Python/Common/LongFilePathOs.py
+++ b/BaseTools/Source/Python/Common/LongFilePathOs.py
@@ -60,6 +60,10 @@ def listdir(path):
         List.append(Item)
     return List
 
+if hasattr(os, 'replace'):
+    def replace(src, dst):
+        return os.replace(LongFilePath(src), LongFilePath(dst))
+
 environ = os.environ
 getcwd = os.getcwd
 chdir = os.chdir
diff --git a/BaseTools/Source/Python/Common/Misc.py b/BaseTools/Source/Python/Common/Misc.py
index d082c58bef..9a63463913 100644
--- a/BaseTools/Source/Python/Common/Misc.py
+++ b/BaseTools/Source/Python/Common/Misc.py
@@ -18,6 +18,7 @@ import re
 import pickle
 import array
 import shutil
+import filecmp
 from random import sample
 from struct import pack
 import uuid
@@ -502,6 +503,60 @@ def SaveFileOnChange(File, Content, IsBinaryFile=True):
 
     return True
 
+## Copy source file only if it is different from the destination file
+#
+#  This method is used to copy file only if the source file and destination
+#  file content are different. This is quite useful to avoid duplicated
+#  file writing.
+#
+#   @param      SrcFile   The path of source file
+#   @param      Dst       The path of destination file or folder
+#
+#   @retval     True      The two files content are different and the file is copied
+#   @retval     False     No copy really happen
+#
+def CopyFileOnChange(SrcFile, Dst):
+    if not os.path.exists(SrcFile):
+        return False
+
+    if os.path.isdir(Dst):
+        DstFile = os.path.join(Dst, os.path.basename(SrcFile))
+    else:
+        DstFile = Dst
+
+    if os.path.exists(DstFile) and filecmp.cmp(SrcFile, DstFile, shallow=False):
+        return False
+
+    DirName = os.path.dirname(DstFile)
+    if not CreateDirectory(DirName):
+        EdkLogger.error(None, FILE_CREATE_FAILURE, "Could not create directory %s" % DirName)
+    else:
+        if DirName == '':
+            DirName = os.getcwd()
+        if not os.access(DirName, os.W_OK):
+            EdkLogger.error(None, PERMISSION_FAILURE, "Do not have write permission on directory %s" % DirName)
+
+    # os.replace and os.rename are the atomic operations in python 3 and 2.
+    # we use these two atomic operations to ensure the file copy is atomic:
+    # copy the src to a temp file in the dst same folder firstly, then
+    # replace or rename the temp file to the destination file.
+    with tempfile.NamedTemporaryFile(dir=DirName, delete=False) as tf:
+        shutil.copy(SrcFile, tf.name)
+        tempname = tf.name
+    try:
+        if hasattr(os, 'replace'):
+            os.replace(tempname, DstFile)
+        else:
+            # os.rename reqire to remove the dst on Windows, otherwise OSError will be raised.
+            if GlobalData.gIsWindows and os.path.exists(DstFile):
+                os.remove(DstFile)
+            os.rename(tempname, DstFile)
+
+    except IOError as X:
+        EdkLogger.error(None, FILE_COPY_FAILURE, ExtraData='IOError %s' % X)
+
+    return True
+
 ## Retrieve and cache the real path name in file system
 #
 #   @param      Root    The root directory of path relative to
-- 
2.17.1.windows.2

Re: [PATCH v2 1/1] BaseTools:Introduce CopyFileOnChange() function to copy cache files

[Bob Feng]

Reviewed-by: Bob Feng <bob.c.feng@intel.com>

-----Original Message-----
From: Shi, Steven 
Sent: Monday, June 17, 2019 4:44 PM
To: devel@edk2.groups.io
Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Rodriguez, Christian <christian.rodriguez@intel.com>
Subject: [PATCH v2 1/1] BaseTools:Introduce CopyFileOnChange() function to copy cache files

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1894

Basetool need a CopyFileOnChange() function to avoid cache file writing race in multi-thread build. Some platforms build fail with file IO writing race issue when the build cache is enabled to store cache files in multi-threads.
This is because common same library cache files (e.g. some libs in MdePkg) can be stored by many different driver modules'
build threads at same time. Current build cache need a function to check whether the same cache file already exist, and only copy source file if it is different from the destination file.

This patch introduces an atomic copy function to avoid duplicated cache files copy.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Christian Rodriguez <christian.rodriguez@intel.com>
Signed-off-by: Steven Shi <steven.shi@intel.com>
---
 BaseTools/Source/Python/AutoGen/AutoGen.py       | 10 +++++-----
 BaseTools/Source/Python/Common/LongFilePathOs.py |  4 ++++
 BaseTools/Source/Python/Common/Misc.py           | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 3f41fbb507..46c8fbe181 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -3901,11 +3901,11 @@ class ModuleAutoGen(AutoGen):
         CreateDirectory (FileDir)
         HashFile = path.join(self.BuildDir, self.Name + '.hash')
         if os.path.exists(HashFile):
-            shutil.copy2(HashFile, FileDir)
+            CopyFileOnChange(HashFile, FileDir)
         if not self.IsLibrary:
             ModuleFile = path.join(self.OutputDir, self.Name + '.inf')
             if os.path.exists(ModuleFile):
-                shutil.copy2(ModuleFile, FileDir)
+                CopyFileOnChange(ModuleFile, FileDir)
         else:
             OutputDir = self.OutputDir.replace('\\', '/').strip('/')
             DebugDir = self.DebugDir.replace('\\', '/').strip('/') @@ -3925,7 +3925,7 @@ class ModuleAutoGen(AutoGen):
                     destination_file = os.path.join(FileDir, sub_dir)
                     destination_dir = os.path.dirname(destination_file)
                     CreateDirectory(destination_dir)
-                    shutil.copy2(File, destination_dir)
+                    CopyFileOnChange(File, destination_dir)
 
     def AttemptModuleCacheCopy(self):
         # If library or Module is binary do not skip by hash @@ -3947,14 +3947,14 @@ class ModuleAutoGen(AutoGen):
                     for root, dir, files in os.walk(FileDir):
                         for f in files:
                             if self.Name + '.hash' in f:
-                                shutil.copy(HashFile, self.BuildDir)
+                                CopyFileOnChange(HashFile, 
+ self.BuildDir)
                             else:
                                 File = path.join(root, f)
                                 sub_dir = os.path.relpath(File, FileDir)
                                 destination_file = os.path.join(self.OutputDir, sub_dir)
                                 destination_dir = os.path.dirname(destination_file)
                                 CreateDirectory(destination_dir)
-                                shutil.copy(File, destination_dir)
+                                CopyFileOnChange(File, destination_dir)
                     if self.Name == "PcdPeim" or self.Name == "PcdDxe":
                         CreatePcdDatabaseCode(self, TemplateString(), TemplateString())
                     return True
diff --git a/BaseTools/Source/Python/Common/LongFilePathOs.py b/BaseTools/Source/Python/Common/LongFilePathOs.py
index ae8a68e4ad..190f36d7ec 100644
--- a/BaseTools/Source/Python/Common/LongFilePathOs.py
+++ b/BaseTools/Source/Python/Common/LongFilePathOs.py
@@ -60,6 +60,10 @@ def listdir(path):
         List.append(Item)
     return List
 
+if hasattr(os, 'replace'):
+    def replace(src, dst):
+        return os.replace(LongFilePath(src), LongFilePath(dst))
+
 environ = os.environ
 getcwd = os.getcwd
 chdir = os.chdir
diff --git a/BaseTools/Source/Python/Common/Misc.py b/BaseTools/Source/Python/Common/Misc.py
index d082c58bef..9a63463913 100644
--- a/BaseTools/Source/Python/Common/Misc.py
+++ b/BaseTools/Source/Python/Common/Misc.py
@@ -18,6 +18,7 @@ import re
 import pickle
 import array
 import shutil
+import filecmp
 from random import sample
 from struct import pack
 import uuid
@@ -502,6 +503,60 @@ def SaveFileOnChange(File, Content, IsBinaryFile=True):
 
     return True
 
+## Copy source file only if it is different from the destination file # 
+#  This method is used to copy file only if the source file and 
+destination #  file content are different. This is quite useful to 
+avoid duplicated #  file writing.
+#
+#   @param      SrcFile   The path of source file
+#   @param      Dst       The path of destination file or folder
+#
+#   @retval     True      The two files content are different and the file is copied
+#   @retval     False     No copy really happen
+#
+def CopyFileOnChange(SrcFile, Dst):
+    if not os.path.exists(SrcFile):
+        return False
+
+    if os.path.isdir(Dst):
+        DstFile = os.path.join(Dst, os.path.basename(SrcFile))
+    else:
+        DstFile = Dst
+
+    if os.path.exists(DstFile) and filecmp.cmp(SrcFile, DstFile, shallow=False):
+        return False
+
+    DirName = os.path.dirname(DstFile)
+    if not CreateDirectory(DirName):
+        EdkLogger.error(None, FILE_CREATE_FAILURE, "Could not create directory %s" % DirName)
+    else:
+        if DirName == '':
+            DirName = os.getcwd()
+        if not os.access(DirName, os.W_OK):
+            EdkLogger.error(None, PERMISSION_FAILURE, "Do not have 
+ write permission on directory %s" % DirName)
+
+    # os.replace and os.rename are the atomic operations in python 3 and 2.
+    # we use these two atomic operations to ensure the file copy is atomic:
+    # copy the src to a temp file in the dst same folder firstly, then
+    # replace or rename the temp file to the destination file.
+    with tempfile.NamedTemporaryFile(dir=DirName, delete=False) as tf:
+        shutil.copy(SrcFile, tf.name)
+        tempname = tf.name
+    try:
+        if hasattr(os, 'replace'):
+            os.replace(tempname, DstFile)
+        else:
+            # os.rename reqire to remove the dst on Windows, otherwise OSError will be raised.
+            if GlobalData.gIsWindows and os.path.exists(DstFile):
+                os.remove(DstFile)
+            os.rename(tempname, DstFile)
+
+    except IOError as X:
+        EdkLogger.error(None, FILE_COPY_FAILURE, ExtraData='IOError %s' 
+ % X)
+
+    return True
+
 ## Retrieve and cache the real path name in file system  #
 #   @param      Root    The root directory of path relative to
--
2.17.1.windows.2