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

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

[Laszlo Ersek]

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

[PATCH 1/4] ArmPkg: list module-internal header files in INF [Sources]

[Laszlo Ersek]

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.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                              | 1 +
 ArmPkg/Library/ArmLib/ArmBaseLib.inf                             | 3 +++
 ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
 3 files changed, 5 insertions(+)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
index a1d027d84ad1..cd2cec248b5e 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
@@ -17,6 +17,7 @@ [Defines]
   ENTRY_POINT                    = InterruptDxeInitialize
 
 [Sources.common]
+  ArmGicDxe.h
   ArmGicDxe.c
   ArmGicCommonDxe.c
 
diff --git a/ArmPkg/Library/ArmLib/ArmBaseLib.inf b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
index 669155688010..5e70990872f2 100644
--- a/ArmPkg/Library/ArmLib/ArmBaseLib.inf
+++ b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
@@ -18,9 +18,11 @@ [Defines]
   LIBRARY_CLASS                  = ArmLib
 
 [Sources]
+  ArmLibPrivate.h
   ArmLib.c
 
 [Sources.ARM]
+  Arm/ArmV7Lib.h
   Arm/ArmV7Lib.c
 
   Arm/ArmLibSupport.S           | GCC
@@ -34,6 +36,7 @@ [Sources.ARM]
   Arm/ArmV7ArchTimerSupport.asm | RVCT
 
 [Sources.AARCH64]
+  AArch64/AArch64Lib.h
   AArch64/AArch64Lib.c
 
   AArch64/ArmLibSupport.S
diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 7724231ae96c..87c9b1150c54 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -26,6 +26,7 @@ [Defines]
 
 [Sources]
   PlatformBm.c
+  PlatformBm.h
 
 [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
-- 
2.19.1.3.g30247aa5d201

[PATCH 2/4] ArmPlatformPkg: list module-internal header files in INF [Sources]

[Laszlo Ersek]

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.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf                     | 1 +
 ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 1 +
 ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf                         | 1 +
 ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf                        | 1 +
 4 files changed, 4 insertions(+)

diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index f17a9301a4ac..a647c016878d 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -17,6 +17,7 @@ [Defines]
   ENTRY_POINT                    = NorFlashInitialise
 
 [Sources.common]
+  NorFlashDxe.h
   NorFlashDxe.c
   NorFlashFvbDxe.c
   NorFlashBlockIoDxe.c
diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
index 4c62311f5d41..8224617f20ab 100644
--- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
+++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
@@ -17,6 +17,7 @@ [Defines]
   LIBRARY_CLASS                  = RealTimeClockLib|DXE_RUNTIME_DRIVER
 
 [Sources.common]
+  PL031RealTimeClock.h
   PL031RealTimeClockLib.c
 
 [Packages]
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
index 0e112710dcfc..f2ac45d171bc 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
@@ -16,6 +16,7 @@ [Defines]
 
 [Sources.common]
   MainMPCore.c
+  PrePeiCore.h
   PrePeiCore.c
 
 [Sources.ARM]
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
index c163a818c407..84c319c3679b 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
@@ -15,6 +15,7 @@ [Defines]
   VERSION_STRING                 = 1.0
 
 [Sources.common]
+  PrePeiCore.h
   PrePeiCore.c
   MainUniCore.c
 
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH 2/4] ArmPlatformPkg: list module-internal header files in INF [Sources]

[Philippe Mathieu-Daudé]

