[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@...>

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@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@...>; Ard Biesheuvel
<ardb+tianocore@...>; Abner Chang <abner.chang@...>; Daniel Schaefer <git@...>
Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

On Fri, 23 Sept 2022 at 03:09, Michael Kubacki
<mikuback@...> 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@...>


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@...>
Sent: Thursday, September 15, 2022 2:03 PM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@...>; Ard
Biesheuvel <ardb+tianocore@...>; Abner Chang <abner.chang@...>; Daniel Schaefer <git@...>
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@...> 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.



Ard Biesheuvel
 

On Fri, 23 Sept 2022 at 03:09, Michael Kubacki
<mikuback@...> 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@...>


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@...>
Sent: Thursday, September 15, 2022 2:03 PM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@...>; Ard
Biesheuvel <ardb+tianocore@...>; Abner Chang <abner.chang@...>; Daniel Schaefer <git@...>
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@...> 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.


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@...>
Sent: Thursday, September 15, 2022 2:03 PM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@...>; Ard
Biesheuvel <ardb+tianocore@...>; Abner Chang <abner.chang@...>; Daniel Schaefer <git@...>
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@...> 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.


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@...>
Sent: Thursday, September 15, 2022 2:03 PM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>; devel@edk2.groups.io; Leif Lindholm <quic_llindhol@...>; Ard
Biesheuvel <ardb+tianocore@...>; Abner Chang <abner.chang@...>; Daniel Schaefer <git@...>
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@...> 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.


Ard Biesheuvel
 

On Thu, 15 Sept 2022 at 22:52, Kinney, Michael D
<michael.d.kinney@...> 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.


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@...>
Sent: Thursday, September 15, 2022 1:47 PM
To: Michael Kubacki <mikuback@...>
Cc: devel@edk2.groups.io; Leif Lindholm <quic_llindhol@...>; Ard Biesheuvel <ardb+tianocore@...>; Abner Chang
<abner.chang@...>; Daniel Schaefer <git@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] [PATCH v1 0/3] EmbeddedPkg: Enable CI

On Thu, 15 Sept 2022 at 21:46, Michael Kubacki
<mikuback@...> 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@...> 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.




Ard Biesheuvel
 

On Thu, 15 Sept 2022 at 21:46, Michael Kubacki
<mikuback@...> 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@...> 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.




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


Ard Biesheuvel
 

On Wed, 7 Sept 2022 at 17:00, Michael Kubacki
<mikuback@...> 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.


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@...> wrote:

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

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@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Abner Chang <abner.chang@...>
Cc: Daniel Schaefer <git@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>

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


Ard Biesheuvel
 

On Wed, 7 Sept 2022 at 04:37, <mikuback@...> wrote:

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

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@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Abner Chang <abner.chang@...>
Cc: Daniel Schaefer <git@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>

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


Michael Kubacki
 

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

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

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@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Abner Chang <abner.chang@...>
Cc: Daniel Schaefer <git@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>

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

--=20
2.28.0.windows.1