Re: [edk2-devel] [PATCH 0/4] Arm, ArmPlatform, Crypto, Embedded: list internal headers in [Sources]

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

On 07/23/19 15:25, Leif Lindholm wrote:
> On Tue, Jul 23, 2019 at 01:02:42PM +0000, Gao, Liming wrote:
>>>>> I am just not pleased with the issue
>>>>> bringing this to the fore is caused by the new caching feature using a
>>>>> different mechanism for tracking header file dependencies than the
>>>>> primary build process.
>>>>
>>>> Ugh... that's a lot of statements compressed into a single sentence. Can
>>>> you please break it down for me? (Yes, I remember the mailing list
>>>> reference you posted earlier, that discussion was too divergent for me.)
>>>
>>> The inclusion of .h files in .inf is not necessary for determining
>>> build-time dependencies on the Makefile level.
>>
>> Right. In fact, build tool will parse source file to find their dependent header files, 
>> and list those header files as the dependency in Makefile. 
> 
> If Visual Studio does not have functionality similar to the GCC
> family's -M flag, then yeah, I can see why the tool needs to take care
> of this itself. Although it would be good if we could find a way to
> not fully maintain our own code for this.
> 
>>> Thus, the warnings come out of a different and unrelated level of the
>>> build system, related to the recent build cache features. Which means
>>> we're checking header file build dependencies through two different
>>> mechanisms at two different points of the build.
>>
>> This is related to build feature --hash. When --hash option is enabled, build will calculate 
>> the hash value of source files specified in INF file. If hash value is not changed, build tool 
>> will not parse source files, and not regenerate Makefile again. So, --hash option 
>> is the incremental way in the build parse phase. If some header files are not 
>> specified in INF file, it will cause hash value inaccurate.
> 
> Right - so we actually have two levels of dependency scanning in the
> build tool itself? One for the .inf file contents, and one for the
> source file contents.
> 
>> Then, Makefile may not 
>> be generated to match the real source code, and cause the incremental build failure.
>> So, the missing header files in INF file may bring the incremental build issue when --hash option. 
>> If you don't enable --hash option, there is no problem even if the missing header files exist.
> 
> Right, but we still see the warning messages without using --hash.
> 
> Thank you very much for the summary - it clarifies the situation greatly.

+1

> Would it be feasible to update the --hash functionality to make use of
> the include dependencies extracted from the source files? (Clearly, we
> know when the source files change, so we would also know when we would
> need to re-run the dependency search.)
> 
> If not, I think we should make the explicit listing of .h files
> in .inf mandatory, triggering a build failure when not the case.

I believe I made the exact same request / suggestion, when Christian
initially posted the patches. The counter-argument was (back then
anyway) that this would break a plethora of platforms. So the idea was
to annoy people with warnings just enough to clean up those INF files,
and then turn the warnings into errors.

If I remember correctly, at least.

Given that the (partly generated) OpenSSL INF files still produce
warnings, I assume that this cleanup is likely incomplete still, in
other (out of tree) platforms as well.

> If it is, then I think we should make it explicitly banned to list .h
> files in .inf. (If there is no other dependency, such as doxygen, also
> making use of .inf listings of .h files.)

This looks a bit too recursive for me:
- rely on hashes saved from a previous run to avoid parsing the #include
  directives,
- use extracted #include dependencies to determine what files to hash.

The possibility of a stale cache concerns me. (*Incremental* dependency
extraction with gcc's -MMD concerns me the same, BTW.)

> And this may well be a topic requiring much longer discussion. While
> undecided, I would definitely prefer if we could disable the warning
> when not actually building with --hash.

That makes sense.

Thanks
Laszlo