From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=XOb11zzl; spf=pass (domain: apple.com, ip: 17.171.2.72, mailfrom: afish@apple.com) Received: from ma1-aaemail-dr-lapp03.apple.com (ma1-aaemail-dr-lapp03.apple.com [17.171.2.72]) by groups.io with SMTP; Thu, 30 May 2019 11:10:38 -0700 Received: from pps.filterd (ma1-aaemail-dr-lapp03.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id x4UI77MR018664; Thu, 30 May 2019 11:10:37 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=mime-version : content-type : sender : from : message-id : subject : date : in-reply-to : cc : to : references; s=20180706; bh=4eS7/11LfwIovGKJPoDw5LZTdDp496vwmXeDeZW6xE8=; b=XOb11zzlhLBP0KHwaaBjAB9Gxgq0uwtloDlJKDf1COAzy7WMtDFN8sN/vedWva0aIqv2 Gn/hq1D0WLAP45qi7MbZF4LvXS8hIhzc/cs5eD5Y8+gC4luX0QBNXN18rFM9JDvW4eWj j3tgQepPzsZtFSo8oefdhmdMREjz2myuSKX+IIdKFxbP/UZV9RFhZUlqKaXkMtwdPDbQ ZTE/uJ9NrEyKgKFMZMkqJFXfSRxy6TA4R7UBcHcPprsl25wmhP8uJYYvCH5s/SL8DLXv V4fhwMXkohVsht2x6wjgpWDuYfbW+nFIX4cWqHnw8TxlSRWElCnw5w2cguuUhmHmjfld fg== Received: from ma1-mtap-s02.corp.apple.com (ma1-mtap-s02.corp.apple.com [17.40.76.6]) by ma1-aaemail-dr-lapp03.apple.com with ESMTP id 2sq4pxkjjd-5 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 30 May 2019 11:10:37 -0700 MIME-version: 1.0 Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by ma1-mtap-s02.corp.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPS id <0PSB006UJX5NUB70@ma1-mtap-s02.corp.apple.com>; Thu, 30 May 2019 11:10:36 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz12.apple.com by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) id <0PSB00600WN8KQ00@nwk-mmpp-sz12.apple.com>; Thu, 30 May 2019 11:10:35 -0700 (PDT) X-Va-A: X-Va-T-CD: 17e290161afadc43df911743530be568 X-Va-E-CD: f42e1f233b026a6701a5dc376868b5d0 X-Va-R-CD: 65855544104c2f96f2c335eea6ab5477 X-Va-CD: 0 X-Va-ID: 30d0aa91-8cd8-459a-84c7-d63757c11e16 X-V-A: X-V-T-CD: 17e290161afadc43df911743530be568 X-V-E-CD: f42e1f233b026a6701a5dc376868b5d0 X-V-R-CD: 65855544104c2f96f2c335eea6ab5477 X-V-CD: 0 X-V-ID: 1207ff89-cea7-434c-b242-7b9a118923af X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-05-30_11:,, signatures=0 Received: from [17.226.41.60] (unknown [17.226.41.60]) by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.3.20181024 64bit (built Oct 24 2018)) with ESMTPSA id <0PSB00M44X4UUR20@nwk-mmpp-sz12.apple.com>; Thu, 30 May 2019 11:10:34 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: Subject: Re: [edk2-devel] Edk2 BaseTools Patches. Date: Thu, 30 May 2019 11:10:08 -0700 In-reply-to: <20190530163754.shb54x46euzszpza@bivouac.eciton.net> Cc: "Feng, Bob C" , "Rodriguez, Christian" , Laszlo Ersek , Mike Kinney , "Gao, Liming" , "Shi, Steven" , "Fan, ZhijuX" To: devel@edk2.groups.io, Leif Lindholm References: <08650203BA1BD64D8AD9B6D5D74A85D160128393@SHSMSX101.ccr.corp.intel.com> <20190530092802.7belyzgxdd76ps2v@bivouac.eciton.net> <3A7DCC9A944C6149BF832E1C9B718ABC01F24E6B@ORSMSX112.amr.corp.intel.com> <08650203BA1BD64D8AD9B6D5D74A85D1601296DA@SHSMSX101.ccr.corp.intel.com> <20190530163754.shb54x46euzszpza@bivouac.eciton.net> X-Mailer: Apple Mail (2.3445.6.18) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-05-30_11:,, signatures=0 Content-type: multipart/alternative; boundary="Boundary_(ID_/caoijfrdCHYW+nzuCrQNw)" --Boundary_(ID_/caoijfrdCHYW+nzuCrQNw) Content-type: text/plain; CHARSET=US-ASCII Content-transfer-encoding: 7BIT > On May 30, 2019, at 9:37 AM, Leif Lindholm wrote: > > Thanks Bob, Christian, > > On Thu, May 30, 2019 at 03:06:48PM +0000, Feng, Bob C wrote: >> Thanks Christian. I add some short description for the patches. >> >> These 5 patches are all for binary cache feature. >> >> [Patch V4 2/2] BaseTools: Refactor hash tracking after checking for Sources section >> [Patch V4 1/2] BaseTools: Add a checking for Sources section in INF file >> >> The above 2 patches is to fix the issue that >> The build tool uses the files list under [sources] section of INF >> file as a input to calculate a module's hash value. But in some INF >> files, [sources] does not list all the "source" files, missing some >> .h files. Path 2/2 use another method to get all source files for a >> module and patch 1/2 do a check whether [sources] list all the >> "source" files. > > I'll be honest - because of the wild variance in whether .h files are > listed in the [sources] section of .inf files, I have always been > unsure as to whether they were just being ignored (and extracted on > the side via mkdep or similar). > Leif, I'm confused too as you can only really know the set of include files by doing the mkdep? I don't see the value of hashing the local include files as any include file change in the mkdep path requires the module to be recompiled. It seems to me having one scheme for hashing and anther four building is going to cause a lot of very subtle errors that are really hard to debug. When you have these kind of errors in your build system you teach people they always need to make clean, so they bypass the hashing and dependency checks. Seems like we may be fighting the makefiles again, but from a 10,000 point of view it seems like the dependency algorithm and the hash need to be tied together. Seems like the makefile already knows if it needs to build it, but I'm not sure if the makefile can run an action if it does not need to build something? Thanks, Andrew Fish > If the intent is to speed up build time, would it not be better to > warn the user - so they notice the problem and fix their modules, > rather than adding extra processing time on having the tools work with > broken .inf files? > > This does not look like material for edk2-stable201905 to me. > >> [PATCH v3 1/1] BaseTools:Extend the binary cache to support library cache >> This patch is to resolve the problem that >> Build tool dose not cache the library binaries now. Whiteout this >> patch, there is 25% extra time cost to rebuild the all module >> dependency libraries if cache miss happen. > > 25% is a big number, so I won't argue against this. But I also won't > argue for it - the BZ was raised very late in the cycle. > >> [PATCH] BaseTools:Update binary cache restore time to current time >> This patch is to make the restored binary file have the current time >> stamp not the binary file original time stamp. > > I can see how the current behaviour could cause problems with some > CI/build systems. If it is properly reviewed and tested, I am OK with > this one going in for edk2-stable201903. > >> [PATCH V5] BaseTools:Make BaseTools support new rules to generate RAW FFS FILE >> This patch is to support the raw ffs file rule. Now build tool does >> not correctly handle this case: >> >> [Rule.Common.USER_DEFINED.MicroCode] >> FILE RAW = $(NAMED_GUID) { >> $(INF_OUTPUT)/$(MODULE_NAME).bin >> } > > This looks like a new feature - not something that should bypass the > freeze period for edk2-stable201905. > Can you explain why this is needed in the stable tag as opposed to > being available from master the day after the tag is made? > > Best Regards, > > Leif > >> >> >> Thanks, >> Bob >> >> -----Original Message----- >> From: Rodriguez, Christian >> Sent: Thursday, May 30, 2019 10:26 PM >> To: Leif Lindholm ; Feng, Bob C >> Cc: Andrew Fish ; Laszlo Ersek ; Kinney, Michael D ; devel@edk2.groups.io; Gao, Liming ; Shi, Steven ; Fan, ZhijuX >> Subject: RE: Edk2 BaseTools Patches. >> >> Hey Leif, >> >> I thought I'd help Bob and gather those BZs for each thread: >> >> [Patch V4 1/2] BaseTools: Add a checking for Sources section in INF file [Patch V4 2/2] BaseTools: Refactor hash tracking after checking for Sources section >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1804 >> >> [PATCH v3 1/1] BaseTools:Extend the binary cache to support library cache >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1797 >> >> [PATCH V5] BaseTools:Make BaseTools support new rules to generate RAW FFS FILE >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1765 >> >> [PATCH] BaseTools:Update binary cache restore time to current time >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1742 >> >> Thanks, >> Christian >> >>> -----Original Message----- >>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org] >>> Sent: Thursday, May 30, 2019 2:28 AM >>> To: Feng, Bob C >>> Cc: Andrew Fish ; Laszlo Ersek ; >>> Kinney, Michael D ; devel@edk2.groups.io; >>> Gao, Liming ; Shi, Steven ; >>> Rodriguez, Christian ; Fan, ZhijuX >>> >>> Subject: Re: Edk2 BaseTools Patches. >>> >>> Hi Bob, >>> >>> On Thu, May 30, 2019 at 06:39:59AM +0000, Feng, Bob C wrote: >>>> Hi, >>>> >>>> Currently, we have 5 Basetools patches which are ready to push. Since >>>> we are in the soft-freeze phase, I'd like to ask for your opinions if >>>> those patches can be pushed to edk2 master. >>> >>> To save me the time of reading through all the threads and getting to >>> grips with all the code, could you summarise the problem these solve >>> and the impact of not including these? >>> >>> Is there a BZ? >>> >>> Regards, >>> >>> Leif >>> >>>> >>>> These 5 patches are to fix the issues for the build cache feature. >>>> >>>> [Patch V4 2/2] BaseTools: Refactor hash tracking after checking for >>>> Sources section >>>> https://edk2.groups.io/g/devel/topic/31835556#41642 >>>> >>>> [Patch V4 1/2] BaseTools: Add a checking for Sources section in INF >>>> file >>>> https://edk2.groups.io/g/devel/topic/31835555#41641 >>>> >>>> [PATCH v3 1/1] BaseTools:Extend the binary cache to support library >>>> cache >>>> https://edk2.groups.io/g/devel/topic/31843505#41655 >>>> >>>> [PATCH V5] BaseTools:Make BaseTools support new rules to generate RAW >>>> FFS FILE >>>> https://edk2.groups.io/g/devel/topic/31830807#41571 >>>> >>>> [PATCH] BaseTools:Update binary cache restore time to current time >>>> https://edk2.groups.io/g/devel/topic/31819590#41468 >>>> >>>> >>>> Thanks, >>>> Bob > > --Boundary_(ID_/caoijfrdCHYW+nzuCrQNw) Content-type: text/html; CHARSET=US-ASCII Content-transfer-encoding: quoted-printable
On May 30= , 2019, at 9:37 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:

