Re: [PATCH 1/6] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.

[Laszlo Ersek <lersek@redhat.com>]

On 11/21/18 11:46, Laszlo Ersek wrote:
> On 11/21/18 06:28, Fu Siyuan wrote:
>> The "Network.dsc.inc" and "Network.fdf.inc" are added for platform owner to
>> easily enable/disable network stack support on their platform, by adding
>>   !include NetworkPkg/Network.dsc.inc
>> and
>>   !include NetworkPkg/Network.fdf.inc
>> to their platform DSC/FDF files.
>>
>> A set of flags can be changed before the include line or in build command
>> line ("-D FLAG=VALUE") to enable or disable related feature set.
>>
>> The default value of these flags are:
>>   DEFINE NETWORK_ENABLE     = TRUE
>>   DEFINE NETWORK_SNP_ENABLE = TRUE
>>   DEFINE NETWORK_IP4_ENABLE = TRUE
>>   DEFINE NETWORK_IP6_ENABLE = TRUE
>>   DEFINE NETWORK_TLS_ENABLE = TRUE
>>   DEFINE NETWORK_HTTP_BOOT_ENABLE       = TRUE
>>   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
>>   DEFINE NETWORK_IPSEC_ENABLE = TRUE
>>   DEFINE NETWORK_ISCSI_ENABLE = TRUE
>>   DEFINE NETWORK_VLAN_ENABLE  = TRUE
> 
> [...]

I'd like to review the ArmVirtPkg and OvmfPkg patches in detail once the
NetworkPkg patch stabilizes.

In addition, I have some higher-level comments. This patch series
effectively renames a few preexistent build flags, such as
"HTTP_BOOT_ENABLE" to "NETWORK_HTTP_BOOT_ENABLE", "TLS_ENABLE" to
"NETWORK_TLS_ENABLE", and so on.

- Please grep the edk2 source tree for the original (disappearing)
flags, and update them as necessary, not just in DSC/FDF files, but also
in documentation. For example, "HTTP_BOOT_ENABLE" and "TLS_ENABLE" are
mentioned in "OvmfPkg/README".

- Please also grep the edk2 Wiki for the same (clone it locally and then
run "git grep"). For example, the
"Testing-SMM-with-QEMU,-KVM-and-libvirt.md" article also refers to
HTTP_BOOT_ENABLE and TLS_ENABLE.

- It is likely a good idea to list this change (that is,
<https://bugzilla.tianocore.org/show_bug.cgi?id=1293>), under
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#proposed-features-1>.
I see that #1278 is listed already, but the build flag renaming is an
incompatible change, and it deserves a mention.

Thanks!
Laszlo