[edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed
Christian Rodriguez
christian.rodriguez at intel.com
Mon May 13 19:39:14 UTC 2019
So the direction put forward by my team is that we should raise an error instead of a warning because a false positive successful build would be detrimental to a continuous integration environment. And it would also enforce the Spec requirements.
The point they are making is that a warning does not emphasizes that there is a false successful build as much as breaking the build. And for now it will be at a limited scope only with the hashing feature enabled so it won't catch people doing a normal build by surprise.
Thanks,
Christian
>-----Original Message-----
>From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
>Christian Rodriguez
>Sent: Monday, May 13, 2019 11:54 AM
>To: Laszlo Ersek <lersek at redhat.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
>
>I think a warning would be reasonable.
>
>I only mention the spec because it requires all headers to be in the sources
>section of the inf, but it's not enforced strictly by BaseTools. Though the
>hashing feature relies on this requirement. It not a big deal, I just wanted to
>make sure false positive build successes were addressed.
>
>Thanks,
>Christian
>
>>-----Original Message-----
>>From: Laszlo Ersek [mailto:lersek at redhat.com]
>>Sent: Monday, May 13, 2019 4:39 AM
>>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 (#40535): https://edk2.groups.io/g/devel/message/40535
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