Re: [RFC] Fine-grained review ownership for MdeModulePkg


Leif Lindholm
 

On Wed, Jun 19, 2019 at 05:09:40AM +0000, Wu, Hao A wrote:
Hello all,

As suggested by Ray and Leif, modules (with wildcard) in MdeModulePkg are
classified to a list of features.

Please note that:
* The below list is a draft at this moment, please help to provide
feedbacks/comments;
* Modules with no clear classification are listed under the 'Misc' section
at the bottom of the list.
Thank you for doing the heavy lifting.

A few comments below on how this could be more easily described in the
Maintainer.txt syntax.

ACPI:
MdeModulePkg/Include/*/*Acpi*.h
MdeModulePkg/Universal/Acpi/

BDS:
MdeModulePkg/Include/Library/PlatformBootManagerLib.h
MdeModulePkg/Include/Library/UefiBootManagerLib.h
MdeModulePkg/Library/PlatformBootManagerLibNull/
MdeModulePkg/Library/UefiBootManagerLib/
MdeModulePkg/*BootManagerLib*/

MdeModulePkg/Universal/BdsDxe/
MdeModulePkg/Universal/BootManagerPolicyDxe/
Or maybe even
MdeModulePkg/*BootManager*/, which would also match the line above.

MdeModulePkg/Universal/LoadFileOnFv2/
MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.*

Console:
MdeModulePkg/Include/Guid/ConnectConInEvent.h
MdeModulePkg/Include/Guid/ConsoleInDevice.h
MdeModulePkg/Include/Guid/ConsoleOutDevice.h
MdeModulePkg/Include/Guid/StandardErrorDevice.h
MdeModulePkg/Include/Guid/TtyTerm.h
MdeModulePkg/Universal/Console/ConPlatformDxe/
MdeModulePkg/Universal/Console/ConSplitterDxe/
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/
MdeModulePkg/Universal/Console/TerminalDxe/
I was intrigued as to why this did not specify
MdeModulePkg/Universal/Console/
See [1] below.

However, even if suggestions included below were not implemented, the
situation could be described as:
F: MdeModulePkg/Universal/Console/
X: MdeModulePkg/Universal/Console/GraphicsOutputDxe/

Core (PEI, DXE and Runtime):
MdeModulePkg/Core/Dxe/*
MdeModulePkg/Core/Dxe/Dispatcher/
MdeModulePkg/Core/Dxe/DxeMain/
MdeModulePkg/Core/Dxe/Event/
MdeModulePkg/Core/Dxe/FwVol*/
MdeModulePkg/Core/Dxe/Hand/
MdeModulePkg/Core/Dxe/Image/
MdeModulePkg/Core/Dxe/Library/
MdeModulePkg/Core/Dxe/Misc/
MdeModulePkg/Core/Dxe/SectionExtraction/
F: MdeModulePkg/Core/Dxe/
X: MdeModulePkg/Core/Dxe/Mem/

MdeModulePkg/Core/DxeIplPeim/
MdeModulePkg/Core/Pei/*
MdeModulePkg/Core/Pei/BootMode/
MdeModulePkg/Core/Pei/CpuIo/
MdeModulePkg/Core/Pei/Dependency/
MdeModulePkg/Core/Pei/Dispatcher/
MdeModulePkg/Core/Pei/FwVol/
MdeModulePkg/Core/Pei/Hob/
MdeModulePkg/Core/Pei/Image/
MdeModulePkg/Core/Pei/PeiMain/
MdeModulePkg/Core/Pei/Ppi/
MdeModulePkg/Core/Pei/Security/
F: MdeModulePkg/Core/Pei/
X: MdeModulePkg/Core/Pei/Memory/
X: MdeModulePkg/Core/Pei/PciCfg2/
X: MdeModulePkg/Core/Pei/Reset/
X: MdeModulePkg/Core/Pei/StatusCode/

I'm going to stop there, because I'm lazy, and I realise this is about
the responsibility areas rather than an actual patch to
Maintainers.txt - and I think my point is made.

Further comments below.

MdeModulePkg/Core/RuntimeDxe/
MdeModulePkg/Include/Guid/Crc32GuidedSectionExtraction.h
MdeModulePkg/Include/Guid/EventExitBootServiceFailed.h
MdeModulePkg/Include/Guid/IdleLoopEvent.h
MdeModulePkg/Include/Guid/LoadModuleAtFixedAddress.h
MdeModulePkg/Include/Library/SecurityManagementLib.h
MdeModulePkg/Library/*SectionExtract*/
MdeModulePkg/Library/DxeSecurityManagementLib/
MdeModulePkg/Universal/PlatformDriOverrideDxe/
MdeModulePkg/Universal/SectionExtraction*/
MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c

