Repo:   https://github.com/lersek/edk2.git
Branch: internal_hdrs

The BaseTools build feature introduced for TianoCore#1804 / in commit
1fa6699e6cd4 ("BaseTools: Add a checking for Sources section in INF
file", 2019-06-10) logs some (non-fatal) warnings about unlisted
internal header files. List those files explicitly.

Note: header files are added in lexicographical order only if the
underlying INF file already keeps the [Sources] and [LibraryClasses]
sections in lexicographical order. Otherwise, header files are added in
rough "logical" order.

The set of INF files to update was determined as follows:

- build the OVMF and ArmVirt platforms

- collect all the warnings

- exclude all warnings that refer to "CryptoPkg/Library/OpensslLib"
  (parts of those INF files are generated, so it's likely that the
  generator should be updated -- but that's for someone else)

- for each remaining INF file, check if there are *other* INF files in
  the same directory (possibly unused by OVMF and ArmVirt platforms)

- fix up each collected INF file as needed (check if at least one C file
  listed in [Sources] actually includes the reported header)

The series was validated as follows:

- build the OVMF and ArmVirt platforms

- rebuild each modified INF with the "-m" switch, via its own package
  platform DSC (not via OVMF / ArmVirt)

- wherever [Sources] is split between arches, rebuild the INF for all
  relevant arches

- ascertain the warnings are gone

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jian Wang <jian.j.wang@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ting Ye <ting.ye@intel.com>

Thanks
Laszlo

Laszlo Ersek (4):
  ArmPkg: list module-internal header files in INF [Sources]
  ArmPlatformPkg: list module-internal header files in INF [Sources]
  CryptoPkg/BaseCryptLib: list module-internal header files in INF
    [Sources]
  EmbeddedPkg: list module-internal header files in INF [Sources]

 ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                                    | 1 +
 ArmPkg/Library/ArmLib/ArmBaseLib.inf                                   | 3 +++
 ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf       | 1 +
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf                     | 1 +
 ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 1 +
 ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf                         | 1 +
 ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf                        | 1 +
 CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf                         | 1 +
 CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf                     | 1 +
 CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf                         | 1 +
 EmbeddedPkg/Library/FdtLib/FdtLib.inf                                  | 1 +
 EmbeddedPkg/Library/PrePiLib/PrePiLib.inf                              | 1 +
 12 files changed, 14 insertions(+)

-- 
2.19.1.3.g30247aa5d201


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44056): https://edk2.groups.io/g/devel/message/44056
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/19/19 18:43, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: internal_hdrs
> 
> The BaseTools build feature introduced for TianoCore#1804 / in commit
> 1fa6699e6cd4 ("BaseTools: Add a checking for Sources section in INF
> file", 2019-06-10) logs some (non-fatal) warnings about unlisted
> internal header files. List those files explicitly.
> 
> Note: header files are added in lexicographical order only if the
> underlying INF file already keeps the [Sources] and [LibraryClasses]
> sections in lexicographical order. Otherwise, header files are added in
> rough "logical" order.
> 
> The set of INF files to update was determined as follows:
> 
> - build the OVMF and ArmVirt platforms
> 
> - collect all the warnings
> 
> - exclude all warnings that refer to "CryptoPkg/Library/OpensslLib"
>   (parts of those INF files are generated, so it's likely that the
>   generator should be updated -- but that's for someone else)
> 
> - for each remaining INF file, check if there are *other* INF files in
>   the same directory (possibly unused by OVMF and ArmVirt platforms)
> 
> - fix up each collected INF file as needed (check if at least one C file
>   listed in [Sources] actually includes the reported header)
> 
> The series was validated as follows:
> 
> - build the OVMF and ArmVirt platforms
> 
> - rebuild each modified INF with the "-m" switch, via its own package
>   platform DSC (not via OVMF / ArmVirt)
> 
> - wherever [Sources] is split between arches, rebuild the INF for all
>   relevant arches
> 
> - ascertain the warnings are gone
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jian Wang <jian.j.wang@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ting Ye <ting.ye@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   ArmPkg: list module-internal header files in INF [Sources]
>   ArmPlatformPkg: list module-internal header files in INF [Sources]
>   CryptoPkg/BaseCryptLib: list module-internal header files in INF
>     [Sources]
>   EmbeddedPkg: list module-internal header files in INF [Sources]
> 
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                                    | 1 +
>  ArmPkg/Library/ArmLib/ArmBaseLib.inf                                   | 3 +++
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf       | 1 +
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf                     | 1 +
>  ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 1 +
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf                         | 1 +
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf                        | 1 +
>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf                         | 1 +
>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf                     | 1 +
>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf                         | 1 +
>  EmbeddedPkg/Library/FdtLib/FdtLib.inf                                  | 1 +
>  EmbeddedPkg/Library/PrePiLib/PrePiLib.inf                              | 1 +
>  12 files changed, 14 insertions(+)
> 

Pushed: bb824f685d76..f6f1e0b7c292.

Thanks!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44161): https://edk2.groups.io/g/devel/message/44161
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Fri, Jul 19, 2019 at 06:43:15PM +0200, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: internal_hdrs
> 
> The BaseTools build feature introduced for TianoCore#1804 / in commit
> 1fa6699e6cd4 ("BaseTools: Add a checking for Sources section in INF
> file", 2019-06-10) logs some (non-fatal) warnings about unlisted
> internal header files. List those files explicitly.

Urgh.
Yeah. I'm still not super comfortable with this duplication of
dependency scanning (as discussed in
https://edk2.groups.io/g/devel/topic/31866190), but I have to confess
I also don't really care enough to do anything about it.

So, while I'm tempted to keep the warnings around as a reminder, if
you prefer to get rid of them - for the pat of the series I was cc:d on:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44135): https://edk2.groups.io/g/devel/message/44135
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/22/19 12:37, Leif Lindholm wrote:
> On Fri, Jul 19, 2019 at 06:43:15PM +0200, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: internal_hdrs
>>
>> The BaseTools build feature introduced for TianoCore#1804 / in commit
>> 1fa6699e6cd4 ("BaseTools: Add a checking for Sources section in INF
>> file", 2019-06-10) logs some (non-fatal) warnings about unlisted
>> internal header files. List those files explicitly.
> 
> Urgh.
> Yeah. I'm still not super comfortable with this duplication of
> dependency scanning (as discussed in
> https://edk2.groups.io/g/devel/topic/31866190), but I have to confess
> I also don't really care enough to do anything about it.
> 
> So, while I'm tempted to keep the warnings around as a reminder, if
> you prefer to get rid of them - for the pat of the series I was cc:d on:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Thanks!

Yes, the warnings are an annoyance, and they are valid too. How the INF
files are caught / reported is a separate question IMO.

Thanks!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44145): https://edk2.groups.io/g/devel/message/44145
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

We could consider checking for these type of issues in
the ECC tool instead of build and make it an error from
ECC instead of a warning.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Monday, July 22, 2019 10:33 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Wang, Jian J
> <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/4] Arm, ArmPlatform,
> Crypto, Embedded: list internal headers in [Sources]
> 
> On 07/22/19 12:37, Leif Lindholm wrote:
> > On Fri, Jul 19, 2019 at 06:43:15PM +0200, Laszlo Ersek
> wrote:
> >> Repo:   https://github.com/lersek/edk2.git
> >> Branch: internal_hdrs
> >>
> >> The BaseTools build feature introduced for
> TianoCore#1804 / in commit
> >> 1fa6699e6cd4 ("BaseTools: Add a checking for Sources
> section in INF
> >> file", 2019-06-10) logs some (non-fatal) warnings
> about unlisted
> >> internal header files. List those files explicitly.
> >
> > Urgh.
> > Yeah. I'm still not super comfortable with this
> duplication of
> > dependency scanning (as discussed in
> > https://edk2.groups.io/g/devel/topic/31866190), but I
> have to confess
> > I also don't really care enough to do anything about
> it.
> >
> > So, while I'm tempted to keep the warnings around as a
> reminder, if
> > you prefer to get rid of them - for the pat of the
> series I was cc:d on:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Thanks!
> 
> Yes, the warnings are an annoyance, and they are valid
> too. How the INF files are caught / reported is a
> separate question IMO.
> 
> Thanks!
> Laszlo
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44146): https://edk2.groups.io/g/devel/message/44146
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Mike,

On 07/22/19 20:47, Michael D Kinney wrote:
> We could consider checking for these type of issues in
> the ECC tool instead of build and make it an error from
> ECC instead of a warning.

I'm sorry, my reply to Leif was ambiguous (or worse).

I meant that the issues underlying the specific warnings (emitted by the
feature from TianoCore#1804) were annoying -- the reports were valid,
and what "annoyed" me was that the INF files had not been in order (i.e.
that they had missed some internal header files).

I wasn't annoyed at the feature itself -- if it helps developers catch
unlisted headers as soon as incomplete INF files are introduced, then
it's not a bad feature IMO.

I'd make a *light* argument for keeping the feature in "build", as
opposed to ECC:

- ECC performs a very deep investigation (and used to produce a number
of false positives / noise).

- Since commit c60377d7f9ec ("BaseTools: ECC tool Python3 adaption",
2019-02-01), ECC has been incompatible with Python2 (and requires
antlr4, rather than antlr3). Thus, "build" works on more systems than ECC.

- I think "unlisted internal headers in INF files" is a problem defined
at the level of "build".

That said, I don't feel strongly about these general questions; I just
wanted to fix the actual warnings, because they were valid.

Thanks
Laszlo

>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Monday, July 22, 2019 10:33 AM
>> To: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>; Wang, Jian J
>> <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 0/4] Arm, ArmPlatform,
>> Crypto, Embedded: list internal headers in [Sources]
>>
>> On 07/22/19 12:37, Leif Lindholm wrote:
>>> On Fri, Jul 19, 2019 at 06:43:15PM +0200, Laszlo Ersek
>> wrote:
>>>> Repo:   https://github.com/lersek/edk2.git
>>>> Branch: internal_hdrs
>>>>
>>>> The BaseTools build feature introduced for
>> TianoCore#1804 / in commit
>>>> 1fa6699e6cd4 ("BaseTools: Add a checking for Sources
>> section in INF
>>>> file", 2019-06-10) logs some (non-fatal) warnings
>> about unlisted
>>>> internal header files. List those files explicitly.
>>>
>>> Urgh.
>>> Yeah. I'm still not super comfortable with this
>> duplication of
>>> dependency scanning (as discussed in
>>> https://edk2.groups.io/g/devel/topic/31866190), but I
>> have to confess
>>> I also don't really care enough to do anything about
>> it.
>>>
>>> So, while I'm tempted to keep the warnings around as a
>> reminder, if
>>> you prefer to get rid of them - for the pat of the
>> series I was cc:d on:
>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Thanks!
>>
>> Yes, the warnings are an annoyance, and they are valid
>> too. How the INF files are caught / reported is a
>> separate question IMO.
>>
>> Thanks!
>> Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44163): https://edk2.groups.io/g/devel/message/44163
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Jul 23, 2019 at 12:56:23AM +0200, Laszlo Ersek wrote:
> Hi Mike,
> 
> On 07/22/19 20:47, Michael D Kinney wrote:
> > We could consider checking for these type of issues in
> > the ECC tool instead of build and make it an error from
> > ECC instead of a warning.
> 
> I'm sorry, my reply to Leif was ambiguous (or worse).
> 
> I meant that the issues underlying the specific warnings (emitted by the
> feature from TianoCore#1804) were annoying -- the reports were valid,
> and what "annoyed" me was that the INF files had not been in order (i.e.
> that they had missed some internal header files).

Whereas I'm annoyed that we now have a manual process to match up with
the automatic dependency generation.

> I wasn't annoyed at the feature itself -- if it helps developers catch
> unlisted headers as soon as incomplete INF files are introduced, then
> it's not a bad feature IMO.

I agree that the optional nature of whether to list local .h files or
not in the .inf was suboptimal. 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.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44233): https://edk2.groups.io/g/devel/message/44233
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/23/19 11:06, Leif Lindholm wrote:
> On Tue, Jul 23, 2019 at 12:56:23AM +0200, Laszlo Ersek wrote:
>> Hi Mike,
>>
>> On 07/22/19 20:47, Michael D Kinney wrote:
>>> We could consider checking for these type of issues in
>>> the ECC tool instead of build and make it an error from
>>> ECC instead of a warning.
>>
>> I'm sorry, my reply to Leif was ambiguous (or worse).
>>
>> I meant that the issues underlying the specific warnings (emitted by the
>> feature from TianoCore#1804) were annoying -- the reports were valid,
>> and what "annoyed" me was that the INF files had not been in order (i.e.
>> that they had missed some internal header files).
> 
> Whereas I'm annoyed that we now have a manual process to match up with
> the automatic dependency generation.
> 
>> I wasn't annoyed at the feature itself -- if it helps developers catch
>> unlisted headers as soon as incomplete INF files are introduced, then
>> it's not a bad feature IMO.
> 
> I agree that the optional nature of whether to list local .h files or
> not in the .inf was suboptimal.

Hmm, has that ever been officially optional?

(The INF spec chapter at
<https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/2_inf_overview/25_[sources]_section.html>
doesn't seem to mention header files at all. Thus, I can imagine both
"mandatory to list headers" by omission, and "optional to list headers"
by omission...)

> 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.)

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44248): https://edk2.groups.io/g/devel/message/44248
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Jul 23, 2019 at 01:54:54PM +0200, Laszlo Ersek wrote:
> >> I wasn't annoyed at the feature itself -- if it helps developers catch
> >> unlisted headers as soon as incomplete INF files are introduced, then
> >> it's not a bad feature IMO.
> > 
> > I agree that the optional nature of whether to list local .h files or
> > not in the .inf was suboptimal.
> 
> Hmm, has that ever been officially optional?
> 
> (The INF spec chapter at
> <https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/2_inf_overview/25_[sources]_section.html>
> doesn't seem to mention header files at all. Thus, I can imagine both
> "mandatory to list headers" by omission, and "optional to list headers"
> by omission...)

Yeah, indeed.

> > 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.

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.

Or I have fundamentally misunderstood what is going on. Which is also
possible. In which case we're maintaining our own header file
dependency tracking infrastructure, despite us in the end relying on
generating Makefiles. Which also feels less than ideal.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44252): https://edk2.groups.io/g/devel/message/44252
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/23/19 14:19, Leif Lindholm wrote:
> On Tue, Jul 23, 2019 at 01:54:54PM +0200, Laszlo Ersek wrote:
>>>> I wasn't annoyed at the feature itself -- if it helps developers catch
>>>> unlisted headers as soon as incomplete INF files are introduced, then
>>>> it's not a bad feature IMO.
>>>
>>> I agree that the optional nature of whether to list local .h files or
>>> not in the .inf was suboptimal.
>>
>> Hmm, has that ever been officially optional?
>>
>> (The INF spec chapter at
>> <https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/2_inf_overview/25_[sources]_section.html>
>> doesn't seem to mention header files at all. Thus, I can imagine both
>> "mandatory to list headers" by omission, and "optional to list headers"
>> by omission...)
> 
> Yeah, indeed.
> 
>>> 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.
> 
> 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.

OK, thanks. Now I understand your point. It hasn't been clear to me that
listing the module-internal headers isn't actually necessary for getting
the build dependencies right.

> Or I have fundamentally misunderstood what is going on. Which is also
> possible. In which case we're maintaining our own header file
> dependency tracking infrastructure, despite us in the end relying on
> generating Makefiles. Which also feels less than ideal.

Right.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44265): https://edk2.groups.io/g/devel/message/44265
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Leif:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
> Sent: Tuesday, July 23, 2019 8:20 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wang, Jian J
> <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/4] Arm, ArmPlatform, Crypto, Embedded: list internal headers in [Sources]
> 
> On Tue, Jul 23, 2019 at 01:54:54PM +0200, Laszlo Ersek wrote:
> > >> I wasn't annoyed at the feature itself -- if it helps developers catch
> > >> unlisted headers as soon as incomplete INF files are introduced, then
> > >> it's not a bad feature IMO.
> > >
> > > I agree that the optional nature of whether to list local .h files or
> > > not in the .inf was suboptimal.
> >
> > Hmm, has that ever been officially optional?
> >
> > (The INF spec chapter at
> > <https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/2_inf_overview/25_[sources]_section.html>
> > doesn't seem to mention header files at all. Thus, I can imagine both
> > "mandatory to list headers" by omission, and "optional to list headers"
> > by omission...)
> 
> Yeah, indeed.
> 
> > > 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. 

> 
> 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. 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.

Thanks
Liming
> 
> Or I have fundamentally misunderstood what is going on. Which is also
> possible. In which case we're maintaining our own header file
> dependency tracking infrastructure, despite us in the end relying on
> generating Makefiles. Which also feels less than ideal.
> 
> /
>     Leif
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44254): https://edk2.groups.io/g/devel/message/44254
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

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.

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.)

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.