Thanks Bob, Christian,

On Thu, May 30, 2019 at 03:06:48PM +0000, Feng, Bob C wrote:=
Thanks Ch= ristian. I add some short description for the patches.

These 5 patches are all for binary cache feature.
[Patch V4 2/2] BaseTools: Refactor hash tracking after checkin= g for Sources section
[Patch V4 1/2] BaseTools: Add a checkin= g for Sources section in INF file

The above 2 = patches is to fix the issue that
The  build tool uses th= e files list under [sources] section of INF
file as a input t= o calculate a module's hash value. But in some INF
files, [so= urces] does not list all the "source" files, missing some
.h = files. Path 2/2 use another method to get all source files for a
module and patch 1/2 do a check whether [sources] list all the
"source" files.

I'll be honest - because of the wild variance in whe= ther .h files are
lis= ted in the [sources] section of .inf files, I have always been
unsure as to whether they were just= being ignored (and extracted on
the side via mkdep or similar).

=
Leif,

I'm con= fused too as you can only really know the set of include files by doing the= mkdep?

I don't see the value of hashin= g the local include files as any include file change in the mkdep path requ= ires the module to be recompiled. It seems to me having one scheme for hash= ing and anther four building is going to cause a lot of very subtle errors = that are really hard to debug. When you have these kind of errors in your b= uild system you teach people they always need to make clean, so they bypass= the hashing and dependency checks. 