Debug:
MdeModulePkg/Include/Guid/DebugMask.h
MdeModulePkg/Include/Library/DebugAgentLib.h
MdeModulePkg/Include/Ppi/Debug.h
MdeModulePkg/Library/*Debug*/
MdeModulePkg/Universal/Debug*/

Decompress:
MdeModulePkg/Include/Guid/LzmaDecompress.h
MdeModulePkg/Library/*Decompress*/

Device:
MdeModulePkg/Bus/Ata/
MdeModulePkg/Bus/I2c/
MdeModulePkg/Bus/Isa/
MdeModulePkg/Bus/Pci/Ehci*/
MdeModulePkg/Bus/Pci/IdeBusPei/
MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/
MdeModulePkg/Bus/Pci/NvmExpress*/
MdeModulePkg/Bus/Pci/PciSioSerialDxe/
MdeModulePkg/Bus/Pci/SataControllerDxe/
MdeModulePkg/Bus/Pci/SdMmc*/
MdeModulePkg/Bus/Pci/Ufs*/
MdeModulePkg/Bus/Pci/Uhci*/
MdeModulePkg/Bus/Pci/Xhci*/
MdeModulePkg/Bus/Scsi/
MdeModulePkg/Bus/Sd/
MdeModulePkg/Bus/Ufs/
MdeModulePkg/Bus/Usb/
MdeModulePkg/Include/*/*Ata*.h
MdeModulePkg/Include/*/*NonDiscoverableDevice*.h
MdeModulePkg/Include/*/*NvmExpress*.h
MdeModulePkg/Include/*/*SerialPort*.h
MdeModulePkg/Include/*/*SdMmc*.h
MdeModulePkg/Include/*/*Ufs*.h
MdeModulePkg/Include/*/*Usb*.h
MdeModulePkg/Include/Guid/S3StorageDeviceInitList.h
MdeModulePkg/Include/Guid/RecoveryDevice.h
MdeModulePkg/Include/Guid/UsbKeyBoardLayout.h
MdeModulePkg/Include/Ppi/StorageSecurityCommand.h
MdeModulePkg/Include/Protocol/Ps2Policy.h
MdeModulePkg/Library/BaseSerialPortLib16550/
MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/
MdeModulePkg/Universal/SerialDxe/

Disk:
MdeModulePkg/Universal/Disk/

EBC:
MdeModulePkg/Include/*/*Ebc*.h
MdeModulePkg/Include/Protocol/DebuggerConfiguration.h
MdeModulePkg/Universal/EbcDxe/

Firmware Update:
MdeModulePkg/Application/CapsuleApp/
MdeModulePkg/Include/*/*Capsule*.h
MdeModulePkg/Include/Library/DisplayUpdateProgressLib.h
MdeModulePkg/Include/Library/FmpAuthenticationLib.h
MdeModulePkg/Include/Protocol/EsrtManagement.h
MdeModulePkg/Include/Protocol/FirmwareManagementProgress.h
MdeModulePkg/Library/DisplayUpdateProgressLib*/
MdeModulePkg/Library/DxeCapsuleLib*/
MdeModulePkg/Library/FmpAuthenticationLibNull/
MdeModulePkg/Universal/Capsule*/
MdeModulePkg/Universal/Esrt*/

