From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::234; helo=mail-it0-x234.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x234.google.com (mail-it0-x234.google.com [IPv6:2607:f8b0:4001:c0b::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 67E932250EDED for ; Sun, 11 Mar 2018 00:09:33 -0800 (PST) Received: by mail-it0-x234.google.com with SMTP id n128-v6so6745904ith.1 for ; Sun, 11 Mar 2018 00:15:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=+WNf3Eaky+qVBexRXtqgg0LsEP7EUePFR+DwsXMAgBc=; b=k030PUlzumLHd/Y8bNdeKDIdh07IOC0kH3jlO3V4LOzJ78U6LtoHwcHQvlDmBhvRiQ fXycTzt3fNKeSki/COBrBZPqd+TC76gJNcZQ0h+q0miV+DasmMssnfpLA3NUB70oqxv5 9Ku9viRGJzz2vCY0/xYKU64ZJUuVygfx5T/8I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=+WNf3Eaky+qVBexRXtqgg0LsEP7EUePFR+DwsXMAgBc=; b=lAM3eVPF40kwnuiJeNZbgLCCr/MUFU4NwAVxYS8TxXj74M/KIlh03w+PdDgNFZeQ7f E2CTGq4h23v/unRC2lSga7kmRllJwcoDja+yiLCkOYq5NP5c8uMrl7yKcwubKvpCY5rq x8lzustX1bIFtCs8SWDZPANUbJnDM1OiEZn0iJAiUsxi0fCYUaQrQUGVKywI7fAf2LWc Hd8sOWrDqinD0uB7A40JOFSnJJtrlDVWitnBwKnsWZmEd/jdZ+vxddJko/W3OrHF7uNZ VODSSMBSY+W1CABU9UDNu5obX7lt6pPwUngceJ6oyVg2avV4PyXLh4ZZf33cJDk01ZW2 y1HQ== X-Gm-Message-State: AElRT7EeY+VR589P04MzT/Fn+YgL5WP+C0ndHLJ/px5NdRXheiC7uPcw REEjZ5goPeYj35mvgx4CzdbVTR3RvLhsSKow4nePfg== X-Google-Smtp-Source: AG47ELsBvLl9cJJ9eQGmf1zBwQrkKHH33Kp4b/4kvDpCtGMvVAO/xubMz2BbTanlGD0PfCx7RsQEvyCUJncz+KV9EOw= X-Received: by 10.36.60.216 with SMTP id m207mr4265879ita.68.1520756151569; Sun, 11 Mar 2018 00:15:51 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Sun, 11 Mar 2018 00:15:50 -0800 (PST) In-Reply-To: <20180311014926.3049-1-lersek@redhat.com> References: <20180311014926.3049-1-lersek@redhat.com> From: Ard Biesheuvel Date: Sun, 11 Mar 2018 08:15:50 +0000 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Anthony Perard , Brijesh Singh , Jordan Justen , Julien Grall , Phil Dennis-Jordan Subject: Re: [PATCH 00/45] ArmVirtPkg, OvmfPkg: list module-internal headers in INF files X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 11 Mar 2018 08:09:33 -0000 Content-Type: text/plain; charset="UTF-8" Hi Laszlo, On 11 March 2018 at 01:48, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: hdr_inf_cleanup > > In > , > 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 > Cc: Ard Biesheuvel > Cc: Brijesh Singh > Cc: Jordan Justen > Cc: Julien Grall > Cc: Phil Dennis-Jordan > > 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 >