Best Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44255): https://edk2.groups.io/g/devel/message/44255
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44266): https://edk2.groups.io/g/devel/message/44266
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Leif:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
> Sent: Tuesday, July 23, 2019 9:26 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/4] Arm, ArmPlatform, Crypto, Embedded: list internal headers in [Sources]
> 
> 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.
> 
> 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.)

The design is to save the step to extract the dependencies from the source files. 
I can further collect the build performance to be taken on the dependencies extraction
from the source files, and decide whether take this way. Another simple way is 
to calculate all source files in module directory and make sure there is no file missing for --hash option.

> 
> If not, I think we should make the explicit listing of .h files
> in .inf mandatory, triggering a build failure when not the case.
> 
> 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.)

I know edk2 also has PI Packaging UPT. PI packaging requires all source 
files are listed in module INF file. Otherwise, some source files will be missed
in the packaging, and can't be rollback.

> 
> 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.

There are more than one usage models for this case. I suggest to provide 
one option to disable warning. 

Thanks
Liming

> 
> Best Regards,
> 
> Leif
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44315): https://edk2.groups.io/g/devel/message/44315
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Jul 24, 2019 at 03:17:56PM +0000, Gao, Liming wrote:
> > 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.)
> 
> The design is to save the step to extract the dependencies from the source files. 
> I can further collect the build performance to be taken on the dependencies extraction
> from the source files, and decide whether take this way. Another simple way is 
> to calculate all source files in module directory and make sure there is no file missing for --hash option.
> 
> > 
> > If not, I think we should make the explicit listing of .h files
> > in .inf mandatory, triggering a build failure when not the case.
> > 
> > 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.)
> 
> I know edk2 also has PI Packaging UPT. PI packaging requires all source 
> files are listed in module INF file. Otherwise, some source files will be missed
> in the packaging, and can't be rollback.

