Re: 回复: 回复: [edk2-devel] [PATCH v1 0/8] Fix new typos reported

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Sounds good. Let me know if anything else is needed for the stable tag.

Thanks,
Michael

On 5/18/2022 2:43 AM, gaoliming wrote:
> Michael:
>    Thanks for your update. I prefer to merge the necessary changes for this stable tag. The remaining change can be reviewed after the stable tag.
> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael
>> Kubacki
>> 发送时间: 2022年5月18日 10:07
>> 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn; ardb@kernel.org
>> 抄送: 'Alexei Fedorov' <Alexei.Fedorov@arm.com>; 'Ankit Sinha'
>> <ankit.sinha@intel.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>;
>> 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Gerd Hoffmann'
>> <kraxel@redhat.com>; 'Guomin Jiang' <guomin.jiang@intel.com>; 'Jiewen
>> Yao' <jiewen.yao@intel.com>; 'Leif Lindholm' <quic_llindhol@quicinc.com>;
>> 'Michael D Kinney' <michael.d.kinney@intel.com>; 'Nate DeSimone'
>> <nathaniel.l.desimone@intel.com>; 'Ray Ni' <ray.ni@intel.com>; 'Sami
>> Mujawar' <sami.mujawar@arm.com>; 'Sean Brogan'
>> <sean.brogan@microsoft.com>; 'Wei6 Xu' <wei6.xu@intel.com>
>> 主题: Re: 回复: [edk2-devel] [PATCH v1 0/8] Fix new typos reported
>>
>> Hi Liming,
>>
>> That should be true but these are intended to be non-functional changes
>> (low risk) that should help ease the decision to move to a new version
>> in the future and help support consumers of the stable tag that might
>> need spelling fixes.
>>
>> For example, Project Mu integrates the stable tag and includes the same
>> checks so they would be beneficial to include from edk2 upstream tag.
>> edk2 might choose to move to a new version in the future to address a
>> critical issue like a security vulnerability in the cspell version and
>> having the changes in place makes that move easier.
>>
>> Revisiting the same changes in the future will also cause some duplicate
>> effort at that time so I am hoping they can be merged now.
>>
>> However, if you prefer to only merge the necessary patches for the tag,
>> the last three patches [9][10][11] in the v2 series are recommended.
>>
>> I pushed those commits as-is from the v2 series to the following PR. I'm
>> using it to check the CI results with these commits.
>>
>> https://github.com/tianocore/edk2/pull/2904
>>
>> Thanks,
>> Michael
>>
>> On 5/17/2022 9:18 PM, gaoliming wrote:
>>> Michael:
>>>     Thanks for your quick work to resolve the CI issue. For this issue, if we
>> use the old stable version cspell version, those new issues will not be reported,
>> right? If yes, can we update CI only to unblock PR first for this stable tag? The
>> change in Packages can be made in future.
>>>
>>> Thanks
>>> Liming
>>>> -----邮件原件-----
>>>> 发件人: Michael Kubacki <mikuback@linux.microsoft.com>
>>>> 发送时间: 2022年5月18日 7:51
>>>> 收件人: devel@edk2.groups.io; ardb@kernel.org
>>>> 抄送: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ankit Sinha
>>>> <ankit.sinha@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>> Bret
>>>> Barkelew <Bret.Barkelew@microsoft.com>; Gerd Hoffmann
>>>> <kraxel@redhat.com>; Guomin Jiang <guomin.jiang@intel.com>; Jiewen
>> Yao
>>>> <jiewen.yao@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
>> Liming
>>>> Gao <gaoliming@byosoft.com.cn>; Michael D Kinney
>>>> <michael.d.kinney@intel.com>; Nate DeSimone
>>>> <nathaniel.l.desimone@intel.com>; Ray Ni <ray.ni@intel.com>; Sami
>>>> Mujawar <sami.mujawar@arm.com>; Sean Brogan
>>>> <sean.brogan@microsoft.com>; Wei6 Xu <wei6.xu@intel.com>
>>>> 主题: Re: [edk2-devel] [PATCH v1 0/8] Fix new typos reported
>>>>
>>>> Hi Ard,
>>>>
>>>> I understand it is frustrating for things that were working to suddenly
>>>> stop and errors to have been missed by the plugin in the past. I'm also
>>>> surprised that some of these issues were previously not caught.
>>>>
>>>> To clarify, adding the words to the ignore list was not really that much
>>>> time. The plugin output gives the words to add to the list (in JSON) so
>>>> that's a copy/paste operation and an IDE can remove duplicate lines
>>>> instantly so that was about a 10-30 second or so solution. Submitting
>>>> the BZ was another 1-2 minutes
>>>>
>>>> Following the the edk2 contribution process to manually add maintainers
>>>> per package, rebase and manually add review tags, parse feedback inline
>>>> to unified diffs over email, generate patch files, and update the cover
>>>> letter was a relatively larger consumer of time. For v2, I took
>>>> ownership of the BZ and spent more time to try to reduce the likelihood
>>>> of unexpected issues appearing in the future.
>>>>
>>>> V2 will do the following:
>>>>      1. Complete BZ 3929.
>>>>      2. Lock the cspell version to v5.20.0 to prevent latest from
>>>>         unexpectedly causing issues in the future.
>>>>      3. Update the common word list in cspell.base.yaml to prevent
>> package
>>>>         level duplication in the future.
>>>>      4. Include Sami's code review tags.
>>>>
>>>> I'm checking the CI results in the PR now and once it passes, I'll send
>>>> it on the list.
>>>>
>>>> https://github.com/tianocore/edk2/pull/2903
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> On 5/17/2022 4:06 PM, Ard Biesheuvel wrote:
>>>>> On Tue, 17 May 2022 at 21:32, Michael Kubacki
>>>>> <mikuback@linux.microsoft.com> wrote:
>>>>>>
>>>>>> As noted in the patch, this BZ was filed to follow up and review those:
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3929
>>>>>>
>>>>>> I don't like doing this either but the spelling errors do exist. I am
>>>>>> trying not to make CI policy changes as those can be controversial even
>>>>>> among maintainers in the same package and is an orthogonal
>> conversation
>>>>>> to addressing pre-existing issues within the presently defined CI policy.
>>>>>>
>>>>>> In this specific case, the ignore list in the package CI YAML file can
>>>>>> be used to explicitly identify known typos and the BZ explicitly tracks
>>>>>> reviewing those so there's a well defined path to resolve and fix the
>> issue.
>>>>>>
>>>>>> I personally feel that's better than ignoring the problem entirely but
>>>>>> it also depends on where your package code ends up getting consumed
>>>> and
>>>>>> the requirements and burden it might place on those consumers. For
>>>>>> example, if it ends up in auto generated documentation and that
>>>>>> documentation has spell check enabled, it becomes a downstream
>>>> override.
>>>>>>
>>>>>> There's currently several PRs active that fix typos so others see some
>>>>>> value in this work (as opposed to disabling spell checking):
>>>>>>       - https://github.com/tianocore/edk2/pull/2900
>>>>>>       - https://github.com/tianocore/edk2/pull/2789
>>>>>>       - https://github.com/tianocore/edk2/pull/1955
>>>>>>
>>>>>> For future changes, I suggested lock the cspell version and I think
>>>>>> that's an option to prevent these from appearing at unknown points in
>>>>>> time. I'm not appointed to make authoritative decisions about that (to
>>>>>> my understanding) so I am making that suggestion for the community to
>>>>>> consider.
>>>>>>
>>>>>> Again, I don't have a strong opinion on this topic, I've been waiting 4
>>>>>> weeks to get the v5 patch series merged (other busy work in between),
>>>>>> and you're the maintainer. It sounds like if I take ownership of BZ
>>>>>> 3929, you might be okay with leaving it enabled? I can do that but
>>>>>> there's so many words in this instance, I wanted someone closer to the
>>>>>> package contents to look at it.
>>>>>>
>>>>>> If you still strongly feel you would prefer to have it disabled, I will
>>>>>> pull that change in and see if any opposing opinions surface. However, I
>>>>>> wanted to double check this is what you want to do right now.
>>>>>>
>>>>>
>>>>> If you feel it is worth your time to fix typos in existing comments, I
>>>>> won't stand in your way. But I don't feel it is worth my time, given
>>>>> that it doesn't actually improve the code, except for by some
>>>>> artifical measure of spelling-correctness, which has no bearing at all
>>>>> on what runs on people's machines, and as far as I can tell, these
>>>>> typoes do not create any confusion regarding what the comments intend
>>>>> to convey.
>>>>>
>>>>> Adding typoed words to the ignorelist is the worst possible solution,
>>>>> because you will be wasting your time only to placate the machine,
>>>>> accumulating technical debt in the code base without actually fixing
>>>>> the problems. So that is out of the question for me.
>>>>>
>>>>> If you want to fix these issues, that is also fine. I will review/ack
>>>>> with priority provided that I actually have any bandwidth available.
>>>>>
>>>>> But if we are working for the CI instead of the other way around,
>>>>> something is seriously wrong. If we can't roll a stable tag because
>>>>> the CI wants us to fix our typoes first, we have to be able to
>>>>> override it. And corrupting the codebase by adding typoes to the
>>>>> ignorelist just to placate the CI is preposterous..
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
>