Re: Edk2 BaseTools Patches.

["Christian Rodriguez" <christian.rodriguez@intel.com>]

See below.

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: Thursday, May 30, 2019 9:38 AM
>To: Feng, Bob C <bob.c.feng@intel.com>
>Cc: Rodriguez, Christian <christian.rodriguez@intel.com>; Andrew Fish
><afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; 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.
>
>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).
>
>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.

The patch does warn the user, so they notice the problem and fix their modules. And somewhere in the email thread for that patch set I mentioned the processing time is almost negligible since the information is already available in memory and it's just a simple set lookup for existence and warning write.

It doesn't really matter if it goes in edk2-stable201905, as long as it goes in eventually because it does fix a bug/corner-case in the hash feature.

>
>> [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 <leif.lindholm@linaro.org>; Feng, Bob C
>> <bob.c.feng@intel.com>
>> Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>;
>> Kinney, Michael D <michael.d.kinney@intel.com>; 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.
>>
>> 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 <bob.c.feng@intel.com>
>> >Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>;
>> >Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
>> >Gao, Liming <liming.gao@intel.com>; 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://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