Inline. tldr: good answers.
If change list to set in name of set object: Reviewed-by: jaben carsey <jaben.car...@intel.com> > -----Original Message----- > From: Rodriguez, Christian > Sent: Friday, May 10, 2019 8:28 AM > To: Carsey, Jaben <jaben.car...@intel.com>; devel@edk2.groups.io > Cc: Feng, Bob C <bob.c.f...@intel.com>; Gao, Liming > <liming....@intel.com>; Zhu, Yonghong <yonghong....@intel.com> > Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not > mentioned in inf are not hashed > Importance: High > > Replies inline. > > >-----Original Message----- > >From: Carsey, Jaben > >Sent: Thursday, May 9, 2019 4:39 PM > >To: devel@edk2.groups.io; Rodriguez, Christian > ><christian.rodrig...@intel.com> > >Cc: Feng, Bob C <bob.c.f...@intel.com>; Gao, Liming > ><liming....@intel.com>; Zhu, Yonghong <yonghong....@intel.com> > >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 <bob.c.f...@intel.com>; Gao, Liming > >> <liming....@intel.com>; Zhu, Yonghong <yonghong....@intel.com> > >> Subject: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned > >> in inf are not hashed > >> > >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1787 > >> > >> 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 <christian.rodrig...@intel.com> > >> Cc: Bob Feng <bob.c.f...@intel.com> > >> Cc: Liming Gao <liming....@intel.com> > >> Cc: Yonghong Zhu <yonghong....@intel.com> > >> --- > >> 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 = 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=lambda x: > >> x.PackageName): > >> @@ -4118,14 +4120,36 @@ class ModuleAutoGen(AutoGen): > >> Content = f.read() > >> f.close() > >> m.update(Content) > >> + > >> # Add Module's source files > >> + localSourceFileList = set() > >> if self.SourceFileList: > >> for File in sorted(self.SourceFileList, key=lambda x: str(x)): > >> + localSourceFileList.add(str(File)) > >> f = open(str(File), 'rb') > >> Content = f.read() > >> f.close() > >> m.update(Content) > >> > >> + # Get a list of Module's local header files not included in > >> metaFile > >> + localHeaderList = set() > >> + if self.SourceDir: > >> + for root, dirs, files in os.walk(self.SourceDir): > >> + for aFile in files: > >> + filePath = 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 different > 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 faster 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. > > >> + localHeaderList.add(filePath) > >> + > >> + # Add Module's local header files > >> + localHeaderList = list(localHeaderList) > >> + for File in sorted(localHeaderList): > >> + f = open(str(File), 'rb') > >> + Content = f.read() > > > >Can you use 'with open(...) as...:' syntax to make sure the file is always > 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 file 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 = path.join(self.BuildDir, self.Name + ".hash") > >> if self.Name not in GlobalData.gModuleHash[self.Arch]: > >> GlobalData.gModuleHash[self.Arch][self.Name] = > >> m.hexdigest() > >> -- > >> 2.19.1.windows.1 > >> > >> > >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40448): https://edk2.groups.io/g/devel/message/40448 Mute This Topic: https://groups.io/mt/31570019/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-