diff mbox series

[edk2] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally

Message ID 20181214112253.10372-1-ard.biesheuvel@linaro.org
State Accepted
Commit a9ff32909b47b1a1c9a02f21113c615552f6a043
Headers show
Series [edk2] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally | expand

Commit Message

Ard Biesheuvel Dec. 14, 2018, 11:22 a.m. UTC 
Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers
from platform DSC/FDF") failed to take into account that the now
unconditionally included IScsiDxe.inf from NetworkPkg requires a
resolution for TcpIoLib. Since specifying such a resolution is harmless
for platforms that have no networking enabled, let's just fix things
by dropping the conditionals around it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/ArmVirt.dsc.inc | 2 --
 1 file changed, 2 deletions(-)

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Dec. 14, 2018, 1:56 p.m. UTC | #1 
On 12/14/18 12:22, Ard Biesheuvel wrote:
> Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers

> from platform DSC/FDF") failed to take into account that the now

> unconditionally included IScsiDxe.inf from NetworkPkg requires a

> resolution for TcpIoLib.


I don't understand why this happened.


(a) I warned *precisely* about this issue, when I reviewed the v2
version of said commit. See bullet (5) in the following message:

http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com


(b) What's more, my comments for the v3 version were summarily ignored
as well. See bullet (2) in:

http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com

And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for
[LibraryClasses.common.UEFI_DRIVER] have been added to
"ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and my
having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses
"OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is
perfectly sufficient.


Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact.
The v3 patch was *not* ready for being pushed, to my eyes. And I was
pretty explicit about that.


> Since specifying such a resolution is harmless

> for platforms that have no networking enabled, let's just fix things

> by dropping the conditionals around it.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/ArmVirt.dsc.inc | 2 --

>  1 file changed, 2 deletions(-)

> 

> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc

> index c3549c84d4c6..89c2db074711 100644

> --- a/ArmVirtPkg/ArmVirt.dsc.inc

> +++ b/ArmVirtPkg/ArmVirt.dsc.inc

> @@ -80,9 +80,7 @@

>    DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf

>    UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf

>    IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf

> -!if $(NETWORK_IP6_ENABLE) == TRUE

>    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf

> -!endif

>  !if $(HTTP_BOOT_ENABLE) == TRUE

>    HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf

>  !endif

> 


I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly
ignoring my explicit requests for updates. However, that would only
result in my having to review more (possibly incomplete) iterations of
the patch.

At least, this incremental fix is in line with my request in (a) -- "we
should make the current TcpIoLib class resolution unconditional". Please
go ahead and push it.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


I should really file a TianoCore BZ about the wrong / redundant
OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for
me to find te motivation for that right now, seeing the disregard for my
earlier reviews.

Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Dec. 14, 2018, 2:03 p.m. UTC | #2 
On Fri, 14 Dec 2018 at 14:56, Laszlo Ersek <lersek@redhat.com> wrote:
>

> On 12/14/18 12:22, Ard Biesheuvel wrote:

> > Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers

> > from platform DSC/FDF") failed to take into account that the now

> > unconditionally included IScsiDxe.inf from NetworkPkg requires a

> > resolution for TcpIoLib.

>

> I don't understand why this happened.

>

>

> (a) I warned *precisely* about this issue, when I reviewed the v2

> version of said commit. See bullet (5) in the following message:

>

> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com

>

>

> (b) What's more, my comments for the v3 version were summarily ignored

> as well. See bullet (2) in:

>

> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com

>

> And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for

> [LibraryClasses.common.UEFI_DRIVER] have been added to

> "ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and my

> having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses

> "OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is

> perfectly sufficient.

>

>

> Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact.

> The v3 patch was *not* ready for being pushed, to my eyes. And I was

> pretty explicit about that.

>

>

> > Since specifying such a resolution is harmless

> > for platforms that have no networking enabled, let's just fix things

> > by dropping the conditionals around it.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  ArmVirtPkg/ArmVirt.dsc.inc | 2 --

> >  1 file changed, 2 deletions(-)

> >

> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc

> > index c3549c84d4c6..89c2db074711 100644

> > --- a/ArmVirtPkg/ArmVirt.dsc.inc

> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc

> > @@ -80,9 +80,7 @@

> >    DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf

> >    UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf

> >    IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf

> > -!if $(NETWORK_IP6_ENABLE) == TRUE

> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf

> > -!endif

> >  !if $(HTTP_BOOT_ENABLE) == TRUE

> >    HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf

> >  !endif

> >

>

> I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly

> ignoring my explicit requests for updates. However, that would only

> result in my having to review more (possibly incomplete) iterations of

> the patch.

>

> At least, this incremental fix is in line with my request in (a) -- "we

> should make the current TcpIoLib class resolution unconditional". Please