On 7/19/19 6:43 PM, Laszlo Ersek wrote:
> 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.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> ---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf                     | 1 +
>  ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 1 +
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf                         | 1 +
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf                        | 1 +
>  4 files changed, 4 insertions(+)
> 
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index f17a9301a4ac..a647c016878d 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -17,6 +17,7 @@ [Defines]
>    ENTRY_POINT                    = NorFlashInitialise
>  
>  [Sources.common]
> +  NorFlashDxe.h
>    NorFlashDxe.c
>    NorFlashFvbDxe.c
>    NorFlashBlockIoDxe.c
> diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
> index 4c62311f5d41..8224617f20ab 100644
> --- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
> +++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
> @@ -17,6 +17,7 @@ [Defines]
>    LIBRARY_CLASS                  = RealTimeClockLib|DXE_RUNTIME_DRIVER
>  
>  [Sources.common]
> +  PL031RealTimeClock.h
>    PL031RealTimeClockLib.c
>  
>  [Packages]
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> index 0e112710dcfc..f2ac45d171bc 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> @@ -16,6 +16,7 @@ [Defines]
>  
>  [Sources.common]
>    MainMPCore.c
> +  PrePeiCore.h
>    PrePeiCore.c
>  
>  [Sources.ARM]
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> index c163a818c407..84c319c3679b 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> @@ -15,6 +15,7 @@ [Defines]
>    VERSION_STRING                 = 1.0
>  
>  [Sources.common]
> +  PrePeiCore.h
>    PrePeiCore.c
>    MainUniCore.c
>  
> 

[PATCH 3/4] CryptoPkg/BaseCryptLib: list module-internal header files in INF [Sources]

[Laszlo Ersek]

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.

Cc: Jian Wang <jian.j.wang@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf     | 1 +
 CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
 CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf     | 1 +
 3 files changed, 3 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index 4c4353747622..99dbad23ed5d 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -34,6 +34,7 @@ [Defines]
 #
 
 [Sources]
+  InternalCryptLib.h
   Hash/CryptMd4Null.c
   Hash/CryptMd5.c
   Hash/CryptSha1.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index a59079d99e05..0e58d2b5b0ea 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -33,6 +33,7 @@ [Defines]
 #
 
 [Sources]
+  InternalCryptLib.h
   Hash/CryptMd4Null.c
   Hash/CryptMd5.c
   Hash/CryptSha1.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 3fd7d65abfca..c79f2bf4c6c0 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -33,6 +33,7 @@ [Defines]
 #
 
 [Sources]
+  InternalCryptLib.h
   Hash/CryptMd4Null.c
   Hash/CryptMd5.c
   Hash/CryptSha1.c
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH 3/4] CryptoPkg/BaseCryptLib: list module-internal header files in INF [Sources]

[Philippe Mathieu-Daudé]

On 7/19/19 6:43 PM, Laszlo Ersek wrote:
> 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.
> 
> Cc: Jian Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> ---
>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf     | 1 +
>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf     | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> index 4c4353747622..99dbad23ed5d 100644
> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> @@ -34,6 +34,7 @@ [Defines]
>  #
>  
>  [Sources]
> +  InternalCryptLib.h
>    Hash/CryptMd4Null.c
>    Hash/CryptMd5.c
>    Hash/CryptSha1.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> index a59079d99e05..0e58d2b5b0ea 100644
> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> @@ -33,6 +33,7 @@ [Defines]
>  #
>  
>  [Sources]
> +  InternalCryptLib.h
>    Hash/CryptMd4Null.c
>    Hash/CryptMd5.c
>    Hash/CryptSha1.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> index 3fd7d65abfca..c79f2bf4c6c0 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> @@ -33,6 +33,7 @@ [Defines]
>  #
>  
>  [Sources]
> +  InternalCryptLib.h
>    Hash/CryptMd4Null.c
>    Hash/CryptMd5.c
>    Hash/CryptSha1.c
> 

Re: [PATCH 3/4] CryptoPkg/BaseCryptLib: list module-internal header files in INF [Sources]

