From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 D39982235228B for ; Sun, 11 Mar 2018 04:42:30 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 704CCEB70F; Sun, 11 Mar 2018 11:48:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-99.rdu2.redhat.com [10.10.120.99]) by smtp.corp.redhat.com (Postfix) with ESMTP id B842F10AF9C0; Sun, 11 Mar 2018 11:48:45 +0000 (UTC) To: Ard Biesheuvel Cc: edk2-devel-01 , Anthony Perard , Brijesh Singh , Jordan Justen , Julien Grall , Phil Dennis-Jordan References: <20180311014926.3049-1-lersek@redhat.com> From: Laszlo Ersek Message-ID: <20157fd6-a776-fe10-6492-55e85ec03b3f@redhat.com> Date: Sun, 11 Mar 2018 12:48:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Sun, 11 Mar 2018 11:48:49 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Sun, 11 Mar 2018 11:48:49 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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 11:42:32 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. > Apart from that, the series looks correct to me. Thanks! Laszlo