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

["Gao, Liming" <liming.gao@intel.com>]

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