From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=jordan.l.justen@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 327C32228352E for ; Mon, 12 Mar 2018 01:37:09 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Mar 2018 01:43:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,460,1515484800"; d="scan'208";a="207400746" Received: from bbappudi-mobl1.amr.corp.intel.com (HELO localhost) ([10.255.231.49]) by orsmga005.jf.intel.com with ESMTP; 12 Mar 2018 01:43:29 -0700 MIME-Version: 1.0 To: Ard Biesheuvel , Laszlo Ersek Message-ID: <152084420932.3437.8016565917691204369@jljusten-skl> From: Jordan Justen In-Reply-To: Cc: edk2-devel-01 , Anthony Perard , Brijesh Singh , Julien Grall , Phil Dennis-Jordan References: <20180311014926.3049-1-lersek@redhat.com> <20157fd6-a776-fe10-6492-55e85ec03b3f@redhat.com> User-Agent: alot/0.6 Date: Mon, 12 Mar 2018 01:43:29 -0700 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: Mon, 12 Mar 2018 08:37:10 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2018-03-11 04:54:51, Ard Biesheuvel wrote: > On 11 March 2018 at 11:48, Laszlo Ersek wrote: > > On 03/11/18 09:15, Ard Biesheuvel wrote: > >> 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. > > > > 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