Seems like we may be fighting the makefiles again, but from a 10,000 poi= nt of view it seems like the dependency algorithm and the hash need to be t= ied together. Seems like the makefile already knows if it needs to build it= , but I'm not sure if the makefile can run an action if it does not need to= build something? 

Thanks,

Andrew Fish

If = the intent is to speed up build time, would it not be better to
warn the user - so they notice the= problem and fix their modules,
rather than adding extra processing time on having the tools work = with
broken .inf files?=

This does not look like material for edk2-stable201905 to = me.

[PATCH v3 1/1] BaseTools:Extend the binary= cache to support library cache
This patch is to resolve the = problem that
Build tool dose not cache the library binaries n= ow. Whiteout this
patch, there is 25% extra time cost to rebu= ild the all module
dependency libraries if cache miss happen.=

2= 5% is a big number, so I won't argue against this. But I also won't<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">argue for it - the BZ was rais= ed very late in the cycle.
[PATCH] BaseTools:U= pdate binary cache restore time to current time
This patch is= to make the restored binary file have the current time
stamp= not the binary file original time stamp.

I can see how the current behaviour= could cause problems with some
CI/build systems. If it is properly reviewed and tested, I am OK w= ith
this one going in f= or edk2-stable201903.

[PATCH V5] BaseTools:Mak= e BaseTools support new rules to generate RAW FFS FILE
This p= atch is to support the raw ffs file rule. Now build tool does
not correctly handle this case:

