From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: michael.johnson@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Thu, 30 May 2019 14:31:23 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 May 2019 14:31:22 -0700 X-ExtLoop1: 1 Received: from orsmsx102.amr.corp.intel.com ([10.22.225.129]) by orsmga003.jf.intel.com with ESMTP; 30 May 2019 14:31:22 -0700 Received: from orsmsx123.amr.corp.intel.com (10.22.240.116) by ORSMSX102.amr.corp.intel.com (10.22.225.129) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 30 May 2019 14:31:22 -0700 Received: from orsmsx121.amr.corp.intel.com ([169.254.10.47]) by ORSMSX123.amr.corp.intel.com ([169.254.1.141]) with mapi id 14.03.0415.000; Thu, 30 May 2019 14:31:22 -0700 From: michael.johnson@intel.com To: "devel@edk2.groups.io" , "afish@apple.com" , Leif Lindholm CC: "Feng, Bob C" , "Rodriguez, Christian" , Laszlo Ersek , "Kinney, Michael D" , "Gao, Liming" , "Shi, Steven" , "Fan, ZhijuX" Subject: Re: [edk2-devel] Edk2 BaseTools Patches. Thread-Topic: [edk2-devel] Edk2 BaseTools Patches. Thread-Index: AdUWjQt6T2cDiXJSSJG3vQ9oXBf1IQAd5skAAARyC9AAB2LEAAADLn8AAAM4ogAACDTMcA== Date: Thu, 30 May 2019 21:31:21 +0000 Message-ID: <9DB7F8038713D946B98D79CA523B7F95E968B5E5@ORSMSX121.amr.corp.intel.com> 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> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Return-Path: michael.johnson@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_9DB7F8038713D946B98D79CA523B7F95E968B5E5ORSMSX121amrcor_" --_000_9DB7F8038713D946B98D79CA523B7F95E968B5E5ORSMSX121amrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable All, These patches are not required for the stable tag. They're improvements n= eeded to enable relatively new build options that are not yet widely used. That said, let me try to clear the air here about what is happening regard= ing the sources/includes and what changes with these patches. The INF spec (section 3.9) says that all local source files, including .h files, must be = included in the sources section. This means a module is not compliant if i= t includes a header file from a directory other than a package include dire= ctory and fails to list it in its sources section. We're already generatin= g a hash which is guaranteed to change whenever the module's source changes= - without invoking mkdep - by hashing each file from the sources section a= s well as *all* the contents of every include folder belonging to each pack= age that the module is dependent on. Every possible 'legally' included header will change the hash, but the has= hes of non-compliant modules will not change when their 'illegally' include= d headers change and we will incorrectly re-use stale cached binaries. To = prevent this, the below patches check for compliance and invalidate the has= h of any non-compliant module. In this way, non-compliance is neither supp= orted nor allowed to poison the cache. Again, since this only has an effect on builds which have enabled this rel= atively new feature, I don't expect any production impact if the stable tag= doesn't take these patches. Nobody is using it yet. -Michael From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andr= ew Fish via Groups.Io Sent: Thursday, May 30, 2019 11:10 AM To: devel@edk2.groups.io; Leif Lindholm Cc: Feng, Bob C ; Rodriguez, Christian ; Laszlo Ersek ; Kinney, Michael D ; Gao, Liming ; Shi, Steven = ; Fan, ZhijuX Subject: Re: [edk2-devel] Edk2 BaseTools Patches. 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 Source= s 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 d= oing the mkdep? I don't see the value of hashing the local include files as any include fi= le 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 ca= use a lot of very subtle errors that are really hard to debug. When you hav= e these kind of errors in your build system you teach people they always ne= ed 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 tie= d 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 b= uild 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 =3D $(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=3D1804 [PATCH v3 1/1] BaseTools:Extend the binary cache to support library cache BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1797 [PATCH V5] BaseTools:Make BaseTools support new rules to generate RAW FFS = FILE BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1765 [PATCH] BaseTools:Update binary cache restore time to current time BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1742 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, Stev= en >; 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 --_000_9DB7F8038713D946B98D79CA523B7F95E968B5E5ORSMSX121amrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

All,

 

These patches are not required for t= he stable tag.  They’re improvements needed to enable relatively= new build options that are not yet widely used.

 

