From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: liming.gao@intel.com) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by groups.io with SMTP; Thu, 23 May 2019 21:58:58 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 May 2019 21:58:57 -0700 X-ExtLoop1: 1 Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga005.jf.intel.com with ESMTP; 23 May 2019 21:58:56 -0700 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 23 May 2019 21:58:56 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 23 May 2019 21:58:55 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.33]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.70]) with mapi id 14.03.0415.000; Fri, 24 May 2019 12:58:54 +0800 From: "Liming Gao" To: "Rodriguez, Christian" , "Feng, Bob C" , "devel@edk2.groups.io" CC: "Zhu, Yonghong" Subject: Re: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources section in INF file Thread-Topic: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources section in INF file Thread-Index: AQHVD99XGkRb7wxuF0ytTHyec3V3X6Z38PQAgAA+CQCAAYuPkA== Date: Fri, 24 May 2019 04:58:53 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E450A09@SHSMSX104.ccr.corp.intel.com> References: <20190521141308.12144-1-christian.rodriguez@intel.com> <08650203BA1BD64D8AD9B6D5D74A85D160114686@SHSMSX101.ccr.corp.intel.com> <3A7DCC9A944C6149BF832E1C9B718ABC01F22900@ORSMSX112.amr.corp.intel.com> In-Reply-To: <3A7DCC9A944C6149BF832E1C9B718ABC01F22900@ORSMSX112.amr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: liming.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Besides, I suggest to separate the change on add new checker. Another pat= ch is about hash logic enhancement. Thanks Liming >-----Original Message----- >From: Rodriguez, Christian >Sent: Thursday, May 23, 2019 9:21 PM >To: Feng, Bob C ; devel@edk2.groups.io >Cc: Gao, Liming ; Zhu, Yonghong > >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 b= ecause >all the modules/libraries are rebuilt or checked for rebuild in the origi= nal >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 >> >>Cc: Gao, Liming ; Zhu, Yonghong >> >>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 thi= s 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 ; Gao, Liming >>; Zhu, Yonghong >>Subject: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources sect= ion >>in INF file >> >>BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1804 >> >>In the Edk2 INF spec 3.9, it states, All HII Unicode format files must b= e 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 ot= her >>information is already cached in memory. No extra IO time is needed. >> >>Signed-off-by: Christian Rodriguez >>Cc: Bob Feng >>Cc: Liming Gao >>Cc: Yonghong Zhu >>--- >> 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) =3D=3D 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 =3D [] >>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 cle= an build >>+ # Conditional can be removed to happen on all builds for MetaFi= le >>compliance >>+ if GlobalData.gUseHashCache: >>+ # Check if header files are listed in metafile >>+ # Get a list of unique module header source files from Meta= File >>+ headerFilesInMetaFileSet =3D set() >>+ for aFile in self._AutoGenObject.SourceFileList: >>+ aFileName =3D str(aFile) >>+ if not aFileName.endswith('.h'): >>+ continue >>+ headerFilesInMetaFileSet.add(aFileName.lower()) >>+ >>+ # Get a list of unique module autogen files >>+ localAutoGenFileSet =3D 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 directo= ry >>+ headerFileDependencySet =3D set() >>+ localSourceDir =3D str(self._AutoGenObject.SourceDir).lower= () >>+ for Dependency in FileDependencyDict.values(): >>+ for aFile in Dependency: >>+ aFileName =3D 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.gModuleBuildT= racking: >>+ >>+ GlobalData.gModuleBuildTracking[self._AutoGenObject.Arch] =3D 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] =3D 'FAIL_METAFILE' >>+ EdkLogger.warn("build","Module MetaFile [Sources] i= s missing >>local header!", >>+ ExtraData =3D "Local Header: " + aFile = + " not found in " + >>self._AutoGenObject.MetaFile.Path >>+ ) >>+ >> DepSet =3D 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 =3D False >>gSikpAutoGenCache =3D 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 =3D 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 =3D "%s broken\n %s [%s]" % \ >> (threading.currentThread().getNam= e(), Command, >>WorkingDir) >>- if self.BuildItem.BuildObject in GlobalData.gModuleBuildTrackin= g and >not >>BuildTask._ErrorFlag.isSet(): >>- GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject]= =3D 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 che= ck test, and >>built successfully >>+ if (self.BuildItem.BuildObject.Arch in GlobalData.gModuleBuildT= racking >>and >>+ self.BuildItem.BuildObject in >>GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch] and >>+ >>GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch][self.Bu= il >dI >>tem.BuildObject] !=3D 'FAIL_METAFILE' and >>+ not BuildTask._ErrorFlag.isSet() >>+ ): >>+ >>GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch][self.Bu= il >dI >>tem.BuildObject] =3D '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 ca= nnot >>be skipped by hash >>- for moduleAutoGenObj in GlobalData.gModuleBuildTracking.keys(): >>- # False =3D=3D FAIL : True =3D=3D Success >>- # Skip invalidating for Successful module builds >>- if GlobalData.gModuleBuildTracking[moduleAutoGenObj] =3D=3D= 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] =3D=3D 'SUCCESS': >>+ continue >> >>- # The module failed to build or failed to start building, f= rom 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 =3D path.join(moduleAutoGenObj.BuildDir, >>moduleAutoGenObj.Name + ".hash") >>- if os.path.exists(ModuleHashFile): >>- os.remove(ModuleHashFile) >>+ # Remove .hash from build >>+ if GlobalData.gUseHashCache: >>+ ModuleHashFile =3D os.path.join(moduleAutoGenObj.Bu= ildDir, >>moduleAutoGenObj.Name + ".hash") >>+ if os.path.exists(ModuleHashFile): >>+ os.remove(ModuleHashFile) >> >>- # Remove .hash file from cache >>- if GlobalData.gBinCacheDest: >>- FileDir =3D path.join(GlobalData.gBinCacheDest, >>moduleAutoGenObj.Arch, moduleAutoGenObj.SourceDir, >>moduleAutoGenObj.MetaFile.BaseName) >>- HashFile =3D path.join(FileDir, moduleAutoGenObj.Name += '.hash') >>- if os.path.exists(HashFile): >>- os.remove(HashFile) >>+ # Remove .hash file from cache >>+ if GlobalData.gUseHashCache and GlobalData.gBinCacheDes= t: >>+ FileDir =3D os.path.join(GlobalData.gBinCacheDest, >>moduleAutoGenObj.Arch, moduleAutoGenObj.SourceDir, >>moduleAutoGenObj.MetaFile.BaseName) >>+ HashFile =3D 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 =3D=3D "genmake": >> return True >> self.BuildModules.append(Ma) >>- # Initialize all modules in tracking to Fal= se (FAIL) >>- if Ma not in GlobalData.gModuleBuildTrackin= g: >>- GlobalData.gModuleBuildTracking[Ma] =3D= False >>+ # Initialize all modules in tracking to 'FA= IL' >>+ if Ma.Arch not in GlobalData.gModuleBuildTr= acking: >>+ GlobalData.gModuleBuildTracking[Ma.Arch= ] =3D dict() >>+ if Ma not in GlobalData.gModuleBuildTrackin= g[Ma.Arch]: >>+ GlobalData.gModuleBuildTracking[Ma.Arch= ][Ma] =3D 'FAIL' >> self.AutoGenTime +=3D int(round((time.time() - Auto= GenStart))) >> MakeStart =3D 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 @@ c= lass >>Build(): >> if self.Target =3D=3D "genmake": >> continue >> self.BuildModules.append(Ma) >>- # Initialize all modules in tracking to False (= FAIL) >>- if Ma not in GlobalData.gModuleBuildTracking: >>- GlobalData.gModuleBuildTracking[Ma] =3D Fal= se >>+ # Initialize all modules in tracking to 'FAIL' >>+ if Ma.Arch not in GlobalData.gModuleBuildTracki= ng: >>+ GlobalData.gModuleBuildTracking[Ma.Arch] = =3D dict() >>+ if Ma not in GlobalData.gModuleBuildTracking[Ma= .Arch]: >>+ GlobalData.gModuleBuildTracking[Ma.Arch][Ma= ] =3D 'FAIL' >> self.Progress.Stop("done!") >> self.AutoGenTime +=3D int(round((time.time() - Auto= GenStart))) >> MakeStart =3D time.time() @@ -2107,6 +2120,7 @@ cla= ss 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 >> >> >>