Graphic:
MdeModulePkg/Include/*/*Logo*.h
MdeModulePkg/Include/Library/BmpSupportLib.h
MdeModulePkg/Include/Library/FrameBufferBltLib.h
MdeModulePkg/Library/BaseBmpSupportLib/
MdeModulePkg/Library/BootLogoLib/
MdeModulePkg/Library/FrameBufferBltLib/
MdeModulePkg/Logo/
MdeModulePkg/Universal/Console/GraphicsOutputDxe/
[1] I would say this suggests GraphicsOutputDxe could be moved to,
say, MdeModulePkg/Universal/Graphics - and Logo be moved there
too.


HII/UI:
MdeModulePkg/Application/BootManagerMenuApp/
MdeModulePkg/Application/UiApp/
MdeModulePkg/Include/*/*FileExplorer*.h
MdeModulePkg/Include/*/*FormBrowser*.h
MdeModulePkg/Include/*/*Hii*.h
MdeModulePkg/Include/Library/CustomizedDisplayLib.h
MdeModulePkg/Include/Protocol/DisplayProtocol.h
MdeModulePkg/Library/*FileExplorer*/
MdeModulePkg/Library/*Hii*/
MdeModulePkg/Library/*UiLib/
MdeModulePkg/Library/CustomizedDisplayLib/
MdeModulePkg/Universal/DisplayEngineDxe/
MdeModulePkg/Universal/FileExplorerDxe/
MdeModulePkg/Universal/Hii*/
MdeModulePkg/Universal/SetupBrowserDxe/

IPMI:
MdeModulePkg/Include/*/*Ipmi*.h
MdeModulePkg/Library/*Ipmi*/

Memory Management:
MdeModulePkg/Application/MemoryProfileInfo/
MdeModulePkg/Core/Dxe/Gcd/
MdeModulePkg/Core/Dxe/Mem/
MdeModulePkg/Core/Pei/Memory/
MdeModulePkg/Include/*/*Mem*.h
MdeModulePkg/Include/*/*IoMmu*.h
MdeModulePkg/Library/*MemoryAllocation*/
MdeModulePkg/Universal/MemoryTest/

PCD:
MdeModulePkg/Application/DumpDynPcd/
MdeModulePkg/Include/*/*Pcd*.h
MdeModulePkg/Universal/PCD/

PCI Bus:
MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe/
MdeModulePkg/Bus/Pci/PciBusDxe/
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/
MdeModulePkg/Core/Pei/PciCfg2/
MdeModulePkg/Include/Library/PciHostBridgeLib.h
MdeModulePkg/Library/PciHostBridgeLibNull/
MdeModulePkg/Universal/PcatSingleSegmentPciCfg2Pei/

Performance:
MdeModulePkg/Include/*/*Perf*.h
MdeModulePkg/Library/*Perf*/

Reset:
MdeModulePkg/Core/Pei/Reset/
MdeModulePkg/Include/*/*Reset*.h
MdeModulePkg/Library/*Reset*/
MdeModulePkg/Universal/ResetSystem*/

S3:
MdeModulePkg/Include/*/*BootScript*.h
MdeModulePkg/Include/*/*LockBox*.h
MdeModulePkg/Include/*/*S3*.h
MdeModulePkg/Library/*LockBox*/
MdeModulePkg/Library/*S3*/
MdeModulePkg/Universal/LockBox/

SMBIOS:
MdeModulePkg/Universal/Smbios*/

SMM:
MdeModulePkg/Application/SmiHandlerProfileInfo/
MdeModulePkg/Core/PiSmmCore/
MdeModulePkg/Include/*/*Smi*.h
MdeModulePkg/Include/*/*Smm*.h
MdeModulePkg/Library/*Smi*/
MdeModulePkg/Library/*Smm*/
MdeModulePkg/Universal/SmmCommunicationBufferDxe/

Status Code:
MdeModulePkg/Core/Pei/StatusCode/
MdeModulePkg/Include/*/*StatusCode*.h
MdeModulePkg/Library/*StatusCode*/
MdeModulePkg/Universal/*StatusCode*/

Variable:
MdeModulePkg/Application/VariableInfo/
MdeModulePkg/Include/*/*FaultTolerantWrite*.h
MdeModulePkg/Include/*/*Var*.h
MdeModulePkg/Include/Guid/SystemNvDataGuid.h
MdeModulePkg/Include/Protocol/SwapAddressRange.h
MdeModulePkg/Library/*Var*/
MdeModulePkg/Universal/FaultTolerantWrite*/
MdeModulePkg/Universal/Variable/

