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

["Laszlo Ersek" <lersek@redhat.com>]

Hello Christian,

On 05/09/19 23:27, Christian Rodriguez wrote:
> 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(+)

I saw the BZ soon after it was reported. I almost commented, but
ultimately I couldn't decide what the real use case was.

With this particular use case (i.e. INF file is missing some
module-specific header files that it could easily list), I think I
disagree, mildly (not too strongly). E.g., we fixed such omissions in a
bunch of INF files, last March, in the series

  [PATCH 00/45]
  ArmVirtPkg, OvmfPkg: list module-internal headers in INF files

So my thinking is that this change is neither really required (people
can simply fix INF files), nor really sufficient.

Here's why the change is not sufficient (deduced purely from the commit
message -- because the commit message clarifies what the BZ report
didn't). Assume you are developing a module (driver or library) that
consumes a new EDKII extension protocol that your series *also*
introduces, under MdeModulePkg/Include/Protocol, in an earlier,
stand-alone patch. So one of your later patches, in the driver or lib
instance, says:

#include <Protocol/EdkiiAwesomeCoolFeature.h>

As you develop the driver, and perhaps even other code that consumes the
protocol produced by the new driver, you realize you need a new member
function, or one of the member functions needs a new parameter. You
change the protocol header file -- for example, you insert a new
function pointer at the *top* of the protocol structure -- and some
consumer code now breaks in testing. Because, the consumer code's hash
was never changed, and so it was never rebuilt, against the member
function field offsets from the updated protocol structure.

In other words, BaseTools doesn't do recursive dependency tracking (with
or without hashes). It's not a huge deal, it's just why I'm saying this
patch is not enough to address the real problem. And, if we had
recursive dependency tracking (i.e. a change in the hash of the *public*
<EdkiiAwesomeCoolFeature.h>  header triggered a rebuild of all dependent
modules), then we wouldn't have to look at module-internal unlisted
header files specifically.

Now, if the present change is being implemented because it's a small
patch, in comparison to updating hundreds of INF files in out-of-tree
platforms, I'm happy with that; just please state this motivation in the
commit message (and the BZ too, if possible).

One comment below, on the code:

> 
> 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')):

".H" files should not be covered, in my opinion, as they stand for C++
header files under at least some toolchains, and C++ is not a supported
edk2 language.

... But, if there are hundreds of .H files out-of-tree, I guess I can be
convinced about this too. :)

Thanks
Laszlo

> +                        continue
> +                    if filePath not in localSourceFileList:
> +                        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()
> +            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()
>