[edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed
Christian Rodriguez
christian.rodriguez at intel.com
Fri May 10 15:28:06 UTC 2019
Replies inline.
>-----Original Message-----
>From: Carsey, Jaben
>Sent: Thursday, May 9, 2019 4:39 PM
>To: devel at edk2.groups.io; Rodriguez, Christian
><christian.rodriguez at intel.com>
>Cc: Feng, Bob C <bob.c.feng at intel.com>; Gao, Liming
><liming.gao at intel.com>; Zhu, Yonghong <yonghong.zhu at intel.com>
>Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned
>in inf are not hashed
>
>Some questions inline.
>
>> -----Original Message-----
>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
>> Christian Rodriguez
>> Sent: Thursday, May 09, 2019 2:27 PM
>> To: devel at edk2.groups.io
>> Cc: Feng, Bob C <bob.c.feng at intel.com>; Gao, Liming
>> <liming.gao at intel.com>; Zhu, Yonghong <yonghong.zhu at 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.rodriguez at intel.com>
>> Cc: Bob Feng <bob.c.feng at intel.com>
>> Cc: Liming Gao <liming.gao at intel.com>
>> Cc: Yonghong Zhu <yonghong.zhu at 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.
>> + 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.
>
>> + 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 (#40443): https://edk2.groups.io/g/devel/message/40443
Mute This Topic: https://groups.io/mt/31570019/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list