OK, this means we should update the documentation to be crystal clear
that .h files need to be listed too.

I am OK to keep the warning enabled for now. But I would also wish
that we start planning for making it an error at some point in the
future.

Best Regards,

Leif


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44323): https://edk2.groups.io/g/devel/message/44323
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/24/19 19:00, Leif Lindholm wrote:
> On Wed, Jul 24, 2019 at 03:17:56PM +0000, Gao, Liming wrote:
>>> 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.)
>>
>> The design is to save the step to extract the dependencies from the source files. 
>> I can further collect the build performance to be taken on the dependencies extraction
>> from the source files, and decide whether take this way. Another simple way is 
>> to calculate all source files in module directory and make sure there is no file missing for --hash option.
>>
>>>
>>> If not, I think we should make the explicit listing of .h files
>>> in .inf mandatory, triggering a build failure when not the case.
>>>
>>> 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.)
>>
>> I know edk2 also has PI Packaging UPT. PI packaging requires all source 
>> files are listed in module INF file. Otherwise, some source files will be missed
>> in the packaging, and can't be rollback.
> 
> OK, this means we should update the documentation to be crystal clear
> that .h files need to be listed too.
> 
> I am OK to keep the warning enabled for now. But I would also wish
> that we start planning for making it an error at some point in the
> future.

Could you please file a Feature Request for that (i.e. s/warning/error/)
in the Bugzilla?

Thanks!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44415): https://edk2.groups.io/g/devel/message/44415
Mute This Topic: https://groups.io/mt/32529014/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-