From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.6007.1652885524594769013 for ; Wed, 18 May 2022 07:52:04 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=h8NPPiFy; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.195.228.134]) by linux.microsoft.com (Postfix) with ESMTPSA id 89EBC20F3D1A; Wed, 18 May 2022 07:52:01 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 89EBC20F3D1A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1652885523; bh=pd3U4wAbiSE3/BGv8H+eK83BPYctdyKQ2QmWPQTuCSo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=h8NPPiFyeAIy0RT0CTGbhR/pCNbMm2aYfrR1iBh6GDbDZNGUF7BH9qGadrbhjGC3L DaFuTgecXtuy8L6EkPabx+OOYORUrY51AMWyMSJb9QhFyB70RAxraUybRPp/BG/6oZ et4JTz5YkTQubImn3SeGR7aCu3pZCwrmZ554rV/A= Message-ID: Date: Wed, 18 May 2022 10:52:00 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: =?UTF-8?B?UmU6IOWbnuWkjTog5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIHYxIDAvOF0gRml4IG5ldyB0eXBvcyByZXBvcnRlZA==?= To: devel@edk2.groups.io, gaoliming@byosoft.com.cn, ardb@kernel.org Cc: 'Alexei Fedorov' , 'Ankit Sinha' , 'Ard Biesheuvel' , 'Bret Barkelew' , 'Gerd Hoffmann' , 'Guomin Jiang' , 'Jiewen Yao' , 'Leif Lindholm' , 'Michael D Kinney' , 'Nate DeSimone' , 'Ray Ni' , 'Sami Mujawar' , 'Sean Brogan' , 'Wei6 Xu' References: <20220517160043.1210-1-mikuback@linux.microsoft.com> <52bf72a9-604e-a1ca-8846-34a7801ef427@linux.microsoft.com> <8d950da7-9e1a-d5ff-6baf-6aeeca36736a@linux.microsoft.com> <8acb2244-9262-7b27-a6c5-ff65dcdeda32@linux.microsoft.com> <025101d86a55$22303a20$6690ae60$@byosoft.com.cn> <9c9a5212-b2ca-415e-80b3-f22c17cedd48@linux.microsoft.com> <02ad01d86a82$a0cb2430$e2616c90$@byosoft.com.cn> From: "Michael Kubacki" In-Reply-To: <02ad01d86a82$a0cb2430$e2616c90$@byosoft.com.cn> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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 th= is stable tag. The remaining change can be reviewed after the stable tag. >=20 > Thanks > Liming >> -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- >> =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Michael >> Kubacki >> =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2022=E5=B9=B45=E6=9C=8818=E6=97=A5= 10:07 >> =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; gaoliming@byosoft.com= .cn; ardb@kernel.org >> =E6=8A=84=E9=80=81: 'Alexei Fedorov' ; 'Ankit Si= nha' >> ; 'Ard Biesheuvel' ; >> 'Bret Barkelew' ; 'Gerd Hoffmann' >> ; 'Guomin Jiang' ; 'Jiewen >> Yao' ; 'Leif Lindholm' = ; >> 'Michael D Kinney' ; 'Nate DeSimone' >> ; 'Ray Ni' ; 'Sami >> Mujawar' ; 'Sean Brogan' >> ; 'Wei6 Xu' >> =E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: [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 >>>> -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- >>>> =E5=8F=91=E4=BB=B6=E4=BA=BA: Michael Kubacki >>>> =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2022=E5=B9=B45=E6=9C=8818=E6=97= =A5 7:51 >>>> =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; ardb@kernel.org >>>> =E6=8A=84=E9=80=81: Alexei Fedorov ; Ankit Sin= ha >>>> ; Ard Biesheuvel ; >> Bret >>>> Barkelew ; Gerd Hoffmann >>>> ; Guomin Jiang ; Jiewen >> Yao >>>> ; Leif Lindholm ; >> Liming >>>> Gao ; Michael D Kinney >>>> ; Nate DeSimone >>>> ; Ray Ni ; Sami >>>> Mujawar ; Sean Brogan >>>> ; Wei6 Xu >>>> =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH v1 0/8] Fix new typos repo= rted >>>> >>>> Hi Ard, >>>> >>>> I understand it is frustrating for things that were working to suddenl= y >>>> stop and errors to have been missed by the plugin in the past. I'm als= o >>>> surprised that some of these issues were previously not caught. >>>> >>>> To clarify, adding the words to the ignore list was not really that mu= ch >>>> time. The plugin output gives the words to add to the list (in JSON) s= o >>>> 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 maintainer= s >>>> per package, rebase and manually add review tags, parse feedback inlin= e >>>> to unified diffs over email, generate patch files, and update the cove= r >>>> 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 likelihoo= d >>>> 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 sen= d >>>> 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 >>>>> wrote: >>>>>> >>>>>> As noted in the patch, this BZ was filed to follow up and review tho= se: >>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D3929 >>>>>> >>>>>> I don't like doing this either but the spelling errors do exist. I a= m >>>>>> trying not to make CI policy changes as those can be controversial e= ven >>>>>> among maintainers in the same package and is an orthogonal >> conversation >>>>>> to addressing pre-existing issues within the presently defined CI po= licy. >>>>>> >>>>>> In this specific case, the ignore list in the package CI YAML file c= an >>>>>> be used to explicitly identify known typos and the BZ explicitly tra= cks >>>>>> reviewing those so there's a well defined path to resolve and fix th= e >> issue. >>>>>> >>>>>> I personally feel that's better than ignoring the problem entirely b= ut >>>>>> 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 so= me >>>>>> 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 i= n >>>>>> time. I'm not appointed to make authoritative decisions about that (= to >>>>>> my understanding) so I am making that suggestion for the community t= o >>>>>> consider. >>>>>> >>>>>> Again, I don't have a strong opinion on this topic, I've been waitin= g 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 t= he >>>>>> package contents to look at it. >>>>>> >>>>>> If you still strongly feel you would prefer to have it disabled, I w= ill >>>>>> pull that change in and see if any opposing opinions surface. Howeve= r, 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 al= l >>>>> 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.. >>>>> >>>>> >>>>> >>>>> >>> >>> >>> >>> >>> >>> >>> >>> >> >> >> >> >=20 >=20 >=20 >=20 >=20 >=20 >=20