From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 13 May 2019 13:19:40 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B7FED30832D1; Mon, 13 May 2019 20:19:39 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-237.rdu2.redhat.com [10.10.123.237]) by smtp.corp.redhat.com (Postfix) with ESMTP id 02A641001DC2; Mon, 13 May 2019 20:19:37 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not mentioned in inf are not hashed To: devel@edk2.groups.io, christian.rodriguez@intel.com, "felixp@ami.com" Cc: "Feng, Bob C" , "Gao, Liming" , "Zhu, Yonghong" References: <20190509212719.6156-1-christian.rodriguez@intel.com> <9333E191E0D52B4999CE63A99BA663A00302C89341@atlms1.us.megatrends.com> <3A7DCC9A944C6149BF832E1C9B718ABC01EDA3B3@ORSMSX112.amr.corp.intel.com> <9333E191E0D52B4999CE63A99BA663A00302C89592@atlms1.us.megatrends.com> <3A7DCC9A944C6149BF832E1C9B718ABC01EDA3F1@ORSMSX112.amr.corp.intel.com> <3A7DCC9A944C6149BF832E1C9B718ABC01F1B988@ORSMSX112.amr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 13 May 2019 22:19:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3A7DCC9A944C6149BF832E1C9B718ABC01F1B988@ORSMSX112.amr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Mon, 13 May 2019 20:19:39 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 05/13/19 20:53, Christian Rodriguez wrote: > I think a warning would be reasonable. >=20 > I only mention the spec because it requires all headers to be in the sou= rces section of the inf, That could be required by the edk2 INF spec, yes. It's totally irrelevant for the UEFI spec however. (Originally you wrote, "[...] This would force users of the hash feature to be UEFI spec complaint [...]".) Laszlo > 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 f= alse positive build successes were addressed. >=20 > Thanks, > Christian >=20 >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, May 13, 2019 4:39 AM >> To: Rodriguez, Christian ; >> devel@edk2.groups.io; felixp@ami.com >> Cc: Feng, Bob C ; Gao, Liming >> ; Zhu, Yonghong >> Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not mentio= ned >> 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. T= hat's >> why the initial patch was a workaround of sorts because it is allowed b= y >> 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 pr= oject >> 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 catc= h >> 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@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >>>> Felix Polyudov >>>> Sent: Friday, May 10, 2019 12:32 PM >>>> To: Rodriguez, Christian ; >>>> devel@edk2.groups.io; 'lersek@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 >>>> >>>> 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@intel.com] >>>>> Sent: Friday, May 10, 2019 3:14 PM >>>>> To: devel@edk2.groups.io; Felix Polyudov; 'lersek@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@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf >>>>>> Of Felix Polyudov >>>>>> Sent: Friday, May 10, 2019 6:41 AM >>>>>> To: devel@edk2.groups.io; 'lersek@redhat.com' ; >>>>>> Rodriguez, Christian >>>>>> Cc: Feng, Bob C ; Gao, Liming >>>>>> ; Zhu, Yonghong >>>>>> Subject: Re: [edk2-devel] [PATCH] BaseTools: Include headers not >>>>>> mentioned in inf are not hashed >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: devel@edk2.groups.io [mailto:devel@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=3D1787 >>>>>>>> >>>>>>>> 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 >>>>>>>> >>>>>>>> Cc: Bob Feng >>>>>>>> Cc: Liming Gao >>>>>>>> Cc: Yonghong Zhu >>>>>>>> --- >>>>>>>> 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 f= ixed. >>>>>> >>>>>> >>>>>> 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. >>>>>> =01 l K=18 q y e ,j a + U ?E e w =D3=8D i vM = *? ^ =7F ,j N6 =CB=ADy8b :) m ? >>>>>> =E8=BA=9B" }y M5 { =DE=B7 j=E8=BA=93 z 'z h+ l ' r = z=1Bm y 6 . =C8=A8 =0F z =EC=B9=B7! +- =E7=B3=8A{^ & >>>> >>>> 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, an= d >> then delete or destroy all copies of the transmission. >>>> =01 l K=18 q y e ,j a + U ?E e w =D3=8E{ i vM = *? ^ =7F ,j N9 =CB=ADy8b :) m ? >>>> =E8=BA=9B" }y M5 { =DE=B7 j=E8=BA=93 z 'z h+ l ' r z= = =1Bm y 6 . =C8=A8 =0F z =EC=B9=B7! +- =E7=B3=8A{^ & >=20 >=20 >=20 >=20