[Rule.Common.= USER_DEFINED.MicroCode]
 FILE RAW =3D $(NAMED_GUID) {          &nb= sp;     $(INF_OUTPUT)/$(MODULE_NAME).bin
 }

This looks like a new feature - not something that should byp= ass the
freeze period f= or edk2-stable201905.
C= an you explain why this is needed in the stable tag as opposed tobeing available from master the = day after the tag is made?
Best Regards,

Le= if



Thanks,
Bob

-----Original Message-----
From: Rodriguez, Christian&nb= sp;
Sent: Thursday, May 30, 2019 10:26 PM
To: Leif Lindholm <leif.lindholm@linaro.org>; Feng, Bob C <bob.c.feng@intel.com>
Cc: Andrew Fish <afish@ap= ple.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; <= a href=3D"mailto:devel@edk2.groups.io" class=3D"">devel@edk2.groups.io;= Gao, Liming <liming.= gao@intel.com>; Shi, Steven <steven.shi@intel.com>; Fan, ZhijuX <zhijux.fan@intel.com>
Subject: RE: Edk2 BaseTools Patches.

He= y Leif,

I thought I'd help Bob and gather thos= e BZs for each thread:

[Patch V4 1/2] BaseTool= s: Add a checking for Sources section in INF file [Patch V4 2/2] BaseTools:= Refactor hash tracking after checking for Sources section
BZ= : https://bugzilla.tianocore.org/show_bug.cgi?id=3D1804

[PATCH v3 1/1] BaseTools:Extend the binary cache to s= upport library cache
BZ: https://bugzilla.tianocore.org/sho= w_bug.cgi?id=3D1797

[PATCH V5] BaseTools:M= ake BaseTools support new rules to generate RAW FFS FILE
BZ: = https://bugzilla.tianocore.org/show_bug.cgi?id=3D1765
<= br class=3D"">[PATCH] BaseTools:Update binary cache restore time to current= time
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D= 1742

Thanks,
Christian

-----Original = Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Thursday, May 30, 2019 2:28 AM
To: Feng, Bob C &= lt;bob.c.feng@intel.com<= /a>>
Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; 
Kinney, Michael D <michael.d.kinney@intel.c= om>; devel@edk2.g= roups.io; 
Gao, Liming <l= iming.gao@intel.com>; Shi, Steven <steven.shi@intel.com>; 
Rodriguez, Christian <christian.rodriguez@i= ntel.com>; Fan, ZhijuX 
<= zhijux.fan@intel.com>
Subject: Re: Edk2 BaseTools Patc= hes.

Hi Bob,

On T= hu, May 30, 2019 at 06:39:59AM +0000, Feng, Bob C wrote:
Hi,

Currently= , we have 5 Basetools patches which are ready to push. Since 
we are in the soft-freez= e phase, I'd like to ask for your opinions if 
those patches can be pushed to edk2 mas= ter.

To save me the time of readi= ng through all the threads and getting to 
grips with all the code, could you summaris= e the problem these solve 
and the impact of not including these?

Is there a BZ?

Regards,

Leif


These 5 patches are to fix the issues for the bu= ild cache feature.

[Patch V4 2/2] BaseTools: R= efactor hash tracking after checking for 
Sources section
https://edk2.g= roups.io/g/devel/topic/31835556#41642

[Pat= ch V4 1/2] BaseTools: Add a checking for Sources section in INF 
file
= https://edk2.groups.io/g/devel/topic/31835555#41641

[PATCH v3 1/1] BaseTools:Extend the binary cache to support library<= span class=3D"Apple-converted-space"> 
cache
https://edk2.groups.io/g/devel/topic/31843505#41655

[PATCH V5] BaseTools:Make BaseTools support new rules to ge= nerate RAW 
FFS FILE
https://edk2.groups.io/g/devel/topic/31830807#41571=

[PATCH] BaseTools:Update binary cache restore= time to current time
https://edk2.groups.io/g/devel/topic/31= 819590#41468


Thanks,
Bob


--Boundary_(ID_/caoijfrdCHYW+nzuCrQNw)--