Re: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed

["Christian Rodriguez" <christian.rodriguez@intel.com>]

Replies inline.

>-----Original Message-----
>From: Carsey, Jaben
>Sent: Thursday, May 9, 2019 4:39 PM
>To: devel@edk2.groups.io; Rodriguez, Christian
><christian.rodriguez@intel.com>
>Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
><liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@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.feng@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@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@intel.com>
>> Cc: Bob Feng <bob.c.feng@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Yonghong Zhu <yonghong.zhu@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
>>
>>
>>