Hi, Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Friday, December 14, 2018 9:56 PM > To: Ard Biesheuvel <[email protected]>; Fu, Siyuan > <[email protected]> > Cc: [email protected]; [email protected] > 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: > > [email protected]">http://mid.mail-archive.com/[email protected] > > > (b) What's more, my comments for the v3 version were summarily ignored > as well. See bullet (2) in: > > [email protected]">http://mid.mail-archive.com/[email protected] > > 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