That said, let me try to clear the a= ir here about what is happening regarding the sources/includes and what cha= nges with these patches.

 

The INF spec (section 3.9) says that all local source files, including .h files, must be in= cluded in the sources section.  This means a module is not compliant i= f it includes a header file from a directory other than a package include d= irectory and fails to list it in its sources section.  We’re already generating a hash which is guaranteed = to change whenever the module’s source changes - without invoking mkd= ep - by hashing each file from the sources section as well as *all* = the contents of every include folder belonging to each package that the module is dependent on.

 

Every possible ‘legally’= included header will change the hash, but the hashes of non-compliant modu= les will not change when their ‘illegally’ included headers change and we will incorrectly re-use stale cached binaries.  To pre= vent this, the below patches check for compliance and invalidate the hash o= f any non-compliant module.  In this way, non-compliance is neither su= pported nor allowed to poison the cache.

 

Again, since this only has an effect= on builds which have enabled this relatively new feature, I don’t ex= pect any production impact if the stable tag doesn’t take these patches.  Nobody is using it yet.

 

-Michael

 

&nb= sp;

From: devel@edk2.groups.io [mailto:= devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
Sent: Thursday, May 30, 2019 11:10 AM
To: devel@edk2.groups.io; Leif Lindholm <leif.lindholm@linaro.or= g>
Cc: Feng, Bob C <bob.c.feng@intel.com>; Rodriguez, Christian = <christian.rodriguez@intel.com>; Laszlo Ersek <lersek@redhat.com&g= t;; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <l= iming.gao@intel.com>; Shi, Steven <steven.shi@intel.com>; Fan, ZhijuX <zhijux.fan@intel.com>
Subject: Re: [edk2-devel] Edk2 BaseTools Patches.=

 

 



On May 30, 2019, at 9:37 AM, Leif Lindholm <leif.lindholm@linaro.org> wr= ote:

 

Thanks Bob, Christian,

On Thu, May 30, 2019 at 03:06:48PM +0000, Feng, Bob C wrote:

Thanks Christian. I add some short description fo= r the patches.

These 5 patches are all for binary cache feature.

[Patch V4 2/2] BaseTools: Refactor hash tracking after checking for Source= s 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 s= ome
.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 se= t 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 b= e 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 kin= d 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 somet= hing? 

 

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 tim= e 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 r= ules 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 =3D $(NAMED_GUID) {
            &n= bsp;   $(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 <leif.lin= dholm@linaro.org>; Feng, Bob C <bob.c.feng@intel.com>
Cc: Andrew Fish <afish@apple.com= >; Laszlo Ersek <lersek@redhat.c= om>; Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Gao, Lim= ing <liming.gao@intel.com>= ;; Shi, Steven <steven.shi@intel= .com>; Fan, ZhijuX <zhiju= x.fan@intel.com>
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: http= s://bugzilla.tianocore.org/show_bug.cgi?id=3D1804

[PATCH v3 1/1] BaseTools:Extend the binary cache to support library cache<= br> BZ: http= s://bugzilla.tianocore.org/show_bug.cgi?id=3D1797

[PATCH V5] BaseTools:Make BaseTools support new rules to generate RAW FFS = FILE
BZ: http= s://bugzilla.tianocore.org/show_bug.cgi?id=3D1765

[PATCH] BaseTools:Update binary cache restore time to current time
BZ: http= s://bugzilla.tianocore.org/show_bug.cgi?id=3D1742

Thanks,
Christian


-----Original Message-----
From: Leif Lindholm [mailto:le= if.lindholm@linaro.org]
Sent: Thursday, May 30, 2019 2:28 AM
To: Feng, Bob C <bob.c.feng@int= el.com>
Cc: Andrew Fish <afish@apple.com= >; Laszlo Ersek <lersek@redhat.c= om>; 
Kinney, Michael D <michae= l.d.kinney@intel.com>; devel@edk2.groups.io; 
Gao, Liming <liming.gao@intel.c= om>; Shi, Steven <steven.= shi@intel.com>;  Rodriguez, Christian <= christian.rodriguez@intel.com>; Fan, ZhijuX 
<zhijux.fan@intel.com> 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://ed= k2.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



 

--_000_9DB7F8038713D946B98D79CA523B7F95E968B5E5ORSMSX121amrcor_--