> go ahead and push it.

>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>


Thanks. Apologies for not looking more carefully whether the feedback
had in fact been incorporated before giving my R-b

Pushed as 9a67ba261fe9..a9ff32909b47

> I should really file a TianoCore BZ about the wrong / redundant

> OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for

> me to find te motivation for that right now, seeing the disregard for my

> earlier reviews.

>


I assumed [incorrectly, I suppose] that people actually build test
platforms that they modify, but apparently not :-)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Fu, Siyuan Dec. 17, 2018, 1:50 a.m. UTC | #3 
Hi, Laszlo

> -----Original Message-----

> From: Laszlo Ersek [mailto:lersek@redhat.com]

> Sent: Friday, December 14, 2018 9:56 PM

> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Fu, Siyuan

> <siyuan.fu@intel.com>

> Cc: edk2-devel@lists.01.org; julien.grall@linaro.org

> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution

> unconditionally

> 

> On 12/14/18 12:22, Ard Biesheuvel wrote:

> > Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers

> > from platform DSC/FDF") failed to take into account that the now

> > unconditionally included IScsiDxe.inf from NetworkPkg requires a

> > resolution for TcpIoLib.

> 

> I don't understand why this happened.

> 

> 

> (a) I warned *precisely* about this issue, when I reviewed the v2

> version of said commit. See bullet (5) in the following message:

> 

> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com

> 

> 

> (b) What's more, my comments for the v3 version were summarily ignored

> as well. See bullet (2) in:

> 

> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com

> 

> And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for

> [LibraryClasses.common.UEFI_DRIVER] have been added to

> "ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and my

> having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses

> "OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is

> perfectly sufficient.

> 

> 

> Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact.

> The v3 patch was *not* ready for being pushed, to my eyes. And I was

> pretty explicit about that.

> 

> 

> > Since specifying such a resolution is harmless

> > for platforms that have no networking enabled, let's just fix things

> > by dropping the conditionals around it.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  ArmVirtPkg/ArmVirt.dsc.inc | 2 --

> >  1 file changed, 2 deletions(-)

> >

> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc

> > index c3549c84d4c6..89c2db074711 100644

> > --- a/ArmVirtPkg/ArmVirt.dsc.inc

> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc

> > @@ -80,9 +80,7 @@

> >    DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf

> >    UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf

> >    IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf

> > -!if $(NETWORK_IP6_ENABLE) == TRUE

> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf

> > -!endif

> >  !if $(HTTP_BOOT_ENABLE) == TRUE

> >    HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf

> >  !endif

> >

> 

> I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly

> ignoring my explicit requests for updates. However, that would only

> result in my having to review more (possibly incomplete) iterations of

> the patch.

> 

> At least, this incremental fix is in line with my request in (a) -- "we

> should make the current TcpIoLib class resolution unconditional". Please

> go ahead and push it.

> 

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> 

> I should really file a TianoCore BZ about the wrong / redundant

> OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for

> me to find te motivation for that right now, seeing the disregard for my

> earlier reviews.

> 

> Laszlo


My apologies for missed your review email of the v3 patch and pushed the
changes. The original patch set was made one month ago and I didn't carefully
checked all the review feedback emails when I started to work on it again
in the last week.

I have created a new patch upon this fix to remove the redundant libraries
and adjust driver order, please check this patch mail.
https://lists.01.org/pipermail/edk2-devel/2018-December/034070.html

And I'm also sorry that this new patch are also not tested for build, I tried
to search the wiki patch for setting up the ARM build toolchain on my windows
OS but failed to make it. I will appreciate if you can help to provide a guide
or any link for ARM package build on windows machine, so I could test my patch
in future.

Best Regards,
Siyuan

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Dec. 17, 2018, 3:06 a.m. UTC | #4 
Siyuan:
  I know the windows GCC pre built binary can be downloaded from https://sourceforge.net/projects/edk2developertoolsforwindows/files/Tool%20Chain%20Binaries/. Although pre-built binary GCC is three years ago, they can still work. For example, after you download gcc492 arm, then you can set GCC49_ARM_PREFIX and GCC49_DLL to point ARM GCC path, then build ARM platform with -a ARM arch and -t GCC49 tool chain. Below is my setting:
     set GCC49_ARM_PREFIX=D:\Tools\GCC\gcc492-arm\bin\
     set GCC49_DLL=D:\Tools\GCC\gcc492-arm\dll\

Thanks
Liming
>-----Original Message-----

>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu,

>Siyuan

>Sent: Monday, December 17, 2018 9:51 AM

>To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel

><ard.biesheuvel@linaro.org>

>Cc: edk2-devel@lists.01.org

