Re: [RFC 0/6] Create central repository for boilerplate configuration

[Laszlo Ersek <lersek@redhat.com>]

On 09/20/17 19:27, Leif Lindholm wrote:
> An awful lot of platform configuration is just repeated verbatim for
> every platform. This is my first stab at eliminating some of this
> redundancy.
> 
> I have additional bits as work in progress, but before I sink too much
> time into it, I would like to try to gather feedback on this approach
> (all the way down to directory structure).
> 
> This first round deals with basic network support and Secure Boot
> requirements.

I can't comment on the general "ConfigPkg" question, and the directory
structure; I'll just assume that it's OK that way. (It will need
documented maintainers though.)

I do have some initial comments:

(1) Please update your git config so that the diff hunk headers display
the INI-style section name that's being modified. Please search the
following sections for "ini":

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09

Normally this is plain "helpful" for reviewers, but given the subject of
this set, I'd say it's "important" (to me as a reviewer anyway), to see
what section lines are removed from.

(2) Replacing a build define called FOOBAR with CONFIG_FOOBAR will break
all downstream build scripts. Is the CONFIG_ prefix a requirement?

(3) I think PCDs should not be included in ConfigPkg DSC include files,
even if several platforms set the same value. The set of libraries and
driver modules commonly used for a given feature is mostly constant
across platforms (and it is easy to extend, incrementally); but I don't
think the same holds for PCDs. Especially if a user wants to change a
PCD for one platform but not the other. Even if repeated settings for a
PCD worked (all on the same level of "specificity"), I'd find the result
confusing.

(4) The Ia32X64 build of OVMF is a bit trickier than the rest; DXE
modules should be built for X64, while PEI modules should be built for
IA32. You can see this in the names of the [Components.IA32] and
[Components.X64] sections. (Point (1) above helps with this.)

For example, in patch #1 you add "ArpDxe" to [Components], but in patch
#3 you remove it from [Components.X64].

(5) In patch #4, there's a section for [LibraryClasses.ARM,
LibraryClasses.AARCH64] -- why is it specific to ARM/AARCH64?


If others like the ConfigPkg idea, I'd like to review the set (or v2)
again, in more detail, but for that, please fix the patch formatting
first, as requested in (1).

Thanks!
Laszlo

> 
> Leif Lindholm (6):
>   ConfigPkg: add new package for holding common config fragments
>   ArmVirtPkg: use	ConfigPkg for common network items
>   OvmfPkg: use ConfigPkg for common network items
>   ConfigPkg: add common Security settings
>   ArmVirtPkg: use ConfigPkg for common security items
>   OvmfPkg: use ConfigPkg for common security items
> 
>  ArmVirtPkg/ArmVirt.dsc.inc           | 25 ++--------
>  ArmVirtPkg/ArmVirtQemu.dsc           | 46 +++---------------
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 24 ++--------
>  ArmVirtPkg/ArmVirtQemuKernel.dsc     | 46 +++---------------
>  ConfigPkg/Network/Network.dsc.inc    | 92 ++++++++++++++++++++++++++++++++++++
>  ConfigPkg/Network/Network.fdf.inc    | 47 ++++++++++++++++++
>  ConfigPkg/Security/Security.dsc.inc  | 67 ++++++++++++++++++++++++++
>  ConfigPkg/Security/Security.fdf.inc  | 17 +++++++
>  OvmfPkg/OvmfPkgIa32.dsc              | 92 ++++--------------------------------
>  OvmfPkg/OvmfPkgIa32.fdf              | 37 +--------------
>  OvmfPkg/OvmfPkgIa32X64.dsc           | 90 ++++-------------------------------
>  OvmfPkg/OvmfPkgIa32X64.fdf           | 37 +--------------
>  OvmfPkg/OvmfPkgX64.dsc               | 92 ++++--------------------------------
>  OvmfPkg/OvmfPkgX64.fdf               | 37 +--------------
>  14 files changed, 276 insertions(+), 473 deletions(-)
>  create mode 100644 ConfigPkg/Network/Network.dsc.inc
>  create mode 100644 ConfigPkg/Network/Network.fdf.inc
>  create mode 100644 ConfigPkg/Security/Security.dsc.inc
>  create mode 100644 ConfigPkg/Security/Security.fdf.inc
>