From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web09.1869.1652856232792976340 for ; Tue, 17 May 2022 23:43:53 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([101.224.116.119]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Wed, 18 May 2022 14:43:46 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 101.224.116.119 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , 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> In-Reply-To: <9c9a5212-b2ca-415e-80b3-f22c17cedd48@linux.microsoft.com> Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IFtlZGsyLWRldmVsXSBbUEFUQ0ggdjEgMC84XSBGaXggbmV3IHR5cG9zIHJlcG9ydGVk?= Date: Wed, 18 May 2022 14:43:47 +0800 Message-ID: <02ad01d86a82$a0cb2430$e2616c90$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHcLZTW2SO+N522U0ndsnapDIZJBgGCzCIMATngQ8oCIYs3VgIUJOhcAwYlZ4kCkTem2gHpEkosAZTkoeOsnGvnEA== Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn 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. =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 Sin= ha' > ; '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] F= ix new typos reported >=20 > Hi Liming, >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > https://github.com/tianocore/edk2/pull/2904 >=20 > Thanks, > Michael >=20 > 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 r= eported, > 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