From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: christian.rodriguez@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Thu, 23 May 2019 06:21:28 -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 fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 May 2019 06:21:27 -0700 X-ExtLoop1: 1 Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by orsmga005.jf.intel.com with ESMTP; 23 May 2019 06:21:26 -0700 Received: from orsmsx125.amr.corp.intel.com (10.22.240.125) by ORSMSX105.amr.corp.intel.com (10.22.225.132) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 23 May 2019 06:21:26 -0700 Received: from orsmsx112.amr.corp.intel.com ([169.254.3.79]) by ORSMSX125.amr.corp.intel.com ([169.254.3.172]) with mapi id 14.03.0415.000; Thu, 23 May 2019 06:21:26 -0700 From: "Christian Rodriguez" 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 Thread-Topic: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources section in INF file Thread-Index: AQHVD99VRdDPq+FySUmN0D86q/9/MaZ4dIYggAA/A/A= Date: Thu, 23 May 2019 13:21:26 +0000 Message-ID: <3A7DCC9A944C6149BF832E1C9B718ABC01F22900@ORSMSX112.amr.corp.intel.com> References: <20190521141308.12144-1-christian.rodriguez@intel.com> <08650203BA1BD64D8AD9B6D5D74A85D160114686@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <08650203BA1BD64D8AD9B6D5D74A85D160114686@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYmQ3Mzc0Y2EtODQyMC00YzMyLTkxNzAtMjU0ODA5YjIzYTZmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiVFkyVFJFVUtHdGZiaWd5NjZ3WWk5c2l1TFFnWGhcLzhtQlhGQlZcLyt5RmxxZk9yRXNGNWhwM2wxUTY5S280bnNFIn0= dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.139] MIME-Version: 1.0 Return-Path: christian.rodriguez@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ok, I can show the build time performance impact here. Though the way I inv= alidated a module/library build is hash specific. What is the best way to i= nvalidate a build in the original incremental build? Or does it matter beca= use all the modules/libraries are rebuilt or checked for rebuild in the ori= ginal 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 so= urce 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 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 ; Gao, Liming >; Zhu, Yonghong >Subject: [edk2-devel] [PATCH] BaseTools: Add a checking for Sources secti= on >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 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 pa= tch >since information is already being fetched for Makefile purposes. All oth= er >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 clea= n build >+ # Conditional can be removed to happen on all builds for MetaFil= e >compliance >+ if GlobalData.gUseHashCache: >+ # Check if header files are listed in metafile >+ # Get a list of unique module header source files from MetaF= ile >+ 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 director= y >+ 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.gModuleBuildTr= acking: >+ >+ GlobalData.gModuleBuildTracking[self._AutoGenObject.Arch] =3D dict() >+ >+ # Check if a module dependency header file is missing from t= he >module's MetaFile >+ for aFile in headerFileDependencySet: >+ if aFile not in headerFilesInMetaFileSet: >+ >GlobalData.gModuleBuildTracking[self._AutoGenObject.Arch][self._AutoGen >Object] =3D 'FAIL_METAFILE' >+ EdkLogger.warn("build","Module MetaFile [Sources] is= 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 -# F= alse -> >Fail : True -> Success >+# Top Dict: Key: Arch Type Value: Dictionary >+# Second Dict: Key: AutoGen Obj Value: 'SUCCESS'\'FAIL'\'FAIL_METAFI= LE' > 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().getName= (), Command, >WorkingDir) >- if self.BuildItem.BuildObject in GlobalData.gModuleBuildTracking= 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 chec= k test, and >built successfully >+ if (self.BuildItem.BuildObject.Arch in GlobalData.gModuleBuildTr= acking >and >+ self.BuildItem.BuildObject in >GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch] and >+ >GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch][self.Bui= ldI >tem.BuildObject] !=3D 'FAIL_METAFILE' and >+ not BuildTask._ErrorFlag.isSet() >+ ): >+ >GlobalData.gModuleBuildTracking[self.BuildItem.BuildObject.Arch][self.Bui= ldI >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 can= not >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][moduleAutoGe >nObj] =3D=3D 'SUCCESS': >+ continue > >- # The module failed to build or failed to start building, fr= om 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.Bui= ldDir, >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.gBinCacheDest= : >+ 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 Fals= e (FAIL) >- if Ma not in GlobalData.gModuleBuildTracking= : >- GlobalData.gModuleBuildTracking[Ma] =3D = False >+ # Initialize all modules in tracking to 'FAI= L' >+ if Ma.Arch not in GlobalData.gModuleBuildTra= cking: >+ GlobalData.gModuleBuildTracking[Ma.Arch]= =3D dict() >+ if Ma not in GlobalData.gModuleBuildTracking= [Ma.Arch]: >+ GlobalData.gModuleBuildTracking[Ma.Arch]= [Ma] =3D 'FAIL' > self.AutoGenTime +=3D int(round((time.time() - AutoG= enStart))) > 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 @@ cl= ass >Build(): > if self.Target =3D=3D "genmake": > continue > self.BuildModules.append(Ma) >- # Initialize all modules in tracking to False (F= AIL) >- if Ma not in GlobalData.gModuleBuildTracking: >- GlobalData.gModuleBuildTracking[Ma] =3D Fals= e >+ # Initialize all modules in tracking to 'FAIL' >+ if Ma.Arch not in GlobalData.gModuleBuildTrackin= g: >+ 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() - AutoG= enStart))) > MakeStart =3D time.time() @@ -2107,6 +2120,7 @@ clas= s 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 > > >