[PATCH v2 1/2] BaseTools/Plugin: Report error if code coverage failure

[PATCH v2 1/2] BaseTools/Plugin: Report error if code coverage failure

[Guo, Gua]

From: Gua Guo <gua.guo@intel.com>

If code coverage exist failure, CI/CD need to catch it

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Signed-off-by: Gua Guo <gua.guo@intel.com>
---
 .../HostBasedUnitTestRunner/HostBasedUnitTestRunner.py    | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py b/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
index a384b55629..d993de9412 100644
--- a/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
+++ b/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
@@ -130,9 +130,13 @@ class HostBasedUnitTestRunner(IUefiBuildPlugin):
 
             if thebuilder.env.GetValue("CODE_COVERAGE") != "FALSE":
                 if thebuilder.env.GetValue("TOOL_CHAIN_TAG") == "GCC5":
-                    self.gen_code_coverage_gcc(thebuilder)
+                    ret = self.gen_code_coverage_gcc(thebuilder)
+                    if ret != 0:
+                        failure_count += 1
                 elif thebuilder.env.GetValue("TOOL_CHAIN_TAG").startswith ("VS"):
-                    self.gen_code_coverage_msvc(thebuilder)
+                    ret = self.gen_code_coverage_msvc(thebuilder)
+                    if ret != 0:
+                        failure_count += 1
                 else:
                     logging.info("Skipping code coverage. Currently, support GCC and MSVC compiler.")
 
-- 
2.39.2.windows.1

[PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

[Guo, Gua]

From: Gua Guo <gua.guo@intel.com>

Azure should install code coverage tool (lcov), it didn't
exist on Fedora and Ubuntu by default.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Oliver Steffen <osteffen@redhat.com>
Cc: Chris Fernald <chfernal@microsoft.com>
Signed-off-by: Gua Guo <gua.guo@intel.com>
---
 .azurepipelines/templates/defaults.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.azurepipelines/templates/defaults.yml b/.azurepipelines/templates/defaults.yml
index 74d6b41783..8412a43cad 100644
--- a/.azurepipelines/templates/defaults.yml
+++ b/.azurepipelines/templates/defaults.yml
@@ -9,4 +9,4 @@
 
 variables:
   default_python_version: ">=3.10.6"
-  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:47addc9"
+  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:3b3eb8f"
-- 
2.39.2.windows.1

Re: [PATCH v2 1/2] BaseTools/Plugin: Report error if code coverage failure

[Michael D Kinney]

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


> -----Original Message-----
> From: Guo, Gua <gua.guo@intel.com>
> Sent: Wednesday, April 26, 2023 5:34 PM
> To: devel@edk2.groups.io
> Cc: Guo, Gua <gua.guo@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Kubacki <mikuback@linux.microsoft.com>
> Subject: [PATCH v2 1/2] BaseTools/Plugin: Report error if code coverage failure
> 
> From: Gua Guo <gua.guo@intel.com>
> 
> If code coverage exist failure, CI/CD need to catch it
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>  .../HostBasedUnitTestRunner/HostBasedUnitTestRunner.py    | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git
> a/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> b/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> index a384b55629..d993de9412 100644
> --- a/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> +++
> b/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> @@ -130,9 +130,13 @@ class HostBasedUnitTestRunner(IUefiBuildPlugin):
> 
> 
>              if thebuilder.env.GetValue("CODE_COVERAGE") != "FALSE":
> 
>                  if thebuilder.env.GetValue("TOOL_CHAIN_TAG") == "GCC5":
> 
> -                    self.gen_code_coverage_gcc(thebuilder)
> 
> +                    ret = self.gen_code_coverage_gcc(thebuilder)
> 
> +                    if ret != 0:
> 
> +                        failure_count += 1
> 
>                  elif thebuilder.env.GetValue("TOOL_CHAIN_TAG").startswith ("VS"):
> 
> -                    self.gen_code_coverage_msvc(thebuilder)
> 
> +                    ret = self.gen_code_coverage_msvc(thebuilder)
> 
> +                    if ret != 0:
> 
> +                        failure_count += 1
> 
>                  else:
> 
>                      logging.info("Skipping code coverage. Currently, support GCC and
> MSVC compiler.")
> 
> 
> 
> --
> 2.39.2.windows.1

Re: [PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

[Michael D Kinney]

Hi Gua,

It is hard to review this type of patch without more context.

Can you provide a link to the PR or branch where the docker image was updated?  That information should be part of the commit message.

Thanks,

Mike

> -----Original Message-----
> From: Guo, Gua <gua.guo@intel.com>
> Sent: Wednesday, April 26, 2023 5:34 PM
> To: devel@edk2.groups.io
> Cc: Guo, Gua <gua.guo@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Kubacki <mikuback@linux.microsoft.com>; Oliver Steffen
> <osteffen@redhat.com>; fernald, chris <chfernal@microsoft.com>
> Subject: [PATCH v2 2/2] .azurepipelines: Choose container that have installed
> lcov
> 
> From: Gua Guo <gua.guo@intel.com>
> 
> Azure should install code coverage tool (lcov), it didn't
> exist on Fedora and Ubuntu by default.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Chris Fernald <chfernal@microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>  .azurepipelines/templates/defaults.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.azurepipelines/templates/defaults.yml
> b/.azurepipelines/templates/defaults.yml
> index 74d6b41783..8412a43cad 100644
> --- a/.azurepipelines/templates/defaults.yml
> +++ b/.azurepipelines/templates/defaults.yml
> @@ -9,4 +9,4 @@
> 
> 
>  variables:
> 
>    default_python_version: ">=3.10.6"
> 
> -  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:47addc9"
> 
> +  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:3b3eb8f"
> 
> --
> 2.39.2.windows.1

Re: [edk2-devel] [PATCH v2 1/2] BaseTools/Plugin: Report error if code coverage failure

[Michael Kubacki]

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 4/26/2023 8:34 PM, Guo, Gua wrote:
> From: Gua Guo <gua.guo@intel.com>
> 
> If code coverage exist failure, CI/CD need to catch it
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>   .../HostBasedUnitTestRunner/HostBasedUnitTestRunner.py    | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py b/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> index a384b55629..d993de9412 100644
> --- a/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> +++ b/BaseTools/Plugin/HostBasedUnitTestRunner/HostBasedUnitTestRunner.py
> @@ -130,9 +130,13 @@ class HostBasedUnitTestRunner(IUefiBuildPlugin):
>   
> 
>               if thebuilder.env.GetValue("CODE_COVERAGE") != "FALSE":
> 
>                   if thebuilder.env.GetValue("TOOL_CHAIN_TAG") == "GCC5":
> 
> -                    self.gen_code_coverage_gcc(thebuilder)
> 
> +                    ret = self.gen_code_coverage_gcc(thebuilder)
> 
> +                    if ret != 0:
> 
> +                        failure_count += 1
> 
>                   elif thebuilder.env.GetValue("TOOL_CHAIN_TAG").startswith ("VS"):
> 
> -                    self.gen_code_coverage_msvc(thebuilder)
> 
> +                    ret = self.gen_code_coverage_msvc(thebuilder)
> 
> +                    if ret != 0:
> 
> +                        failure_count += 1
> 
>                   else:
> 
>                       logging.info("Skipping code coverage. Currently, support GCC and MSVC compiler.")
> 
>   
> 

Re: [PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

[Guo, Gua]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

Sure, I will update the commit message.
________________________________
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Thursday, April 27, 2023 8:52:40 AM
To: Guo, Gua <gua.guo@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Oliver Steffen <osteffen@redhat.com>; fernald, chris <chfernal@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

Hi Gua,

It is hard to review this type of patch without more context.

Can you provide a link to the PR or branch where the docker image was updated?  That information should be part of the commit message.

Thanks,

Mike

> -----Original Message-----
> From: Guo, Gua <gua.guo@intel.com>
> Sent: Wednesday, April 26, 2023 5:34 PM
> To: devel@edk2.groups.io
> Cc: Guo, Gua <gua.guo@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Kubacki <mikuback@linux.microsoft.com>; Oliver Steffen
> <osteffen@redhat.com>; fernald, chris <chfernal@microsoft.com>
> Subject: [PATCH v2 2/2] .azurepipelines: Choose container that have installed
> lcov
>
> From: Gua Guo <gua.guo@intel.com>
>
> Azure should install code coverage tool (lcov), it didn't
> exist on Fedora and Ubuntu by default.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Chris Fernald <chfernal@microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>  .azurepipelines/templates/defaults.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.azurepipelines/templates/defaults.yml
> b/.azurepipelines/templates/defaults.yml
> index 74d6b41783..8412a43cad 100644
> --- a/.azurepipelines/templates/defaults.yml
> +++ b/.azurepipelines/templates/defaults.yml
> @@ -9,4 +9,4 @@
>
>
>  variables:
>
>    default_python_version: ">=3.10.6"
>
> -  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:47addc9"
>
> +  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:3b3eb8f"
>
> --
> 2.39.2.windows.1


[-- Attachment #2: Type: text/html, Size: 3384 bytes --]

Re: [PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

[Michael Kubacki]

I agree with Mike that there should be more context in the commit 
message about the change that added lcov installation to the container 
(https://github.com/tianocore/containers/commit/f1c7a20f641852034b0d7e8c1b209407c92560f2).

Ideally, a summary of changes from the previous container version to 
this one would be provided.

As for the container change itself, it looks fine and I'll add an R-b 
now. As discussed, I think it would be nice to move to the Fedora 37 
image when possible.

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 4/26/2023 8:34 PM, gua.guo@intel.com wrote:
> From: Gua Guo <gua.guo@intel.com>
> 
> Azure should install code coverage tool (lcov), it didn't
> exist on Fedora and Ubuntu by default.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Chris Fernald <chfernal@microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>   .azurepipelines/templates/defaults.yml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.azurepipelines/templates/defaults.yml b/.azurepipelines/templates/defaults.yml
> index 74d6b41783..8412a43cad 100644
> --- a/.azurepipelines/templates/defaults.yml
> +++ b/.azurepipelines/templates/defaults.yml
> @@ -9,4 +9,4 @@
>   
> 
>   variables:
> 
>     default_python_version: ">=3.10.6"
> 
> -  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:47addc9"
> 
> +  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:3b3eb8f"
> 

Re: [edk2-devel] [PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

[Chris Fernald]

Except the context previously mentioned by Mike and Michael, looks good 
to me.

Reviewed-by: Chris Fernald <chfernal@microsoft.com>

On 4/26/2023 5:34 PM, Guo, Gua wrote:
> From: Gua Guo <gua.guo@intel.com>
>
> Azure should install code coverage tool (lcov), it didn't
> exist on Fedora and Ubuntu by default.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Chris Fernald <chfernal@microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>   .azurepipelines/templates/defaults.yml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.azurepipelines/templates/defaults.yml b/.azurepipelines/templates/defaults.yml
> index 74d6b41783..8412a43cad 100644
> --- a/.azurepipelines/templates/defaults.yml
> +++ b/.azurepipelines/templates/defaults.yml
> @@ -9,4 +9,4 @@
>   
>
>   variables:
>
>     default_python_version: ">=3.10.6"
>
> -  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:47addc9"
>
> +  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:3b3eb8f"
>

Re: [PATCH v2 2/2] .azurepipelines: Choose container that have installed lcov

[Oliver Steffen]

We could also switch to Fedora 37, while we are already at it.
See: https://edk2.groups.io/g/devel/message/102947

But this patch looks fine to me.

Reviewed-by: Oliver Steffen <osteffen@redhat.com>

Quoting gua.guo@intel.com (2023-04-27 02:34:16)
> From: Gua Guo <gua.guo@intel.com>
>
> Azure should install code coverage tool (lcov), it didn't
> exist on Fedora and Ubuntu by default.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Chris Fernald <chfernal@microsoft.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>  .azurepipelines/templates/defaults.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.azurepipelines/templates/defaults.yml b/.azurepipelines/templates/defaults.yml
> index 74d6b41783..8412a43cad 100644
> --- a/.azurepipelines/templates/defaults.yml
> +++ b/.azurepipelines/templates/defaults.yml
> @@ -9,4 +9,4 @@
>
>  variables:
>    default_python_version: ">=3.10.6"
> -  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:47addc9"
> +  default_linux_image: "ghcr.io/tianocore/containers/fedora-35-test:3b3eb8f"
> --
> 2.39.2.windows.1
>