* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-12 20:43 [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants Rebecca Cran
@ 2021-06-12 23:22 ` Peter Grehan
2021-06-16 15:58 ` Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 0 replies; 24+ messages in thread
From: Peter Grehan @ 2021-06-12 23:22 UTC (permalink / raw)
To: devel, rebecca
On 6/13/21 6:43 AM, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
>
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> 2 files changed, 79 deletions(-)
>
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d8792812ab..cbf896e89b 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -31,8 +31,6 @@
> DEFINE SECURE_BOOT_ENABLE = FALSE
> DEFINE SMM_REQUIRE = FALSE
> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> - DEFINE TPM_ENABLE = FALSE
> - DEFINE TPM_CONFIG_ENABLE = FALSE
>
> #
> # Network definition
> @@ -221,16 +219,8 @@
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>
> -
> -!if $(TPM_ENABLE) == TRUE
> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> -!else
> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> -!endif
>
> [LibraryClasses.common]
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -292,11 +282,6 @@
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>
> -!if $(TPM_ENABLE) == TRUE
> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> -!endif
> -
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>
> [LibraryClasses.common.DXE_CORE]
> @@ -366,9 +351,6 @@
> !endif
> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> -!if $(TPM_ENABLE) == TRUE
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> -!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -563,22 +545,12 @@
>
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>
> -!if $(TPM_ENABLE) == TRUE
> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> -!endif
> -
> # MdeModulePkg resolution sets up the system display resolution
> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>
> -[PcdsDynamicHii]
> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> - gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -!endif
> -
> ################################################################################
> #
> # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -618,19 +590,6 @@
> <LibraryClasses>
> }
>
> -!if $(TPM_ENABLE) == TRUE
> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> - <LibraryClasses>
> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> - }
> -!endif
> -
> #
> # DXE Phase modules
> #
> @@ -653,9 +612,6 @@
> <LibraryClasses>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -!endif
> -!if $(TPM_ENABLE) == TRUE
> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> !endif
> }
>
> @@ -841,23 +797,3 @@
> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> }
>
> -
> - #
> - # TPM support
> - #
> -!if $(TPM_ENABLE) == TRUE
> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> - <LibraryClasses>
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> - }
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 3eff36dac1..fbd63a395a 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> !endif
>
> -!if $(TPM_ENABLE) == TRUE
> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> -!endif
> -
> ################################################################################
>
> [FV.DXEFV]
> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> !endif
>
> -#
> -# TPM support
> -#
> -!if $(TPM_ENABLE) == TRUE
> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> -
> ################################################################################
>
> [FV.FVMAIN_COMPACT]
>
Reviewed-by: Peter Grehan <grehan@freebsd.org>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-12 20:43 [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants Rebecca Cran
2021-06-12 23:22 ` [edk2-devel] " Peter Grehan
@ 2021-06-16 15:58 ` Ard Biesheuvel
2021-06-16 19:00 ` [edk2-devel] " Sean
2021-06-22 17:57 ` Laszlo Ersek
` (2 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2021-06-16 15:58 UTC (permalink / raw)
To: Rebecca Cran, Michael Kinney, Sean Brogan
Cc: Peter Grehan, Laszlo Ersek, Ard Biesheuvel, Jordan Justen,
edk2-devel-groups-io
(+ Sean, Mike)
On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
>
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
>
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Strangely enough, this patch gets rejected by PatchCheck for lack of a
Signed-off-by line
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
The patch itself looks good to me
Acked-by: Ard Biesheuvel <ardb@kernel.org>
so if anyone else manages to fix the CI issue, feel free to push the
patch with my R-b (and Peter's, given in reply to this message)
(I will go offline for 3 weeks after Friday)
> ---
> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> 2 files changed, 79 deletions(-)
>
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d8792812ab..cbf896e89b 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -31,8 +31,6 @@
> DEFINE SECURE_BOOT_ENABLE = FALSE
> DEFINE SMM_REQUIRE = FALSE
> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> - DEFINE TPM_ENABLE = FALSE
> - DEFINE TPM_CONFIG_ENABLE = FALSE
>
> #
> # Network definition
> @@ -221,16 +219,8 @@
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>
> -
> -!if $(TPM_ENABLE) == TRUE
> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> -!else
> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> -!endif
>
> [LibraryClasses.common]
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -292,11 +282,6 @@
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>
> -!if $(TPM_ENABLE) == TRUE
> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> -!endif
> -
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>
> [LibraryClasses.common.DXE_CORE]
> @@ -366,9 +351,6 @@
> !endif
> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> -!if $(TPM_ENABLE) == TRUE
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> -!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -563,22 +545,12 @@
>
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>
> -!if $(TPM_ENABLE) == TRUE
> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> -!endif
> -
> # MdeModulePkg resolution sets up the system display resolution
> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>
> -[PcdsDynamicHii]
> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> - gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -!endif
> -
> ################################################################################
> #
> # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -618,19 +590,6 @@
> <LibraryClasses>
> }
>
> -!if $(TPM_ENABLE) == TRUE
> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> - <LibraryClasses>
> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> - }
> -!endif
> -
> #
> # DXE Phase modules
> #
> @@ -653,9 +612,6 @@
> <LibraryClasses>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -!endif
> -!if $(TPM_ENABLE) == TRUE
> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> !endif
> }
>
> @@ -841,23 +797,3 @@
> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> }
>
> -
> - #
> - # TPM support
> - #
> -!if $(TPM_ENABLE) == TRUE
> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> - <LibraryClasses>
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> - }
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 3eff36dac1..fbd63a395a 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> !endif
>
> -!if $(TPM_ENABLE) == TRUE
> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> -!endif
> -
> ################################################################################
>
> [FV.DXEFV]
> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> !endif
>
> -#
> -# TPM support
> -#
> -!if $(TPM_ENABLE) == TRUE
> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> -
> ################################################################################
>
> [FV.FVMAIN_COMPACT]
> --
> 2.32.0
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-16 15:58 ` Ard Biesheuvel
@ 2021-06-16 19:00 ` Sean
2021-06-16 21:55 ` Ard Biesheuvel
2021-06-17 21:53 ` Michael D Kinney
0 siblings, 2 replies; 24+ messages in thread
From: Sean @ 2021-06-16 19:00 UTC (permalink / raw)
To: ardb, Michael Kinney
Cc: Peter Grehan, Laszlo Ersek, Ard Biesheuvel, Jordan Justen,
Sean Brogan, Rebecca Cran, devel
Ard,
The PR you are trying to "push" has a mergify merge in it which is
causing patchcheck to fail.
https://github.com/tianocore/edk2/pull/1727/commits
Mike,
I think github has better features now around things like auto complete
PR when "checks pass" and allow rebase merging and finally protections
to only allow linear history. Might be time to talk about changes to
mergify/github. I know you are busy so maybe opening up to more of the
edk2 community or actively looking for someone willing to provide best
practices for github usage (rust-lang and nodejs both do a lot with
github).
Thanks
Sean
On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> (+ Sean, Mike)
>
> On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
>>
>> TPM support hasn't been tested and any lines in the .dsc and .fdf files
>> that appear to show support are bogus. Remove them.
>>
>> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>>
>> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
>
> Strangely enough, this patch gets rejected by PatchCheck for lack of a
> Signed-off-by line
>
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
>
> The patch itself looks good to me
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> so if anyone else manages to fix the CI issue, feel free to push the
> patch with my R-b (and Peter's, given in reply to this message)
>
> (I will go offline for 3 weeks after Friday)
>
>> ---
>> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>> 2 files changed, 79 deletions(-)
>>
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>> index d8792812ab..cbf896e89b 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>> @@ -31,8 +31,6 @@
>> DEFINE SECURE_BOOT_ENABLE = FALSE
>> DEFINE SMM_REQUIRE = FALSE
>> DEFINE SOURCE_DEBUG_ENABLE = FALSE
>> - DEFINE TPM_ENABLE = FALSE
>> - DEFINE TPM_CONFIG_ENABLE = FALSE
>>
>> #
>> # Network definition
>> @@ -221,16 +219,8 @@
>> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>>
>> -
>> -!if $(TPM_ENABLE) == TRUE
>> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
>> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
>> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
>> -!else
>> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>> -!endif
>>
>> [LibraryClasses.common]
>> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>> @@ -292,11 +282,6 @@
>> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>
>> -!if $(TPM_ENABLE) == TRUE
>> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>> -!endif
>> -
>> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>>
>> [LibraryClasses.common.DXE_CORE]
>> @@ -366,9 +351,6 @@
>> !endif
>> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
>> -!if $(TPM_ENABLE) == TRUE
>> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>> -!endif
>>
>> [LibraryClasses.common.UEFI_APPLICATION]
>> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
>> @@ -563,22 +545,12 @@
>>
>> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>>
>> -!if $(TPM_ENABLE) == TRUE
>> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
>> -!endif
>> -
>> # MdeModulePkg resolution sets up the system display resolution
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
>> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
>> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>>
>> -[PcdsDynamicHii]
>> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
>> - gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
>> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
>> -!endif
>> -
>> ################################################################################
>> #
>> # Components Section - list of all EDK II Modules needed by this Platform.
>> @@ -618,19 +590,6 @@
>> <LibraryClasses>
>> }
>>
>> -!if $(TPM_ENABLE) == TRUE
>> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>> - <LibraryClasses>
>> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>> - }
>> -!endif
>> -
>> #
>> # DXE Phase modules
>> #
>> @@ -653,9 +612,6 @@
>> <LibraryClasses>
>> !if $(SECURE_BOOT_ENABLE) == TRUE
>> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> -!endif
>> -!if $(TPM_ENABLE) == TRUE
>> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
>> !endif
>> }
>>
>> @@ -841,23 +797,3 @@
>> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>> }
>>
>> -
>> - #
>> - # TPM support
>> - #
>> -!if $(TPM_ENABLE) == TRUE
>> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
>> - <LibraryClasses>
>> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
>> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
>> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
>> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>> - }
>> -!if $(TPM_CONFIG_ENABLE) == TRUE
>> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>> -!endif
>> -!endif
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
>> index 3eff36dac1..fbd63a395a 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
>> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
>> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
>> !endif
>>
>> -!if $(TPM_ENABLE) == TRUE
>> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> -!endif
>> -
>> ################################################################################
>>
>> [FV.DXEFV]
>> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>> !endif
>>
>> -#
>> -# TPM support
>> -#
>> -!if $(TPM_ENABLE) == TRUE
>> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
>> -!if $(TPM_CONFIG_ENABLE) == TRUE
>> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>> -!endif
>> -!endif
>> -
>> ################################################################################
>>
>> [FV.FVMAIN_COMPACT]
>> --
>> 2.32.0
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-16 19:00 ` [edk2-devel] " Sean
@ 2021-06-16 21:55 ` Ard Biesheuvel
2021-06-16 21:59 ` Michael D Kinney
2021-06-17 21:53 ` Michael D Kinney
1 sibling, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2021-06-16 21:55 UTC (permalink / raw)
To: Sean Brogan
Cc: Michael Kinney, Peter Grehan, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Sean Brogan, Rebecca Cran, edk2-devel-groups-io
On Wed, 16 Jun 2021 at 21:00, Sean Brogan <spbrogan@outlook.com> wrote:
>
> Ard,
>
> The PR you are trying to "push" has a mergify merge in it which is
> causing patchcheck to fail.
>
> https://github.com/tianocore/edk2/pull/1727/commits
>
Ah, I missed that. So the base of my branch was out of date, and the
merge was done automatically, right? I think we still want linear
history in EDK2, so this should probably be rejected before even
reaching the point where the tools perform the merge by themselves.
>
>
> Mike,
>
> I think github has better features now around things like auto complete
> PR when "checks pass" and allow rebase merging and finally protections
> to only allow linear history. Might be time to talk about changes to
> mergify/github. I know you are busy so maybe opening up to more of the
> edk2 community or actively looking for someone willing to provide best
> practices for github usage (rust-lang and nodejs both do a lot with
> github).
>
>
> Thanks
> Sean
>
>
>
>
>
> On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > (+ Sean, Mike)
> >
> > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> >>
> >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> >> that appear to show support are bogus. Remove them.
> >>
> >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> >>
> >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> >
> > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > Signed-off-by line
> >
> > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> >
> > The patch itself looks good to me
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > so if anyone else manages to fix the CI issue, feel free to push the
> > patch with my R-b (and Peter's, given in reply to this message)
> >
> > (I will go offline for 3 weeks after Friday)
> >
> >> ---
> >> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> >> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> >> 2 files changed, 79 deletions(-)
> >>
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> index d8792812ab..cbf896e89b 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> @@ -31,8 +31,6 @@
> >> DEFINE SECURE_BOOT_ENABLE = FALSE
> >> DEFINE SMM_REQUIRE = FALSE
> >> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> >> - DEFINE TPM_ENABLE = FALSE
> >> - DEFINE TPM_CONFIG_ENABLE = FALSE
> >>
> >> #
> >> # Network definition
> >> @@ -221,16 +219,8 @@
> >> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> >>
> >> -
> >> -!if $(TPM_ENABLE) == TRUE
> >> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> >> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> >> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> >> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >> -!else
> >> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> >> -!endif
> >>
> >> [LibraryClasses.common]
> >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >> @@ -292,11 +282,6 @@
> >> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> >> -!endif
> >> -
> >> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> >>
> >> [LibraryClasses.common.DXE_CORE]
> >> @@ -366,9 +351,6 @@
> >> !endif
> >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> >> -!if $(TPM_ENABLE) == TRUE
> >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> >> -!endif
> >>
> >> [LibraryClasses.common.UEFI_APPLICATION]
> >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >> @@ -563,22 +545,12 @@
> >>
> >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> >> -!endif
> >> -
> >> # MdeModulePkg resolution sets up the system display resolution
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> >>
> >> -[PcdsDynamicHii]
> >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> >> - gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> >> -!endif
> >> -
> >> ################################################################################
> >> #
> >> # Components Section - list of all EDK II Modules needed by this Platform.
> >> @@ -618,19 +590,6 @@
> >> <LibraryClasses>
> >> }
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> >> - <LibraryClasses>
> >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> - }
> >> -!endif
> >> -
> >> #
> >> # DXE Phase modules
> >> #
> >> @@ -653,9 +612,6 @@
> >> <LibraryClasses>
> >> !if $(SECURE_BOOT_ENABLE) == TRUE
> >> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> -!endif
> >> -!if $(TPM_ENABLE) == TRUE
> >> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> >> !endif
> >> }
> >>
> >> @@ -841,23 +797,3 @@
> >> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> >> }
> >>
> >> -
> >> - #
> >> - # TPM support
> >> - #
> >> -!if $(TPM_ENABLE) == TRUE
> >> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> >> - <LibraryClasses>
> >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> >> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> - }
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> index 3eff36dac1..fbd63a395a 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> >> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> >> !endif
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> >> -!endif
> >> -
> >> ################################################################################
> >>
> >> [FV.DXEFV]
> >> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> >> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> >> !endif
> >>
> >> -#
> >> -# TPM support
> >> -#
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> -
> >> ################################################################################
> >>
> >> [FV.FVMAIN_COMPACT]
> >> --
> >> 2.32.0
> >>
> >>
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-16 21:55 ` Ard Biesheuvel
@ 2021-06-16 21:59 ` Michael D Kinney
0 siblings, 0 replies; 24+ messages in thread
From: Michael D Kinney @ 2021-06-16 21:59 UTC (permalink / raw)
To: Ard Biesheuvel, Sean Brogan, Kinney, Michael D
Cc: Peter Grehan, Laszlo Ersek, Ard Biesheuvel, Justen, Jordan L,
Sean Brogan, Rebecca Cran, edk2-devel-groups-io
Hi Ard,
I was not merged into edk2/master.
PatchCheck failed due to the merge commit in the submitted PR.
You will just need to resubmit the PR without the merge commit and rebased against latest edk2/master, and it will then be able to go through.
Mike
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, June 16, 2021 2:55 PM
> To: Sean Brogan <spbrogan@outlook.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>; edk2-devel-groups-io <devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> On Wed, 16 Jun 2021 at 21:00, Sean Brogan <spbrogan@outlook.com> wrote:
> >
> > Ard,
> >
> > The PR you are trying to "push" has a mergify merge in it which is
> > causing patchcheck to fail.
> >
> > https://github.com/tianocore/edk2/pull/1727/commits
> >
>
> Ah, I missed that. So the base of my branch was out of date, and the
> merge was done automatically, right? I think we still want linear
> history in EDK2, so this should probably be rejected before even
> reaching the point where the tools perform the merge by themselves.
>
> >
> >
> > Mike,
> >
> > I think github has better features now around things like auto complete
> > PR when "checks pass" and allow rebase merging and finally protections
> > to only allow linear history. Might be time to talk about changes to
> > mergify/github. I know you are busy so maybe opening up to more of the
> > edk2 community or actively looking for someone willing to provide best
> > practices for github usage (rust-lang and nodejs both do a lot with
> > github).
> >
> >
> > Thanks
> > Sean
> >
> >
> >
> >
> >
> > On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > > (+ Sean, Mike)
> > >
> > > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> > >>
> > >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> > >> that appear to show support are bogus. Remove them.
> > >>
> > >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> > >>
> > >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > >
> > > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > > Signed-off-by line
> > >
> > > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> > >
> > > The patch itself looks good to me
> > >
> > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > so if anyone else manages to fix the CI issue, feel free to push the
> > > patch with my R-b (and Peter's, given in reply to this message)
> > >
> > > (I will go offline for 3 weeks after Friday)
> > >
> > >> ---
> > >> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > >> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > >> 2 files changed, 79 deletions(-)
> > >>
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> index d8792812ab..cbf896e89b 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> @@ -31,8 +31,6 @@
> > >> DEFINE SECURE_BOOT_ENABLE = FALSE
> > >> DEFINE SMM_REQUIRE = FALSE
> > >> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> > >> - DEFINE TPM_ENABLE = FALSE
> > >> - DEFINE TPM_CONFIG_ENABLE = FALSE
> > >>
> > >> #
> > >> # Network definition
> > >> @@ -221,16 +219,8 @@
> > >> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > >> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > >>
> > >> -
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > >> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > >> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > >> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > >> -!else
> > >> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > >> -!endif
> > >>
> > >> [LibraryClasses.common]
> > >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > >> @@ -292,11 +282,6 @@
> > >> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > >> -!endif
> > >> -
> > >> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > >>
> > >> [LibraryClasses.common.DXE_CORE]
> > >> @@ -366,9 +351,6 @@
> > >> !endif
> > >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > >> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > >> -!endif
> > >>
> > >> [LibraryClasses.common.UEFI_APPLICATION]
> > >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > >> @@ -563,22 +545,12 @@
> > >>
> > >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> > >> -!endif
> > >> -
> > >> # MdeModulePkg resolution sets up the system display resolution
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > >>
> > >> -[PcdsDynamicHii]
> > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > >> -
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > >> -!endif
> > >> -
> > >> ################################################################################
> > >> #
> > >> # Components Section - list of all EDK II Modules needed by this Platform.
> > >> @@ -618,19 +590,6 @@
> > >> <LibraryClasses>
> > >> }
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > >> - <LibraryClasses>
> > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> - }
> > >> -!endif
> > >> -
> > >> #
> > >> # DXE Phase modules
> > >> #
> > >> @@ -653,9 +612,6 @@
> > >> <LibraryClasses>
> > >> !if $(SECURE_BOOT_ENABLE) == TRUE
> > >> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > >> -!endif
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > >> !endif
> > >> }
> > >>
> > >> @@ -841,23 +797,3 @@
> > >> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > >> }
> > >>
> > >> -
> > >> - #
> > >> - # TPM support
> > >> - #
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > >> - <LibraryClasses>
> > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > >> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> - }
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> index 3eff36dac1..fbd63a395a 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > >> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > >> !endif
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > >> -!endif
> > >> -
> > >> ################################################################################
> > >>
> > >> [FV.DXEFV]
> > >> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > >> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > >> !endif
> > >>
> > >> -#
> > >> -# TPM support
> > >> -#
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> -
> > >> ################################################################################
> > >>
> > >> [FV.FVMAIN_COMPACT]
> > >> --
> > >> 2.32.0
> > >>
> > >>
> > >
> > >
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-16 19:00 ` [edk2-devel] " Sean
2021-06-16 21:55 ` Ard Biesheuvel
@ 2021-06-17 21:53 ` Michael D Kinney
2021-06-17 21:54 ` Michael D Kinney
2021-06-22 15:17 ` Laszlo Ersek
1 sibling, 2 replies; 24+ messages in thread
From: Michael D Kinney @ 2021-06-17 21:53 UTC (permalink / raw)
To: devel@edk2.groups.io, spbrogan@outlook.com, ardb@kernel.org,
Kinney, Michael D
Cc: Peter Grehan, Laszlo Ersek, Ard Biesheuvel, Justen, Jordan L,
Sean Brogan, Rebecca Cran
Hi Sean,
Mergify had added a queue feature to handle the rebases automatically and make sure
CI passes in the order that the PRs are being applied to the base branch.
I have setup the edk2-codereview repo with a smaller set of CI tests to skip the
compiles to provide a way to experiment with these features quickly. I have
also simplified the Azure Pipelines templates to remove the FINISHED and FAILED
jobs and the 10 second delay. I have also removed all references to status
check names from the Mergify config file, so the status checks are only
configured using the GitHub protected branches feature.
Here is the Mergify config that is working:
https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml
Here is the Azure Pipelines file that has been simplified removing FINISHED and FAILED jobs
(also removes DEBUG, RELEASE, NOOPT builds for fast testing):
https://github.com/tianocore/edk2-codereview/blob/master/.azurepipelines/templates/pr-gate-build-job.yml
I then did an experiment sending a PR that is out of date with the base branch.
Mergify auto rebased and ran CI tests and added commits from the PR to the base
branch.
I did a 2nd experiment sending 2 PRs at the same time. The first PR finishes its
CI tests and was merged. The 2nd PR was auto rebased and CI tests run and was merged.
No developer interaction required on either one after submitting the PR. This
resolves a common issue seen by Maintainers that process a number of unrelated
patch sets at the same time. They can all be submitted on top of each other without
having to wait for each one to complete and rebase the next one before submitting.
The only open issue remaining is that Mergify auto rebase adds a merge commit
to the set of commits in the PR. The merge commit is not added to the base branch
when it is done. Since the merge commit is present in the PR, the patch check fails.
We need to update patch check to ignore merge commits by the Mergify agent. I have
temporarily disabled the patch check CI status check in edk2-codereview to
work around this issue. Once I have a fix for patch check, I will re-enable the
patch check status check.
I also think this approach will fix the issue that has been seen where Mergify merged
before all the platform tests were completed. We just need to make sure the
platform checks are in the list of enabled status checks in the GitHub protected
branch configuration. I will verify this issue is resolved using the edk2-codereview
repo.
Best regards,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> Sent: Wednesday, June 16, 2021 12:00 PM
> To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>;
> devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> Ard,
>
> The PR you are trying to "push" has a mergify merge in it which is
> causing patchcheck to fail.
>
> https://github.com/tianocore/edk2/pull/1727/commits
>
>
>
> Mike,
>
> I think github has better features now around things like auto complete
> PR when "checks pass" and allow rebase merging and finally protections
> to only allow linear history. Might be time to talk about changes to
> mergify/github. I know you are busy so maybe opening up to more of the
> edk2 community or actively looking for someone willing to provide best
> practices for github usage (rust-lang and nodejs both do a lot with
> github).
>
>
> Thanks
> Sean
>
>
>
>
>
> On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > (+ Sean, Mike)
> >
> > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> >>
> >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> >> that appear to show support are bogus. Remove them.
> >>
> >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> >>
> >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> >
> > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > Signed-off-by line
> >
> > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> >
> > The patch itself looks good to me
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > so if anyone else manages to fix the CI issue, feel free to push the
> > patch with my R-b (and Peter's, given in reply to this message)
> >
> > (I will go offline for 3 weeks after Friday)
> >
> >> ---
> >> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> >> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> >> 2 files changed, 79 deletions(-)
> >>
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> index d8792812ab..cbf896e89b 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> @@ -31,8 +31,6 @@
> >> DEFINE SECURE_BOOT_ENABLE = FALSE
> >> DEFINE SMM_REQUIRE = FALSE
> >> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> >> - DEFINE TPM_ENABLE = FALSE
> >> - DEFINE TPM_CONFIG_ENABLE = FALSE
> >>
> >> #
> >> # Network definition
> >> @@ -221,16 +219,8 @@
> >> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> >>
> >> -
> >> -!if $(TPM_ENABLE) == TRUE
> >> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> >> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> >> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> >> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >> -!else
> >> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> >> -!endif
> >>
> >> [LibraryClasses.common]
> >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >> @@ -292,11 +282,6 @@
> >> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> >> -!endif
> >> -
> >> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> >>
> >> [LibraryClasses.common.DXE_CORE]
> >> @@ -366,9 +351,6 @@
> >> !endif
> >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> >> -!if $(TPM_ENABLE) == TRUE
> >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> >> -!endif
> >>
> >> [LibraryClasses.common.UEFI_APPLICATION]
> >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >> @@ -563,22 +545,12 @@
> >>
> >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00}
> >> -!endif
> >> -
> >> # MdeModulePkg resolution sets up the system display resolution
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> >>
> >> -[PcdsDynamicHii]
> >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> >> -
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> >> -!endif
> >> -
> >> ################################################################################
> >> #
> >> # Components Section - list of all EDK II Modules needed by this Platform.
> >> @@ -618,19 +590,6 @@
> >> <LibraryClasses>
> >> }
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> >> - <LibraryClasses>
> >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> - }
> >> -!endif
> >> -
> >> #
> >> # DXE Phase modules
> >> #
> >> @@ -653,9 +612,6 @@
> >> <LibraryClasses>
> >> !if $(SECURE_BOOT_ENABLE) == TRUE
> >> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> -!endif
> >> -!if $(TPM_ENABLE) == TRUE
> >> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> >> !endif
> >> }
> >>
> >> @@ -841,23 +797,3 @@
> >> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> >> }
> >>
> >> -
> >> - #
> >> - # TPM support
> >> - #
> >> -!if $(TPM_ENABLE) == TRUE
> >> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> >> - <LibraryClasses>
> >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> >> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> - }
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> index 3eff36dac1..fbd63a395a 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> >> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> >> !endif
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> >> -!endif
> >> -
> >> ################################################################################
> >>
> >> [FV.DXEFV]
> >> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> >> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> >> !endif
> >>
> >> -#
> >> -# TPM support
> >> -#
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> -
> >> ################################################################################
> >>
> >> [FV.FVMAIN_COMPACT]
> >> --
> >> 2.32.0
> >>
> >>
> >
> >
> >
> >
> >
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-17 21:53 ` Michael D Kinney
@ 2021-06-17 21:54 ` Michael D Kinney
2021-06-18 4:11 ` Michael D Kinney
2021-06-22 15:17 ` Laszlo Ersek
1 sibling, 1 reply; 24+ messages in thread
From: Michael D Kinney @ 2021-06-17 21:54 UTC (permalink / raw)
To: devel@edk2.groups.io, spbrogan@outlook.com, ardb@kernel.org,
Kinney, Michael D
Cc: Peter Grehan, Laszlo Ersek, Ard Biesheuvel, Justen, Jordan L,
Sean Brogan, Rebecca Cran
Link to Mergify queue feature documentation
https://docs.mergify.io/actions/queue/#merge-queue-problem
Mike
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, June 17, 2021 2:53 PM
> To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> Hi Sean,
>
> Mergify had added a queue feature to handle the rebases automatically and make sure
> CI passes in the order that the PRs are being applied to the base branch.
>
> I have setup the edk2-codereview repo with a smaller set of CI tests to skip the
> compiles to provide a way to experiment with these features quickly. I have
> also simplified the Azure Pipelines templates to remove the FINISHED and FAILED
> jobs and the 10 second delay. I have also removed all references to status
> check names from the Mergify config file, so the status checks are only
> configured using the GitHub protected branches feature.
>
> Here is the Mergify config that is working:
>
> https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml
>
> Here is the Azure Pipelines file that has been simplified removing FINISHED and FAILED jobs
> (also removes DEBUG, RELEASE, NOOPT builds for fast testing):
>
> https://github.com/tianocore/edk2-codereview/blob/master/.azurepipelines/templates/pr-gate-build-job.yml
>
> I then did an experiment sending a PR that is out of date with the base branch.
> Mergify auto rebased and ran CI tests and added commits from the PR to the base
> branch.
>
> I did a 2nd experiment sending 2 PRs at the same time. The first PR finishes its
> CI tests and was merged. The 2nd PR was auto rebased and CI tests run and was merged.
> No developer interaction required on either one after submitting the PR. This
> resolves a common issue seen by Maintainers that process a number of unrelated
> patch sets at the same time. They can all be submitted on top of each other without
> having to wait for each one to complete and rebase the next one before submitting.
>
> The only open issue remaining is that Mergify auto rebase adds a merge commit
> to the set of commits in the PR. The merge commit is not added to the base branch
> when it is done. Since the merge commit is present in the PR, the patch check fails.
> We need to update patch check to ignore merge commits by the Mergify agent. I have
> temporarily disabled the patch check CI status check in edk2-codereview to
> work around this issue. Once I have a fix for patch check, I will re-enable the
> patch check status check.
>
> I also think this approach will fix the issue that has been seen where Mergify merged
> before all the platform tests were completed. We just need to make sure the
> platform checks are in the list of enabled status checks in the GitHub protected
> branch configuration. I will verify this issue is resolved using the edk2-codereview
> repo.
>
> Best regards,
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> > Sent: Wednesday, June 16, 2021 12:00 PM
> > To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>;
> > devel@edk2.groups.io
> > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Ard,
> >
> > The PR you are trying to "push" has a mergify merge in it which is
> > causing patchcheck to fail.
> >
> > https://github.com/tianocore/edk2/pull/1727/commits
> >
> >
> >
> > Mike,
> >
> > I think github has better features now around things like auto complete
> > PR when "checks pass" and allow rebase merging and finally protections
> > to only allow linear history. Might be time to talk about changes to
> > mergify/github. I know you are busy so maybe opening up to more of the
> > edk2 community or actively looking for someone willing to provide best
> > practices for github usage (rust-lang and nodejs both do a lot with
> > github).
> >
> >
> > Thanks
> > Sean
> >
> >
> >
> >
> >
> > On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > > (+ Sean, Mike)
> > >
> > > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> > >>
> > >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> > >> that appear to show support are bogus. Remove them.
> > >>
> > >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> > >>
> > >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > >
> > > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > > Signed-off-by line
> > >
> > > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> > >
> > > The patch itself looks good to me
> > >
> > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > so if anyone else manages to fix the CI issue, feel free to push the
> > > patch with my R-b (and Peter's, given in reply to this message)
> > >
> > > (I will go offline for 3 weeks after Friday)
> > >
> > >> ---
> > >> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > >> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > >> 2 files changed, 79 deletions(-)
> > >>
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> index d8792812ab..cbf896e89b 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> @@ -31,8 +31,6 @@
> > >> DEFINE SECURE_BOOT_ENABLE = FALSE
> > >> DEFINE SMM_REQUIRE = FALSE
> > >> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> > >> - DEFINE TPM_ENABLE = FALSE
> > >> - DEFINE TPM_CONFIG_ENABLE = FALSE
> > >>
> > >> #
> > >> # Network definition
> > >> @@ -221,16 +219,8 @@
> > >> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > >> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > >>
> > >> -
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > >> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > >> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > >> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > >> -!else
> > >> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > >> -!endif
> > >>
> > >> [LibraryClasses.common]
> > >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > >> @@ -292,11 +282,6 @@
> > >> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > >> -!endif
> > >> -
> > >> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > >>
> > >> [LibraryClasses.common.DXE_CORE]
> > >> @@ -366,9 +351,6 @@
> > >> !endif
> > >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > >> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > >> -!endif
> > >>
> > >> [LibraryClasses.common.UEFI_APPLICATION]
> > >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > >> @@ -563,22 +545,12 @@
> > >>
> > >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00,
> > 0x00, 0x00, 0x00, 0x00, 0x00}
> > >> -!endif
> > >> -
> > >> # MdeModulePkg resolution sets up the system display resolution
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > >>
> > >> -[PcdsDynamicHii]
> > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > >> -
> > gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > >> -!endif
> > >> -
> > >> ################################################################################
> > >> #
> > >> # Components Section - list of all EDK II Modules needed by this Platform.
> > >> @@ -618,19 +590,6 @@
> > >> <LibraryClasses>
> > >> }
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > >> - <LibraryClasses>
> > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> - }
> > >> -!endif
> > >> -
> > >> #
> > >> # DXE Phase modules
> > >> #
> > >> @@ -653,9 +612,6 @@
> > >> <LibraryClasses>
> > >> !if $(SECURE_BOOT_ENABLE) == TRUE
> > >> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > >> -!endif
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > >> !endif
> > >> }
> > >>
> > >> @@ -841,23 +797,3 @@
> > >> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > >> }
> > >>
> > >> -
> > >> - #
> > >> - # TPM support
> > >> - #
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > >> - <LibraryClasses>
> > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > >> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> - }
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> index 3eff36dac1..fbd63a395a 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > >> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > >> !endif
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > >> -!endif
> > >> -
> > >> ################################################################################
> > >>
> > >> [FV.DXEFV]
> > >> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > >> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > >> !endif
> > >>
> > >> -#
> > >> -# TPM support
> > >> -#
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> -
> > >> ################################################################################
> > >>
> > >> [FV.FVMAIN_COMPACT]
> > >> --
> > >> 2.32.0
> > >>
> > >>
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-17 21:54 ` Michael D Kinney
@ 2021-06-18 4:11 ` Michael D Kinney
0 siblings, 0 replies; 24+ messages in thread
From: Michael D Kinney @ 2021-06-18 4:11 UTC (permalink / raw)
To: devel@edk2.groups.io, spbrogan@outlook.com, ardb@kernel.org,
Kinney, Michael D
Cc: Peter Grehan, Laszlo Ersek, Ard Biesheuvel, Justen, Jordan L,
Sean Brogan, Rebecca Cran
Hi Sean,
Here is the proposed fix to PatchCheck.py to not flag a Mergify merge commit as an error:
https://github.com/tianocore/edk2-codereview/commit/1b264739387ae940b42d70b5cd25b7c68ed21318
I have re-enabled PatchCheck in GitHub branch protection rules, and I did 2 PRs at same time and
they were auto rebased by Mergify and passed PatchCheck test.
Best regards,
Mike
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, June 17, 2021 2:55 PM
> To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> Link to Mergify queue feature documentation
>
> https://docs.mergify.io/actions/queue/#merge-queue-problem
>
> Mike
>
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Thursday, June 17, 2021 2:53 PM
> > To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> > Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Hi Sean,
> >
> > Mergify had added a queue feature to handle the rebases automatically and make sure
> > CI passes in the order that the PRs are being applied to the base branch.
> >
> > I have setup the edk2-codereview repo with a smaller set of CI tests to skip the
> > compiles to provide a way to experiment with these features quickly. I have
> > also simplified the Azure Pipelines templates to remove the FINISHED and FAILED
> > jobs and the 10 second delay. I have also removed all references to status
> > check names from the Mergify config file, so the status checks are only
> > configured using the GitHub protected branches feature.
> >
> > Here is the Mergify config that is working:
> >
> > https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml
> >
> > Here is the Azure Pipelines file that has been simplified removing FINISHED and FAILED jobs
> > (also removes DEBUG, RELEASE, NOOPT builds for fast testing):
> >
> > https://github.com/tianocore/edk2-codereview/blob/master/.azurepipelines/templates/pr-gate-build-job.yml
> >
> > I then did an experiment sending a PR that is out of date with the base branch.
> > Mergify auto rebased and ran CI tests and added commits from the PR to the base
> > branch.
> >
> > I did a 2nd experiment sending 2 PRs at the same time. The first PR finishes its
> > CI tests and was merged. The 2nd PR was auto rebased and CI tests run and was merged.
> > No developer interaction required on either one after submitting the PR. This
> > resolves a common issue seen by Maintainers that process a number of unrelated
> > patch sets at the same time. They can all be submitted on top of each other without
> > having to wait for each one to complete and rebase the next one before submitting.
> >
> > The only open issue remaining is that Mergify auto rebase adds a merge commit
> > to the set of commits in the PR. The merge commit is not added to the base branch
> > when it is done. Since the merge commit is present in the PR, the patch check fails.
> > We need to update patch check to ignore merge commits by the Mergify agent. I have
> > temporarily disabled the patch check CI status check in edk2-codereview to
> > work around this issue. Once I have a fix for patch check, I will re-enable the
> > patch check status check.
> >
> > I also think this approach will fix the issue that has been seen where Mergify merged
> > before all the platform tests were completed. We just need to make sure the
> > platform checks are in the list of enabled status checks in the GitHub protected
> > branch configuration. I will verify this issue is resolved using the edk2-codereview
> > repo.
> >
> > Best regards,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> > > Sent: Wednesday, June 16, 2021 12:00 PM
> > > To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > > Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> <rebecca@bsdio.com>;
> > > devel@edk2.groups.io
> > > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> > >
> > > Ard,
> > >
> > > The PR you are trying to "push" has a mergify merge in it which is
> > > causing patchcheck to fail.
> > >
> > > https://github.com/tianocore/edk2/pull/1727/commits
> > >
> > >
> > >
> > > Mike,
> > >
> > > I think github has better features now around things like auto complete
> > > PR when "checks pass" and allow rebase merging and finally protections
> > > to only allow linear history. Might be time to talk about changes to
> > > mergify/github. I know you are busy so maybe opening up to more of the
> > > edk2 community or actively looking for someone willing to provide best
> > > practices for github usage (rust-lang and nodejs both do a lot with
> > > github).
> > >
> > >
> > > Thanks
> > > Sean
> > >
> > >
> > >
> > >
> > >
> > > On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > > > (+ Sean, Mike)
> > > >
> > > > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> > > >>
> > > >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> > > >> that appear to show support are bogus. Remove them.
> > > >>
> > > >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> > > >>
> > > >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > > >
> > > > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > > > Signed-off-by line
> > > >
> > > > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> > > >
> > > > The patch itself looks good to me
> > > >
> > > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > so if anyone else manages to fix the CI issue, feel free to push the
> > > > patch with my R-b (and Peter's, given in reply to this message)
> > > >
> > > > (I will go offline for 3 weeks after Friday)
> > > >
> > > >> ---
> > > >> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > > >> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > > >> 2 files changed, 79 deletions(-)
> > > >>
> > > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> index d8792812ab..cbf896e89b 100644
> > > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> @@ -31,8 +31,6 @@
> > > >> DEFINE SECURE_BOOT_ENABLE = FALSE
> > > >> DEFINE SMM_REQUIRE = FALSE
> > > >> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> > > >> - DEFINE TPM_ENABLE = FALSE
> > > >> - DEFINE TPM_CONFIG_ENABLE = FALSE
> > > >>
> > > >> #
> > > >> # Network definition
> > > >> @@ -221,16 +219,8 @@
> > > >>
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > > >> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > > >>
> > > >> -
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > > >> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > > >> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > > >> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > > >> -!else
> > > >> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > > >> -!endif
> > > >>
> > > >> [LibraryClasses.common]
> > > >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > > >> @@ -292,11 +282,6 @@
> > > >> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > > >> -!endif
> > > >> -
> > > >> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > > >>
> > > >> [LibraryClasses.common.DXE_CORE]
> > > >> @@ -366,9 +351,6 @@
> > > >> !endif
> > > >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > > >> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > > >> -!endif
> > > >>
> > > >> [LibraryClasses.common.UEFI_APPLICATION]
> > > >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > > >> @@ -563,22 +545,12 @@
> > > >>
> > > >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > 0x00,
> > > 0x00, 0x00, 0x00, 0x00, 0x00}
> > > >> -!endif
> > > >> -
> > > >> # MdeModulePkg resolution sets up the system display resolution
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > > >>
> > > >> -[PcdsDynamicHii]
> > > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -
> > >
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > > >> -!endif
> > > >> -
> > > >> ################################################################################
> > > >> #
> > > >> # Components Section - list of all EDK II Modules needed by this Platform.
> > > >> @@ -618,19 +590,6 @@
> > > >> <LibraryClasses>
> > > >> }
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > > >> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > > >> - <LibraryClasses>
> > > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > >> - }
> > > >> -!endif
> > > >> -
> > > >> #
> > > >> # DXE Phase modules
> > > >> #
> > > >> @@ -653,9 +612,6 @@
> > > >> <LibraryClasses>
> > > >> !if $(SECURE_BOOT_ENABLE) == TRUE
> > > >> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > > >> -!endif
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > > >> !endif
> > > >> }
> > > >>
> > > >> @@ -841,23 +797,3 @@
> > > >> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > > >> }
> > > >>
> > > >> -
> > > >> - #
> > > >> - # TPM support
> > > >> - #
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > > >> - <LibraryClasses>
> > > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > > >> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > >> - }
> > > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > > >> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > > >> -!endif
> > > >> -!endif
> > > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> index 3eff36dac1..fbd63a395a 100644
> > > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > >> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > > >> !endif
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > > >> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > > >> -!endif
> > > >> -
> > > >> ################################################################################
> > > >>
> > > >> [FV.DXEFV]
> > > >> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > > >> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > > >> !endif
> > > >>
> > > >> -#
> > > >> -# TPM support
> > > >> -#
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > > >> -!endif
> > > >> -!endif
> > > >> -
> > > >> ################################################################################
> > > >>
> > > >> [FV.FVMAIN_COMPACT]
> > > >> --
> > > >> 2.32.0
> > > >>
> > > >>
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-17 21:53 ` Michael D Kinney
2021-06-17 21:54 ` Michael D Kinney
@ 2021-06-22 15:17 ` Laszlo Ersek
2021-06-22 15:38 ` Michael D Kinney
1 sibling, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-22 15:17 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
On 06/17/21 23:53, Kinney, Michael D wrote:
> Hi Sean,
>
> Mergify had added a queue feature to handle the rebases automatically and make sure
> CI passes in the order that the PRs are being applied to the base branch.
I'm opposed to *unconditional* auto-rebase.
On one hand, it is indeed unreasonable to require a human to manually
rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
merge requests for unrelated modules should not block each other.
On the other hand, auto-rebase is a bad idea if both series modify at
least one module in common (especially if both series modify at least
one *file* in common). In case there is a contextual conflict, even if
the conflict can be auto-resolved, and even if that resolution
*compiles*, it has to be reviewed by a human first.
I regularly use the git-range-diff command for this.
At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
-- in different packages); such issues are difficult to debug.
Bisectability helps for sure, but only if the community treats
bisectability with high priority in the first place. (That is, if every
contributor builds their patch set at every stage, before submitting it
for review.)
Can we restrict the auto-rebase feature to such merge requests whose
cumulative diffstats do not intersect?
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-22 15:17 ` Laszlo Ersek
@ 2021-06-22 15:38 ` Michael D Kinney
2021-06-23 15:16 ` Laszlo Ersek
0 siblings, 1 reply; 24+ messages in thread
From: Michael D Kinney @ 2021-06-22 15:38 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org, Kinney, Michael D
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
Hi Laszlo,
I am trying the following configuration that is very conservative:
actions:
queue:
method: rebase
rebase_fallback: none
name: default
The auto rebase only attempts a strict rebase. If that attempt at a strict rebase fails
then it will show that there is a conflict that the developer must take care of.
I believe any combination of 2 PRs that have overlapping diff stat should fail
a strict rebase. The following link describes the method and rebase_fallback
settings in the queue command.
https://docs.mergify.io/actions/queue/#id2
I would be more concerned if we used a method of merge or a rebase_fallback of
merge.
Are there examples you can think of where the diff stat overlap and the strict
rebase will succeed?
Another option to consider is to define an additional 'auto-rebase' label that is
off by default to enable the auto rebase feature. By default the PR must be synced
with head when submitted. Only if a maintainer sets the 'auto-rebase' label will
an auto-rebase be attempted.
I also want to make it easy for non-maintainers to submit PRs and get CI test results.
So auto rebase may be useful for that use case. Perhaps the 'auto-rebase' label
can be considered when the 'push' label is also set.
Thanks,
Mike
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, June 22, 2021 8:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> On 06/17/21 23:53, Kinney, Michael D wrote:
> > Hi Sean,
> >
> > Mergify had added a queue feature to handle the rebases automatically and make sure
> > CI passes in the order that the PRs are being applied to the base branch.
>
> I'm opposed to *unconditional* auto-rebase.
>
> On one hand, it is indeed unreasonable to require a human to manually
> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
> merge requests for unrelated modules should not block each other.
>
> On the other hand, auto-rebase is a bad idea if both series modify at
> least one module in common (especially if both series modify at least
> one *file* in common). In case there is a contextual conflict, even if
> the conflict can be auto-resolved, and even if that resolution
> *compiles*, it has to be reviewed by a human first.
>
> I regularly use the git-range-diff command for this.
>
> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
> -- in different packages); such issues are difficult to debug.
>
> Bisectability helps for sure, but only if the community treats
> bisectability with high priority in the first place. (That is, if every
> contributor builds their patch set at every stage, before submitting it
> for review.)
>
> Can we restrict the auto-rebase feature to such merge requests whose
> cumulative diffstats do not intersect?
>
> Thanks,
> Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-22 15:38 ` Michael D Kinney
@ 2021-06-23 15:16 ` Laszlo Ersek
2021-06-23 18:44 ` Michael D Kinney
0 siblings, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-23 15:16 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]
On 06/22/21 17:38, Kinney, Michael D wrote:
> Hi Laszlo,
>
> I am trying the following configuration that is very conservative:
>
> actions:
> queue:
> method: rebase
> rebase_fallback: none
> name: default
>
> The auto rebase only attempts a strict rebase. If that attempt at a
> strict rebase fails then it will show that there is a conflict that
> the developer must take care of.
>
> I believe any combination of 2 PRs that have overlapping diff stat
> should fail a strict rebase. The following link describes the method
> and rebase_fallback settings in the queue command.
>
> https://docs.mergify.io/actions/queue/#id2
>
> I would be more concerned if we used a method of merge or a
> rebase_fallback of merge.
>
> Are there examples you can think of where the diff stat overlap and
> the strict rebase will succeed?
I've read the strict rebase definition and the above link in the mergify
documentation, but I'm none the wiser.
Consider the following test case (with master @ 7471751a4d81):
git checkout -b b1 master
git am b1.patch # attached
git checkout -b b2 master
git am b2.patch # attached
git branch b2-rebase b2
git rebase b1 b2-rebase
Locally, this produces the following message for me:
> First, rewinding head to replay your work on top of it...
> Applying: world
> Using index info to reconstruct a base tree...
> M ReadMe.rst
> Falling back to patching base and 3-way merge...
> Auto-merging ReadMe.rst
The rebase succeeds and produces the expected result, but that result is
*exactly* what a human should review.
I don't know if mergify catches the above. While the rebase succeeds
locally, it should not succeed in mergify.
Using the "git rebase -i" (interactive) command, which uses a different
rebase backend (based on git-cherry-pick, not on git-am), and specifying
a single "pick" command, the rebase still succeeds; this time without
producing any diagnostic messages even. So from an auto-rebase
perspective, it's even less desirable.
Thanks
Laszlo
>
> Another option to consider is to define an additional 'auto-rebase' label that is
> off by default to enable the auto rebase feature. By default the PR must be synced
> with head when submitted. Only if a maintainer sets the 'auto-rebase' label will
> an auto-rebase be attempted.
>
> I also want to make it easy for non-maintainers to submit PRs and get CI test results.
> So auto rebase may be useful for that use case. Perhaps the 'auto-rebase' label
> can be considered when the 'push' label is also set.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, June 22, 2021 8:17 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>
>> On 06/17/21 23:53, Kinney, Michael D wrote:
>>> Hi Sean,
>>>
>>> Mergify had added a queue feature to handle the rebases automatically and make sure
>>> CI passes in the order that the PRs are being applied to the base branch.
>>
>> I'm opposed to *unconditional* auto-rebase.
>>
>> On one hand, it is indeed unreasonable to require a human to manually
>> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
>> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
>> merge requests for unrelated modules should not block each other.
>>
>> On the other hand, auto-rebase is a bad idea if both series modify at
>> least one module in common (especially if both series modify at least
>> one *file* in common). In case there is a contextual conflict, even if
>> the conflict can be auto-resolved, and even if that resolution
>> *compiles*, it has to be reviewed by a human first.
>>
>> I regularly use the git-range-diff command for this.
>>
>> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
>> -- in different packages); such issues are difficult to debug.
>>
>> Bisectability helps for sure, but only if the community treats
>> bisectability with high priority in the first place. (That is, if every
>> contributor builds their patch set at every stage, before submitting it
>> for review.)
>>
>> Can we restrict the auto-rebase feature to such merge requests whose
>> cumulative diffstats do not intersect?
>>
>> Thanks,
>> Laszlo
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: b1.patch --]
[-- Type: text/x-patch; name="b1.patch", Size: 583 bytes --]
From 04970caf98f8deeecec2d0bc8808cd8bd34cc530 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 23 Jun 2021 16:19:25 +0200
Subject: [PATCH] hello
---
ReadMe.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/ReadMe.rst b/ReadMe.rst
index 8f5db11281bf..30f890ef532e 100644
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@ -3,6 +3,7 @@ EDK II Project
==============
A modern, feature-rich, cross-platform firmware development
+ HELLO
environment for the UEFI and PI specifications from www.uefi.org.
Core CI Build Status
--
2.19.1.3.g30247aa5d201
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: b2.patch --]
[-- Type: text/x-patch; name="b2.patch", Size: 589 bytes --]
From 925e93af76af27526011ad33875fa26cc4cdc6c0 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 23 Jun 2021 16:19:51 +0200
Subject: [PATCH] world
---
ReadMe.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/ReadMe.rst b/ReadMe.rst
index 8f5db11281bf..cfc364db7406 100644
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@ -4,6 +4,7 @@ EDK II Project
A modern, feature-rich, cross-platform firmware development
environment for the UEFI and PI specifications from www.uefi.org.
+ WORLD
Core CI Build Status
--------------------
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-23 15:16 ` Laszlo Ersek
@ 2021-06-23 18:44 ` Michael D Kinney
2021-06-23 19:44 ` Laszlo Ersek
0 siblings, 1 reply; 24+ messages in thread
From: Michael D Kinney @ 2021-06-23 18:44 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org, Kinney, Michael D
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
Hi Laszlo,
Thank you for the test case.
I created 2 PRs against edk2-codereview using your patches.
I made minor update to commit messages to pass patch check.
https://github.com/tianocore/edk2-codereview/pull/18
https://github.com/tianocore/edk2-codereview/pull/19
Found another issue with PatchCheck for the Mergify merge commit and fixed that.
Mergify did process #18 and merged it in after passing all CI. Mergify rebased
#19 successfully and merged it after passing all CI. I do not think this was
your expected result.
I looked more closely at the patches you provided. They were not overlapping
in the lines of Readme.rst. This is why no merge conflict was detected.
I then created 2 new PRs that added text to the same line # in Readme.rst.
https://github.com/tianocore/edk2-codereview/pull/21
https://github.com/tianocore/edk2-codereview/pull/22
PR #21 passed all CI tests and was merged. Mergify then attempted to
rebase #22 and got a merge conflict and is still in the open state waiting
for the developer to manually handle the merge conflict.
Mike
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, June 23, 2021 8:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> On 06/22/21 17:38, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > I am trying the following configuration that is very conservative:
> >
> > actions:
> > queue:
> > method: rebase
> > rebase_fallback: none
> > name: default
> >
> > The auto rebase only attempts a strict rebase. If that attempt at a
> > strict rebase fails then it will show that there is a conflict that
> > the developer must take care of.
> >
> > I believe any combination of 2 PRs that have overlapping diff stat
> > should fail a strict rebase. The following link describes the method
> > and rebase_fallback settings in the queue command.
> >
> > https://docs.mergify.io/actions/queue/#id2
> >
> > I would be more concerned if we used a method of merge or a
> > rebase_fallback of merge.
> >
> > Are there examples you can think of where the diff stat overlap and
> > the strict rebase will succeed?
>
> I've read the strict rebase definition and the above link in the mergify
> documentation, but I'm none the wiser.
>
> Consider the following test case (with master @ 7471751a4d81):
>
> git checkout -b b1 master
> git am b1.patch # attached
> git checkout -b b2 master
> git am b2.patch # attached
> git branch b2-rebase b2
> git rebase b1 b2-rebase
>
> Locally, this produces the following message for me:
>
> > First, rewinding head to replay your work on top of it...
> > Applying: world
> > Using index info to reconstruct a base tree...
> > M ReadMe.rst
> > Falling back to patching base and 3-way merge...
> > Auto-merging ReadMe.rst
>
> The rebase succeeds and produces the expected result, but that result is
> *exactly* what a human should review.
>
> I don't know if mergify catches the above. While the rebase succeeds
> locally, it should not succeed in mergify.
>
> Using the "git rebase -i" (interactive) command, which uses a different
> rebase backend (based on git-cherry-pick, not on git-am), and specifying
> a single "pick" command, the rebase still succeeds; this time without
> producing any diagnostic messages even. So from an auto-rebase
> perspective, it's even less desirable.
>
> Thanks
> Laszlo
>
> >
> > Another option to consider is to define an additional 'auto-rebase' label that is
> > off by default to enable the auto rebase feature. By default the PR must be synced
> > with head when submitted. Only if a maintainer sets the 'auto-rebase' label will
> > an auto-rebase be attempted.
> >
> > I also want to make it easy for non-maintainers to submit PRs and get CI test results.
> > So auto rebase may be useful for that use case. Perhaps the 'auto-rebase' label
> > can be considered when the 'push' label is also set.
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Tuesday, June 22, 2021 8:17 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> >> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> >> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >>
> >> On 06/17/21 23:53, Kinney, Michael D wrote:
> >>> Hi Sean,
> >>>
> >>> Mergify had added a queue feature to handle the rebases automatically and make sure
> >>> CI passes in the order that the PRs are being applied to the base branch.
> >>
> >> I'm opposed to *unconditional* auto-rebase.
> >>
> >> On one hand, it is indeed unreasonable to require a human to manually
> >> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
> >> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
> >> merge requests for unrelated modules should not block each other.
> >>
> >> On the other hand, auto-rebase is a bad idea if both series modify at
> >> least one module in common (especially if both series modify at least
> >> one *file* in common). In case there is a contextual conflict, even if
> >> the conflict can be auto-resolved, and even if that resolution
> >> *compiles*, it has to be reviewed by a human first.
> >>
> >> I regularly use the git-range-diff command for this.
> >>
> >> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
> >> -- in different packages); such issues are difficult to debug.
> >>
> >> Bisectability helps for sure, but only if the community treats
> >> bisectability with high priority in the first place. (That is, if every
> >> contributor builds their patch set at every stage, before submitting it
> >> for review.)
> >>
> >> Can we restrict the auto-rebase feature to such merge requests whose
> >> cumulative diffstats do not intersect?
> >>
> >> Thanks,
> >> Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-23 18:44 ` Michael D Kinney
@ 2021-06-23 19:44 ` Laszlo Ersek
2021-06-23 22:07 ` Michael D Kinney
0 siblings, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-23 19:44 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
On 06/23/21 20:44, Kinney, Michael D wrote:
>
> Hi Laszlo,
>
> Thank you for the test case.
>
> I created 2 PRs against edk2-codereview using your patches.
> I made minor update to commit messages to pass patch check.
>
> https://github.com/tianocore/edk2-codereview/pull/18
> https://github.com/tianocore/edk2-codereview/pull/19
>
> Found another issue with PatchCheck for the Mergify merge commit and
> fixed that.
>
> Mergify did process #18 and merged it in after passing all CI. Mergify
> rebased #19 successfully and merged it after passing all CI. I do not
> think this was your expected result.
Indeed, my "desired" result at least would have been for mergify to
reject the rebase.
> I looked more closely at the patches you provided. They were not
> overlapping in the lines of Readme.rst. This is why no merge conflict
> was detected.
More precisely, a contextual conflict *was* determined between the
patches, but that conflict was auto-resolved.
This is risky when done in an automated fashion. It is an extremely
convenient feature of git, when used interactively; that is, when the
auto-merge (automatic conflict resolution) is semantically verified by a
human. Git takes away the chore of conflict resolution, presents a
"likely good" end result, and a human only needs to *look* at the end
result, not *implement* it.
But that "human look" is exactly what's missing from mergify.
Basically what I'd like for mergify is to turn off automatic conflict
resolution.
More or less, speaking in terms of the stand-alone "patch" utility
<https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
to set the "fuzz factor" to zero.
One way a human reviews such context differences is with git-range-diff.
Continuing my previous example commands:
$ git range-diff --color master..b2 b1..b2-rebase
1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
@@ -6,8 +6,8 @@
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@
-
A modern, feature-rich, cross-platform firmware development
+ HELLO
environment for the UEFI and PI specifications from www.uefi.org.
+ WORLD
This output shows that the "world" addition is the same (it is identical
between pre-rebase and post-rebase in the commit), but the context has
changed. During the rebase, the leading empty line of the context
disappeared, and a HELLO line in the middle of the leading context
appeared.
This result may or may not be semantically correct; it needs a human
decision. What if the original purpose of the "world" patch author was
to say WORLD but only without HELLO? When they looked at the code, there
was no HELLO yet.
git-range-diff is very powerful, but reading its output takes some
getting used to. (Colorization with the "--color" option is basically
required for understanding; I can't reproduce it in this email, alas.)
I don't want to obsess about this forever, I just want us all to be
aware that this risk exists.
Thanks,
Laszlo
>
> I then created 2 new PRs that added text to the same line # in Readme.rst.
>
> https://github.com/tianocore/edk2-codereview/pull/21
> https://github.com/tianocore/edk2-codereview/pull/22
>
> PR #21 passed all CI tests and was merged. Mergify then attempted to
> rebase #22 and got a merge conflict and is still in the open state waiting
> for the developer to manually handle the merge conflict.
This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things exist.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-23 19:44 ` Laszlo Ersek
@ 2021-06-23 22:07 ` Michael D Kinney
2021-06-24 1:09 ` 回复: " gaoliming
2021-06-28 12:23 ` Laszlo Ersek
0 siblings, 2 replies; 24+ messages in thread
From: Michael D Kinney @ 2021-06-23 22:07 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, spbrogan@outlook.com,
ardb@kernel.org, Kinney, Michael D
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
Hi Laszlo,
I understand your point.
I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
prevent bad commits.
I think you are saying that you want to make sure a maintainer carefully reviews changes
across multiple PRs that are in the same area of code. The CI checks will of course make
sure the code builds and passes the basic boot tests, but those tests do not have full
coverage so an interaction issue between two PRs that pass build and boot but have
unintended behavior side effects are what require detailed manual review.
I am going to remove the auto-rebase by default and add a optional label that can
be set by a maintainer to enable auto-rebase. If a maintainer is confident that
a set of PRs being submitted at the same time with the 'push' label are independent,
then the maintainer can also set 'auto-rebase'. If they are not confident, then
they can send PRs one at a time with only 'push' label and manually rebase each
additional PR and review the manual rebase to make sure there are no unintended
side effects.
Any objections to this direction?
Thanks,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Wednesday, June 23, 2021 12:45 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> On 06/23/21 20:44, Kinney, Michael D wrote:
> >
> > Hi Laszlo,
> >
> > Thank you for the test case.
> >
> > I created 2 PRs against edk2-codereview using your patches.
> > I made minor update to commit messages to pass patch check.
> >
> > https://github.com/tianocore/edk2-codereview/pull/18
> > https://github.com/tianocore/edk2-codereview/pull/19
> >
> > Found another issue with PatchCheck for the Mergify merge commit and
> > fixed that.
> >
> > Mergify did process #18 and merged it in after passing all CI. Mergify
> > rebased #19 successfully and merged it after passing all CI. I do not
> > think this was your expected result.
>
> Indeed, my "desired" result at least would have been for mergify to
> reject the rebase.
>
> > I looked more closely at the patches you provided. They were not
> > overlapping in the lines of Readme.rst. This is why no merge conflict
> > was detected.
>
> More precisely, a contextual conflict *was* determined between the
> patches, but that conflict was auto-resolved.
>
> This is risky when done in an automated fashion. It is an extremely
> convenient feature of git, when used interactively; that is, when the
> auto-merge (automatic conflict resolution) is semantically verified by a
> human. Git takes away the chore of conflict resolution, presents a
> "likely good" end result, and a human only needs to *look* at the end
> result, not *implement* it.
>
> But that "human look" is exactly what's missing from mergify.
>
> Basically what I'd like for mergify is to turn off automatic conflict
> resolution.
>
> More or less, speaking in terms of the stand-alone "patch" utility
> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> to set the "fuzz factor" to zero.
>
>
> One way a human reviews such context differences is with git-range-diff.
> Continuing my previous example commands:
>
> $ git range-diff --color master..b2 b1..b2-rebase
>
> 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
> @@ -6,8 +6,8 @@
> --- a/ReadMe.rst
> +++ b/ReadMe.rst
> @@
> -
> A modern, feature-rich, cross-platform firmware development
> + HELLO
> environment for the UEFI and PI specifications from www.uefi.org.
> + WORLD
>
> This output shows that the "world" addition is the same (it is identical
> between pre-rebase and post-rebase in the commit), but the context has
> changed. During the rebase, the leading empty line of the context
> disappeared, and a HELLO line in the middle of the leading context
> appeared.
>
> This result may or may not be semantically correct; it needs a human
> decision. What if the original purpose of the "world" patch author was
> to say WORLD but only without HELLO? When they looked at the code, there
> was no HELLO yet.
>
> git-range-diff is very powerful, but reading its output takes some
> getting used to. (Colorization with the "--color" option is basically
> required for understanding; I can't reproduce it in this email, alas.)
>
> I don't want to obsess about this forever, I just want us all to be
> aware that this risk exists.
>
> Thanks,
> Laszlo
>
> >
> > I then created 2 new PRs that added text to the same line # in Readme.rst.
> >
> > https://github.com/tianocore/edk2-codereview/pull/21
> > https://github.com/tianocore/edk2-codereview/pull/22
> >
> > PR #21 passed all CI tests and was merged. Mergify then attempted to
> > rebase #22 and got a merge conflict and is still in the open state waiting
> > for the developer to manually handle the merge conflict.
>
> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
>
> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
> exist.
>
> Thanks
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-23 22:07 ` Michael D Kinney
@ 2021-06-24 1:09 ` gaoliming
2021-06-24 1:20 ` Michael D Kinney
2021-06-28 12:23 ` Laszlo Ersek
1 sibling, 1 reply; 24+ messages in thread
From: gaoliming @ 2021-06-24 1:09 UTC (permalink / raw)
To: devel, michael.d.kinney, lersek, spbrogan, ardb
Cc: 'Peter Grehan', 'Ard Biesheuvel',
'Justen, Jordan L', 'Sean Brogan',
'Rebecca Cran'
Mike:
If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.
Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> Kinney
> 发送时间: 2021年6月24日 6:08
> 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> 抄送: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> <rebecca@bsdio.com>
> 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> remnants
>
> Hi Laszlo,
>
> I understand your point.
>
> I am trying to balance the ease of use for developers, reducing overhead for
> maintainers, and
> prevent bad commits.
>
> I think you are saying that you want to make sure a maintainer carefully
> reviews changes
> across multiple PRs that are in the same area of code. The CI checks will of
> course make
> sure the code builds and passes the basic boot tests, but those tests do not
> have full
> coverage so an interaction issue between two PRs that pass build and boot
> but have
> unintended behavior side effects are what require detailed manual review.
>
> I am going to remove the auto-rebase by default and add a optional label that
> can
> be set by a maintainer to enable auto-rebase. If a maintainer is confident
> that
> a set of PRs being submitted at the same time with the 'push' label are
> independent,
> then the maintainer can also set 'auto-rebase'. If they are not confident,
> then
> they can send PRs one at a time with only 'push' label and manually rebase
> each
> additional PR and review the manual rebase to make sure there are no
> unintended
> side effects.
>
> Any objections to this direction?
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> > Sent: Wednesday, June 23, 2021 12:45 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> > Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Rebecca Cran <rebecca@bsdio.com>
> > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> remnants
> >
> > On 06/23/21 20:44, Kinney, Michael D wrote:
> > >
> > > Hi Laszlo,
> > >
> > > Thank you for the test case.
> > >
> > > I created 2 PRs against edk2-codereview using your patches.
> > > I made minor update to commit messages to pass patch check.
> > >
> > > https://github.com/tianocore/edk2-codereview/pull/18
> > > https://github.com/tianocore/edk2-codereview/pull/19
> > >
> > > Found another issue with PatchCheck for the Mergify merge commit and
> > > fixed that.
> > >
> > > Mergify did process #18 and merged it in after passing all CI. Mergify
> > > rebased #19 successfully and merged it after passing all CI. I do not
> > > think this was your expected result.
> >
> > Indeed, my "desired" result at least would have been for mergify to
> > reject the rebase.
> >
> > > I looked more closely at the patches you provided. They were not
> > > overlapping in the lines of Readme.rst. This is why no merge conflict
> > > was detected.
> >
> > More precisely, a contextual conflict *was* determined between the
> > patches, but that conflict was auto-resolved.
> >
> > This is risky when done in an automated fashion. It is an extremely
> > convenient feature of git, when used interactively; that is, when the
> > auto-merge (automatic conflict resolution) is semantically verified by a
> > human. Git takes away the chore of conflict resolution, presents a
> > "likely good" end result, and a human only needs to *look* at the end
> > result, not *implement* it.
> >
> > But that "human look" is exactly what's missing from mergify.
> >
> > Basically what I'd like for mergify is to turn off automatic conflict
> > resolution.
> >
> > More or less, speaking in terms of the stand-alone "patch" utility
> > <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> > to set the "fuzz factor" to zero.
> >
> >
> > One way a human reviews such context differences is with git-range-diff.
> > Continuing my previous example commands:
> >
> > $ git range-diff --color master..b2 b1..b2-rebase
> >
> > 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
> > @@ -6,8 +6,8 @@
> > --- a/ReadMe.rst
> > +++ b/ReadMe.rst
> > @@
> > -
> > A modern, feature-rich, cross-platform firmware development
> > + HELLO
> > environment for the UEFI and PI specifications from www.uefi.org.
> > + WORLD
> >
> > This output shows that the "world" addition is the same (it is identical
> > between pre-rebase and post-rebase in the commit), but the context has
> > changed. During the rebase, the leading empty line of the context
> > disappeared, and a HELLO line in the middle of the leading context
> > appeared.
> >
> > This result may or may not be semantically correct; it needs a human
> > decision. What if the original purpose of the "world" patch author was
> > to say WORLD but only without HELLO? When they looked at the code,
> there
> > was no HELLO yet.
> >
> > git-range-diff is very powerful, but reading its output takes some
> > getting used to. (Colorization with the "--color" option is basically
> > required for understanding; I can't reproduce it in this email, alas.)
> >
> > I don't want to obsess about this forever, I just want us all to be
> > aware that this risk exists.
> >
> > Thanks,
> > Laszlo
> >
> > >
> > > I then created 2 new PRs that added text to the same line # in
> Readme.rst.
> > >
> > > https://github.com/tianocore/edk2-codereview/pull/21
> > > https://github.com/tianocore/edk2-codereview/pull/22
> > >
> > > PR #21 passed all CI tests and was merged. Mergify then attempted to
> > > rebase #22 and got a merge conflict and is still in the open state waiting
> > > for the developer to manually handle the merge conflict.
> >
> > This case is not worrisome; when there is a clear conflict that cannot be
> auto-resolved, I'm not concerned.
> >
> > My concern is the sneaky contextual conflict that *appears* auto-resolvable,
> but is semantically broken. Those things
> > exist.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-24 1:09 ` 回复: " gaoliming
@ 2021-06-24 1:20 ` Michael D Kinney
2021-06-24 6:26 ` Michael D Kinney
0 siblings, 1 reply; 24+ messages in thread
From: Michael D Kinney @ 2021-06-24 1:20 UTC (permalink / raw)
To: devel@edk2.groups.io, gaoliming@byosoft.com.cn, lersek@redhat.com,
spbrogan@outlook.com, ardb@kernel.org, Kinney, Michael D
Cc: 'Peter Grehan', 'Ard Biesheuvel',
Justen, Jordan L, 'Sean Brogan', 'Rebecca Cran'
Liming,
Yes. I am working on a configuration with will keep the current 'push' behavior
and add the 'auto-rebase' option.
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> Sent: Wednesday, June 23, 2021 6:10 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; spbrogan@outlook.com;
> ardb@kernel.org
> Cc: 'Peter Grehan' <grehan@freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; 'Sean Brogan' <sean.brogan@microsoft.com>; 'Rebecca Cran' <rebecca@bsdio.com>
> Subject: 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> Mike:
> If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
> If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.
>
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> > Kinney
> > 发送时间: 2021年6月24日 6:08
> > 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> > ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > 抄送: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> > <rebecca@bsdio.com>
> > 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > remnants
> >
> > Hi Laszlo,
> >
> > I understand your point.
> >
> > I am trying to balance the ease of use for developers, reducing overhead for
> > maintainers, and
> > prevent bad commits.
> >
> > I think you are saying that you want to make sure a maintainer carefully
> > reviews changes
> > across multiple PRs that are in the same area of code. The CI checks will of
> > course make
> > sure the code builds and passes the basic boot tests, but those tests do not
> > have full
> > coverage so an interaction issue between two PRs that pass build and boot
> > but have
> > unintended behavior side effects are what require detailed manual review.
> >
> > I am going to remove the auto-rebase by default and add a optional label that
> > can
> > be set by a maintainer to enable auto-rebase. If a maintainer is confident
> > that
> > a set of PRs being submitted at the same time with the 'push' label are
> > independent,
> > then the maintainer can also set 'auto-rebase'. If they are not confident,
> > then
> > they can send PRs one at a time with only 'push' label and manually rebase
> > each
> > additional PR and review the manual rebase to make sure there are no
> > unintended
> > side effects.
> >
> > Any objections to this direction?
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > > Sent: Wednesday, June 23, 2021 12:45 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> > > Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > Rebecca Cran <rebecca@bsdio.com>
> > > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > remnants
> > >
> > > On 06/23/21 20:44, Kinney, Michael D wrote:
> > > >
> > > > Hi Laszlo,
> > > >
> > > > Thank you for the test case.
> > > >
> > > > I created 2 PRs against edk2-codereview using your patches.
> > > > I made minor update to commit messages to pass patch check.
> > > >
> > > > https://github.com/tianocore/edk2-codereview/pull/18
> > > > https://github.com/tianocore/edk2-codereview/pull/19
> > > >
> > > > Found another issue with PatchCheck for the Mergify merge commit and
> > > > fixed that.
> > > >
> > > > Mergify did process #18 and merged it in after passing all CI. Mergify
> > > > rebased #19 successfully and merged it after passing all CI. I do not
> > > > think this was your expected result.
> > >
> > > Indeed, my "desired" result at least would have been for mergify to
> > > reject the rebase.
> > >
> > > > I looked more closely at the patches you provided. They were not
> > > > overlapping in the lines of Readme.rst. This is why no merge conflict
> > > > was detected.
> > >
> > > More precisely, a contextual conflict *was* determined between the
> > > patches, but that conflict was auto-resolved.
> > >
> > > This is risky when done in an automated fashion. It is an extremely
> > > convenient feature of git, when used interactively; that is, when the
> > > auto-merge (automatic conflict resolution) is semantically verified by a
> > > human. Git takes away the chore of conflict resolution, presents a
> > > "likely good" end result, and a human only needs to *look* at the end
> > > result, not *implement* it.
> > >
> > > But that "human look" is exactly what's missing from mergify.
> > >
> > > Basically what I'd like for mergify is to turn off automatic conflict
> > > resolution.
> > >
> > > More or less, speaking in terms of the stand-alone "patch" utility
> > > <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> > > to set the "fuzz factor" to zero.
> > >
> > >
> > > One way a human reviews such context differences is with git-range-diff.
> > > Continuing my previous example commands:
> > >
> > > $ git range-diff --color master..b2 b1..b2-rebase
> > >
> > > 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
> > > @@ -6,8 +6,8 @@
> > > --- a/ReadMe.rst
> > > +++ b/ReadMe.rst
> > > @@
> > > -
> > > A modern, feature-rich, cross-platform firmware development
> > > + HELLO
> > > environment for the UEFI and PI specifications from www.uefi.org.
> > > + WORLD
> > >
> > > This output shows that the "world" addition is the same (it is identical
> > > between pre-rebase and post-rebase in the commit), but the context has
> > > changed. During the rebase, the leading empty line of the context
> > > disappeared, and a HELLO line in the middle of the leading context
> > > appeared.
> > >
> > > This result may or may not be semantically correct; it needs a human
> > > decision. What if the original purpose of the "world" patch author was
> > > to say WORLD but only without HELLO? When they looked at the code,
> > there
> > > was no HELLO yet.
> > >
> > > git-range-diff is very powerful, but reading its output takes some
> > > getting used to. (Colorization with the "--color" option is basically
> > > required for understanding; I can't reproduce it in this email, alas.)
> > >
> > > I don't want to obsess about this forever, I just want us all to be
> > > aware that this risk exists.
> > >
> > > Thanks,
> > > Laszlo
> > >
> > > >
> > > > I then created 2 new PRs that added text to the same line # in
> > Readme.rst.
> > > >
> > > > https://github.com/tianocore/edk2-codereview/pull/21
> > > > https://github.com/tianocore/edk2-codereview/pull/22
> > > >
> > > > PR #21 passed all CI tests and was merged. Mergify then attempted to
> > > > rebase #22 and got a merge conflict and is still in the open state waiting
> > > > for the developer to manually handle the merge conflict.
> > >
> > > This case is not worrisome; when there is a clear conflict that cannot be
> > auto-resolved, I'm not concerned.
> > >
> > > My concern is the sneaky contextual conflict that *appears* auto-resolvable,
> > but is semantically broken. Those things
> > > exist.
> > >
> > > Thanks
> > > Laszlo
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-24 1:20 ` Michael D Kinney
@ 2021-06-24 6:26 ` Michael D Kinney
0 siblings, 0 replies; 24+ messages in thread
From: Michael D Kinney @ 2021-06-24 6:26 UTC (permalink / raw)
To: devel@edk2.groups.io, gaoliming@byosoft.com.cn, lersek@redhat.com,
spbrogan@outlook.com, ardb@kernel.org, Kinney, Michael D
Cc: 'Peter Grehan', 'Ard Biesheuvel',
Justen, Jordan L, 'Sean Brogan', 'Rebecca Cran'
Hi,
I have checked in a new Mergify configuration file into edk2-codereview. Have not had a chance to
test it yet, but I think it has all the updates that have been discussed:
1) Allows non EDK II Maintainers to open a PR and for an EDK II Maintainers to set labels.
2) Removed status check names from Mergify config and use the status checks listed in branch protections
3) Simplify the number of rules in Mergify config
4) Add support for 'main' branch.
5) Optional support for auto-rebase using 'auto-rebase' label.
https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml
Please try doing some additional tests to see if it behaves as expected.
I will get back to this on Monday or Tuesday next week and will start preparing a
few code reviews to update the edk2 repo with these changes.
Thanks,
Mike
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 23, 2021 6:21 PM
> To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; lersek@redhat.com; spbrogan@outlook.com; ardb@kernel.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: 'Peter Grehan' <grehan@freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; 'Sean Brogan' <sean.brogan@microsoft.com>; 'Rebecca Cran' <rebecca@bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> Liming,
>
> Yes. I am working on a configuration with will keep the current 'push' behavior
> and add the 'auto-rebase' option.
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> > Sent: Wednesday, June 23, 2021 6:10 PM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; spbrogan@outlook.com;
> > ardb@kernel.org
> > Cc: 'Peter Grehan' <grehan@freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; 'Sean Brogan' <sean.brogan@microsoft.com>; 'Rebecca Cran' <rebecca@bsdio.com>
> > Subject: 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Mike:
> > If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
> > If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> > > Kinney
> > > 发送时间: 2021年6月24日 6:08
> > > 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> > > ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > > 抄送: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > > <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > > Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> > > <rebecca@bsdio.com>
> > > 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > > remnants
> > >
> > > Hi Laszlo,
> > >
> > > I understand your point.
> > >
> > > I am trying to balance the ease of use for developers, reducing overhead for
> > > maintainers, and
> > > prevent bad commits.
> > >
> > > I think you are saying that you want to make sure a maintainer carefully
> > > reviews changes
> > > across multiple PRs that are in the same area of code. The CI checks will of
> > > course make
> > > sure the code builds and passes the basic boot tests, but those tests do not
> > > have full
> > > coverage so an interaction issue between two PRs that pass build and boot
> > > but have
> > > unintended behavior side effects are what require detailed manual review.
> > >
> > > I am going to remove the auto-rebase by default and add a optional label that
> > > can
> > > be set by a maintainer to enable auto-rebase. If a maintainer is confident
> > > that
> > > a set of PRs being submitted at the same time with the 'push' label are
> > > independent,
> > > then the maintainer can also set 'auto-rebase'. If they are not confident,
> > > then
> > > they can send PRs one at a time with only 'push' label and manually rebase
> > > each
> > > additional PR and review the manual rebase to make sure there are no
> > > unintended
> > > side effects.
> > >
> > > Any objections to this direction?
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > > Ersek
> > > > Sent: Wednesday, June 23, 2021 12:45 PM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> > > > Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > > <ardb+tianocore@kernel.org>; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > > Rebecca Cran <rebecca@bsdio.com>
> > > > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > > remnants
> > > >
> > > > On 06/23/21 20:44, Kinney, Michael D wrote:
> > > > >
> > > > > Hi Laszlo,
> > > > >
> > > > > Thank you for the test case.
> > > > >
> > > > > I created 2 PRs against edk2-codereview using your patches.
> > > > > I made minor update to commit messages to pass patch check.
> > > > >
> > > > > https://github.com/tianocore/edk2-codereview/pull/18
> > > > > https://github.com/tianocore/edk2-codereview/pull/19
> > > > >
> > > > > Found another issue with PatchCheck for the Mergify merge commit and
> > > > > fixed that.
> > > > >
> > > > > Mergify did process #18 and merged it in after passing all CI. Mergify
> > > > > rebased #19 successfully and merged it after passing all CI. I do not
> > > > > think this was your expected result.
> > > >
> > > > Indeed, my "desired" result at least would have been for mergify to
> > > > reject the rebase.
> > > >
> > > > > I looked more closely at the patches you provided. They were not
> > > > > overlapping in the lines of Readme.rst. This is why no merge conflict
> > > > > was detected.
> > > >
> > > > More precisely, a contextual conflict *was* determined between the
> > > > patches, but that conflict was auto-resolved.
> > > >
> > > > This is risky when done in an automated fashion. It is an extremely
> > > > convenient feature of git, when used interactively; that is, when the
> > > > auto-merge (automatic conflict resolution) is semantically verified by a
> > > > human. Git takes away the chore of conflict resolution, presents a
> > > > "likely good" end result, and a human only needs to *look* at the end
> > > > result, not *implement* it.
> > > >
> > > > But that "human look" is exactly what's missing from mergify.
> > > >
> > > > Basically what I'd like for mergify is to turn off automatic conflict
> > > > resolution.
> > > >
> > > > More or less, speaking in terms of the stand-alone "patch" utility
> > > > <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> > > > to set the "fuzz factor" to zero.
> > > >
> > > >
> > > > One way a human reviews such context differences is with git-range-diff.
> > > > Continuing my previous example commands:
> > > >
> > > > $ git range-diff --color master..b2 b1..b2-rebase
> > > >
> > > > 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
> > > > @@ -6,8 +6,8 @@
> > > > --- a/ReadMe.rst
> > > > +++ b/ReadMe.rst
> > > > @@
> > > > -
> > > > A modern, feature-rich, cross-platform firmware development
> > > > + HELLO
> > > > environment for the UEFI and PI specifications from www.uefi.org.
> > > > + WORLD
> > > >
> > > > This output shows that the "world" addition is the same (it is identical
> > > > between pre-rebase and post-rebase in the commit), but the context has
> > > > changed. During the rebase, the leading empty line of the context
> > > > disappeared, and a HELLO line in the middle of the leading context
> > > > appeared.
> > > >
> > > > This result may or may not be semantically correct; it needs a human
> > > > decision. What if the original purpose of the "world" patch author was
> > > > to say WORLD but only without HELLO? When they looked at the code,
> > > there
> > > > was no HELLO yet.
> > > >
> > > > git-range-diff is very powerful, but reading its output takes some
> > > > getting used to. (Colorization with the "--color" option is basically
> > > > required for understanding; I can't reproduce it in this email, alas.)
> > > >
> > > > I don't want to obsess about this forever, I just want us all to be
> > > > aware that this risk exists.
> > > >
> > > > Thanks,
> > > > Laszlo
> > > >
> > > > >
> > > > > I then created 2 new PRs that added text to the same line # in
> > > Readme.rst.
> > > > >
> > > > > https://github.com/tianocore/edk2-codereview/pull/21
> > > > > https://github.com/tianocore/edk2-codereview/pull/22
> > > > >
> > > > > PR #21 passed all CI tests and was merged. Mergify then attempted to
> > > > > rebase #22 and got a merge conflict and is still in the open state waiting
> > > > > for the developer to manually handle the merge conflict.
> > > >
> > > > This case is not worrisome; when there is a clear conflict that cannot be
> > > auto-resolved, I'm not concerned.
> > > >
> > > > My concern is the sneaky contextual conflict that *appears* auto-resolvable,
> > > but is semantically broken. Those things
> > > > exist.
> > > >
> > > > Thanks
> > > > Laszlo
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-23 22:07 ` Michael D Kinney
2021-06-24 1:09 ` 回复: " gaoliming
@ 2021-06-28 12:23 ` Laszlo Ersek
2021-07-07 6:00 ` Michael D Kinney
1 sibling, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-28 12:23 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
On 06/24/21 00:07, Kinney, Michael D wrote:
> Hi Laszlo,
>
> I understand your point.
>
> I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
> prevent bad commits.
>
> I think you are saying that you want to make sure a maintainer carefully reviews changes
> across multiple PRs that are in the same area of code. The CI checks will of course make
> sure the code builds and passes the basic boot tests, but those tests do not have full
> coverage so an interaction issue between two PRs that pass build and boot but have
> unintended behavior side effects are what require detailed manual review.
>
> I am going to remove the auto-rebase by default and add a optional label that can
> be set by a maintainer to enable auto-rebase. If a maintainer is confident that
> a set of PRs being submitted at the same time with the 'push' label are independent,
> then the maintainer can also set 'auto-rebase'. If they are not confident, then
> they can send PRs one at a time with only 'push' label and manually rebase each
> additional PR and review the manual rebase to make sure there are no unintended
> side effects.
Sounds great, thank you!
Laszlo
>
> Any objections to this direction?
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, June 23, 2021 12:45 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>
>> On 06/23/21 20:44, Kinney, Michael D wrote:
>>>
>>> Hi Laszlo,
>>>
>>> Thank you for the test case.
>>>
>>> I created 2 PRs against edk2-codereview using your patches.
>>> I made minor update to commit messages to pass patch check.
>>>
>>> https://github.com/tianocore/edk2-codereview/pull/18
>>> https://github.com/tianocore/edk2-codereview/pull/19
>>>
>>> Found another issue with PatchCheck for the Mergify merge commit and
>>> fixed that.
>>>
>>> Mergify did process #18 and merged it in after passing all CI. Mergify
>>> rebased #19 successfully and merged it after passing all CI. I do not
>>> think this was your expected result.
>>
>> Indeed, my "desired" result at least would have been for mergify to
>> reject the rebase.
>>
>>> I looked more closely at the patches you provided. They were not
>>> overlapping in the lines of Readme.rst. This is why no merge conflict
>>> was detected.
>>
>> More precisely, a contextual conflict *was* determined between the
>> patches, but that conflict was auto-resolved.
>>
>> This is risky when done in an automated fashion. It is an extremely
>> convenient feature of git, when used interactively; that is, when the
>> auto-merge (automatic conflict resolution) is semantically verified by a
>> human. Git takes away the chore of conflict resolution, presents a
>> "likely good" end result, and a human only needs to *look* at the end
>> result, not *implement* it.
>>
>> But that "human look" is exactly what's missing from mergify.
>>
>> Basically what I'd like for mergify is to turn off automatic conflict
>> resolution.
>>
>> More or less, speaking in terms of the stand-alone "patch" utility
>> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
>> to set the "fuzz factor" to zero.
>>
>>
>> One way a human reviews such context differences is with git-range-diff.
>> Continuing my previous example commands:
>>
>> $ git range-diff --color master..b2 b1..b2-rebase
>>
>> 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
>> @@ -6,8 +6,8 @@
>> --- a/ReadMe.rst
>> +++ b/ReadMe.rst
>> @@
>> -
>> A modern, feature-rich, cross-platform firmware development
>> + HELLO
>> environment for the UEFI and PI specifications from www.uefi.org.
>> + WORLD
>>
>> This output shows that the "world" addition is the same (it is identical
>> between pre-rebase and post-rebase in the commit), but the context has
>> changed. During the rebase, the leading empty line of the context
>> disappeared, and a HELLO line in the middle of the leading context
>> appeared.
>>
>> This result may or may not be semantically correct; it needs a human
>> decision. What if the original purpose of the "world" patch author was
>> to say WORLD but only without HELLO? When they looked at the code, there
>> was no HELLO yet.
>>
>> git-range-diff is very powerful, but reading its output takes some
>> getting used to. (Colorization with the "--color" option is basically
>> required for understanding; I can't reproduce it in this email, alas.)
>>
>> I don't want to obsess about this forever, I just want us all to be
>> aware that this risk exists.
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> I then created 2 new PRs that added text to the same line # in Readme.rst.
>>>
>>> https://github.com/tianocore/edk2-codereview/pull/21
>>> https://github.com/tianocore/edk2-codereview/pull/22
>>>
>>> PR #21 passed all CI tests and was merged. Mergify then attempted to
>>> rebase #22 and got a merge conflict and is still in the open state waiting
>>> for the developer to manually handle the merge conflict.
>>
>> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
>>
>> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
>> exist.
>>
>> Thanks
>> Laszlo
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-28 12:23 ` Laszlo Ersek
@ 2021-07-07 6:00 ` Michael D Kinney
2021-07-07 8:53 ` Laszlo Ersek
0 siblings, 1 reply; 24+ messages in thread
From: Michael D Kinney @ 2021-07-07 6:00 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, spbrogan@outlook.com,
ardb@kernel.org, Kinney, Michael D
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
Hi Laszlo,
I did many experiments and could not get the exact behavior I proposed.
Here is the best I can do with the behavior of GitHub and Mergify:
1) I further simplified Mergify configuration so personal builds ('push'
label not set) are no longer auto closed. Any developer doing a
personal build that wants to abandon the change should manually close
the PR. We may need to periodically review PRs that have been open
for an extended period of time and close them and developers can reopen
if that was a mistake.
2) The 'push' label always does the safest possible rebase and merge.
If many PRs are queued at a time, then each one is rebased in turn,
all CI checks are run and if all CI checks pass, then PR is
added with linear history to the base branch.
3) If a maintainer wants to manually send a sequence of PRs through
one at a time and review the state before sending the next one, then
I recommending sending each PR as a personal build (always rebase against
latest base branch before submitting PR). Review the commits and status
checks. If the PR looks good and passes all status checks and the state
from GitHub is that the PR is 'up-to-date' with the base branch, then
the maintainer can set the 'push' label and the PR is merged immediately
without re-running the status checks since there have been no changes.
If other PRs were merged into the base branch while the PR status checks
were run, then the personal build will show that the branch is out-of-date
with the base branch. The maintainer can do a local rebase and review
the changes and do a forced push of the updated PR. This will trigger the
personal build again. If that passes and the branch is 'up-to-date', then
the 'push' label can be set to immediately merge. Repeat as needed.
NOTE: If there is a lot of PR activity from other maintainers at the same
time as this manual process is being used, then the manual process may
have to rebase and force push several times until there is a quiet window.
The simplified Mergify configuration file is shown below.
queue_rules:
- name: default
conditions:
- base~=(^main|^master|^stable/)
- label=push
pull_request_rules:
- name: Automatically merge a PR when all required checks pass and 'push' label is present
conditions:
- base~=(^main|^master|^stable/)
- label=push
actions:
queue:
method: rebase
rebase_fallback: none
name: default
- name: Post a comment on a PR that can not be merged due to a merge conflict
conditions:
- base~=(^main|^master|^stable/)
- conflict
actions:
comment:
message: PR can not be merged due to conflict. Please rebase and resubmit
Let me know if this process of using personal builds and setting 'push' label
if PR is 'up-to-date' is acceptable. I have tested this process with a few
different scenarios in the edk2-codereview repo, and they all worked as expected.
Best regards,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Monday, June 28, 2021 5:23 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> On 06/24/21 00:07, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > I understand your point.
> >
> > I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
> > prevent bad commits.
> >
> > I think you are saying that you want to make sure a maintainer carefully reviews changes
> > across multiple PRs that are in the same area of code. The CI checks will of course make
> > sure the code builds and passes the basic boot tests, but those tests do not have full
> > coverage so an interaction issue between two PRs that pass build and boot but have
> > unintended behavior side effects are what require detailed manual review.
> >
> > I am going to remove the auto-rebase by default and add a optional label that can
> > be set by a maintainer to enable auto-rebase. If a maintainer is confident that
> > a set of PRs being submitted at the same time with the 'push' label are independent,
> > then the maintainer can also set 'auto-rebase'. If they are not confident, then
> > they can send PRs one at a time with only 'push' label and manually rebase each
> > additional PR and review the manual rebase to make sure there are no unintended
> > side effects.
>
> Sounds great, thank you!
> Laszlo
>
> >
> > Any objections to this direction?
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> >> Sent: Wednesday, June 23, 2021 12:45 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> >> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> >> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >>
> >> On 06/23/21 20:44, Kinney, Michael D wrote:
> >>>
> >>> Hi Laszlo,
> >>>
> >>> Thank you for the test case.
> >>>
> >>> I created 2 PRs against edk2-codereview using your patches.
> >>> I made minor update to commit messages to pass patch check.
> >>>
> >>> https://github.com/tianocore/edk2-codereview/pull/18
> >>> https://github.com/tianocore/edk2-codereview/pull/19
> >>>
> >>> Found another issue with PatchCheck for the Mergify merge commit and
> >>> fixed that.
> >>>
> >>> Mergify did process #18 and merged it in after passing all CI. Mergify
> >>> rebased #19 successfully and merged it after passing all CI. I do not
> >>> think this was your expected result.
> >>
> >> Indeed, my "desired" result at least would have been for mergify to
> >> reject the rebase.
> >>
> >>> I looked more closely at the patches you provided. They were not
> >>> overlapping in the lines of Readme.rst. This is why no merge conflict
> >>> was detected.
> >>
> >> More precisely, a contextual conflict *was* determined between the
> >> patches, but that conflict was auto-resolved.
> >>
> >> This is risky when done in an automated fashion. It is an extremely
> >> convenient feature of git, when used interactively; that is, when the
> >> auto-merge (automatic conflict resolution) is semantically verified by a
> >> human. Git takes away the chore of conflict resolution, presents a
> >> "likely good" end result, and a human only needs to *look* at the end
> >> result, not *implement* it.
> >>
> >> But that "human look" is exactly what's missing from mergify.
> >>
> >> Basically what I'd like for mergify is to turn off automatic conflict
> >> resolution.
> >>
> >> More or less, speaking in terms of the stand-alone "patch" utility
> >> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> >> to set the "fuzz factor" to zero.
> >>
> >>
> >> One way a human reviews such context differences is with git-range-diff.
> >> Continuing my previous example commands:
> >>
> >> $ git range-diff --color master..b2 b1..b2-rebase
> >>
> >> 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
> >> @@ -6,8 +6,8 @@
> >> --- a/ReadMe.rst
> >> +++ b/ReadMe.rst
> >> @@
> >> -
> >> A modern, feature-rich, cross-platform firmware development
> >> + HELLO
> >> environment for the UEFI and PI specifications from www.uefi.org.
> >> + WORLD
> >>
> >> This output shows that the "world" addition is the same (it is identical
> >> between pre-rebase and post-rebase in the commit), but the context has
> >> changed. During the rebase, the leading empty line of the context
> >> disappeared, and a HELLO line in the middle of the leading context
> >> appeared.
> >>
> >> This result may or may not be semantically correct; it needs a human
> >> decision. What if the original purpose of the "world" patch author was
> >> to say WORLD but only without HELLO? When they looked at the code, there
> >> was no HELLO yet.
> >>
> >> git-range-diff is very powerful, but reading its output takes some
> >> getting used to. (Colorization with the "--color" option is basically
> >> required for understanding; I can't reproduce it in this email, alas.)
> >>
> >> I don't want to obsess about this forever, I just want us all to be
> >> aware that this risk exists.
> >>
> >> Thanks,
> >> Laszlo
> >>
> >>>
> >>> I then created 2 new PRs that added text to the same line # in Readme.rst.
> >>>
> >>> https://github.com/tianocore/edk2-codereview/pull/21
> >>> https://github.com/tianocore/edk2-codereview/pull/22
> >>>
> >>> PR #21 passed all CI tests and was merged. Mergify then attempted to
> >>> rebase #22 and got a merge conflict and is still in the open state waiting
> >>> for the developer to manually handle the merge conflict.
> >>
> >> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
> >>
> >> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
> >> exist.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>
> >>
> >>
> >>
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-07-07 6:00 ` Michael D Kinney
@ 2021-07-07 8:53 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2021-07-07 8:53 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, spbrogan@outlook.com,
ardb@kernel.org
Cc: Peter Grehan, Ard Biesheuvel, Justen, Jordan L, Sean Brogan,
Rebecca Cran
On 07/07/21 08:00, Kinney, Michael D wrote:
> Hi Laszlo,
>
> I did many experiments and could not get the exact behavior I proposed.
>
> Here is the best I can do with the behavior of GitHub and Mergify:
>
> 1) I further simplified Mergify configuration so personal builds ('push'
> label not set) are no longer auto closed. Any developer doing a
> personal build that wants to abandon the change should manually close
> the PR.
This sounds OK to me.
> We may need to periodically review PRs that have been open
> for an extended period of time and close them and developers can reopen
> if that was a mistake.
Yes, I've been periodically checking PRs that were stuck open anyway.
>
> 2) The 'push' label always does the safest possible rebase and merge.
> If many PRs are queued at a time, then each one is rebased in turn,
> all CI checks are run and if all CI checks pass, then PR is
> added with linear history to the base branch.
OK.
>
> 3) If a maintainer wants to manually send a sequence of PRs through
> one at a time and review the state before sending the next one, then
> I recommending sending each PR as a personal build (always rebase against
> latest base branch before submitting PR). Review the commits and status
> checks. If the PR looks good and passes all status checks and the state
> from GitHub is that the PR is 'up-to-date' with the base branch, then
> the maintainer can set the 'push' label and the PR is merged immediately
> without re-running the status checks since there have been no changes.
So this is the key development. OK.
>
> If other PRs were merged into the base branch while the PR status checks
> were run, then the personal build will show that the branch is out-of-date
> with the base branch. The maintainer can do a local rebase and review
> the changes and do a forced push of the updated PR. This will trigger the
> personal build again. If that passes and the branch is 'up-to-date', then
> the 'push' label can be set to immediately merge. Repeat as needed.
>
> NOTE: If there is a lot of PR activity from other maintainers at the same
> time as this manual process is being used, then the manual process may
> have to rebase and force push several times until there is a quiet window.
Makes sense.
>
>
> The simplified Mergify configuration file is shown below.
>
> queue_rules:
> - name: default
> conditions:
> - base~=(^main|^master|^stable/)
> - label=push
>
> pull_request_rules:
> - name: Automatically merge a PR when all required checks pass and 'push' label is present
> conditions:
> - base~=(^main|^master|^stable/)
> - label=push
> actions:
> queue:
> method: rebase
> rebase_fallback: none
> name: default
>
> - name: Post a comment on a PR that can not be merged due to a merge conflict
> conditions:
> - base~=(^main|^master|^stable/)
> - conflict
> actions:
> comment:
> message: PR can not be merged due to conflict. Please rebase and resubmit
>
>
> Let me know if this process of using personal builds and setting 'push' label
> if PR is 'up-to-date' is acceptable. I have tested this process with a few
> different scenarios in the edk2-codereview repo, and they all worked as expected.
Sounds all fine to me; thank you for working it out!
Laszlo
>
> Best regards,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Monday, June 28, 2021 5:23 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>
>> On 06/24/21 00:07, Kinney, Michael D wrote:
>>> Hi Laszlo,
>>>
>>> I understand your point.
>>>
>>> I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
>>> prevent bad commits.
>>>
>>> I think you are saying that you want to make sure a maintainer carefully reviews changes
>>> across multiple PRs that are in the same area of code. The CI checks will of course make
>>> sure the code builds and passes the basic boot tests, but those tests do not have full
>>> coverage so an interaction issue between two PRs that pass build and boot but have
>>> unintended behavior side effects are what require detailed manual review.
>>>
>>> I am going to remove the auto-rebase by default and add a optional label that can
>>> be set by a maintainer to enable auto-rebase. If a maintainer is confident that
>>> a set of PRs being submitted at the same time with the 'push' label are independent,
>>> then the maintainer can also set 'auto-rebase'. If they are not confident, then
>>> they can send PRs one at a time with only 'push' label and manually rebase each
>>> additional PR and review the manual rebase to make sure there are no unintended
>>> side effects.
>>
>> Sounds great, thank you!
>> Laszlo
>>
>>>
>>> Any objections to this direction?
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>>>> Sent: Wednesday, June 23, 2021 12:45 PM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>>>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>>>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>>>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>>>
>>>> On 06/23/21 20:44, Kinney, Michael D wrote:
>>>>>
>>>>> Hi Laszlo,
>>>>>
>>>>> Thank you for the test case.
>>>>>
>>>>> I created 2 PRs against edk2-codereview using your patches.
>>>>> I made minor update to commit messages to pass patch check.
>>>>>
>>>>> https://github.com/tianocore/edk2-codereview/pull/18
>>>>> https://github.com/tianocore/edk2-codereview/pull/19
>>>>>
>>>>> Found another issue with PatchCheck for the Mergify merge commit and
>>>>> fixed that.
>>>>>
>>>>> Mergify did process #18 and merged it in after passing all CI. Mergify
>>>>> rebased #19 successfully and merged it after passing all CI. I do not
>>>>> think this was your expected result.
>>>>
>>>> Indeed, my "desired" result at least would have been for mergify to
>>>> reject the rebase.
>>>>
>>>>> I looked more closely at the patches you provided. They were not
>>>>> overlapping in the lines of Readme.rst. This is why no merge conflict
>>>>> was detected.
>>>>
>>>> More precisely, a contextual conflict *was* determined between the
>>>> patches, but that conflict was auto-resolved.
>>>>
>>>> This is risky when done in an automated fashion. It is an extremely
>>>> convenient feature of git, when used interactively; that is, when the
>>>> auto-merge (automatic conflict resolution) is semantically verified by a
>>>> human. Git takes away the chore of conflict resolution, presents a
>>>> "likely good" end result, and a human only needs to *look* at the end
>>>> result, not *implement* it.
>>>>
>>>> But that "human look" is exactly what's missing from mergify.
>>>>
>>>> Basically what I'd like for mergify is to turn off automatic conflict
>>>> resolution.
>>>>
>>>> More or less, speaking in terms of the stand-alone "patch" utility
>>>> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
>>>> to set the "fuzz factor" to zero.
>>>>
>>>>
>>>> One way a human reviews such context differences is with git-range-diff.
>>>> Continuing my previous example commands:
>>>>
>>>> $ git range-diff --color master..b2 b1..b2-rebase
>>>>
>>>> 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
>>>> @@ -6,8 +6,8 @@
>>>> --- a/ReadMe.rst
>>>> +++ b/ReadMe.rst
>>>> @@
>>>> -
>>>> A modern, feature-rich, cross-platform firmware development
>>>> + HELLO
>>>> environment for the UEFI and PI specifications from www.uefi.org.
>>>> + WORLD
>>>>
>>>> This output shows that the "world" addition is the same (it is identical
>>>> between pre-rebase and post-rebase in the commit), but the context has
>>>> changed. During the rebase, the leading empty line of the context
>>>> disappeared, and a HELLO line in the middle of the leading context
>>>> appeared.
>>>>
>>>> This result may or may not be semantically correct; it needs a human
>>>> decision. What if the original purpose of the "world" patch author was
>>>> to say WORLD but only without HELLO? When they looked at the code, there
>>>> was no HELLO yet.
>>>>
>>>> git-range-diff is very powerful, but reading its output takes some
>>>> getting used to. (Colorization with the "--color" option is basically
>>>> required for understanding; I can't reproduce it in this email, alas.)
>>>>
>>>> I don't want to obsess about this forever, I just want us all to be
>>>> aware that this risk exists.
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>>
>>>>> I then created 2 new PRs that added text to the same line # in Readme.rst.
>>>>>
>>>>> https://github.com/tianocore/edk2-codereview/pull/21
>>>>> https://github.com/tianocore/edk2-codereview/pull/22
>>>>>
>>>>> PR #21 passed all CI tests and was merged. Mergify then attempted to
>>>>> rebase #22 and got a merge conflict and is still in the open state waiting
>>>>> for the developer to manually handle the merge conflict.
>>>>
>>>> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
>>>>
>>>> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
>>>> exist.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-12 20:43 [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants Rebecca Cran
2021-06-12 23:22 ` [edk2-devel] " Peter Grehan
2021-06-16 15:58 ` Ard Biesheuvel
@ 2021-06-22 17:57 ` Laszlo Ersek
2021-06-24 7:37 ` [edk2-devel] " Laszlo Ersek
2021-06-24 8:03 ` Laszlo Ersek
4 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-22 17:57 UTC (permalink / raw)
To: Rebecca Cran, Peter Grehan, Ard Biesheuvel, Jordan Justen; +Cc: devel
On 06/12/21 22:43, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
>
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> 2 files changed, 79 deletions(-)
I'll try to review this patch soon; I'm dealing with the usual post-PTO
email apocalypse.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-12 20:43 [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants Rebecca Cran
` (2 preceding siblings ...)
2021-06-22 17:57 ` Laszlo Ersek
@ 2021-06-24 7:37 ` Laszlo Ersek
2021-06-24 8:03 ` Laszlo Ersek
4 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-24 7:37 UTC (permalink / raw)
To: devel, rebecca, Peter Grehan, Ard Biesheuvel, Jordan Justen
On 06/12/21 22:43, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
>
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> 2 files changed, 79 deletions(-)
>
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d8792812ab..cbf896e89b 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -31,8 +31,6 @@
> DEFINE SECURE_BOOT_ENABLE = FALSE
> DEFINE SMM_REQUIRE = FALSE
> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> - DEFINE TPM_ENABLE = FALSE
> - DEFINE TPM_CONFIG_ENABLE = FALSE
>
> #
> # Network definition
> @@ -221,16 +219,8 @@
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>
> -
> -!if $(TPM_ENABLE) == TRUE
> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> -!else
> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> -!endif
>
> [LibraryClasses.common]
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -292,11 +282,6 @@
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>
> -!if $(TPM_ENABLE) == TRUE
> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> -!endif
> -
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>
> [LibraryClasses.common.DXE_CORE]
> @@ -366,9 +351,6 @@
> !endif
> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> -!if $(TPM_ENABLE) == TRUE
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> -!endif
>
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -563,22 +545,12 @@
>
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>
> -!if $(TPM_ENABLE) == TRUE
> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> -!endif
> -
> # MdeModulePkg resolution sets up the system display resolution
> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>
> -[PcdsDynamicHii]
> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> - gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -!endif
> -
> ################################################################################
> #
> # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -618,19 +590,6 @@
> <LibraryClasses>
> }
>
> -!if $(TPM_ENABLE) == TRUE
> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> - <LibraryClasses>
> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> - }
> -!endif
> -
> #
> # DXE Phase modules
> #
> @@ -653,9 +612,6 @@
> <LibraryClasses>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -!endif
> -!if $(TPM_ENABLE) == TRUE
> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> !endif
> }
>
> @@ -841,23 +797,3 @@
> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> }
>
> -
> - #
> - # TPM support
> - #
> -!if $(TPM_ENABLE) == TRUE
> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> - <LibraryClasses>
> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> - }
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 3eff36dac1..fbd63a395a 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> !endif
>
> -!if $(TPM_ENABLE) == TRUE
> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> -!endif
> -
> ################################################################################
>
> [FV.DXEFV]
> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> !endif
>
> -#
> -# TPM support
> -#
> -!if $(TPM_ENABLE) == TRUE
> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> -
> ################################################################################
>
> [FV.FVMAIN_COMPACT]
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
2021-06-12 20:43 [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants Rebecca Cran
` (3 preceding siblings ...)
2021-06-24 7:37 ` [edk2-devel] " Laszlo Ersek
@ 2021-06-24 8:03 ` Laszlo Ersek
4 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2021-06-24 8:03 UTC (permalink / raw)
To: devel, rebecca, Peter Grehan, Ard Biesheuvel, Jordan Justen
On 06/12/21 22:43, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
>
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> 2 files changed, 79 deletions(-)
Merged as commit 12e34cd2f790, via
<https://github.com/tianocore/edk2/pull/1759>.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread