[edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed
Bob Feng
bob.c.feng at intel.com
Tue May 14 02:52:51 UTC 2019
I entered a new BZ https://bugzilla.tianocore.org/show_bug.cgi?id=1804 to track the [Sources] section check function.
Agree that "minimize the performance hit by saving the data from ModuleMakefile.GetFileDependency() to memory and reuse it later in GenMake to save on IO and search."
Thanks,
Bob
-----Original Message-----
From: Rodriguez, Christian
Sent: Tuesday, May 14, 2019 2:41 AM
To: Feng, Bob C <bob.c.feng at intel.com>; Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io; felixp at ami.com
Cc: Gao, Liming <liming.gao at intel.com>; Zhu, Yonghong <yonghong.zhu at intel.com>
Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed
Yes there could be a performance hit. It might be better to make it a script.
But this is only going to happen when the hash feature is enabled, not ever build. Also we can minimize the performance hit by saving the data from ModuleMakefile.GetFileDependency() to memory and reuse it later in GenMake to save on IO and search.
Thanks,
Christian
>-----Original Message-----
>From: Feng, Bob C
>Sent: Monday, May 13, 2019 5:23 AM
>To: Laszlo Ersek <lersek at redhat.com>; Rodriguez, Christian
><christian.rodriguez at intel.com>; devel at edk2.groups.io; felixp at ami.com
>Cc: Gao, Liming <liming.gao at intel.com>; Zhu, Yonghong
><yonghong.zhu at intel.com>
>Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not
>mentioned in inf are not hashed
>
>I think checking if [Source] includes all the "source" file under
>module's folder for each module during build would make build slow
>down, we have been doing a lot to improve the build performance.
>Maybe we could create a new python script under BaseTools/Scripts
>folder to do this check.
>
>Thanks,
>Bob
>
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek at redhat.com]
>Sent: Monday, May 13, 2019 7:39 PM
>To: Rodriguez, Christian <christian.rodriguez at intel.com>;
>devel at edk2.groups.io; felixp at ami.com
>Cc: Feng, Bob C <bob.c.feng at intel.com>; Gao, Liming
><liming.gao at intel.com>; Zhu, Yonghong <yonghong.zhu at intel.com>
>Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not
>mentioned in inf are not hashed
>
>On 05/10/19 21:45, Rodriguez, Christian wrote:
>> Hashing is not changing file format requirements as Basetools has no
>requirement on this even though the spec does have file requirements.
>That's why the initial patch was a workaround of sorts because it is
>allowed by Basetools to have local headers not in the sources section of the meta file.
>>
>> Always breaking the build is outside of the scope of this BZ and my
>> project
>priorities. I agree it should be done, but it's out of my scope.
>>
>> I am specifically targeting the hashing feature, which relies on UEFI
>> Spec
>requirements.
>
>I think breaking the build immediately (and unconditionally) could
>catch platforms by surprise. Can we make this a warning vs. an error?
>And, I'm totally OK if it's available only with --hash, for now.
>
>BTW -- I'm not sure why the UEFI spec is relevant here.
>
>Thanks
>Laszlo
>
>>> -----Original Message-----
>>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf
>>> Of Felix Polyudov
>>> Sent: Friday, May 10, 2019 12:32 PM
>>> To: Rodriguez, Christian <christian.rodriguez at intel.com>;
>>> devel at edk2.groups.io; 'lersek at redhat.com' <lersek at redhat.com>
>>> Cc: Feng, Bob C <bob.c.feng at intel.com>; Gao, Liming
>>> <liming.gao at intel.com>; Zhu, Yonghong <yonghong.zhu at intel.com>
>>> Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not
>>> mentioned in inf are not hashed
>>>
>>> My suggestion would be to always break a build (no matter what the
>>> hashing settings are).
>>> Hashing is just an optimization technique, usage of which should not
>>> be changing source file formatting requirements.
>>>
>>>> -----Original Message-----
>>>> From: Rodriguez, Christian [mailto:christian.rodriguez at intel.com]
>>>> Sent: Friday, May 10, 2019 3:14 PM
>>>> To: devel at edk2.groups.io; Felix Polyudov; 'lersek at redhat.com'
>>>> Cc: Feng, Bob C; Gao, Liming; Zhu, Yonghong
>>>> Subject: RE: [edk2-devel] [PATCH] BaseTools: Include headers not
>>>> mentioned in inf are not hashed
>>>>
>>>> After talking to my colleagues about this, the direction seems to
>>>> be to fundamentally change this BZ. Instead of building this sort
>>>> of workaround feature, we should use the information gathered from
>>>> this
>>> feature to cause the build to break when the hash feature is enabled.
>>> This would force users of the hash feature to be UEFI spec complaint.
>>>>
>>>> What do you guys think; Laszlo and Felix?
>>>>
>>>> I'll update the BZ when I get your input.
>>>>
>>>> Thanks,
>>>> Christian Rodriguez
>>>>
>>>>> -----Original Message-----
>>>>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf
>>>>> Of Felix Polyudov
>>>>> Sent: Friday, May 10, 2019 6:41 AM
>>>>> To: devel at edk2.groups.io; 'lersek at redhat.com' <lersek at redhat.com>;
>>>>> Rodriguez, Christian <christian.rodriguez at intel.com>
>>>>> Cc: Feng, Bob C <bob.c.feng at intel.com>; Gao, Liming
>>>>> <liming.gao at intel.com>; Zhu, Yonghong <yonghong.zhu at intel.com>
>>>>> Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not
>>>>> mentioned in inf are not hashed
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On
>>>>>> Behalf Of Laszlo Ersek
>>>>>> Sent: Thursday, May 09, 2019 7:53 PM
>>>>>>
>>>>>> Hello Christian,
>>>>>>
>>>>>> On 05/09/19 23:27, Christian Rodriguez wrote:
>>>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1787
>>>>>>>
>>>>>>> Get a list of local header files that are not present in the
>>>>>>> MetaFile for this module. Add those local header files into the
>>>>>>> hashing algorithm for a module. If a local header file is not
>>>>>>> present in the MetaFile, the module will still build correctly
>>>>>>> though the hashing system didn't know about it before.
>>>>>>>
>>>>>>> Signed-off-by: Christian Rodriguez
>>>>>>> <christian.rodriguez at intel.com>
>>>>>>> Cc: Bob Feng <bob.c.feng at intel.com>
>>>>>>> Cc: Liming Gao <liming.gao at intel.com>
>>>>>>> Cc: Yonghong Zhu <yonghong.zhu at intel.com>
>>>>>>> ---
>>>>>>> BaseTools/Source/Python/AutoGen/AutoGen.py | 24
>>>>>>> ++++++++++++++++++++++++
>>>>>>> 1 file changed, 24 insertions(+)
>>>>>>
>>>>>> I saw the BZ soon after it was reported. I almost commented, but
>>>>>> ultimately I couldn't decide what the real use case was.
>>>>>>
>>>>>> With this particular use case (i.e. INF file is missing some
>>>>>> module-specific header files that it could easily list), I think
>>>>>> I disagree, mildly (not too strongly). E.g., we fixed such
>>>>>> omissions in a bunch of INF files, last March, in the series
>>>>>
>>>>> I agree with Lazlo.
>>>>> According to section 3.9 of the INF specification (https://edk2-
>>>>> docs.gitbooks.io/edk-ii-inf-
>>>>> specification/3_edk_ii_inf_file_format/39_[sources]_sections.html)
>>>>> , all source files (including module header files) must be listed
>>>>> in the [Sources] section.
>>>>> Here is the quote:
>>>>> "All HII Unicode format files must be listed in this section as
>>>>> well as any other "source" type file, such as local module header
>>>>> files, Vfr files,
>>> etc. "
>>>>>
>>>>> So, if file X is used by module Y, but is not listed in Y.inf,
>>>>> it's a violation of the INF spec., which makes it a bug that has to be fixed.
>>>>>
>>>>>
>>>>> Please consider the environment before printing this email.
>>>>>
>>>>> The information contained in this message may be confidential and
>>>>> proprietary to American Megatrends, Inc. This communication is
>>>>> intended to be read only by the individual or entity to whom it is
>>>>> addressed or by their designee. If the reader of this message is
>>>>> not the intended recipient, you are on notice that any
>>>>> distribution of this message, in any form, is strictly prohibited.
>>>>> Please promptly notify the sender by reply e-mail or by telephone
>>>>> at 770-246-8600, and
>>> then delete or destroy all copies of the transmission.
>>>>> l K q y e ,j a + U ?E e w Ӎ i vM *? ^ ,j N6 ˭y8b :) m ?
>>>>> 躛" }y M5 { j躓 z 'z h+ l ' r zm y 6 . Ȩ z 칷! +- 糊{^ &
>>>
>>> Please consider the environment before printing this email.
>>>
>>> The information contained in this message may be confidential and
>>> proprietary to American Megatrends, Inc. This communication is
>>> intended to be read only by the individual or entity to whom it is
>>> addressed or by their designee. If the reader of this message is not
>>> the intended recipient, you are on notice that any distribution of
>>> this message, in any form, is strictly prohibited. Please promptly
>>> notify the sender by reply e-mail or by telephone at 770-246-8600,
>>> and
>then delete or destroy all copies of the transmission.
>>> l K q y e ,j a + U ?E e w ӎ{ i vM *? ^ ,j N9 ˭y8b :) m ?
>>> 躛" }y M5 { j躓 z 'z h+ l ' r zm y 6 . Ȩ z 칷! +- 糊{^ &
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40566): https://edk2.groups.io/g/devel/message/40566
Mute This Topic: https://groups.io/mt/31570019/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list