Misc:
I have no issue with the below, but just wanted to point out that if
there was an overarching MdeModulePkg/
section, these would automatically match against that anyway - we
don't need to explicitly tag everything.

MdeModulePkg/Application/HelloWorld/
MdeModulePkg/Include/Guid/MdeModulePkgTokenSpace.h
MdeModulePkg/Include/Guid/MtcVendor.h
MdeModulePkg/Include/Guid/ZeroGuid.h
MdeModulePkg/Include/Library/CpuExceptionHandlerLib.h
MdeModulePkg/Include/Library/PlatformHookLib.h
MdeModulePkg/Include/Library/RecoveryLib.h
MdeModulePkg/Include/Library/SortLib.h
MdeModulePkg/Include/Library/TpmMeasurementLib.h
MdeModulePkg/Include/Protocol/Dpc.h
MdeModulePkg/Include/Protocol/LoadPe32Image.h
MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
MdeModulePkg/Include/Protocol/Print2.h
MdeModulePkg/Library/BaseHobLibNull/
MdeModulePkg/Library/BasePlatformHookLibNull/
MdeModulePkg/Library/BaseSortLib/
MdeModulePkg/Library/CpuExceptionHandlerLibNull/
MdeModulePkg/Library/DxePrintLibPrint2Protocol/
MdeModulePkg/Library/PeiRecoveryLibNull/
MdeModulePkg/Library/PlatformHookLibSerialPortPpi/
MdeModulePkg/Library/TpmMeasurementLibNull/
MdeModulePkg/Library/UefiSortLib/
MdeModulePkg/Universal/DevicePathDxe/
MdeModulePkg/Universal/DriverHealthManagerDxe/
MdeModulePkg/Universal/DriverSampleDxe/
MdeModulePkg/Universal/FvSimpleFileSystemDxe/
MdeModulePkg/Universal/LegacyRegion2Dxe/
MdeModulePkg/Universal/Metronome/
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/
MdeModulePkg/Universal/PrintDxe/
MdeModulePkg/Universal/RegularExpressionDxe/
MdeModulePkg/Universal/TimestampDxe/
MdeModulePkg/Universal/WatchdogTimerDxe/
Looking through this made me realise it would be useful to have a way
of quickly looking up section matches for a given path. So I added a
command line option -l/--lookup to GetMaintainer.py - inline below:

/
Leif

From 11a04f4690e7b8bc348483a26bb3840b74b1fd9d Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@...>
Date: Wed, 19 Jun 2019 10:07:26 +0100
Subject: [RFC PATCH 1/1] BaseTools: add lookup function to GetMaintainer.py

Add command line option -l/--lookup, which lets you specify a path
(existent or not in the current tree) to look up maintainers for.

Signed-off-by: Leif Lindholm <leif.lindholm@...>
---
BaseTools/Scripts/GetMaintainer.py | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/GetMaintainer.py b/BaseTools/Scripts/GetMaintainer.py
index 616342cdb434..444e601c55d1 100644
--- a/BaseTools/Scripts/GetMaintainer.py
+++ b/BaseTools/Scripts/GetMaintainer.py
@@ -160,15 +160,22 @@ if __name__ == '__main__':
help='git revision to examine (default: HEAD)',
nargs='?',
default='HEAD')
+ PARSER.add_argument('-l', '--lookup',
+ help='Find section matches for path LOOKUP',
+ required=False)
ARGS = PARSER.parse_args()

REPO = SetupGit.locate_repo()

- FILES = get_modified_files(REPO, ARGS)
CONFIG_FILE = os.path.join(REPO.working_dir, 'Maintainers.txt')

SECTIONS = parse_maintainers_file(CONFIG_FILE)

+ if ARGS.lookup:
+ FILES = [ARGS.lookup]
+ else:
+ FILES = get_modified_files(REPO, ARGS)
+
ADDRESSES = []

for file in FILES:
--
2.11.0

Join devel@edk2.groups.io to automatically receive all group messages.