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

[Laszlo Ersek <lersek@redhat.com>]

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