[PATCH v1 0/3] EmbeddedPkg: Enable CI

[PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046

Adds EmbeddedPkg to edk2 CI.

Due to a number of build errors (some intentional) such as
'BUILD_EPOCH' only being defined for GCC in
VirtualRealTimeClockLib.inf, the package is only run on GCC
build agents.

This still allows build to be verified and non-build CI checks
to be performed.

In the edk2 PR for this change, you can see that the package only
runs on GCC and CI passes with this configuration.

https://github.com/tianocore/edk2/pull/3299

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Michael Kubacki (3):
  EmbeddedPkg/AcpiLib: Fix code formatting errors
  EmbeddedPkg: Add CI YAML file
  EmbeddedPkg: Only run in CI for GCC5

 EmbeddedPkg/Library/AcpiLib/AcpiLib.c           | 88 +++++++++----------
 .azurepipelines/templates/pr-gate-build-job.yml |  4 +
 .pytool/CISettings.py                           |  1 +
 EmbeddedPkg/EmbeddedPkg.ci.yaml                 | 89 ++++++++++++++++++++
 EmbeddedPkg/EmbeddedPkg.dec                     |  8 ++
 EmbeddedPkg/Include/Library/AcpiLib.h           | 22 ++---
 6 files changed, 158 insertions(+), 54 deletions(-)
 create mode 100644 EmbeddedPkg/EmbeddedPkg.ci.yaml

-- 
2.28.0.windows.1

[PATCH v1 1/3] EmbeddedPkg/AcpiLib: Fix code formatting errors

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046

This package did not have CI enabled so code changes were merged
that fail uncrustify formatting. This change updates those files
to include uncustify formatting.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 EmbeddedPkg/Library/AcpiLib/AcpiLib.c | 88 ++++++++++----------
 EmbeddedPkg/Include/Library/AcpiLib.h | 22 ++---
 2 files changed, 56 insertions(+), 54 deletions(-)

diff --git a/EmbeddedPkg/Library/AcpiLib/AcpiLib.c b/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
index ea2ad63b2039..cb593a7b2a3a 100644
--- a/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
+++ b/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
@@ -192,13 +192,13 @@ LocateAndInstallAcpiFromFv (
 EFI_STATUS
 EFIAPI
 AcpiUpdateChecksum (
-  IN OUT  UINT8      *Buffer,
-  IN      UINTN      Size
+  IN OUT  UINT8  *Buffer,
+  IN      UINTN  Size
   )
 {
-  UINTN ChecksumOffset;
+  UINTN  ChecksumOffset;
 
-  if (Buffer == NULL || Size == 0) {
+  if ((Buffer == NULL) || (Size == 0)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -237,11 +237,11 @@ AcpiUpdateChecksum (
 EFI_STATUS
 EFIAPI
 AcpiLocateTableBySignature (
-  IN      EFI_ACPI_SDT_PROTOCOL           *AcpiSdtProtocol,
-  IN      UINT32                          TableSignature,
-  IN OUT  UINTN                           *Index,
-  OUT     EFI_ACPI_DESCRIPTION_HEADER     **Table,
-  OUT     UINTN                           *TableKey
+  IN      EFI_ACPI_SDT_PROTOCOL        *AcpiSdtProtocol,
+  IN      UINT32                       TableSignature,
+  IN OUT  UINTN                        *Index,
+  OUT     EFI_ACPI_DESCRIPTION_HEADER  **Table,
+  OUT     UINTN                        *TableKey
   )
 {
   EFI_STATUS              Status;
@@ -249,9 +249,10 @@ AcpiLocateTableBySignature (
   EFI_ACPI_TABLE_VERSION  TableVersion;
   UINTN                   TableIndex;
 
-  if (AcpiSdtProtocol == NULL
-      || Table == NULL
-      || TableKey == NULL) {
+  if (  (AcpiSdtProtocol == NULL)
+     || (Table == NULL)
+     || (TableKey == NULL))
+  {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -261,7 +262,7 @@ AcpiLocateTableBySignature (
   // Search for ACPI Table with matching signature
   //
   TableVersion = 0;
-  TableIndex = *Index;
+  TableIndex   = *Index;
   while (!EFI_ERROR (Status)) {
     Status = AcpiSdtProtocol->GetAcpiTable (
                                 TableIndex,
@@ -301,26 +302,26 @@ AcpiLocateTableBySignature (
 EFI_STATUS
 EFIAPI
 AcpiAmlObjectUpdateInteger (
-  IN  EFI_ACPI_SDT_PROTOCOL           *AcpiSdtProtocol,
-  IN  EFI_ACPI_HANDLE                 TableHandle,
-  IN  CHAR8                           *AsciiObjectPath,
-  IN  UINTN                           Value
+  IN  EFI_ACPI_SDT_PROTOCOL  *AcpiSdtProtocol,
+  IN  EFI_ACPI_HANDLE        TableHandle,
+  IN  CHAR8                  *AsciiObjectPath,
+  IN  UINTN                  Value
   )
 {
-  EFI_STATUS            Status;
-  EFI_ACPI_HANDLE       ObjectHandle;
-  EFI_ACPI_HANDLE       DataHandle;
-  EFI_ACPI_DATA_TYPE    DataType;
-  UINT8                 *Buffer;
-  UINTN                 BufferSize;
-  UINTN                 DataSize;
+  EFI_STATUS          Status;
+  EFI_ACPI_HANDLE     ObjectHandle;
+  EFI_ACPI_HANDLE     DataHandle;
+  EFI_ACPI_DATA_TYPE  DataType;
+  UINT8               *Buffer;
+  UINTN               BufferSize;
+  UINTN               DataSize;
 
-  if (AcpiSdtProtocol == NULL || AsciiObjectPath == NULL) {
+  if ((AcpiSdtProtocol == NULL) || (AsciiObjectPath == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
 
   ObjectHandle = NULL;
-  DataHandle = NULL;
+  DataHandle   = NULL;
 
   Status = AcpiSdtProtocol->FindPath (TableHandle, AsciiObjectPath, &ObjectHandle);
   if (EFI_ERROR (Status)) {
@@ -332,6 +333,7 @@ AcpiAmlObjectUpdateInteger (
     Status = EFI_NOT_FOUND;
     goto Exit;
   }
+
   ASSERT (DataType == EFI_ACPI_DATA_TYPE_OPCODE);
   ASSERT (Buffer != NULL);
 
@@ -350,7 +352,7 @@ AcpiAmlObjectUpdateInteger (
   ASSERT (DataType == EFI_ACPI_DATA_TYPE_OPCODE);
   ASSERT (Buffer != NULL);
 
-  if (Buffer[0] == AML_ZERO_OP || Buffer[0] == AML_ONE_OP) {
+  if ((Buffer[0] == AML_ZERO_OP) || (Buffer[0] == AML_ONE_OP)) {
     Status = AcpiSdtProtocol->SetOption (DataHandle, 0, (VOID *)&Value, sizeof (UINT8));
     ASSERT_EFI_ERROR (Status);
   } else {
@@ -358,26 +360,26 @@ AcpiAmlObjectUpdateInteger (
     // Check the size of data object
     //
     switch (Buffer[0]) {
-    case AML_BYTE_PREFIX:
-      DataSize = sizeof (UINT8);
-      break;
+      case AML_BYTE_PREFIX:
+        DataSize = sizeof (UINT8);
+        break;
 
-    case AML_WORD_PREFIX:
-      DataSize = sizeof (UINT16);
-      break;
+      case AML_WORD_PREFIX:
+        DataSize = sizeof (UINT16);
+        break;
 
-    case AML_DWORD_PREFIX:
-      DataSize = sizeof (UINT32);
-      break;
+      case AML_DWORD_PREFIX:
+        DataSize = sizeof (UINT32);
+        break;
 
-    case AML_QWORD_PREFIX:
-      DataSize = sizeof (UINT64);
-      break;
+      case AML_QWORD_PREFIX:
+        DataSize = sizeof (UINT64);
+        break;
 
-    default:
-      // The data type of the ACPI object is not an integer
-      Status = EFI_INVALID_PARAMETER;
-      goto Exit;
+      default:
+        // The data type of the ACPI object is not an integer
+        Status = EFI_INVALID_PARAMETER;
+        goto Exit;
     }
 
     Status = AcpiSdtProtocol->SetOption (DataHandle, 1, (VOID *)&Value, DataSize);
diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h b/EmbeddedPkg/Include/Library/AcpiLib.h
index 9dbacd85b0ef..29137a4a53df 100644
--- a/EmbeddedPkg/Include/Library/AcpiLib.h
+++ b/EmbeddedPkg/Include/Library/AcpiLib.h
@@ -144,8 +144,8 @@ LocateAndInstallAcpiFromFv (
 EFI_STATUS
 EFIAPI
 AcpiUpdateChecksum (
-  IN OUT  UINT8      *Buffer,
-  IN      UINTN      Size
+  IN OUT  UINT8  *Buffer,
+  IN      UINTN  Size
   );
 
 /**
@@ -168,11 +168,11 @@ AcpiUpdateChecksum (
 EFI_STATUS
 EFIAPI
 AcpiLocateTableBySignature (
-  IN      EFI_ACPI_SDT_PROTOCOL           *AcpiSdtProtocol,
-  IN      UINT32                          TableSignature,
-  IN OUT  UINTN                           *Index,
-  OUT     EFI_ACPI_DESCRIPTION_HEADER     **Table,
-  OUT     UINTN                           *TableKey
+  IN      EFI_ACPI_SDT_PROTOCOL        *AcpiSdtProtocol,
+  IN      UINT32                       TableSignature,
+  IN OUT  UINTN                        *Index,
+  OUT     EFI_ACPI_DESCRIPTION_HEADER  **Table,
+  OUT     UINTN                        *TableKey
   );
 
 /**
@@ -193,10 +193,10 @@ AcpiLocateTableBySignature (
 EFI_STATUS
 EFIAPI
 AcpiAmlObjectUpdateInteger (
-  IN  EFI_ACPI_SDT_PROTOCOL           *AcpiSdtProtocol,
-  IN  EFI_ACPI_HANDLE                 TableHandle,
-  IN  CHAR8                           *AsciiObjectPath,
-  IN  UINTN                           Value
+  IN  EFI_ACPI_SDT_PROTOCOL  *AcpiSdtProtocol,
+  IN  EFI_ACPI_HANDLE        TableHandle,
+  IN  CHAR8                  *AsciiObjectPath,
+  IN  UINTN                  Value
   );
 
 #endif // __ACPI_LIB_H__
-- 
2.28.0.windows.1

[PATCH v1 2/3] EmbeddedPkg: Add CI YAML file

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046

Adds EmbeddedPkg to the list of supported build packages for edk2
CI and fixes Library Class Check errors reported.

These changes allow EmbeddedPkg to pass NO-TARGET CI testing.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 .pytool/CISettings.py           |  1 +
 EmbeddedPkg/EmbeddedPkg.ci.yaml | 89 ++++++++++++++++++++
 EmbeddedPkg/EmbeddedPkg.dec     |  8 ++
 3 files changed, 98 insertions(+)

diff --git a/.pytool/CISettings.py b/.pytool/CISettings.py
index cf9e0d77b19b..aef850a54549 100644
--- a/.pytool/CISettings.py
+++ b/.pytool/CISettings.py
@@ -53,6 +53,7 @@ class Settings(CiBuildSettingsManager, UpdateSettingsManager, SetupSettingsManag
                 "ArmPlatformPkg",
                 "ArmVirtPkg",
                 "DynamicTablesPkg",
+                "EmbeddedPkg",
                 "EmulatorPkg",
                 "MdePkg",
                 "MdeModulePkg",
diff --git a/EmbeddedPkg/EmbeddedPkg.ci.yaml b/EmbeddedPkg/EmbeddedPkg.ci.yaml
new file mode 100644
index 000000000000..21f30108a29f
--- /dev/null
+++ b/EmbeddedPkg/EmbeddedPkg.ci.yaml
@@ -0,0 +1,89 @@
+## @file
+# Core CI configuration for EmbeddedPkg
+#
+# Copyright (c) Microsoft Corporation
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+{
+    ## options defined .pytool/Plugin/LicenseCheck
+    "LicenseCheck": {
+        "IgnoreFiles": []
+    },
+
+    "EccCheck": {
+        ## Exception sample looks like below:
+        ## "ExceptionList": [
+        ##     "<ErrorID>", "<KeyWord>"
+        ## ]
+        "ExceptionList": [
+        ],
+        ## Both file path and directory path are accepted.
+        "IgnoreFiles": []
+    },
+
+    ## options defined .pytool/Plugin/CompilerPlugin
+    "CompilerPlugin": {
+        "DscPath": "EmbeddedPkg.dsc"
+    },
+
+    ## options defined .pytool/Plugin/HostUnitTestCompilerPlugin
+    "HostUnitTestCompilerPlugin": {
+        "DscPath": "" # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/CharEncodingCheck
+    "CharEncodingCheck": {
+        "IgnoreFiles": []
+    },
+
+    ## options defined .pytool/Plugin/DependencyCheck
+    "DependencyCheck": {
+        "AcceptableDependencies": [
+            "ArmPkg/ArmPkg.dec",
+            "ArmPlatformPkg/ArmPlatformPkg.dec",
+            "EmbeddedPkg/EmbeddedPkg.dec",
+            "MdeModulePkg/MdeModulePkg.dec",
+            "MdePkg/MdePkg.dec"
+        ],
+        # For host based unit tests
+        "AcceptableDependencies-HOST_APPLICATION":[],
+        # For UEFI shell based apps
+        "AcceptableDependencies-UEFI_APPLICATION":[],
+        "IgnoreInf": []
+    },
+
+    ## options defined .pytool/Plugin/DscCompleteCheck
+    "DscCompleteCheck": {
+        "IgnoreInf": [""],
+        "DscPath": ""  # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/HostUnitTestDscCompleteCheck
+    "HostUnitTestDscCompleteCheck": {
+        "IgnoreInf": [""],
+        "DscPath": "" # Don't support this test
+    },
+
+    ## options defined .pytool/Plugin/GuidCheck
+    "GuidCheck": {
+        "IgnoreGuidName": [],
+        "IgnoreGuidValue": [],
+        "IgnoreFoldersAndFiles": [],
+        "IgnoreDuplicates": [],
+    },
+
+    ## options defined .pytool/Plugin/LibraryClassCheck
+    "LibraryClassCheck": {
+        "IgnoreHeaderFile": []
+    },
+
+    ## options defined .pytool/Plugin/SpellCheck
+    "SpellCheck": {
+        "AuditOnly": True,           # Fails right now with over 270 errors
+        "IgnoreFiles": [],           # use gitignore syntax to ignore errors in matching files
+        "ExtendWords": [],           # words to extend to the dictionary for this package
+        "IgnoreStandardPaths": [],   # Standard Plugin defined paths that should be ignore
+        "AdditionalIncludePaths": [] # Additional paths to spell check (wildcards supported)
+    }
+}
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 637888e0fd4f..341ef5e6a679 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -36,6 +36,14 @@ [LibraryClasses.common]
   GdbSerialLib|Include/Library/GdbSerialLib.h
   DebugAgentTimerLib|Include/Library/DebugAgentTimerLib.h
   NorFlashInfoLib|Include/Library/NorFlashInfoLib.h
+  HalRuntimeServicesLib|Include/Library/HalRuntimeServicesLib.h
+  PrePiHobListPointerLib|Include/Library/PrePiHobListPointerLib.h
+  TimeBaseLib|Include/Library/TimeBaseLib.h
+  AcpiLib|Include/Library/AcpiLib.h
+  AndroidBootImgLib|Include/Library/AndroidBootImgLib.h
+  DmaLib|Include/Library/DmaLib.h
+  EfiFileLib|Include/Library/EfiFileLib.h
+  FdtLoadLib|Include/Library/FdtLoadLib.h
 
   DtPlatformDtbLoaderLib|Include/Library/DtPlatformDtbLoaderLib.h
 
-- 
2.28.0.windows.1

[PATCH v1 3/3] EmbeddedPkg: Only run in CI for GCC5

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046

This package currently does not build on non-GCC toolchains.

This change adds the package to edk2 CI so it can benefit from
ongoing CI and only tests the package against GCC.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 .azurepipelines/templates/pr-gate-build-job.yml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
index 0e4ad019bf03..54a74a1a9873 100644
--- a/.azurepipelines/templates/pr-gate-build-job.yml
+++ b/.azurepipelines/templates/pr-gate-build-job.yml
@@ -21,6 +21,10 @@ jobs:
   #Use matrix to speed up the build process
   strategy:
     matrix:
+      ${{ if eq(parameters.tool_chain_tag, 'GCC5') }}:
+        TARGET_GCC_ONLY:
+          Build.Pkgs: 'EmbeddedPkg'
+          Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
       TARGET_ARM_ARMPLATFORM:
         Build.Pkgs: 'ArmPkg,ArmPlatformPkg'
         Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
-- 
2.28.0.windows.1

Re: [edk2-devel] [PATCH v1 3/3] EmbeddedPkg: Only run in CI for GCC5

[Michael Kubacki]

I forgot to add .azurepipelines maintainers to this patch. To avoid 
resending the whole series to the list +Cc here and v2 will include the 
maintainers directly on this patch.

Thanks,
Michael

On 9/6/2022 10:36 PM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046
> 
> This package currently does not build on non-GCC toolchains.
> 
> This change adds the package to edk2 CI so it can benefit from
> ongoing CI and only tests the package against GCC.
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>   .azurepipelines/templates/pr-gate-build-job.yml | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
> index 0e4ad019bf03..54a74a1a9873 100644
> --- a/.azurepipelines/templates/pr-gate-build-job.yml
> +++ b/.azurepipelines/templates/pr-gate-build-job.yml
> @@ -21,6 +21,10 @@ jobs:
>     #Use matrix to speed up the build process
>     strategy:
>       matrix:
> +      ${{ if eq(parameters.tool_chain_tag, 'GCC5') }}:
> +        TARGET_GCC_ONLY:
> +          Build.Pkgs: 'EmbeddedPkg'
> +          Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
>         TARGET_ARM_ARMPLATFORM:
>           Build.Pkgs: 'ArmPkg,ArmPlatformPkg'
>           Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'

Re: [edk2-devel] [PATCH v1 3/3] EmbeddedPkg: Only run in CI for GCC5

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Tuesday, September 6, 2022 8:27 PM
> To: devel@edk2.groups.io
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang <abner.chang@amd.com>;
> Daniel Schaefer <git@danielschaefer.me>; Sean Brogan <sean.brogan@microsoft.com>; Barkelew, Bret
> <bret.barkelew@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel] [PATCH v1 3/3] EmbeddedPkg: Only run in CI for GCC5
> 
> I forgot to add .azurepipelines maintainers to this patch. To avoid
> resending the whole series to the list +Cc here and v2 will include the
> maintainers directly on this patch.
> 
> Thanks,
> Michael
> 
> On 9/6/2022 10:36 PM, Michael Kubacki wrote:
> > From: Michael Kubacki <michael.kubacki@microsoft.com>
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046
> >
> > This package currently does not build on non-GCC toolchains.
> >
> > This change adds the package to edk2 CI so it can benefit from
> > ongoing CI and only tests the package against GCC.
> >
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Abner Chang <abner.chang@amd.com>
> > Cc: Daniel Schaefer <git@danielschaefer.me>
> > Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > ---
> >   .azurepipelines/templates/pr-gate-build-job.yml | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
> > index 0e4ad019bf03..54a74a1a9873 100644
> > --- a/.azurepipelines/templates/pr-gate-build-job.yml
> > +++ b/.azurepipelines/templates/pr-gate-build-job.yml
> > @@ -21,6 +21,10 @@ jobs:
> >     #Use matrix to speed up the build process
> >     strategy:
> >       matrix:
> > +      ${{ if eq(parameters.tool_chain_tag, 'GCC5') }}:
> > +        TARGET_GCC_ONLY:
> > +          Build.Pkgs: 'EmbeddedPkg'
> > +          Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'
> >         TARGET_ARM_ARMPLATFORM:
> >           Build.Pkgs: 'ArmPkg,ArmPlatformPkg'
> >           Build.Targets: 'DEBUG,RELEASE,NO-TARGET,NOOPT'

Re: [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Ard Biesheuvel]

On Wed, 7 Sept 2022 at 04:37, <mikuback@linux.microsoft.com> wrote:
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046
>
> Adds EmbeddedPkg to edk2 CI.
>
> Due to a number of build errors (some intentional) such as
> 'BUILD_EPOCH' only being defined for GCC in
> VirtualRealTimeClockLib.inf, the package is only run on GCC
> build agents.
>
> This still allows build to be verified and non-build CI checks
> to be performed.
>
> In the edk2 PR for this change, you can see that the package only
> runs on GCC and CI passes with this configuration.
>
> https://github.com/tianocore/edk2/pull/3299
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Michael Kubacki (3):
>   EmbeddedPkg/AcpiLib: Fix code formatting errors
>   EmbeddedPkg: Add CI YAML file
>   EmbeddedPkg: Only run in CI for GCC5
>

NAK until we have a discussion about strictness of CI and ways to
permit manual override of merge time CI restrictions.


>  EmbeddedPkg/Library/AcpiLib/AcpiLib.c           | 88 +++++++++----------
>  .azurepipelines/templates/pr-gate-build-job.yml |  4 +
>  .pytool/CISettings.py                           |  1 +
>  EmbeddedPkg/EmbeddedPkg.ci.yaml                 | 89 ++++++++++++++++++++
>  EmbeddedPkg/EmbeddedPkg.dec                     |  8 ++
>  EmbeddedPkg/Include/Library/AcpiLib.h           | 22 ++---
>  6 files changed, 158 insertions(+), 54 deletions(-)
>  create mode 100644 EmbeddedPkg/EmbeddedPkg.ci.yaml
>
> --
> 2.28.0.windows.1
>

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael Kubacki]

When would you like to have that discussion?

The Tianocore Tool, CI, Codebase meeting is every week. In that meeting 
we've discussed getting all edk2 packages to at least run CI.

https://github.com/tianocore/edk2/discussions/2614

If you prefer to have it here, that's fine as well.

On 9/7/2022 3:37 AM, Ard Biesheuvel wrote:
> On Wed, 7 Sept 2022 at 04:37, <mikuback@linux.microsoft.com> wrote:
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4046
>>
>> Adds EmbeddedPkg to edk2 CI.
>>
>> Due to a number of build errors (some intentional) such as
>> 'BUILD_EPOCH' only being defined for GCC in
>> VirtualRealTimeClockLib.inf, the package is only run on GCC
>> build agents.
>>
>> This still allows build to be verified and non-build CI checks
>> to be performed.
>>
>> In the edk2 PR for this change, you can see that the package only
>> runs on GCC and CI passes with this configuration.
>>
>> https://github.com/tianocore/edk2/pull/3299
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Abner Chang <abner.chang@amd.com>
>> Cc: Daniel Schaefer <git@danielschaefer.me>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Michael Kubacki (3):
>>    EmbeddedPkg/AcpiLib: Fix code formatting errors
>>    EmbeddedPkg: Add CI YAML file
>>    EmbeddedPkg: Only run in CI for GCC5
>>
> 
> NAK until we have a discussion about strictness of CI and ways to
> permit manual override of merge time CI restrictions.
> 
> 
>>   EmbeddedPkg/Library/AcpiLib/AcpiLib.c           | 88 +++++++++----------
>>   .azurepipelines/templates/pr-gate-build-job.yml |  4 +
>>   .pytool/CISettings.py                           |  1 +
>>   EmbeddedPkg/EmbeddedPkg.ci.yaml                 | 89 ++++++++++++++++++++
>>   EmbeddedPkg/EmbeddedPkg.dec                     |  8 ++
>>   EmbeddedPkg/Include/Library/AcpiLib.h           | 22 ++---
>>   6 files changed, 158 insertions(+), 54 deletions(-)
>>   create mode 100644 EmbeddedPkg/EmbeddedPkg.ci.yaml
>>
>> --
>> 2.28.0.windows.1
>>
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Ard Biesheuvel]

On Wed, 7 Sept 2022 at 17:00, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> When would you like to have that discussion?
>
> The Tianocore Tool, CI, Codebase meeting is every week. In that meeting
> we've discussed getting all edk2 packages to at least run CI.
>
> https://github.com/tianocore/edk2/discussions/2614
>
> If you prefer to have it here, that's fine as well.
>

In a nutshell, I am fine with enabling this, as long as I can override
the CI and merge PRs that the CI thinks have issues.

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael Kubacki]

Hi Ard,

I haven't seen any action items for the v1 series.

Can you please check the series again and let me know if you have any 
further concerns?

Thanks,
Michael

On 9/7/2022 11:16 AM, Ard Biesheuvel wrote:
> On Wed, 7 Sept 2022 at 17:00, Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
>>
>> When would you like to have that discussion?
>>
>> The Tianocore Tool, CI, Codebase meeting is every week. In that meeting
>> we've discussed getting all edk2 packages to at least run CI.
>>
>> https://github.com/tianocore/edk2/discussions/2614
>>
>> If you prefer to have it here, that's fine as well.
>>
> 
> In a nutshell, I am fine with enabling this, as long as I can override
> the CI and merge PRs that the CI thinks have issues.
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Ard Biesheuvel]

On Thu, 15 Sept 2022 at 21:46, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> Hi Ard,
>
> I haven't seen any action items for the v1 series.
>
> Can you please check the series again and let me know if you have any
> further concerns?
>

The only thing I'd like to know is how I can override the CI and merge
a PR that was rejected by the checks you are enabling here.




> On 9/7/2022 11:16 AM, Ard Biesheuvel wrote:
> > On Wed, 7 Sept 2022 at 17:00, Michael Kubacki
> > <mikuback@linux.microsoft.com> wrote:
> >>
> >> When would you like to have that discussion?
> >>
> >> The Tianocore Tool, CI, Codebase meeting is every week. In that meeting
> >> we've discussed getting all edk2 packages to at least run CI.
> >>
> >> https://github.com/tianocore/edk2/discussions/2614
> >>
> >> If you prefer to have it here, that's fine as well.
> >>
> >
> > In a nutshell, I am fine with enabling this, as long as I can override
> > the CI and merge PRs that the CI thinks have issues.
> >
> >
> > 
> >

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael D Kinney]

Ard,

Why would you want to do that?  The whole point of CI is to establish a minimum quality level for all code in the project.

They can be disabled with updates to the YAML file.  Checks can be disabled completely and may of the checks support exception lists.

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, September 15, 2022 1:47 PM
> To: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: devel@edk2.groups.io; Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang
> <abner.chang@amd.com>; Daniel Schaefer <git@danielschaefer.me>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI
> 
> On Thu, 15 Sept 2022 at 21:46, Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
> >
> > Hi Ard,
> >
> > I haven't seen any action items for the v1 series.
> >
> > Can you please check the series again and let me know if you have any
> > further concerns?
> >
> 
> The only thing I'd like to know is how I can override the CI and merge
> a PR that was rejected by the checks you are enabling here.
> 
> 
> 
> 
> > On 9/7/2022 11:16 AM, Ard Biesheuvel wrote:
> > > On Wed, 7 Sept 2022 at 17:00, Michael Kubacki
> > > <mikuback@linux.microsoft.com> wrote:
> > >>
> > >> When would you like to have that discussion?
> > >>
> > >> The Tianocore Tool, CI, Codebase meeting is every week. In that meeting
> > >> we've discussed getting all edk2 packages to at least run CI.
> > >>
> > >> https://github.com/tianocore/edk2/discussions/2614
> > >>
> > >> If you prefer to have it here, that's fine as well.
> > >>
> > >
> > > In a nutshell, I am fine with enabling this, as long as I can override
> > > the CI and merge PRs that the CI thinks have issues.
> > >
> > >
> > > 
> > >

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Ard Biesheuvel]

On Thu, 15 Sept 2022 at 22:52, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> Why would you want to do that?  The whole point of CI is to establish a minimum quality level for all code in the project.
>
> They can be disabled with updates to the YAML file.  Checks can be disabled completely and may of the checks support exception lists.
>

If the only way to prevent this from happening is to turn it off again
in the YAML file, I'd prefer not to turn it on to begin with.

I agree that code quality is important, but IMO the checks we have at
the moment are way too strict, and 90% of the time I spend on
reviewing and merging patches is on crustify and patchcheck errors.
This is simply not worth my time.

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael D Kinney]

Hi Ard,

If there is content you do not think needs to follow the min quality criteria, perhaps it can be moved out of edk2 repo?  Maybe to edk2-staging or edk2-archive?

Thanks,

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, September 15, 2022 2:03 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@quicinc.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang <abner.chang@amd.com>; Daniel Schaefer <git@danielschaefer.me>
> Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI
> 
> On Thu, 15 Sept 2022 at 22:52, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Ard,
> >
> > Why would you want to do that?  The whole point of CI is to establish a minimum quality level for all code in the project.
> >
> > They can be disabled with updates to the YAML file.  Checks can be disabled completely and may of the checks support
> exception lists.
> >
> 
> If the only way to prevent this from happening is to turn it off again
> in the YAML file, I'd prefer not to turn it on to begin with.
> 
> I agree that code quality is important, but IMO the checks we have at
> the moment are way too strict, and 90% of the time I spend on
> reviewing and merging patches is on crustify and patchcheck errors.
> This is simply not worth my time.

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael Kubacki]

What are the next steps?

Thanks,
Michael

On 9/15/2022 5:54 PM, Kinney, Michael D wrote:
> Hi Ard,
> 
> If there is content you do not think needs to follow the min quality criteria, perhaps it can be moved out of edk2 repo?  Maybe to edk2-staging or edk2-archive?
> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: Ard Biesheuvel <ardb@kernel.org>
>> Sent: Thursday, September 15, 2022 2:03 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@quicinc.com>; Ard
>> Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang <abner.chang@amd.com>; Daniel Schaefer <git@danielschaefer.me>
>> Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI
>>
>> On Thu, 15 Sept 2022 at 22:52, Kinney, Michael D
>> <michael.d.kinney@intel.com> wrote:
>>>
>>> Ard,
>>>
>>> Why would you want to do that?  The whole point of CI is to establish a minimum quality level for all code in the project.
>>>
>>> They can be disabled with updates to the YAML file.  Checks can be disabled completely and may of the checks support
>> exception lists.
>>>
>>
>> If the only way to prevent this from happening is to turn it off again
>> in the YAML file, I'd prefer not to turn it on to begin with.
>>
>> I agree that code quality is important, but IMO the checks we have at
>> the moment are way too strict, and 90% of the time I spend on
>> reviewing and merging patches is on crustify and patchcheck errors.
>> This is simply not worth my time.

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Ard Biesheuvel]

On Fri, 23 Sept 2022 at 03:09, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> What are the next steps?
>

As long as we are using the most lenient setting, I'm ok with this

Acked-by: Ard Biesheuvel <ardb@kernel.org>


> On 9/15/2022 5:54 PM, Kinney, Michael D wrote:
> > Hi Ard,
> >
> > If there is content you do not think needs to follow the min quality criteria, perhaps it can be moved out of edk2 repo?  Maybe to edk2-staging or edk2-archive?
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel <ardb@kernel.org>
> >> Sent: Thursday, September 15, 2022 2:03 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>
> >> Cc: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@quicinc.com>; Ard
> >> Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang <abner.chang@amd.com>; Daniel Schaefer <git@danielschaefer.me>
> >> Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI
> >>
> >> On Thu, 15 Sept 2022 at 22:52, Kinney, Michael D
> >> <michael.d.kinney@intel.com> wrote:
> >>>
> >>> Ard,
> >>>
> >>> Why would you want to do that?  The whole point of CI is to establish a minimum quality level for all code in the project.
> >>>
> >>> They can be disabled with updates to the YAML file.  Checks can be disabled completely and may of the checks support
> >> exception lists.
> >>>
> >>
> >> If the only way to prevent this from happening is to turn it off again
> >> in the YAML file, I'd prefer not to turn it on to begin with.
> >>
> >> I agree that code quality is important, but IMO the checks we have at
> >> the moment are way too strict, and 90% of the time I spend on
> >> reviewing and merging patches is on crustify and patchcheck errors.
> >> This is simply not worth my time.

Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

[Michael D Kinney]

Thanks Ard.  

Let us know after the merge if this level of checks is an issue.
We do want to make it as easy as possible for all developers to
meet the min quality criteria.

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Friday, September 23, 2022 4:47 AM
> To: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Abner Chang <abner.chang@amd.com>; Daniel Schaefer <git@danielschaefer.me>
> Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI
> 
> On Fri, 23 Sept 2022 at 03:09, Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
> >
> > What are the next steps?
> >
> 
> As long as we are using the most lenient setting, I'm ok with this
> 
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> 
> 
> > On 9/15/2022 5:54 PM, Kinney, Michael D wrote:
> > > Hi Ard,
> > >
> > > If there is content you do not think needs to follow the min quality criteria, perhaps it can be moved out of edk2 repo?  Maybe
> to edk2-staging or edk2-archive?
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > >> -----Original Message-----
> > >> From: Ard Biesheuvel <ardb@kernel.org>
> > >> Sent: Thursday, September 15, 2022 2:03 PM
> > >> To: Kinney, Michael D <michael.d.kinney@intel.com>
> > >> Cc: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@quicinc.com>; Ard
> > >> Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang <abner.chang@amd.com>; Daniel Schaefer <git@danielschaefer.me>
> > >> Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI
> > >>
> > >> On Thu, 15 Sept 2022 at 22:52, Kinney, Michael D
> > >> <michael.d.kinney@intel.com> wrote:
> > >>>
> > >>> Ard,
> > >>>
> > >>> Why would you want to do that?  The whole point of CI is to establish a minimum quality level for all code in the project.
> > >>>
> > >>> They can be disabled with updates to the YAML file.  Checks can be disabled completely and may of the checks support
> > >> exception lists.
> > >>>
> > >>
> > >> If the only way to prevent this from happening is to turn it off again
> > >> in the YAML file, I'd prefer not to turn it on to begin with.
> > >>
> > >> I agree that code quality is important, but IMO the checks we have at
> > >> the moment are way too strict, and 90% of the time I spend on
> > >> reviewing and merging patches is on crustify and patchcheck errors.
> > >> This is simply not worth my time.
> 
> 
> 
>