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

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

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Monday, May 6, 2019 10:28 PM
> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [Patch v4 3/3] NetworkPkg: Add package level include DSC file
> 
> 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.
> 
Yes. After confirm with Jiaxin or Siyuan, I will send v5 version path. Thanks for your comments. 

> 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
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >
> >
> >
> >
> 
> 
>