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.web09.136.1652839635066669363 for ; Tue, 17 May 2022 19:07:15 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=WzrS37Tw; 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 D443820F723A; Tue, 17 May 2022 19:07:12 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D443820F723A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1652839634; bh=3u6XzvI6CwLPrenlnevYhAgibNT6uWuWO926IvFrhG8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=WzrS37TwfEWrjO3ekdywnHTIPpswhrtvbHZPcILbgkHhOpwgt2GIpMAuPWGOVPfVy sabcoya9HUi8AIreAoziLh1NDsq394A4tK8+DeF6+SUYY6mQwOl2ElErvHJEbhbrv/ 156NkqTSg4zb10wNxQkwaUbmS6/EHjeCFPyzrg0M= Message-ID: <9c9a5212-b2ca-415e-80b3-f22c17cedd48@linux.microsoft.com> Date: Tue, 17 May 2022 22:07:11 -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?UmU6IOWbnuWkjTogW2VkazItZGV2ZWxdIFtQQVRDSCB2MSAwLzhdIEZpeCBuZXcgdHlwb3MgcmVwb3J0ZWQ=?= 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> From: "Michael Kubacki" In-Reply-To: <025101d86a55$22303a20$6690ae60$@byosoft.com.cn> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Liming, That should be true but these are intended to be non-functional changes=20 (low risk) that should help ease the decision to move to a new version=20 in the future and help support consumers of the stable tag that might=20 need spelling fixes. For example, Project Mu integrates the stable tag and includes the same=20 checks so they would be beneficial to include from edk2 upstream tag.=20 edk2 might choose to move to a new version in the future to address a=20 critical issue like a security vulnerability in the cspell version and=20 having the changes in place makes that move easier. Revisiting the same changes in the future will also cause some duplicate=20 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,=20 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=20 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 thi= s stable tag? The change in Packages can be made in future. >=20 > 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 Sinha >> ; Ard Biesheuvel ; Bre= t >> Barkelew ; Gerd Hoffmann >> ; Guomin Jiang ; Jiewen Yao >> ; Leif Lindholm ; Limin= g >> 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 report= ed >> >> 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 packag= e >> 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 >>> wrote: >>>> >>>> As noted in the patch, this BZ was filed to follow up and review those= : >>>> 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 eve= n >>>> among maintainers in the same package and is an orthogonal conversatio= n >>>> to addressing pre-existing issues within the presently defined CI poli= cy. >>>> >>>> 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 track= s >>>> 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 wil= l >>>> 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.. >>> >>> >>> >>> >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20