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

Wed May 18 06:43:47 UTC 2022

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 (#89882): https://edk2.groups.io/g/devel/message/89882
Mute This Topic: https://groups.io/mt/91181009/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-