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.120, mailfrom: jaben.carsey@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Fri, 10 May 2019 09:14:50 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 May 2019 09:14:49 -0700 X-ExtLoop1: 1 Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 10 May 2019 09:14:49 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 10 May 2019 09:14:48 -0700 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.107]) by FMSMSX112.amr.corp.intel.com ([169.254.5.167]) with mapi id 14.03.0415.000; Fri, 10 May 2019 09:14:48 -0700 From: "Carsey, Jaben" To: "Rodriguez, Christian" , "devel@edk2.groups.io" CC: "Feng, Bob C" , "Gao, Liming" , "Zhu, Yonghong" Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed Thread-Topic: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed Thread-Index: AQHVBq4PdUG0FlWYxU2Tc9jPwsVOEKZjcrKQgAF/OAD//5ZDkA== Date: Fri, 10 May 2019 16:14:48 +0000 Message-ID: References: <20190509212719.6156-1-christian.rodriguez@intel.com> <3A7DCC9A944C6149BF832E1C9B718ABC01EDA27B@ORSMSX112.amr.corp.intel.com> In-Reply-To: <3A7DCC9A944C6149BF832E1C9B718ABC01EDA27B@ORSMSX112.amr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTQ4NWIxNTUtYzcyYi00OGZmLWFjOTItYmM3YmQwMWRiMjE5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUjlzUVYrdXVGRDhralJ4WWthNFRxYW9ZZzJHXC9KQ1hIYTZ6NEd3U3l5Nkg3MVJKWUoxaVBhcUFjRFRDUW83N2oifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Return-Path: jaben.carsey@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Inline. tldr: good answers.=20 If change list to set in name of set object: Reviewed-by: jaben carsey > -----Original Message----- > From: Rodriguez, Christian > Sent: Friday, May 10, 2019 8:28 AM > To: Carsey, Jaben ; devel@edk2.groups.io > Cc: Feng, Bob C ; Gao, Liming > ; Zhu, Yonghong > Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not > mentioned in inf are not hashed > Importance: High >=20 > Replies inline. >=20 > >-----Original Message----- > >From: Carsey, Jaben > >Sent: Thursday, May 9, 2019 4:39 PM > >To: devel@edk2.groups.io; Rodriguez, Christian > > > >Cc: Feng, Bob C ; Gao, Liming > >; Zhu, Yonghong > >Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not > mentioned > >in inf are not hashed > > > >Some questions inline. > > > >> -----Original Message----- > >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > >> Christian Rodriguez > >> Sent: Thursday, May 09, 2019 2:27 PM > >> To: devel@edk2.groups.io > >> Cc: Feng, Bob C ; Gao, Liming > >> ; Zhu, Yonghong > >> Subject: [edk2-devel] [PATCH] BaseTools: Include headers not mentione= d > >> in inf are not hashed > >> > >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1787 > >> > >> Get a list of local header files that are not present in the MetaFile > >> for this module. Add those local header files into the hashing > >> algorithm for a module. If a local header file is not present in the > >> MetaFile, the module will still build correctly though the hashing > >> system didn't know about it before. > >> > >> Signed-off-by: Christian Rodriguez > >> Cc: Bob Feng > >> Cc: Liming Gao > >> Cc: Yonghong Zhu > >> --- > >> BaseTools/Source/Python/AutoGen/AutoGen.py | 24 > >> ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py > >> b/BaseTools/Source/Python/AutoGen/AutoGen.py > >> index 31721a6f9f..bd282d3ec1 100644 > >> --- a/BaseTools/Source/Python/AutoGen/AutoGen.py > >> +++ b/BaseTools/Source/Python/AutoGen/AutoGen.py > >> @@ -4098,8 +4098,10 @@ class ModuleAutoGen(AutoGen): > >> if self.Name in GlobalData.gModuleHash[self.Arch] and > >> GlobalData.gBinCacheSource and self.AttemptModuleCacheCopy(): > >> return False > >> m =3D hashlib.md5() > >> + > >> # Add Platform level hash > >> m.update(GlobalData.gPlatformHash.encode('utf-8')) > >> + > >> # Add Package level hash > >> if self.DependentPackageList: > >> for Pkg in sorted(self.DependentPackageList, key=3Dlambd= a x: > >> x.PackageName): > >> @@ -4118,14 +4120,36 @@ class ModuleAutoGen(AutoGen): > >> Content =3D f.read() > >> f.close() > >> m.update(Content) > >> + > >> # Add Module's source files > >> + localSourceFileList =3D set() > >> if self.SourceFileList: > >> for File in sorted(self.SourceFileList, key=3Dlambda x: = str(x)): > >> + localSourceFileList.add(str(File)) > >> f =3D open(str(File), 'rb') > >> Content =3D f.read() > >> f.close() > >> m.update(Content) > >> > >> + # Get a list of Module's local header files not included in = metaFile > >> + localHeaderList =3D set() > >> + if self.SourceDir: > >> + for root, dirs, files in os.walk(self.SourceDir): > >> + for aFile in files: > >> + filePath =3D os.path.join(self.WorkspaceDir, > >> + os.path.join(root, > >> aFile)) > >> + if not filePath.endswith(('.h', '.H')): > >> + continue > >> + if filePath not in localSourceFileList: > > > >Confused about localSourceFileList. > >Why is it a set and named list? > >Why not just use self.SourceFileList? > > > The naming convention could be better and I can address that in a differ= ent > patch, if we decide to go forward with this idea overall. > It should probably be named a set. > The reason to using this new set is for a few reasons: > 1. self.SourceFileList contains source file paths of class PathClass and= not type > str > 2. If we want to use self.SourceFileList you must convert PathClass to a= str for > string comparison > The set just allows for a unique list of objects and theoretically faste= r to > check existence. I agree and really prefer the set datatype for this operation, the name is= confusing. I wonder what the ROI is for the custom PathClass sometimes. = Seems confusing often. >=20 > >> + localHeaderList.add(filePath) > >> + > >> + # Add Module's local header files > >> + localHeaderList =3D list(localHeaderList) > >> + for File in sorted(localHeaderList): > >> + f =3D open(str(File), 'rb') > >> + Content =3D f.read() > > > >Can you use 'with open(...) as...:' syntax to make sure the file is alw= ays > closed? > I just used the same implementation as the above existing code. I can > definitely change it to use 'with open(...)'. > Though explicitly calling f.close() as below should be making sure the f= ile is > always closed. Agreed, except if something raises an exception. I think we should change= all the code myself. > > > >> + f.close() > >> + m.update(Content) > >> + > >> ModuleHashFile =3D path.join(self.BuildDir, self.Name + ".ha= sh") > >> if self.Name not in GlobalData.gModuleHash[self.Arch]: > >> GlobalData.gModuleHash[self.Arch][self.Name] =3D > >> m.hexdigest() > >> -- > >> 2.19.1.windows.1 > >> > >> > >>=20