Re: [edk2-devel] [Patch v4 3/3] NetworkPkg: Add package level include DSC file

["Laszlo Ersek" <lersek@redhat.com>]

On 05/06/19 15:24, Liming Gao wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, May 6, 2019 7:54 PM
>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [Patch v4 3/3] NetworkPkg: Add package level include DSC file
>>
>> On 05/05/19 16:53, Gao, Liming wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
>>>> Sent: Tuesday, April 30, 2019 10:42 PM
>>>> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
>>>> Subject: Re: [edk2-devel] [Patch v4 3/3] NetworkPkg: Add package level include DSC file
>>>>
>>>> Hi Liming,
>>>>
>>>> please read my email until my signature below:
>>>>
>>>> On 04/29/19 16:50, Liming Gao wrote:
>>>>> Platform DSC can include Network.dsc.inc to enable network features.
>>>>>
>>>>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>>>>> ---
>>>>>  NetworkPkg/Network.dsc.inc | 40 ++++++++++++++++++++++++++++++++++++++++
>>>>>  NetworkPkg/NetworkPkg.dsc  | 22 +---------------------
>>>>>  2 files changed, 41 insertions(+), 21 deletions(-)
>>>>>  create mode 100644 NetworkPkg/Network.dsc.inc
>>>>>
>>>>> diff --git a/NetworkPkg/Network.dsc.inc b/NetworkPkg/Network.dsc.inc
>>>>> new file mode 100644
>>>>> index 0000000000..d51ee1b040
>>>>> --- /dev/null
>>>>> +++ b/NetworkPkg/Network.dsc.inc
>>>>> @@ -0,0 +1,40 @@
>>>>> +## @file
>>>>> +# Network DSC include file for Platform DSC
>>>>> +#
>>>>> +# This file includes all required information to enable Network features.
>>>>> +# It can be included to a platform DSC file by using "!include NetworkPkg/Network.dsc.inc".
>>>>> +#
>>>>> +# This file defines one build flag PLATFORMX64_ENABLE to support
>>>>> +# IA32 PEI and X64 DXE platform. Its default value is FALSE.
>>>>> +#
>>>>> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>>>> +#
>>>>> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>> +#
>>>>> +##
>>>>> +
>>>>> +[Defines]
>>>>> +!include NetworkPkg/NetworkDefines.dsc.inc
>>>>> +
>>>>> +!ifndef PLATFORMX64_ENABLE
>>>>> +  #
>>>>> +  # PLATFORMX64_ENABLE is set to TRUE when PEI is IA32 and DXE is X64 platform
>>>>> +  #
>>>>> +  DEFINE PLATFORMX64_ENABLE = FALSE
>>>>> +!endif
>>>>> +
>>>>> +[PcdsFixedAtBuild]
>>>>> +!include NetworkPkg/NetworkPcds.dsc.inc
>>>>> +
>>>>> +[LibraryClasses]
>>>>> +!include NetworkPkg/NetworkLibs.dsc.inc
>>>>> +
>>>>> +!if $(PLATFORMX64_ENABLE) == TRUE
>>>>> +[Components.X64]
>>>>> +!include NetworkPkg/NetworkComponents.dsc.inc
>>>>> +
>>>>> +!else
>>>>> +[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
>>>>> +!include NetworkPkg/NetworkComponents.dsc.inc
>>>>> +
>>>>> +!endif
>>>>
>>>> My point (9) in <https://edk2.groups.io/g/devel/message/39770> was
>>>> PLATFORMX64_ENABLE should be dropped completely, and we should use the
>>>> following instead:
>>>>
>>>> !if ("X64" in $(ARCH))
>>>> [Components.X64]
>>>> !include NetworkPkg/NetworkComponents.dsc.inc
>>>> !else
>>>> [Components.IA32, Components.ARM, Components.AARCH64]
>>>> !include NetworkPkg/NetworkComponents.dsc.inc
>>>> !endif
>>>>
>>>> I don't feel strongly about this patch, but I'd still like to hear your
>>>> opinion on this approach.
>>> This solution can't handle NetworkPkg.dsc. NetworkPkg.dsc includes IA32, X64, ARM and AARCH64.
>>> With this solution, NetworkPkg.dsc will only build drivers for X64 arch.
>>
>> How so? If you run "build -a ARM -p NetworkPkg/NetworkPkg.dsc ...", then
>> the second branch will be selected (the "in" operator will evaluate to
>> false), and the second branch !includes "NetworkComponents.dsc.inc" in
>> [Components.ARM].
>>
>> Can you provide an example command line where the above would break?
>>
>> Do you mean "build -a ARM -a X64 -p NetworkPkg/NetworkPkg.dsc ..."?
> 
> Yes. I usually build -p NetworkPkg/NetworkPkg.dsc -a IA32 -a X64 and make sure the modules are built both for IA32 and X64. 

OK, that certainly makes sense. A multi-arch build such as "-a IA32 -a
X64" may have a different meaning dependent on the presence of an FDF
file reference in the DSC file. I agree a separate flag is useful for
distinguishing IA32 PEI + X64 DXE special case from the rest of the
multi-arch builds.

Other than that: as I suggested under patch v3 2/3, point (11), I think
we should include TlsLib in "NetworkLibs.dsc.inc", and then we can
remove the TlsLib resolution too, from "NetworkPkg.dsc". I guess we
should await Jiaxin's and Siyuan's feedback on that.

Either way, for this patch:

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

Thanks!
Laszlo

>>
>> Thanks
>> Laszlo
>>
>>> But, we expect NetworkPkg.dsc
>>> build the drivers for each ARCH.
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>> diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
>>>>> index be9d9248e4..fe2fcf7b3c 100644
>>>>> --- a/NetworkPkg/NetworkPkg.dsc
>>>>> +++ b/NetworkPkg/NetworkPkg.dsc
>>>>> @@ -41,12 +41,6 @@
>>>>>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>>>>>    SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>>>>>
>>>>> -  DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf
>>>>> -  NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
>>>>> -  IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf
>>>>> -  UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf
>>>>> -  TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
>>>>> -  HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf
>>>>>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>>>>    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
>>>>>    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>>>>> @@ -103,24 +97,10 @@
>>>>>  ###################################################################################################
>>>>>
>>>>>  [Components]
>>>>> -  NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>>>>> -  NetworkPkg/TcpDxe/TcpDxe.inf
>>>>> -  NetworkPkg/Udp6Dxe/Udp6Dxe.inf
>>>>> -  NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
>>>>> -  NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
>>>>> -  NetworkPkg/DnsDxe/DnsDxe.inf
>>>>> -  NetworkPkg/HttpDxe/HttpDxe.inf
>>>>> -  NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
>>>>> -  NetworkPkg/HttpBootDxe/HttpBootDxe.inf
>>>>>    NetworkPkg/WifiConnectionManagerDxe/WifiConnectionManagerDxe.inf
>>>>> -
>>>>>    NetworkPkg/Application/VConfig/VConfig.inf
>>>>>
>>>>> -[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
>>>>> -  NetworkPkg/IScsiDxe/IScsiDxe.inf
>>>>> -  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
>>>>> -  NetworkPkg/TlsDxe/TlsDxe.inf
>>>>> -  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
>>>>> +  !include NetworkPkg/Network.dsc.inc
>>>>>
>>>>>  [BuildOptions]
>>>>>    *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
>>>>>
>>>>
>>>>
>>>>
>>>
> 
> 
> 
>