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 <[email protected]>; [email protected] >Cc: Gao, Liming <[email protected]>; Zhu, Yonghong ><[email protected]> >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: [email protected]; Rodriguez, Christian >><[email protected]> >>Cc: Gao, Liming <[email protected]>; Zhu, Yonghong >><[email protected]> >>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: [email protected] [mailto:[email protected]] On Behalf Of >>Christian Rodriguez >>Sent: Tuesday, May 21, 2019 10:13 PM >>To: [email protected] >>Cc: Feng, Bob C <[email protected]>; Gao, Liming >><[email protected]>; Zhu, Yonghong <[email protected]> >>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 <[email protected]> >>Cc: Bob Feng <[email protected]> >>Cc: Liming Gao <[email protected]> >>Cc: Yonghong Zhu <[email protected]> >>--- >> 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 >> >> >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41300): https://edk2.groups.io/g/devel/message/41300 Mute This Topic: https://groups.io/mt/31697075/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
