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; gaolim...@byosoft.com.cn; a...@kernel.org > 抄送: 'Alexei Fedorov' <alexei.fedo...@arm.com>; 'Ankit Sinha' > <ankit.si...@intel.com>; 'Ard Biesheuvel' <ardb+tianoc...@kernel.org>; > 'Bret Barkelew' <bret.barke...@microsoft.com>; 'Gerd Hoffmann' > <kra...@redhat.com>; 'Guomin Jiang' <guomin.ji...@intel.com>; 'Jiewen > Yao' <jiewen....@intel.com>; 'Leif Lindholm' <quic_llind...@quicinc.com>; > 'Michael D Kinney' <michael.d.kin...@intel.com>; 'Nate DeSimone' > <nathaniel.l.desim...@intel.com>; 'Ray Ni' <ray...@intel.com>; 'Sami > Mujawar' <sami.muja...@arm.com>; 'Sean Brogan' > <sean.bro...@microsoft.com>; 'Wei6 Xu' <wei6...@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 <mikub...@linux.microsoft.com> > >> 发送时间: 2022年5月18日 7:51 > >> 收件人: devel@edk2.groups.io; a...@kernel.org > >> 抄送: Alexei Fedorov <alexei.fedo...@arm.com>; Ankit Sinha > >> <ankit.si...@intel.com>; Ard Biesheuvel <ardb+tianoc...@kernel.org>; > Bret > >> Barkelew <bret.barke...@microsoft.com>; Gerd Hoffmann > >> <kra...@redhat.com>; Guomin Jiang <guomin.ji...@intel.com>; Jiewen > Yao > >> <jiewen....@intel.com>; Leif Lindholm <quic_llind...@quicinc.com>; > Liming > >> Gao <gaolim...@byosoft.com.cn>; Michael D Kinney > >> <michael.d.kin...@intel.com>; Nate DeSimone > >> <nathaniel.l.desim...@intel.com>; Ray Ni <ray...@intel.com>; Sami > >> Mujawar <sami.muja...@arm.com>; Sean Brogan > >> <sean.bro...@microsoft.com>; Wei6 Xu <wei6...@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 > >>> <mikub...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
