From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22c; helo=mail-it0-x22c.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22c.google.com (mail-it0-x22c.google.com [IPv6:2607:f8b0:4001:c0b::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 076D32235228B for ; Sun, 11 Mar 2018 04:48:33 -0700 (PDT) Received: by mail-it0-x22c.google.com with SMTP id l187-v6so7984175ith.4 for ; Sun, 11 Mar 2018 04:54:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=W+zEvX1ALpyaig6Rgyk9d0jWKzo+qDGbfzV6WxjqDt4=; b=c+SXguc7VhE/KfLTOLAAPI3Dy0zy47gBbpoBHXVgpwZB4ZjRzazJWOzkptvjyQ56dO ujcWJxE0zjWCwcl2rjSk9VC+tJmYwpeOc/WIccpf+fR2cyWXMgxGWOZB1QHZl5BaHm8E /GcPoibOJhW7gTdF0xyAZ5lamELyahQwYOLP4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=W+zEvX1ALpyaig6Rgyk9d0jWKzo+qDGbfzV6WxjqDt4=; b=HLv8eT/PQa/Dem/BWwPQe2sBsxswmCw2+WEPdWFgHV3IVwhThzeHm0uoDh0H4CNBz7 coEMDLeP09ukUnXMdwuTbJPIjrQLu1cKMvxyWR8hPrL67qR930tfSTXyuuuLbpvD7uWZ SAnnqCm2/do9y9XS8NG0vAHX7pOR83sqh25u5yaNWev/H+/dSeKntl+ek5NSeZyTnO3t Hwp7RRxjNAvZESh7inY9nki1S7WqvENVX6gXTSXpNCURDzyNYGXRzqPQle0RobLLf2cM 0xh/9F62xHjoB4JWW+dBk/yYWC3ZlUUjisL7wV2ZYnhYKgzlEYxJsDgxpUEQrl4ltn1m ZUkw== X-Gm-Message-State: AElRT7Gi9IBQynbQxW9crUTaxj9Sge7dbmvMP0B4dC0iVsNCzudkx1mB eU9h+mVZY63UrH6OdHNCvKBQnq7TtspCI7bnkEk3vw== X-Google-Smtp-Source: AG47ELuWQk9BAJv9i0rr4KL9s6ANPJ+lhA39UeKV07sEPdLqIBO+jYvrjX4oJQo8H9fg+Tt7hIZe+4gqFSC0ef1Uf8U= X-Received: by 10.36.230.69 with SMTP id e66mr4720592ith.42.1520769292433; Sun, 11 Mar 2018 04:54:52 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Sun, 11 Mar 2018 04:54:51 -0700 (PDT) In-Reply-To: <20157fd6-a776-fe10-6492-55e85ec03b3f@redhat.com> References: <20180311014926.3049-1-lersek@redhat.com> <20157fd6-a776-fe10-6492-55e85ec03b3f@redhat.com> From: Ard Biesheuvel Date: Sun, 11 Mar 2018 11:54:51 +0000 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Anthony Perard , Brijesh Singh , Jordan Justen , Julien Grall , Phil Dennis-Jordan 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:48:34 -0000 Content-Type: text/plain; charset="UTF-8" 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. In any case, I am happy with this to go in as is, if you prefer. Reviewed-by: Ard Biesheuvel