Re: [PATCH 00/45] ArmVirtPkg, OvmfPkg: list module-internal headers in INF files

[Jordan Justen <jordan.l.justen@intel.com>]

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>