Re: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources section in INF file

["Liming Gao" <liming.gao@intel.com>]

Besides,  I suggest to separate the change on add new checker.  Another patch is about hash logic enhancement.

Thanks
Liming
>-----Original Message-----
>From: Rodriguez, Christian
>Sent: Thursday, May 23, 2019 9:21 PM
>To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io
>Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong
><yonghong.zhu@intel.com>
>Subject: RE: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources
>section in INF file
>
>Ok, I can show the build time performance impact here. Though the way I
>invalidated a module/library build is hash specific. What is the best way to
>invalidate a build in the original incremental build? Or does it matter because
>all the modules/libraries are rebuilt or checked for rebuild in the original
>incremental build?
>
>I'm guessing it's that second choice above, so I can move the conditional check
>to just the hash invalidation part and have the rest of the header source check
>always on.
>
>Thanks,
>Christian
>
>>-----Original Message-----
>>From: Feng, Bob C
>>Sent: Thursday, May 23, 2019 2:39 AM
>>To: devel@edk2.groups.io; Rodriguez, Christian
>><christian.rodriguez@intel.com>
>>Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong
>><yonghong.zhu@intel.com>
>>Subject: RE: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources
>>section in INF file
>>
>>Hi Christian,
>>
>>Since this is a INF spec required checking, I think the condition of this check
>>should be removed.
>>Would you show the build time to see what's the performance impact if we
>>always do this check?
>>
>>Thanks,
>>Bob
>>
>>-----Original Message-----
>>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>Christian Rodriguez
>>Sent: Tuesday, May 21, 2019 10:13 PM
>>To: devel@edk2.groups.io
>>Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
>><liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
>>Subject: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources section
>>in INF file
>>
>>BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1804
>>
>>In the Edk2 INF spec 3.9, it states, All HII Unicode format files must be listed
>in
>>[Sources] section. Add a check to see if [Sources] section lists all the "source"
>>type files of a module. Performance impact should be minimal with this
>patch
>>since information is already being fetched for Makefile purposes. All other
>>information is already cached in memory. No extra IO time is needed.
>>
>>Signed-off-by: Christian Rodriguez <christian.rodriguez@intel.com>
>>Cc: Bob Feng <bob.c.feng@intel.com>
>>Cc: Liming Gao <liming.gao@intel.com>
>>Cc: Yonghong Zhu <yonghong.zhu@intel.com>
>>---
>> BaseTools/Source/Python/AutoGen/AutoGen.py   |  6 ++++--
>> BaseTools/Source/Python/AutoGen/GenMake.py   | 45
>>+++++++++++++++++++++++++++++++++++++++++++++
>> BaseTools/Source/Python/Common/GlobalData.py |  3 ++-
>> BaseTools/Source/Python/build/build.py       | 66
>>++++++++++++++++++++++++++++++++++++++++--------------------------
>> 4 files changed, 91 insertions(+), 29 deletions(-)
>>
>>diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py
>>b/BaseTools/Source/Python/AutoGen/AutoGen.py
>>index a843f294b9..0bc27fb2b3 100644
>>--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
>>+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
>>@@ -3989,7 +3989,8 @@ class ModuleAutoGen(AutoGen):
>>             for LibraryAutoGen in self.LibraryAutoGenList:
>>                 LibraryAutoGen.CreateMakeFile()
>>
>>-        if self.CanSkip():
>>+        # Don't enable if hash feature enabled, CanSkip uses timestamps to
>>determine build skipping
>>+        if not GlobalData.gUseHashCache and self.CanSkip():
>>             return
>>
>>         if len(self.CustomMakefile) == 0:
>>@@ -4032,7 +4033,8 @@ class ModuleAutoGen(AutoGen):
>>             for LibraryAutoGen in self.LibraryAutoGenList:
>>                 LibraryAutoGen.CreateCodeFile()
>>
>>-        if self.CanSkip():
>>+        # Don't enable if hash feature enabled, CanSkip uses timestamps to
>>determine build skipping
>>+        if not GlobalData.gUseHashCache and self.CanSkip():
>>             return
>>
>>         AutoGenList = []
>>diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
>>b/BaseTools/Source/Python/AutoGen/GenMake.py
>>index 0e0f9fd9b0..8765ffa188 100644
>>--- a/BaseTools/Source/Python/AutoGen/GenMake.py
>>+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
>>@@ -905,6 +905,51 @@ cleanlib:
>>                                     ForceIncludedFile,
>>                                     self._AutoGenObject.IncludePathList +
>>self._AutoGenObject.BuildOptionIncPathList
>>                                     )
>>+
>>+        # Only do it during hashing feature for now to save time on clean build
>>+        # Conditional can be removed to happen on all builds for MetaFile
>>compliance
>>+        if GlobalData.gUseHashCache:
>>+            # Check if header files are listed in metafile
>>+            # Get a list of unique module header source files from MetaFile
>>+            headerFilesInMetaFileSet = set()
>>+            for aFile in self._AutoGenObject.SourceFileList:
>>+                aFileName = str(aFile)
>>+                if not aFileName.endswith('.h'):
>>+                    continue
>>+                headerFilesInMetaFileSet.add(aFileName.lower())
>>+
>>+            # Get a list of unique module autogen files
>>+            localAutoGenFileSet = set()
>>+            for aFile in self._AutoGenObject.AutoGenFileList:
>>+                localAutoGenFileSet.add(str(aFile).lower())
>>+
>>+            # Get a list of unique module dependency header files
>>+            # Exclude autogen files and files not in the source directory
>>+            headerFileDependencySet = set()
>>+            localSourceDir = str(self._AutoGenObject.SourceDir).lower()
>>+            for Dependency in FileDependencyDict.values():
>>+                for aFile in Dependency:
>>+                    aFileName = str(aFile).lower()
>>+                    if not aFileName.endswith('.h'):
>>+                        continue
>>+                    if aFileName in localAutoGenFileSet:
>>+                        continue
>>+                    if localSourceDir not in aFileName:
>>+                        continue
>>+                    headerFileDependencySet.add(aFileName)
>>+
>>+            # Ensure that gModuleBuildTracking has been initialized per
>>architecture
>>+            if self._AutoGenObject.Arch not in GlobalData.gModuleBuildTracking:
>>+
>>+ GlobalData.gModuleBuildTracking[self._AutoGenObject.Arch] = dict()
>>+
>>+            # Check if a module dependency header file is missing from the
>>module's MetaFile
>>+            for aFile in headerFileDependencySet:
>>+                if aFile not in headerFilesInMetaFileSet:
>>+
>>GlobalData.gModuleBuildTracking[self._AutoGenObject.Arch][self._AutoGe
>n
>>Object] = 'FAIL_METAFILE'
>>+                    EdkLogger.warn("build","Module MetaFile [Sources] is missing
>>local header!",
>>+                                ExtraData = "Local Header: " + aFile + " not found in " +
>>self._AutoGenObject.MetaFile.Path
>>+                                )
>>+
>>         DepSet = None
>>         for File,Dependency in FileDependencyDict.items():
>>             if not Dependency:
>>diff --git a/BaseTools/Source/Python/Common/GlobalData.py
>>b/BaseTools/Source/Python/Common/GlobalData.py
>>index 95e28a988f..bd45a43728 100644
>>--- a/BaseTools/Source/Python/Common/GlobalData.py
>>+++ b/BaseTools/Source/Python/Common/GlobalData.py
>>@@ -110,7 +110,8 @@ gEnableGenfdsMultiThread = False
>>gSikpAutoGenCache = set()
>>
>> # Dictionary for tracking Module build status as success or failure -# False ->
>>Fail : True -> Success
>>+# Top Dict:     Key: Arch Type              Value: Dictionary
>>+# Second Dict:  Key: AutoGen Obj    Value:
>'SUCCESS'\'FAIL'\'FAIL_METAFILE'
>> gModuleBuildTracking = dict()
>>
>> # Dictionary of booleans that dictate whether a module or diff --git
>>a/BaseTools/Source/Python/build/build.py
>>b/BaseTools/Source/Python/build/build.py
>>index 027061191c..0a58dd16ef 100644
>>--- a/BaseTools/Source/Python/build/build.py
>>+++ b/BaseTools/Source/Python/build/build.py
>>@@ -625,8 +625,16 @@ class BuildTask:
>>             BuildTask._ErrorFlag.set()
>>             BuildTask._ErrorMessage = "%s broken\n    %s [%s]" % \
>>                                       (threading.currentThread().getName(), Command,
>>WorkingDir)
>>-        if self.BuildItem.BuildObject in GlobalData.gModuleBuildTracking and
>not
>>BuildTask._ErrorFlag.isSet():
>>-            GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject] = True
>>+
>>+        # Set the value used by hash invalidation flow in
>>GlobalData.gModuleBuildTracking to 'SUCCESS'
>>+        # If Module or Lib is being tracked, it did not fail header check test, and
>>built successfully
>>+        if (self.BuildItem.BuildObject.Arch in GlobalData.gModuleBuildTracking
>>and
>>+           self.BuildItem.BuildObject in
>>GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch] and
>>+
>>GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch][self.Buil
>dI
>>tem.BuildObject] != 'FAIL_METAFILE' and
>>+           not BuildTask._ErrorFlag.isSet()
>>+           ):
>>+
>>GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch][self.Buil
>dI
>>tem.BuildObject] = 'SUCCESS'
>>+
>>         # indicate there's a thread is available for another build task
>>         BuildTask._RunningQueueLock.acquire()
>>         BuildTask._RunningQueue.pop(self.BuildItem)
>>@@ -1154,27 +1162,27 @@ class Build():
>>     #
>>     #
>>     def invalidateHash(self):
>>-        # GlobalData.gModuleBuildTracking contains only modules that cannot
>>be skipped by hash
>>-        for moduleAutoGenObj in GlobalData.gModuleBuildTracking.keys():
>>-            # False == FAIL : True == Success
>>-            # Skip invalidating for Successful module builds
>>-            if GlobalData.gModuleBuildTracking[moduleAutoGenObj] == True:
>>-                continue
>>+        # GlobalData.gModuleBuildTracking contains only modules or libs that
>>cannot be skipped by hash
>>+        for moduleAutoGenObjArch in
>GlobalData.gModuleBuildTracking.keys():
>>+            for moduleAutoGenObj in
>>GlobalData.gModuleBuildTracking[moduleAutoGenObjArch].keys():
>>+                # Skip invalidating for Successful Module/Lib builds
>>+                if
>>GlobalData.gModuleBuildTracking[moduleAutoGenObjArch][moduleAutoG
>e
>>nObj] == 'SUCCESS':
>>+                    continue
>>
>>-            # The module failed to build or failed to start building, from this point
>>on
>>+                # The module failed to build, failed to start building,
>>+ or failed the header check test from this point on
>>
>>-            # Remove .hash from build
>>-            if GlobalData.gUseHashCache:
>>-                ModuleHashFile = path.join(moduleAutoGenObj.BuildDir,
>>moduleAutoGenObj.Name + ".hash")
>>-                if os.path.exists(ModuleHashFile):
>>-                    os.remove(ModuleHashFile)
>>+                # Remove .hash from build
>>+                if GlobalData.gUseHashCache:
>>+                    ModuleHashFile = os.path.join(moduleAutoGenObj.BuildDir,
>>moduleAutoGenObj.Name + ".hash")
>>+                    if os.path.exists(ModuleHashFile):
>>+                        os.remove(ModuleHashFile)
>>
>>-            # Remove .hash file from cache
>>-            if GlobalData.gBinCacheDest:
>>-                FileDir = path.join(GlobalData.gBinCacheDest,
>>moduleAutoGenObj.Arch, moduleAutoGenObj.SourceDir,
>>moduleAutoGenObj.MetaFile.BaseName)
>>-                HashFile = path.join(FileDir, moduleAutoGenObj.Name + '.hash')
>>-                if os.path.exists(HashFile):
>>-                    os.remove(HashFile)
>>+                # Remove .hash file from cache
>>+                if GlobalData.gUseHashCache and GlobalData.gBinCacheDest:
>>+                    FileDir = os.path.join(GlobalData.gBinCacheDest,
>>moduleAutoGenObj.Arch, moduleAutoGenObj.SourceDir,
>>moduleAutoGenObj.MetaFile.BaseName)
>>+                    HashFile = os.path.join(FileDir, moduleAutoGenObj.Name +
>>'.hash')
>>+                    if os.path.exists(HashFile):
>>+                        os.remove(HashFile)
>>
>>     ## Build a module or platform
>>     #
>>@@ -1833,9 +1841,11 @@ class Build():
>>                                 if self.Target == "genmake":
>>                                     return True
>>                             self.BuildModules.append(Ma)
>>-                            # Initialize all modules in tracking to False (FAIL)
>>-                            if Ma not in GlobalData.gModuleBuildTracking:
>>-                                GlobalData.gModuleBuildTracking[Ma] = False
>>+                            # Initialize all modules in tracking to 'FAIL'
>>+                            if Ma.Arch not in GlobalData.gModuleBuildTracking:
>>+                                GlobalData.gModuleBuildTracking[Ma.Arch] = dict()
>>+                            if Ma not in GlobalData.gModuleBuildTracking[Ma.Arch]:
>>+                                GlobalData.gModuleBuildTracking[Ma.Arch][Ma] = 'FAIL'
>>                     self.AutoGenTime += int(round((time.time() - AutoGenStart)))
>>                     MakeStart = time.time()
>>                     for Ma in self.BuildModules:
>>@@ -1919,6 +1929,7 @@ class Build():
>>                     # Save MAP buffer into MAP file.
>>                     #
>>                     self._SaveMapFile (MapBuffer, Wa)
>>+        self.invalidateHash()
>>
>>     def _GenFfsCmd(self,ArchList):
>>         # convert dictionary of Cmd:(Inf,Arch) @@ -2017,9 +2028,11 @@ class
>>Build():
>>                             if self.Target == "genmake":
>>                                 continue
>>                         self.BuildModules.append(Ma)
>>-                        # Initialize all modules in tracking to False (FAIL)
>>-                        if Ma not in GlobalData.gModuleBuildTracking:
>>-                            GlobalData.gModuleBuildTracking[Ma] = False
>>+                        # Initialize all modules in tracking to 'FAIL'
>>+                        if Ma.Arch not in GlobalData.gModuleBuildTracking:
>>+                            GlobalData.gModuleBuildTracking[Ma.Arch] = dict()
>>+                        if Ma not in GlobalData.gModuleBuildTracking[Ma.Arch]:
>>+                            GlobalData.gModuleBuildTracking[Ma.Arch][Ma] = 'FAIL'
>>                     self.Progress.Stop("done!")
>>                     self.AutoGenTime += int(round((time.time() - AutoGenStart)))
>>                     MakeStart = time.time() @@ -2107,6 +2120,7 @@ class Build():
>>                     # Save MAP buffer into MAP file.
>>                     #
>>                     self._SaveMapFile(MapBuffer, Wa)
>>+        self.invalidateHash()
>>
>>     ## Generate GuidedSectionTools.txt in the FV directories.
>>     #
>>--
>>2.19.1.windows.1
>>
>>
>>