>Subject: Re: [edk2] [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib

>resolution unconditionally

>

>Hi, Laszlo

>

>> -----Original Message-----

>> From: Laszlo Ersek [mailto:lersek@redhat.com]

>> Sent: Friday, December 14, 2018 9:56 PM

>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Fu, Siyuan

>> <siyuan.fu@intel.com>

>> Cc: edk2-devel@lists.01.org; julien.grall@linaro.org

>> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution

>> unconditionally

>>

>> On 12/14/18 12:22, Ard Biesheuvel wrote:

>> > Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers

>> > from platform DSC/FDF") failed to take into account that the now

>> > unconditionally included IScsiDxe.inf from NetworkPkg requires a

>> > resolution for TcpIoLib.

>>

>> I don't understand why this happened.

>>

>>

>> (a) I warned *precisely* about this issue, when I reviewed the v2

>> version of said commit. See bullet (5) in the following message:

>>

>> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-

>58eb4eb2afbd@redhat.com

>>

>>

>> (b) What's more, my comments for the v3 version were summarily ignored

>> as well. See bullet (2) in:

>>

>> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-

>1be98cee4340@redhat.com

>>

>> And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for

>> [LibraryClasses.common.UEFI_DRIVER] have been added to

>> "ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and

>my

>> having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses

>> "OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is

>> perfectly sufficient.

>>

>>

>> Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact.

>> The v3 patch was *not* ready for being pushed, to my eyes. And I was

>> pretty explicit about that.

>>

>>

>> > Since specifying such a resolution is harmless

>> > for platforms that have no networking enabled, let's just fix things

>> > by dropping the conditionals around it.

>> >

>> > Contributed-under: TianoCore Contribution Agreement 1.1

>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> > ---

>> >  ArmVirtPkg/ArmVirt.dsc.inc | 2 --

>> >  1 file changed, 2 deletions(-)

>> >

>> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc

>> > index c3549c84d4c6..89c2db074711 100644

>> > --- a/ArmVirtPkg/ArmVirt.dsc.inc

>> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc

>> > @@ -80,9 +80,7 @@

>> >    DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf

>> >    UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf

>> >    IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf

>> > -!if $(NETWORK_IP6_ENABLE) == TRUE

>> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf

>> > -!endif

>> >  !if $(HTTP_BOOT_ENABLE) == TRUE

>> >    HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf

>> >  !endif

>> >

>>

>> I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly

>> ignoring my explicit requests for updates. However, that would only

>> result in my having to review more (possibly incomplete) iterations of

>> the patch.

>>

>> At least, this incremental fix is in line with my request in (a) -- "we

>> should make the current TcpIoLib class resolution unconditional". Please

>> go ahead and push it.

>>

>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>>

>> I should really file a TianoCore BZ about the wrong / redundant

>> OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for

>> me to find te motivation for that right now, seeing the disregard for my

>> earlier reviews.

>>

>> Laszlo

>

>My apologies for missed your review email of the v3 patch and pushed the

>changes. The original patch set was made one month ago and I didn't carefully

>checked all the review feedback emails when I started to work on it again

>in the last week.

>

>I have created a new patch upon this fix to remove the redundant libraries

>and adjust driver order, please check this patch mail.

>https://lists.01.org/pipermail/edk2-devel/2018-December/034070.html

>

>And I'm also sorry that this new patch are also not tested for build, I tried

>to search the wiki patch for setting up the ARM build toolchain on my

>windows

>OS but failed to make it. I will appreciate if you can help to provide a guide

>or any link for ARM package build on windows machine, so I could test my

>patch

>in future.

>

>Best Regards,

>Siyuan

>

>_______________________________________________

>edk2-devel mailing list

>edk2-devel@lists.01.org

>https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Fu, Siyuan Dec. 17, 2018, 3:22 a.m. UTC | #5 
Thanks Liming, I will try it.

BestRegards
Fu Siyuan

> -----Original Message-----

> From: Gao, Liming

> Sent: Monday, December 17, 2018 11:07 AM

> To: Fu, Siyuan <siyuan.fu@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard

> Biesheuvel <ard.biesheuvel@linaro.org>

> Cc: edk2-devel@lists.01.org

> Subject: RE: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution

> unconditionally

> 

> Siyuan:

>   I know the windows GCC pre built binary can be downloaded from

> https://sourceforge.net/projects/edk2developertoolsforwindows/files/Tool%20Cha

> in%20Binaries/. Although pre-built binary GCC is three years ago, they can

> still work. For example, after you download gcc492 arm, then you can set

> GCC49_ARM_PREFIX and GCC49_DLL to point ARM GCC path, then build ARM platform

> with -a ARM arch and -t GCC49 tool chain. Below is my setting:

>      set GCC49_ARM_PREFIX=D:\Tools\GCC\gcc492-arm\bin\

>      set GCC49_DLL=D:\Tools\GCC\gcc492-arm\dll\

> 

> Thanks

> Liming

> >-----Original Message-----

> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu,

> >Siyuan

> >Sent: Monday, December 17, 2018 9:51 AM

> >To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel

> ><ard.biesheuvel@linaro.org>

> >Cc: edk2-devel@lists.01.org

> >Subject: Re: [edk2] [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib

> >resolution unconditionally

> >

> >Hi, Laszlo

> >

> >> -----Original Message-----

> >> From: Laszlo Ersek [mailto:lersek@redhat.com]

> >> Sent: Friday, December 14, 2018 9:56 PM

> >> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Fu, Siyuan

> >> <siyuan.fu@intel.com>

> >> Cc: edk2-devel@lists.01.org; julien.grall@linaro.org

> >> Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution

> >> unconditionally

> >>

> >> On 12/14/18 12:22, Ard Biesheuvel wrote:

> >> > Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers

> >> > from platform DSC/FDF") failed to take into account that the now

> >> > unconditionally included IScsiDxe.inf from NetworkPkg requires a

> >> > resolution for TcpIoLib.

> >>

> >> I don't understand why this happened.

> >>

> >>

> >> (a) I warned *precisely* about this issue, when I reviewed the v2

> >> version of said commit. See bullet (5) in the following message:

> >>

> >> http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-

> >58eb4eb2afbd@redhat.com

> >>

> >>

> >> (b) What's more, my comments for the v3 version were summarily ignored

> >> as well. See bullet (2) in:

> >>

> >> http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-

> >1be98cee4340@redhat.com

> >>

> >> And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for

> >> [LibraryClasses.common.UEFI_DRIVER] have been added to

> >> "ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and

> >my

> >> having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses

> >> "OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is

> >> perfectly sufficient.

> >>

> >>

> >> Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact.

> >> The v3 patch was *not* ready for being pushed, to my eyes. And I was

> >> pretty explicit about that.

> >>

> >>

> >> > Since specifying such a resolution is harmless

> >> > for platforms that have no networking enabled, let's just fix things

> >> > by dropping the conditionals around it.

> >> >

> >> > Contributed-under: TianoCore Contribution Agreement 1.1

> >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> > ---

> >> >  ArmVirtPkg/ArmVirt.dsc.inc | 2 --

> >> >  1 file changed, 2 deletions(-)

> >> >

> >> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc

> >> > index c3549c84d4c6..89c2db074711 100644

> >> > --- a/ArmVirtPkg/ArmVirt.dsc.inc

> >> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc

> >> > @@ -80,9 +80,7 @@

> >> >    DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf

> >> >    UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf

> >> >    IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf

> >> > -!if $(NETWORK_IP6_ENABLE) == TRUE

> >> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf

> >> > -!endif

> >> >  !if $(HTTP_BOOT_ENABLE) == TRUE

> >> >    HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf

> >> >  !endif

> >> >

> >>

> >> I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly

> >> ignoring my explicit requests for updates. However, that would only

> >> result in my having to review more (possibly incomplete) iterations of

> >> the patch.

> >>

> >> At least, this incremental fix is in line with my request in (a) -- "we

> >> should make the current TcpIoLib class resolution unconditional". Please

> >> go ahead and push it.

> >>

> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> >>

> >> I should really file a TianoCore BZ about the wrong / redundant

> >> OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for

> >> me to find te motivation for that right now, seeing the disregard for my

> >> earlier reviews.

> >>

> >> Laszlo

> >

> >My apologies for missed your review email of the v3 patch and pushed the

> >changes. The original patch set was made one month ago and I didn't carefully

> >checked all the review feedback emails when I started to work on it again

> >in the last week.

> >

> >I have created a new patch upon this fix to remove the redundant libraries

> >and adjust driver order, please check this patch mail.

> >https://lists.01.org/pipermail/edk2-devel/2018-December/034070.html

> >

> >And I'm also sorry that this new patch are also not tested for build, I tried

> >to search the wiki patch for setting up the ARM build toolchain on my

> >windows

> >OS but failed to make it. I will appreciate if you can help to provide a

> guide

> >or any link for ARM package build on windows machine, so I could test my

> >patch

> >in future.

> >

> >Best Regards,

> >Siyuan

> >

> >_______________________________________________

> >edk2-devel mailing list

> >edk2-devel@lists.01.org

> >https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index c3549c84d4c6..89c2db074711 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -80,9 +80,7 @@ 
   DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf
   UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf
   IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf
-!if $(NETWORK_IP6_ENABLE) == TRUE
   TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
-!endif
 !if $(HTTP_BOOT_ENABLE) == TRUE
   HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf
 !endif