[Wang, Jian J]

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, July 20, 2019 12:43 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: [PATCH 3/4] CryptoPkg/BaseCryptLib: list module-internal header
> files in INF [Sources]
> 
> 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.
> 
> Cc: Jian Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf     | 1 +
>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf     | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> index 4c4353747622..99dbad23ed5d 100644
> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> @@ -34,6 +34,7 @@ [Defines]
>  #
> 
>  [Sources]
> +  InternalCryptLib.h
>    Hash/CryptMd4Null.c
>    Hash/CryptMd5.c
>    Hash/CryptSha1.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> index a59079d99e05..0e58d2b5b0ea 100644
> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> @@ -33,6 +33,7 @@ [Defines]
>  #
> 
>  [Sources]
> +  InternalCryptLib.h
>    Hash/CryptMd4Null.c
>    Hash/CryptMd5.c
>    Hash/CryptSha1.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> index 3fd7d65abfca..c79f2bf4c6c0 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> @@ -33,6 +33,7 @@ [Defines]
>  #
> 
>  [Sources]
> +  InternalCryptLib.h
>    Hash/CryptMd4Null.c
>    Hash/CryptMd5.c
>    Hash/CryptSha1.c
> --
> 2.19.1.3.g30247aa5d201
> 

[PATCH 4/4] EmbeddedPkg: list module-internal header files in INF [Sources]

[Laszlo Ersek]

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.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 EmbeddedPkg/Library/FdtLib/FdtLib.inf     | 1 +
 EmbeddedPkg/Library/PrePiLib/PrePiLib.inf | 1 +
 2 files changed, 2 insertions(+)

diff --git a/EmbeddedPkg/Library/FdtLib/FdtLib.inf b/EmbeddedPkg/Library/FdtLib/FdtLib.inf
index f5cd7591e24e..354c1709c953 100644
--- a/EmbeddedPkg/Library/FdtLib/FdtLib.inf
+++ b/EmbeddedPkg/Library/FdtLib/FdtLib.inf
@@ -20,6 +20,7 @@ [Defines]
 #
 
 [Sources]
+  libfdt_internal.h
   fdt_empty_tree.c
   fdt_overlay.c
   fdt_ro.c
diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
index 83a0edcb8d94..3c749ca22943 100644
--- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
+++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
@@ -27,6 +27,7 @@ [Defines]
 #
 
 [Sources.common]
+  PrePi.h
   FwVol.c
   PrePiLib.c
 
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH 4/4] EmbeddedPkg: list module-internal header files in INF [Sources]

[Philippe Mathieu-Daudé]

On 7/19/19 6:43 PM, Laszlo Ersek wrote:
> 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.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> ---
>  EmbeddedPkg/Library/FdtLib/FdtLib.inf     | 1 +
>  EmbeddedPkg/Library/PrePiLib/PrePiLib.inf | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/EmbeddedPkg/Library/FdtLib/FdtLib.inf b/EmbeddedPkg/Library/FdtLib/FdtLib.inf
> index f5cd7591e24e..354c1709c953 100644
> --- a/EmbeddedPkg/Library/FdtLib/FdtLib.inf
> +++ b/EmbeddedPkg/Library/FdtLib/FdtLib.inf
> @@ -20,6 +20,7 @@ [Defines]
>  #
>  
>  [Sources]
> +  libfdt_internal.h
>    fdt_empty_tree.c
>    fdt_overlay.c
>    fdt_ro.c
> diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> index 83a0edcb8d94..3c749ca22943 100644
> --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> @@ -27,6 +27,7 @@ [Defines]
>  #
>  
>  [Sources.common]
> +  PrePi.h
>    FwVol.c
>    PrePiLib.c
>  
> 

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

[Leif Lindholm]

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

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

[Laszlo Ersek]

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

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

[Michael D Kinney]

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

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

[Laszlo Ersek]

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

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

[Leif Lindholm]

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

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

[Laszlo Ersek]

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

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

[Leif Lindholm]

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

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

[Liming Gao]

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

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

[Leif Lindholm]

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

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

[Laszlo Ersek]

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

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

[Liming Gao]

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

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

[Leif Lindholm]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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