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

Thu May 19 01:23:46 UTC 2022

For the changes in CI , Reviewed-by: Liming Gao <gaoliming at byosoft.com.cn>. 

Create PR https://github.com/tianocore/edk2/pull/2906 to merge it for this stable tag.

Thanks
Liming
> -----邮件原件-----
> 发件人: Michael Kubacki <mikuback at linux.microsoft.com>
> 发送时间: 2022年5月18日 22:52
> 收件人: devel at edk2.groups.io; gaoliming at byosoft.com.cn; ardb at kernel.org
> 抄送: 'Alexei Fedorov' <Alexei.Fedorov at arm.com>; 'Ankit Sinha'
> <ankit.sinha at intel.com>; 'Ard Biesheuvel' <ardb+tianocore at kernel.org>;
> 'Bret Barkelew' <Bret.Barkelew at microsoft.com>; 'Gerd Hoffmann'
> <kraxel at redhat.com>; 'Guomin Jiang' <guomin.jiang at intel.com>; 'Jiewen
> Yao' <jiewen.yao at intel.com>; 'Leif Lindholm' <quic_llindhol at quicinc.com>;
> 'Michael D Kinney' <michael.d.kinney at intel.com>; 'Nate DeSimone'
> <nathaniel.l.desimone at intel.com>; 'Ray Ni' <ray.ni at intel.com>; 'Sami
> Mujawar' <sami.mujawar at arm.com>; 'Sean Brogan'
> <sean.brogan at microsoft.com>; 'Wei6 Xu' <wei6.xu at intel.com>
> 主题: Re: 回复: 回复: [edk2-devel] [PATCH v1 0/8] Fix new typos reported
> 
> 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 at edk2.groups.io <devel at edk2.groups.io> 代表 Michael
> >> Kubacki
> >> 发送时间: 2022年5月18日 10:07
> >> 收件人: devel at edk2.groups.io; gaoliming at byosoft.com.cn;
> ardb at kernel.org
> >> 抄送: 'Alexei Fedorov' <Alexei.Fedorov at arm.com>; 'Ankit Sinha'
> >> <ankit.sinha at intel.com>; 'Ard Biesheuvel' <ardb+tianocore at kernel.org>;
> >> 'Bret Barkelew' <Bret.Barkelew at microsoft.com>; 'Gerd Hoffmann'
> >> <kraxel at redhat.com>; 'Guomin Jiang' <guomin.jiang at intel.com>; 'Jiewen
> >> Yao' <jiewen.yao at intel.com>; 'Leif Lindholm'
> <quic_llindhol at quicinc.com>;
> >> 'Michael D Kinney' <michael.d.kinney at intel.com>; 'Nate DeSimone'
> >> <nathaniel.l.desimone at intel.com>; 'Ray Ni' <ray.ni at intel.com>; 'Sami
> >> Mujawar' <sami.mujawar at arm.com>; 'Sean Brogan'
> >> <sean.brogan at microsoft.com>; 'Wei6 Xu' <wei6.xu at 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 at linux.microsoft.com>
> >>>> 发送时间: 2022年5月18日 7:51
> >>>> 收件人: devel at edk2.groups.io; ardb at kernel.org
> >>>> 抄送: Alexei Fedorov <Alexei.Fedorov at arm.com>; Ankit Sinha
> >>>> <ankit.sinha at intel.com>; Ard Biesheuvel
> <ardb+tianocore at kernel.org>;
> >> Bret
> >>>> Barkelew <Bret.Barkelew at microsoft.com>; Gerd Hoffmann
> >>>> <kraxel at redhat.com>; Guomin Jiang <guomin.jiang at intel.com>; Jiewen
> >> Yao
> >>>> <jiewen.yao at intel.com>; Leif Lindholm <quic_llindhol at quicinc.com>;
> >> Liming
> >>>> Gao <gaoliming at byosoft.com.cn>; Michael D Kinney
> >>>> <michael.d.kinney at intel.com>; Nate DeSimone
> >>>> <nathaniel.l.desimone at intel.com>; Ray Ni <ray.ni at intel.com>; Sami
> >>>> Mujawar <sami.mujawar at arm.com>; Sean Brogan
> >>>> <sean.brogan at microsoft.com>; Wei6 Xu <wei6.xu at 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 at 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..
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> > 
> >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89898): https://edk2.groups.io/g/devel/message/89898
Mute This Topic: https://groups.io/mt/91199992/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-