Hi Mike,
Thanks for the suggestion but I think I'm just going to avoid making
these changes in edk2, at least for now.
If package maintainers don't appreciate the work and we have to figure
out who wants what every time a patch is submitted it truly does consume
more of my time than it should.
Hopefully, a productive result from this series can be a realization
that senior members of this community set the culture and less hostility
toward contributors would encourage more contribution.
I agree the comment in the CI YAML file would be helpful or a section in
a package readme that states the maintainer's preferences.
Thanks,
Michael
On 12/15/2022 11:57 AM, Michael D Kinney wrote:
Michael,
I appreciate your efforts to improve code quality in all aspects.
I think this is the only part of the change that is being asked
to be adjusted (specifically for the ArmPkg and ArmVirtPkg) is
the following.
- "AuditOnly": True,
+ "AuditOnly": False,
Different packages may choose different levels if quality or
quality checks based on the maturity of the package and what
is deemed critical to the package maintainers.
There are obvious checks such as compile failures that we can
all agree on. Checks that are not directly related to
functionality should perhaps be flexible and the policy be
allowed to be adjusted by the package maintainers.
I personally would prefer to see as many packages as possible
take advantage of the CI checks that have been enabled, so I
think it is good to start with an approach where everything is
on by default, and if a package maintainer would prefer a
specific package to have one disabled, then defer to that request.
I also think it would be good to add comments to the package
YAML file if a check is disabled that clearly states that
the choice to diverge from the standard CI checks was made
by the package maintainer.
With this approach, I think your series can go forward with
AuditOnly set to True for the ArmPkg and ArtVirtPkg with
comments that this setting is different than the default
due to requests from the maintainers.
Best regards,
Mike
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Thursday, December 15, 2022 8:39 AM
To: devel@edk2.groups.io; quic_llind...@quicinc.com
Cc: a...@kernel.org; Ard Biesheuvel <ardb+tianoc...@kernel.org>; Sami Mujawar
<sami.muja...@arm.com>; Kinney, Michael D
<michael.d.kin...@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 13/14] ArmPkg: Turn off spellcheck audit
mode
On 12/15/2022 5:42 AM, Leif Lindholm wrote:
On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
I'm just trying to understand your position.
Are you saying you would rather people check in typos and then later have
patches come into the package to fix them?
For example, like these:
- ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
- ArmPkg: https://edk2.groups.io/g/devel/message/97022
Why not just have the code checked in without typos in the first place?
A little fairy once whispered in my ear that if I stopped myself and
tried to rephrase whenever I found myself using the work "just", I
would meet less friction in context-stripping communication mediums
such as email. They weren't wrong.
This topic is met with friction regardless of how it is phrased.
The friction exists because the community chose to enable spell checking
in CI and it is not wanted here.
The mechanism chosen to ignore words was through YAML files rather than
a button. A common YAML file can store words for the whole project and
packages can add package-specific words. The community was expected to
make the contributions necessary in the common file to minimize impact
on package maintainers.
The spellcheck log outputs the exact code that needs to be copied/pasted
to the exception list - whether the global list or package list. If you
run CI locally, you can copy/paste the exact lines needed.
This is a patch series I work on in my spare time to try to improve the
project. I am tired of Ard's dismissive and passive aggressive responses
such as those in https://edk2.groups.io/g/devel/message/97433 so I
revoke the series and will let others decide what they want to do.
Checking in typos creates more review work, makes the code history have typo
fix patches that could be avoided, and impacts accessibility.
I spend far more time with edk2 overhead such as email formatting problems,
keeping track of maintainer email address changes when updating patches,
mapping email replies back to code, and so on that does not improve the
code. A spell checker only takes seconds and is built into edk2 CI.
We didn't disable the spellchecker for the ARM* packages (only)
because we hate correct spelling. We disabled it because it throws
false positives left right and centre. So we end up needing to update
the .ci.yaml every time we mention an architectural concept we haven't
mentioned before. Or add a copyright line mentioning a new
organisation. Or correctly use apostrophes in ways the spellchecker
doesn't expect.
Here is the current (and very incomplete, oh - and package specific)
exception list for ArmPkg:
https://github.com/tianocore/edk2/blob/master/ArmPkg/ArmPkg.ci.yaml#L95
If it had a aggressiveness knob and we could turn it down from 11 to
3 and work our way up from there, that would be another story.
Failing that, a push-through-anyway-this-isn't-a-typo label would be a
reasonable compromise.
But for now, I agree with Ard.
/
Leif
On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
<mikub...@linux.microsoft.com> wrote:
Yes. It will also reduce frequency of incoming patches that must be
reviewed and merged due to people continuously fixing trivial spelling
errors.
In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
add a button to the GitHub UI that permits me to override a negative
CI result on a PR.
On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
On Wed, 14 Dec 2022 at 23:53, <mikub...@linux.microsoft.com> wrote:
From: Michael Kubacki <michael.kuba...@microsoft.com>
Audit mode was enabled for the spellcheck CI plugin. It is no longer
needed with recent changes. Spelling errors can be checked in the
package moving forward.
Cc: Leif Lindholm <quic_llind...@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
Cc: Sami Mujawar <sami.muja...@arm.com>
Signed-off-by: Michael Kubacki <michael.kuba...@microsoft.com>
Will this patch result in PRs potentially being rejected by pre-merge
CI due to trivial spelling errors?
---
ArmPkg/ArmPkg.ci.yaml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index c8dface6821a..a304c7966cf7 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -87,7 +87,7 @@
## options defined .pytool/Plugin/SpellCheck
"SpellCheck": {
- "AuditOnly": True,
+ "AuditOnly": False,
"IgnoreFiles": [
"Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
], # use gitignore syntax to ignore
errors
@@ -148,6 +148,7 @@
"fcmplt",
"ffreestanding",
"frsub",
+ "hauser",
"hisilicon",
"iccabpr",
"iccbpr",
--
2.28.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97511): https://edk2.groups.io/g/devel/message/97511
Mute This Topic: https://groups.io/mt/95678218/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
