ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 3 ++- OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 8 +++++--- OvmfPkg/AcpiPlatformDxe/Qemu.c | 1 - OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 8 +++++--- OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf | 1 + OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf | 6 +++++- OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 1 + OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 1 + OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf | 1 + OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf | 1 + OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf | 1 + OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf | 1 + OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf | 1 + OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf | 1 + OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 + OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf | 1 + OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf | 1 + OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 1 + OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf | 3 ++- OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf | 1 + OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c | 2 +- OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf | 1 + OvmfPkg/PlatformDxe/Platform.inf | 2 ++ OvmfPkg/PlatformPei/PlatformPei.inf | 3 +++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 2 ++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 2 ++ OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 7 +++++-- OvmfPkg/Virtio10Dxe/Virtio10.inf | 1 + OvmfPkg/VirtioBlkDxe/VirtioBlk.inf | 1 + OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 + OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf | 1 + OvmfPkg/VirtioRngDxe/VirtioRng.inf | 1 + OvmfPkg/VirtioScsiDxe/VirtioScsi.inf | 1 + OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf | 9 +++++---- 35 files changed, 61 insertions(+), 17 deletions(-)
Repo: https://github.com/lersek/edk2.git
Branch: hdr_inf_cleanup
In
<http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com>,
Mike explained why it's a good idea to list module-internal *.h files in
the [Sources*] sections of the INF files:
On 11/23/15 21:28, Kinney, Michael D wrote:
> There are 2 reasons to list all source files used in a module build in
> the [Sources] section.
>
> 1) Support incremental builds. If a change to the .h file is made,
> then the module may not be rebuilt if the .h file is not listed in
> [Sources]
> 2) Support of UEFI Distribution Package distribution format. The UPT
> tools that creates UDP packages uses the [Sources] section for the
> inventory of files. If a file is missing, then it will not be
> included in the UDP file.
In only two years and three-four months, I've finally come around
addressing (1) under ArmVirtPkg and OvmfPkg. The affected *.inf and *.h
files were located with the following crude script:
> #!/bin/bash
>
> export LC_ALL=C
>
> find ArmVirtPkg/ OvmfPkg/ -type f -name '*.inf' \
> | sort \
> | while read INF; do
> INF_D=$(dirname -- "$INF")
> INF_F=$(basename -- "$INF")
> (
> cd "$INF_D"
> find -type f -name '*.h' \
> | cut -c 3- \
> | sort \
> | while read REL_H; do
> if ! grep -q -F -e " $REL_H" -- "$INF_F"; then
> printf '%s: %s\n' "$INF" "$REL_H"
> fi
> done
> )
> done
This patch set brings the output down to nil, from the following 45
lines (note that the patch count equaling 45 is a coincidence):
> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf: PlatformBm.h
> ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf: PrePi.h
> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf: AcpiPlatform.h
> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf: QemuLoader.h
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf: AcpiPlatform.h
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf: QemuLoader.h
> OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf: BlockIo.h
> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: CsmSupportLib.h
> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyInterrupt.h
> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyPlatform.h
> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyRegion.h
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf: Fvb.h
> OvmfPkg/IoMmuDxe/IoMmuDxe.inf: AmdSevIoMmu.h
> OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf: AcpiTimerLib.h
> OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf: AcpiTimerLib.h
> OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf: AcpiTimerLib.h
> OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf: X64/VirtualMemory.h
> OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf: LoadLinuxLib.h
> OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf: LockBoxLib.h
> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf: LockBoxLib.h
> OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf: NvVarsFileLib.h
> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf: DebugLibDetect.h
> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf: DebugLibDetect.h
> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf: ExtraRootBusMap.h
> OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf: SerializeVariablesLib.h
> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf: VirtioMmioDevice.h
> OvmfPkg/PlatformDxe/Platform.inf: Platform.h
> OvmfPkg/PlatformDxe/Platform.inf: PlatformConfig.h
> OvmfPkg/PlatformPei/PlatformPei.inf: Cmos.h
> OvmfPkg/PlatformPei/PlatformPei.inf: Platform.h
> OvmfPkg/PlatformPei/PlatformPei.inf: Xen.h
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf: FwBlockService.h
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf: QemuFlash.h
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf: FwBlockService.h
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf: QemuFlash.h
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: Qemu.h
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: UnalignedIoInternal.h
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: VbeShim.h
> OvmfPkg/Virtio10Dxe/Virtio10.inf: Virtio10.h
> OvmfPkg/VirtioBlkDxe/VirtioBlk.inf: VirtioBlk.h
> OvmfPkg/VirtioNetDxe/VirtioNet.inf: VirtioNet.h
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf: VirtioPciDevice.h
> OvmfPkg/VirtioRngDxe/VirtioRng.inf: VirtioRng.h
> OvmfPkg/VirtioScsiDxe/VirtioScsi.inf: VirtioScsi.h
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf: DriverBinding.h
In the future, we shall ask for patches to be respun unless they (a)
keep the [Sources*] sections sorted and (b) list any new module-internal
header files there.
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Phil Dennis-Jordan <phil@philjordan.eu>
Thanks,
Laszlo
Laszlo Ersek (45):
ArmVirtPkg/PlatformBootManagerLib: list "PlatformBm.h" in INF file
ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: sort [Sources*] sections in
INF
ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: list "PrePi.h" in INF file
OvmfPkg/AcpiPlatformDxe: sort [Sources*] sections in the INF files
OvmfPkg/AcpiPlatformDxe: list "AcpiPlatform.h" in the INF files
OvmfPkg/AcpiPlatformDxe: don't #include "QemuLoader.h" in "Qemu.c"
OvmfPkg/AcpiPlatformDxe: list "QemuLoader.h" in the INF files
OvmfPkg/BlockMmioToBlockIoDxe: list "BlockIo.h" in the INF file
OvmfPkg/CsmSupportLib: sort [Sources*] sections in the INF file
OvmfPkg/CsmSupportLib: list "CsmSupportLib.h" in the INF file
OvmfPkg/CsmSupportLib: list "LegacyInterrupt.h" in the INF file
OvmfPkg/CsmSupportLib: list "LegacyPlatform.h" in the INF file
OvmfPkg/CsmSupportLib: list "LegacyRegion.h" in the INF file
OvmfPkg/EmuVariableFvbRuntimeDxe: list "Fvb.h" in the INF file
OvmfPkg/IoMmuDxe: list "AmdSevIoMmu.h" in the INF file
OvmfPkg/AcpiTimerLib: list "AcpiTimerLib.h" in the INF files
OvmfPkg/BaseMemEncryptSevLib: list "X64/VirtualMemory.h" in the INF
file
OvmfPkg/LoadLinuxLib: list "LoadLinuxLib.h" in the INF file
OvmfPkg/LockBoxLib: list "LockBoxLib.h" in the INF files
OvmfPkg/NvVarsFileLib: list "NvVarsFileLib.h" in the INF file
OvmfPkg/PlatformDebugLibIoPort: list "DebugLibDetect.h" in the INF
files
OvmfPkg/QemuBootOrderLib: sort [Sources*] sections in the INF file
OvmfPkg/QemuBootOrderLib: list "ExtraRootBusMap.h" in the INF file
OvmfPkg/SerializeVariablesLib: list "SerializeVariablesLib.h" in INF
file
OvmfPkg/VirtioMmioDeviceLib: list "VirtioMmioDevice.h" in the INF file
OvmfPkg/VirtioMmioDeviceLib: improve style of
mMmioDeviceProtocolTemplate
OvmfPkg/PlatformDxe: list "Platform.h" in the INF file
OvmfPkg/PlatformDxe: list "PlatformConfig.h" in the INF file
OvmfPkg/PlatformPei: list "Cmos.h" in the INF file
OvmfPkg/PlatformPei: list "Platform.h" in the INF file
OvmfPkg/PlatformPei: list "Xen.h" in the INF file
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: list "FwBlockService.h" in
INFs
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: list "QemuFlash.h" in INF
files
OvmfPkg/QemuVideoDxe: sort [Sources*] sections in the INF file
OvmfPkg/QemuVideoDxe: list "Qemu.h" in the INF file
OvmfPkg/QemuVideoDxe: list "UnalignedIoInternal.h" in the INF file
OvmfPkg/QemuVideoDxe: list "VbeShim.h" in the INF file
OvmfPkg/Virtio10Dxe: list "Virtio10.h" in the INF file
OvmfPkg/VirtioBlkDxe: list "VirtioBlk.h" in the INF file
OvmfPkg/VirtioNetDxe: list "VirtioNet.h" in the INF file
OvmfPkg/VirtioPciDeviceDxe: list "VirtioPciDevice.h" in the INF file
OvmfPkg/VirtioRngDxe: list "VirtioRng.h" in the INF file
OvmfPkg/VirtioScsiDxe: list "VirtioScsi.h" in the INF file
OvmfPkg/XenPvBlkDxe: sort [Sources*] sections in the INF file
OvmfPkg/XenPvBlkDxe: list "DriverBinding.h" in the INF file
ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 3 ++-
OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 8 +++++---
OvmfPkg/AcpiPlatformDxe/Qemu.c | 1 -
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 8 +++++---
OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf | 1 +
OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf | 6 +++++-
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 1 +
OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 1 +
OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf | 1 +
OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf | 1 +
OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf | 1 +
OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf | 1 +
OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf | 1 +
OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf | 1 +
OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf | 1 +
OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf | 1 +
OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 1 +
OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf | 3 ++-
OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf | 1 +
OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c | 2 +-
OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf | 1 +
OvmfPkg/PlatformDxe/Platform.inf | 2 ++
OvmfPkg/PlatformPei/PlatformPei.inf | 3 +++
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 2 ++
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 2 ++
OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 7 +++++--
OvmfPkg/Virtio10Dxe/Virtio10.inf | 1 +
OvmfPkg/VirtioBlkDxe/VirtioBlk.inf | 1 +
OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 +
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf | 1 +
OvmfPkg/VirtioRngDxe/VirtioRng.inf | 1 +
OvmfPkg/VirtioScsiDxe/VirtioScsi.inf | 1 +
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf | 9 +++++----
35 files changed, 61 insertions(+), 17 deletions(-)
--
2.14.1.3.gb7cf6e02401b
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 03/11/18 02:48, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: hdr_inf_cleanup > > In > <http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com>, > Mike explained why it's a good idea to list module-internal *.h files in > the [Sources*] sections of the INF files: > > On 11/23/15 21:28, Kinney, Michael D wrote: >> There are 2 reasons to list all source files used in a module build in >> the [Sources] section. >> >> 1) Support incremental builds. If a change to the .h file is made, >> then the module may not be rebuilt if the .h file is not listed in >> [Sources] >> 2) Support of UEFI Distribution Package distribution format. The UPT >> tools that creates UDP packages uses the [Sources] section for the >> inventory of files. If a file is missing, then it will not be >> included in the UDP file. Commit range d154a4dfaec4..ea30f8e81399. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Laszlo, On 11 March 2018 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote: > Repo: https://github.com/lersek/edk2.git > Branch: hdr_inf_cleanup > > In > <http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com>, > Mike explained why it's a good idea to list module-internal *.h files in > the [Sources*] sections of the INF files: > > On 11/23/15 21:28, Kinney, Michael D wrote: >> There are 2 reasons to list all source files used in a module build in >> the [Sources] section. >> >> 1) Support incremental builds. If a change to the .h file is made, >> then the module may not be rebuilt if the .h file is not listed in >> [Sources] >> 2) Support of UEFI Distribution Package distribution format. The UPT >> tools that creates UDP packages uses the [Sources] section for the >> inventory of files. If a file is missing, then it will not be >> included in the UDP file. > > In only two years and three-four months, I've finally come around > addressing (1) under ArmVirtPkg and OvmfPkg. Thanks for doing this. However, while I highly appreciate your thoroughness and verbosity in most cases, I do think you've crossed a line this time :-) Do we *really* need 4 different patches for CsmSupportLib, each adding a single .h file to [Sources], with an elaborate description how it is being used? If it is used, it needs to be listed, and if it is not, it needs to be removed, that's all there is to it IMO. Apart from that, the series looks correct to me. Thanks, Ard. > The affected *.inf and *.h > files were located with the following crude script: > >> #!/bin/bash >> >> export LC_ALL=C >> >> find ArmVirtPkg/ OvmfPkg/ -type f -name '*.inf' \ >> | sort \ >> | while read INF; do >> INF_D=$(dirname -- "$INF") >> INF_F=$(basename -- "$INF") >> ( >> cd "$INF_D" >> find -type f -name '*.h' \ >> | cut -c 3- \ >> | sort \ >> | while read REL_H; do >> if ! grep -q -F -e " $REL_H" -- "$INF_F"; then >> printf '%s: %s\n' "$INF" "$REL_H" >> fi >> done >> ) >> done > > This patch set brings the output down to nil, from the following 45 > lines (note that the patch count equaling 45 is a coincidence): > >> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf: PlatformBm.h >> ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf: PrePi.h >> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf: AcpiPlatform.h >> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf: QemuLoader.h >> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf: AcpiPlatform.h >> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf: QemuLoader.h >> OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf: BlockIo.h >> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: CsmSupportLib.h >> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyInterrupt.h >> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyPlatform.h >> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyRegion.h >> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf: Fvb.h >> OvmfPkg/IoMmuDxe/IoMmuDxe.inf: AmdSevIoMmu.h >> OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf: AcpiTimerLib.h >> OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf: AcpiTimerLib.h >> OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf: AcpiTimerLib.h >> OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf: X64/VirtualMemory.h >> OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf: LoadLinuxLib.h >> OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf: LockBoxLib.h >> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf: LockBoxLib.h >> OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf: NvVarsFileLib.h >> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf: DebugLibDetect.h >> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf: DebugLibDetect.h >> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf: ExtraRootBusMap.h >> OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf: SerializeVariablesLib.h >> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf: VirtioMmioDevice.h >> OvmfPkg/PlatformDxe/Platform.inf: Platform.h >> OvmfPkg/PlatformDxe/Platform.inf: PlatformConfig.h >> OvmfPkg/PlatformPei/PlatformPei.inf: Cmos.h >> OvmfPkg/PlatformPei/PlatformPei.inf: Platform.h >> OvmfPkg/PlatformPei/PlatformPei.inf: Xen.h >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf: FwBlockService.h >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf: QemuFlash.h >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf: FwBlockService.h >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf: QemuFlash.h >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: Qemu.h >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: UnalignedIoInternal.h >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: VbeShim.h >> OvmfPkg/Virtio10Dxe/Virtio10.inf: Virtio10.h >> OvmfPkg/VirtioBlkDxe/VirtioBlk.inf: VirtioBlk.h >> OvmfPkg/VirtioNetDxe/VirtioNet.inf: VirtioNet.h >> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf: VirtioPciDevice.h >> OvmfPkg/VirtioRngDxe/VirtioRng.inf: VirtioRng.h >> OvmfPkg/VirtioScsiDxe/VirtioScsi.inf: VirtioScsi.h >> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf: DriverBinding.h > > In the future, we shall ask for patches to be respun unless they (a) > keep the [Sources*] sections sorted and (b) list any new module-internal > header files there. > > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Brijesh Singh <brijesh.singh@amd.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Julien Grall <julien.grall@linaro.org> > Cc: Phil Dennis-Jordan <phil@philjordan.eu> > > Thanks, > Laszlo > > Laszlo Ersek (45): > ArmVirtPkg/PlatformBootManagerLib: list "PlatformBm.h" in INF file > ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: sort [Sources*] sections in > INF > ArmVirtPkg/ArmVirtPrePiUniCoreRelocatable: list "PrePi.h" in INF file > OvmfPkg/AcpiPlatformDxe: sort [Sources*] sections in the INF files > OvmfPkg/AcpiPlatformDxe: list "AcpiPlatform.h" in the INF files > OvmfPkg/AcpiPlatformDxe: don't #include "QemuLoader.h" in "Qemu.c" > OvmfPkg/AcpiPlatformDxe: list "QemuLoader.h" in the INF files > OvmfPkg/BlockMmioToBlockIoDxe: list "BlockIo.h" in the INF file > OvmfPkg/CsmSupportLib: sort [Sources*] sections in the INF file > OvmfPkg/CsmSupportLib: list "CsmSupportLib.h" in the INF file > OvmfPkg/CsmSupportLib: list "LegacyInterrupt.h" in the INF file > OvmfPkg/CsmSupportLib: list "LegacyPlatform.h" in the INF file > OvmfPkg/CsmSupportLib: list "LegacyRegion.h" in the INF file > OvmfPkg/EmuVariableFvbRuntimeDxe: list "Fvb.h" in the INF file > OvmfPkg/IoMmuDxe: list "AmdSevIoMmu.h" in the INF file > OvmfPkg/AcpiTimerLib: list "AcpiTimerLib.h" in the INF files > OvmfPkg/BaseMemEncryptSevLib: list "X64/VirtualMemory.h" in the INF > file > OvmfPkg/LoadLinuxLib: list "LoadLinuxLib.h" in the INF file > OvmfPkg/LockBoxLib: list "LockBoxLib.h" in the INF files > OvmfPkg/NvVarsFileLib: list "NvVarsFileLib.h" in the INF file > OvmfPkg/PlatformDebugLibIoPort: list "DebugLibDetect.h" in the INF > files > OvmfPkg/QemuBootOrderLib: sort [Sources*] sections in the INF file > OvmfPkg/QemuBootOrderLib: list "ExtraRootBusMap.h" in the INF file > OvmfPkg/SerializeVariablesLib: list "SerializeVariablesLib.h" in INF > file > OvmfPkg/VirtioMmioDeviceLib: list "VirtioMmioDevice.h" in the INF file > OvmfPkg/VirtioMmioDeviceLib: improve style of > mMmioDeviceProtocolTemplate > OvmfPkg/PlatformDxe: list "Platform.h" in the INF file > OvmfPkg/PlatformDxe: list "PlatformConfig.h" in the INF file > OvmfPkg/PlatformPei: list "Cmos.h" in the INF file > OvmfPkg/PlatformPei: list "Platform.h" in the INF file > OvmfPkg/PlatformPei: list "Xen.h" in the INF file > OvmfPkg/QemuFlashFvbServicesRuntimeDxe: list "FwBlockService.h" in > INFs > OvmfPkg/QemuFlashFvbServicesRuntimeDxe: list "QemuFlash.h" in INF > files > OvmfPkg/QemuVideoDxe: sort [Sources*] sections in the INF file > OvmfPkg/QemuVideoDxe: list "Qemu.h" in the INF file > OvmfPkg/QemuVideoDxe: list "UnalignedIoInternal.h" in the INF file > OvmfPkg/QemuVideoDxe: list "VbeShim.h" in the INF file > OvmfPkg/Virtio10Dxe: list "Virtio10.h" in the INF file > OvmfPkg/VirtioBlkDxe: list "VirtioBlk.h" in the INF file > OvmfPkg/VirtioNetDxe: list "VirtioNet.h" in the INF file > OvmfPkg/VirtioPciDeviceDxe: list "VirtioPciDevice.h" in the INF file > OvmfPkg/VirtioRngDxe: list "VirtioRng.h" in the INF file > OvmfPkg/VirtioScsiDxe: list "VirtioScsi.h" in the INF file > OvmfPkg/XenPvBlkDxe: sort [Sources*] sections in the INF file > OvmfPkg/XenPvBlkDxe: list "DriverBinding.h" in the INF file > > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + > ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 3 ++- > OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 8 +++++--- > OvmfPkg/AcpiPlatformDxe/Qemu.c | 1 - > OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 8 +++++--- > OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf | 1 + > OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf | 6 +++++- > OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 1 + > OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 1 + > OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf | 1 + > OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf | 1 + > OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf | 1 + > OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf | 1 + > OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf | 1 + > OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf | 1 + > OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 + > OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf | 1 + > OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf | 1 + > OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 1 + > OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf | 3 ++- > OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf | 1 + > OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c | 2 +- > OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf | 1 + > OvmfPkg/PlatformDxe/Platform.inf | 2 ++ > OvmfPkg/PlatformPei/PlatformPei.inf | 3 +++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 2 ++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 2 ++ > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 7 +++++-- > OvmfPkg/Virtio10Dxe/Virtio10.inf | 1 + > OvmfPkg/VirtioBlkDxe/VirtioBlk.inf | 1 + > OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 + > OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf | 1 + > OvmfPkg/VirtioRngDxe/VirtioRng.inf | 1 + > OvmfPkg/VirtioScsiDxe/VirtioScsi.inf | 1 + > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf | 9 +++++---- > 35 files changed, 61 insertions(+), 17 deletions(-) > > -- > 2.14.1.3.gb7cf6e02401b > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/11/18 09:15, Ard Biesheuvel wrote: > Hi Laszlo, > > On 11 March 2018 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote: >> Repo: https://github.com/lersek/edk2.git >> Branch: hdr_inf_cleanup >> >> In >> <http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com>, >> Mike explained why it's a good idea to list module-internal *.h files in >> the [Sources*] sections of the INF files: >> >> On 11/23/15 21:28, Kinney, Michael D wrote: >>> There are 2 reasons to list all source files used in a module build in >>> the [Sources] section. >>> >>> 1) Support incremental builds. If a change to the .h file is made, >>> then the module may not be rebuilt if the .h file is not listed in >>> [Sources] >>> 2) Support of UEFI Distribution Package distribution format. The UPT >>> tools that creates UDP packages uses the [Sources] section for the >>> inventory of files. If a file is missing, then it will not be >>> included in the UDP file. >> >> In only two years and three-four months, I've finally come around >> addressing (1) under ArmVirtPkg and OvmfPkg. > > Thanks for doing this. > > However, while I highly appreciate your thoroughness and verbosity in > most cases, I do think you've crossed a line this time :-) > > Do we *really* need 4 different patches for CsmSupportLib, each adding > a single .h file to [Sources], with an elaborate description how it is > being used? If it is used, it needs to be listed, and if it is not, it > needs to be removed, that's all there is to it IMO. The structuring of the patch series reflects my thinking process and the work I did precisely. I didn't (couldn't) investigate multiple header files at once / in parallel; I investigated them one by one. It's easy to squash patches, and it's hard to split them, so I maintain that writing up and posting these patches one by one, in v1, was the right thing to do. Personally I find it much easier to read many trivial patches than half as many complex / divergent ones. If you prefer that I squash patches into one per module, I can do that (I'd wait for more feedback first though). Second, I disagree that it's as simple as "list it if it's used". I didn't just want to dump the .h filenames into the INF files; I wanted to see each time whether the use of the header file was justified in the first place -- this is not a given if there are multiple INF files in the same directory, or an INF file has architecture-specific Sources sections. For example, in patch 06/45, I removed "QemuLoader.h" from "Qemu.c", and "Qemu.c" is only built into one of the INF files under "OvmfPkg/AcpiPlatformDxe". (Ultimately I had to list "QemuLoader.h" in both INF files, in patch 07/45, due to "QemuFwCfgAcpi.c", which is built into both INFs.) For another example, in patch 37/45, I added "VbeShim.h" to [Sources.Ia32, Sources.X64], and not to another of the [Sources*] sections. The same applies to patch 17/45, where "X64/VirtualMemory.h" belongs under [Sources.X64] only. I find this is not as easy as it looks, and I meant to be thorough. If you don't have time to wade through the patches, I'll thank you if you ACK just the first three (ArmVirtPkg) patches. > Apart from that, the series looks correct to me. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 March 2018 at 11:48, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/11/18 09:15, Ard Biesheuvel wrote: >> Hi Laszlo, >> >> On 11 March 2018 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote: >>> Repo: https://github.com/lersek/edk2.git >>> Branch: hdr_inf_cleanup >>> >>> In >>> <http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com>, >>> Mike explained why it's a good idea to list module-internal *.h files in >>> the [Sources*] sections of the INF files: >>> >>> On 11/23/15 21:28, Kinney, Michael D wrote: >>>> There are 2 reasons to list all source files used in a module build in >>>> the [Sources] section. >>>> >>>> 1) Support incremental builds. If a change to the .h file is made, >>>> then the module may not be rebuilt if the .h file is not listed in >>>> [Sources] >>>> 2) Support of UEFI Distribution Package distribution format. The UPT >>>> tools that creates UDP packages uses the [Sources] section for the >>>> inventory of files. If a file is missing, then it will not be >>>> included in the UDP file. >>> >>> In only two years and three-four months, I've finally come around >>> addressing (1) under ArmVirtPkg and OvmfPkg. >> >> Thanks for doing this. >> >> However, while I highly appreciate your thoroughness and verbosity in >> most cases, I do think you've crossed a line this time :-) >> >> Do we *really* need 4 different patches for CsmSupportLib, each adding >> a single .h file to [Sources], with an elaborate description how it is >> being used? If it is used, it needs to be listed, and if it is not, it >> needs to be removed, that's all there is to it IMO. > > The structuring of the patch series reflects my thinking process and the > work I did precisely. I didn't (couldn't) investigate multiple header > files at once / in parallel; I investigated them one by one. It's easy > to squash patches, and it's hard to split them, so I maintain that > writing up and posting these patches one by one, in v1, was the right > thing to do. Personally I find it much easier to read many trivial > patches than half as many complex / divergent ones. If you prefer that I > squash patches into one per module, I can do that (I'd wait for more > feedback first though). > > Second, I disagree that it's as simple as "list it if it's used". I > didn't just want to dump the .h filenames into the INF files; I wanted > to see each time whether the use of the header file was justified in the > first place -- this is not a given if there are multiple INF files in > the same directory, or an INF file has architecture-specific Sources > sections. > > For example, in patch 06/45, I removed "QemuLoader.h" from "Qemu.c", and > "Qemu.c" is only built into one of the INF files under > "OvmfPkg/AcpiPlatformDxe". (Ultimately I had to list "QemuLoader.h" in > both INF files, in patch 07/45, due to "QemuFwCfgAcpi.c", which is built > into both INFs.) > > For another example, in patch 37/45, I added "VbeShim.h" to > [Sources.Ia32, Sources.X64], and not to another of the [Sources*] > sections. The same applies to patch 17/45, where "X64/VirtualMemory.h" > belongs under [Sources.X64] only. > > I find this is not as easy as it looks, and I meant to be thorough. If > you don't have time to wade through the patches, I'll thank you if you > ACK just the first three (ArmVirtPkg) patches. > Please understand that this is not criticism on your thinking process, and I highly value the quality of your work in general, and for this series in particular. I am merely saying that it is not always necessary to share your personal journey resulting in the patches at this level of detail, simply because it doesn't scale. In any case, I am happy with this to go in as is, if you prefer. Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 2018-03-11 04:54:51, Ard Biesheuvel wrote: > On 11 March 2018 at 11:48, Laszlo Ersek <lersek@redhat.com> wrote: > > On 03/11/18 09:15, Ard Biesheuvel wrote: > >> Hi Laszlo, > >> > >> On 11 March 2018 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote: > >>> Repo: https://github.com/lersek/edk2.git > >>> Branch: hdr_inf_cleanup > >>> > >>> In > >>> <http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com>, > >>> Mike explained why it's a good idea to list module-internal *.h files in > >>> the [Sources*] sections of the INF files: > >>> > >>> On 11/23/15 21:28, Kinney, Michael D wrote: > >>>> There are 2 reasons to list all source files used in a module build in > >>>> the [Sources] section. > >>>> > >>>> 1) Support incremental builds. If a change to the .h file is made, > >>>> then the module may not be rebuilt if the .h file is not listed in > >>>> [Sources] > >>>> 2) Support of UEFI Distribution Package distribution format. The UPT > >>>> tools that creates UDP packages uses the [Sources] section for the > >>>> inventory of files. If a file is missing, then it will not be > >>>> included in the UDP file. > >>> > >>> In only two years and three-four months, I've finally come around > >>> addressing (1) under ArmVirtPkg and OvmfPkg. > >> > >> Thanks for doing this. > >> > >> However, while I highly appreciate your thoroughness and verbosity in > >> most cases, I do think you've crossed a line this time :-) > >> > >> Do we *really* need 4 different patches for CsmSupportLib, each adding > >> a single .h file to [Sources], with an elaborate description how it is > >> being used? If it is used, it needs to be listed, and if it is not, it > >> needs to be removed, that's all there is to it IMO. > > > > The structuring of the patch series reflects my thinking process and the > > work I did precisely. I didn't (couldn't) investigate multiple header > > files at once / in parallel; I investigated them one by one. It's easy > > to squash patches, and it's hard to split them, so I maintain that > > writing up and posting these patches one by one, in v1, was the right > > thing to do. Personally I find it much easier to read many trivial > > patches than half as many complex / divergent ones. If you prefer that I > > squash patches into one per module, I can do that (I'd wait for more > > feedback first though). > > > > Second, I disagree that it's as simple as "list it if it's used". I > > didn't just want to dump the .h filenames into the INF files; I wanted > > to see each time whether the use of the header file was justified in the > > first place -- this is not a given if there are multiple INF files in > > the same directory, or an INF file has architecture-specific Sources > > sections. > > > > For example, in patch 06/45, I removed "QemuLoader.h" from "Qemu.c", and > > "Qemu.c" is only built into one of the INF files under > > "OvmfPkg/AcpiPlatformDxe". (Ultimately I had to list "QemuLoader.h" in > > both INF files, in patch 07/45, due to "QemuFwCfgAcpi.c", which is built > > into both INFs.) > > > > For another example, in patch 37/45, I added "VbeShim.h" to > > [Sources.Ia32, Sources.X64], and not to another of the [Sources*] > > sections. The same applies to patch 17/45, where "X64/VirtualMemory.h" > > belongs under [Sources.X64] only. > > > > I find this is not as easy as it looks, and I meant to be thorough. If > > you don't have time to wade through the patches, I'll thank you if you > > ACK just the first three (ArmVirtPkg) patches. > > > > Please understand that this is not criticism on your thinking process, > and I highly value the quality of your work in general, and for this > series in particular. > > I am merely saying that it is not always necessary to share your > personal journey resulting in the patches at this level of detail, > simply because it doesn't scale. True. Originally I was going to suggest that it might be worth making 1 patch per package, but after looking over the changes, it seems that scope is maybe a bit to large for that. > In any case, I am happy with this to go in as is, if you prefer. Also after looking it over, it appears that Laszlo put quite a bit of information into each commit message. I agree that it might be sliced a little too finely, but I guess after seeing the effort he put into it, I prefer Laszlo go ahead and keep the separate commits. Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/12/18 09:43, Jordan Justen wrote: > On 2018-03-11 04:54:51, Ard Biesheuvel wrote: >> I am merely saying that it is not always necessary to share your >> personal journey resulting in the patches at this level of detail, >> simply because it doesn't scale. > > True. Message received, loud and clear. :) > Originally I was going to suggest that it might be worth making > 1 patch per package, but after looking over the changes, it seems that > scope is maybe a bit to large for that. > >> In any case, I am happy with this to go in as is, if you prefer. > > Also after looking it over, it appears that Laszlo put quite a bit of > information into each commit message. I agree that it might be sliced > a little too finely, but I guess after seeing the effort he put into > it, I prefer Laszlo go ahead and keep the separate commits. > > Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> Thank you both for putting up with the excessive detail. I'll attempt to do better next time. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 2018-03-12 05:10:34, Laszlo Ersek wrote: > On 03/12/18 09:43, Jordan Justen wrote: > > On 2018-03-11 04:54:51, Ard Biesheuvel wrote: > > >> I am merely saying that it is not always necessary to share your > >> personal journey resulting in the patches at this level of detail, > >> simply because it doesn't scale. > > > > True. > > Message received, loud and clear. :) Well, I didn't mean for it too be loud and clear. :) I meant to say, at a first glance, it appeared to be overly partitioned. After looking closer it seemed reasonable. I don't think there's a simple answer to this question, but it is probably better to err on the side of being a bit over partitioned. Taking the task of adding a bunch of .h files to .inf files. If they can be easily identified, then it seems reaonable to have a single patch for a package. If it takes more careful analysis, then I guess it's fine to capture that via separate commits. Sorting .c files in package starts to get a little more dicey. What if a mistake is made? It might be nice to be able to bisect commits to find the issue. (Although the build error probably will lead straight to it as well. :) -Jordan > > Originally I was going to suggest that it might be worth making > > 1 patch per package, but after looking over the changes, it seems that > > scope is maybe a bit to large for that. > > > >> In any case, I am happy with this to go in as is, if you prefer. > > > > Also after looking it over, it appears that Laszlo put quite a bit of > > information into each commit message. I agree that it might be sliced > > a little too finely, but I guess after seeing the effort he put into > > it, I prefer Laszlo go ahead and keep the separate commits. > > > > Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > > Thank you both for putting up with the excessive detail. I'll attempt to > do better next time. > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/12/18 17:57, Jordan Justen wrote: > On 2018-03-12 05:10:34, Laszlo Ersek wrote: >> On 03/12/18 09:43, Jordan Justen wrote: >>> On 2018-03-11 04:54:51, Ard Biesheuvel wrote: >> >>>> I am merely saying that it is not always necessary to share your >>>> personal journey resulting in the patches at this level of detail, >>>> simply because it doesn't scale. >>> >>> True. >> >> Message received, loud and clear. :) > > Well, I didn't mean for it too be loud and clear. :) I meant to say, > at a first glance, it appeared to be overly partitioned. After looking > closer it seemed reasonable. My apologies -- by "loud and clear" I meant that I intend to be serious about saving reviewers time, with shorter commit messages, and (if I can manage) with better structured patch sets. Thank you! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/11/18 12:54, Ard Biesheuvel wrote: > I am merely saying that it is not always necessary to share your > personal journey resulting in the patches at this level of detail, > simply because it doesn't scale. Doesn't scale for me, or doesn't scale for reviewers? If the latter, do you suggest that I keep such detailed notes out of the v1 posting as well? (Because, I imagine, if I edit them down for v2 only, then I may have wasted reviewer time already.) The recurrent bottleneck for me is trying to figure out what this or that part of the patch was meant to solve, and why that way. I've also encouraged contributors to capture their exact scenario / use case in commit messages; the more specific the better. (IIRC, one example is commit f5404a3eba1d, "OvmfPkg: Increase the maximum size for Authenticated variables", 2016-03-25.) IOW, I tend to find the focus too wide, and the information lacking. However, if I end up wasting your time instead of saving it, then I'm doing it wrong. I wrote up the commit messages the way I did because I thought it would save time for me, if I had to review the patches (I tend to verify patches maybe a bit too pedantically too, and I appreciate when the commit messages give me crutches for that). If it has the opposite effect on you, then I'm doing it wrong. > In any case, I am happy with this to go in as is, if you prefer. > > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Thank you -- peeking ahead at Jordan's review as well, I think I'll save you guys another round of this. I'm honestly confused now about how I should word my future commit messages. Therefore I can't simply promise "I'll keep them short"; I might not know *how* (i.e. what to leave out). I'll need to actively work on that. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 March 2018 at 12:06, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/11/18 12:54, Ard Biesheuvel wrote: > >> I am merely saying that it is not always necessary to share your >> personal journey resulting in the patches at this level of detail, >> simply because it doesn't scale. > > Doesn't scale for me, or doesn't scale for reviewers? > > If the latter, do you suggest that I keep such detailed notes out of the > v1 posting as well? (Because, I imagine, if I edit them down for v2 > only, then I may have wasted reviewer time already.) > > The recurrent bottleneck for me is trying to figure out what this or > that part of the patch was meant to solve, and why that way. I've also > encouraged contributors to capture their exact scenario / use case in > commit messages; the more specific the better. (IIRC, one example is > commit f5404a3eba1d, "OvmfPkg: Increase the maximum size for > Authenticated variables", 2016-03-25.) IOW, I tend to find the focus too > wide, and the information lacking. > > However, if I end up wasting your time instead of saving it, then I'm > doing it wrong. I wrote up the commit messages the way I did because I > thought it would save time for me, if I had to review the patches (I > tend to verify patches maybe a bit too pedantically too, and I > appreciate when the commit messages give me crutches for that). If it > has the opposite effect on you, then I'm doing it wrong. > >> In any case, I am happy with this to go in as is, if you prefer. >> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Thank you -- peeking ahead at Jordan's review as well, I think I'll save > you guys another round of this. > > I'm honestly confused now about how I should word my future commit > messages. Therefore I can't simply promise "I'll keep them short"; I > might not know *how* (i.e. what to leave out). I'll need to actively > work on that. > The issue you are addressing is the fact that changes to header files will not trigger rebuilds if they are not listed in the module's .INF file. So while I fully expect you to confirm that any .h files you add to those .INF files are in fact depended upon, a reviewer or other 'consumer' of the patch is highly unlikely to be interested in reading novels about how each individual .h file is used exactly, and simply dropping the unused ones and adding the used ones should suffice. As I see it, the commit log should explain the rationale of the change, not a stream-of-consciousness account of how it came into being (and I know I am exaggerating here, but only to clarify the distinction) _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/12/18 13:23, Ard Biesheuvel wrote: > the commit log should explain the rationale of the change, not a > stream-of-consciousness account of how it came into being I've turned this into a reminder where I keep my notes. Thanks for your help! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.