For the changes in CI , Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn>.
Create PR https://github.com/tianocore/edk2/pull/2906 to merge it for this stable tag. Thanks Liming > -----邮件原件----- > 发件人: Michael Kubacki <mikub...@linux.microsoft.com> > 发送时间: 2022年5月18日 22:52 > 收件人: 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 > > 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; 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 (#89898): https://edk2.groups.io/g/devel/message/89898 Mute This Topic: https://groups.io/mt/91199992/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
