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.web10.2505.1652923436906752780 for ; Wed, 18 May 2022 18:23:58 -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 ; Thu, 19 May 2022 09:23:44 +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: "'Michael Kubacki'" , , 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> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IOWbnuWkjTogW2VkazItZGV2ZWxdIFtQQVRDSCB2MSAwLzhdIEZpeCBuZXcgdHlwb3MgcmVwb3J0ZWQ=?= Date: Thu, 19 May 2022 09:23:46 +0800 Message-ID: <030c01d86b1f$164b9540$42e2bfc0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHcLZTW2SO+N522U0ndsnapDIZJBgGCzCIMATngQ8oCIYs3VgIUJOhcAwYlZ4kCkTem2gHpEkosAZTkoeMA30GLHAFmN1RnrIt0B0A= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn For the changes in CI , Reviewed-by: Liming Gao .= =20 Create PR https://github.com/tianocore/edk2/pull/2906 to merge it for this = stable tag. 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 = 22:52 > =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: =E5=9B=9E=E5=A4=8D: [edk2-dev= el] [PATCH v1 0/8] Fix new typos reported >=20 > Sounds good. Let me know if anything else is needed for the stable tag. >=20 > Thanks, > Michael >=20 > 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 ta= g. > > > > 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.c= om.cn; > ardb@kernel.org > >> =E6=8A=84=E9=80=81: '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' > >> =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 change= s > >> (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 sam= e > >> 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 duplica= te > >> 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 issu= e, if > we > >> use the old stable version cspell version, those new issues will not b= e > reported, > >> right? If yes, can we update CI only to unblock PR first for this stab= le 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 S= inha > >>>> ; 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 re= ported > >>>> > >>>> Hi Ard, > >>>> > >>>> I understand it is frustrating for things that were working to sudde= nly > >>>> stop and errors to have been missed by the plugin in the past. I'm a= lso > >>>> 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. Submittin= g > >>>> 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 co= ver > >>>> 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 likelih= ood > >>>> 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 s= end > >>>> 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 t= hose: > >>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D3929 > >>>>>> > >>>>>> 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 t= racks > >>>>>> 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 thin= k > >>>>>> 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 wait= ing 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 B= Z > >>>>>> 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. Howe= ver, > 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, give= n > >>>>> 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 solutio= n, > >>>>> because you will be wasting your time only to placate the machine, > >>>>> accumulating technical debt in the code base without actually fixin= g > >>>>> 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/a= ck > >>>>> 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 > >