DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 2 +- DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCmn600LibArm/SsdtCmn600Generator.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlParser.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/Tree/AmlNode.c | 2 +- DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepo.c | 4 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.c | 2 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c | 2 +- FatPkg/EnhancedFatDxe/FileSpace.c | 2 +- FatPkg/EnhancedFatDxe/ReadWrite.c | 2 +- FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 2 +- PrmPkg/Application/PrmInfo/PrmInfo.c | 2 +- PrmPkg/Library/DxePrmModuleDiscoveryLib/DxePrmModuleDiscoveryLib.c | 2 +- PrmPkg/PrmLoaderDxe/PrmLoaderDxe.c | 2 +- PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestImage.c | 2 +- PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestProtocol.c | 2 +- StandaloneMmPkg/Core/Dependency.c | 2 +- StandaloneMmPkg/Core/Dispatcher.c | 18 +++---- UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.c | 18 +++---- UnitTestFrameworkPkg/Library/UnitTestBootLibNull/UnitTestBootLibNull.c | 2 +- UnitTestFrameworkPkg/Library/UnitTestBootLibUsbClass/UnitTestBootLibUsbClass.c | 2 +- UnitTestFrameworkPkg/Library/UnitTestLib/Log.c | 2 +- UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLib.c | 2 +- UnitTestFrameworkPkg/Test/UnitTest/Sample/SampleUnitTest/SampleUnitTest.c | 2 +- ArmPkg/ArmPkg.ci.yaml | 55 +++++++++++++++++++- ArmVirtPkg/ArmVirtPkg.ci.yaml | 25 +++++---- DynamicTablesPkg/DynamicTablesPkg.ci.yaml | 31 ++++++++--- DynamicTablesPkg/Include/Library/DynamicPlatRepoLib.h | 4 +- DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.h | 2 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.h | 2 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.h | 2 +- DynamicTablesPkg/Readme.md | 4 +- FatPkg/EnhancedFatDxe/Fat.h | 2 +- FatPkg/FatPkg.ci.yaml | 22 ++++++-- FmpDevicePkg/FmpDevicePkg.ci.yaml | 4 ++ PrmPkg/Include/Library/PrmModuleDiscoveryLib.h | 2 +- PrmPkg/PrmLoaderDxe/PrmAcpiTable.h | 4 +- PrmPkg/PrmPkg.ci.yaml | 9 ++++ PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTest.h | 4 +- StandaloneMmPkg/Include/Guid/MmCoreData.h | 2 +- StandaloneMmPkg/StandaloneMmPkg.ci.yaml | 9 ++++ UnitTestFrameworkPkg/PrivateInclude/Library/UnitTestBootLib.h | 2 +- UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h | 2 +- UnitTestFrameworkPkg/UnitTestFrameworkPkg.ci.yaml | 29 +++++++---- 45 files changed, 209 insertions(+), 91 deletions(-)
From: Michael Kubacki <michael.kubacki@microsoft.com> The SpellCheck plugin began reporting new typos that were previously missed. This is impacting edk2 pull requests from being completed. A change in the cspell plugin or some other upstream component may have caused them to appear now. This patch series mitigates the issue by fixing legitimate spelling issues and adding new ignored words if appropriate. Fixes in ArmPkg were deferred due to the number of reported issues in the package. The maintainers should follow up by reviewing the extended word list for SpellCheck in ArmPkg.ci.yaml and determining the best resolution for each item. That follow up task for ArmPkg was filed in the following BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3929 Cc: Alexei Fedorov <Alexei.Fedorov@arm.com> Cc: Ankit Sinha <ankit.sinha@intel.com> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Guomin Jiang <guomin.jiang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Leif Lindholm <quic_llindhol@quicinc.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Michael Kubacki <mikuback@linux.microsoft.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Sami Mujawar <sami.mujawar@arm.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> Cc: Wei6 Xu <wei6.xu@intel.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> Michael Kubacki (8): PrmPkg: Fix new typos reported StandaloneMmPkg: Fix new typos reported DynamicTablesPkg: Fix new typos reported UnitTestFrameworkPkg: Fix new typos reported FatPkg: Fix new typos reported FmpDevicePkg: Fix new typos reported ArmPkg: Ignore new typos reported ArmVirtPkg: Add new ignored spelling errors DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 2 +- DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCmn600LibArm/SsdtCmn600Generator.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlParser.c | 2 +- DynamicTablesPkg/Library/Common/AmlLib/Tree/AmlNode.c | 2 +- DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepo.c | 4 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.c | 2 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c | 2 +- FatPkg/EnhancedFatDxe/FileSpace.c | 2 +- FatPkg/EnhancedFatDxe/ReadWrite.c | 2 +- FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 2 +- PrmPkg/Application/PrmInfo/PrmInfo.c | 2 +- PrmPkg/Library/DxePrmModuleDiscoveryLib/DxePrmModuleDiscoveryLib.c | 2 +- PrmPkg/PrmLoaderDxe/PrmLoaderDxe.c | 2 +- PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestImage.c | 2 +- PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestProtocol.c | 2 +- StandaloneMmPkg/Core/Dependency.c | 2 +- StandaloneMmPkg/Core/Dispatcher.c | 18 +++---- UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.c | 18 +++---- UnitTestFrameworkPkg/Library/UnitTestBootLibNull/UnitTestBootLibNull.c | 2 +- UnitTestFrameworkPkg/Library/UnitTestBootLibUsbClass/UnitTestBootLibUsbClass.c | 2 +- UnitTestFrameworkPkg/Library/UnitTestLib/Log.c | 2 +- UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLib.c | 2 +- UnitTestFrameworkPkg/Test/UnitTest/Sample/SampleUnitTest/SampleUnitTest.c | 2 +- ArmPkg/ArmPkg.ci.yaml | 55 +++++++++++++++++++- ArmVirtPkg/ArmVirtPkg.ci.yaml | 25 +++++---- DynamicTablesPkg/DynamicTablesPkg.ci.yaml | 31 ++++++++--- DynamicTablesPkg/Include/Library/DynamicPlatRepoLib.h | 4 +- DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.h | 2 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.h | 2 +- DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.h | 2 +- DynamicTablesPkg/Readme.md | 4 +- FatPkg/EnhancedFatDxe/Fat.h | 2 +- FatPkg/FatPkg.ci.yaml | 22 ++++++-- FmpDevicePkg/FmpDevicePkg.ci.yaml | 4 ++ PrmPkg/Include/Library/PrmModuleDiscoveryLib.h | 2 +- PrmPkg/PrmLoaderDxe/PrmAcpiTable.h | 4 +- PrmPkg/PrmPkg.ci.yaml | 9 ++++ PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTest.h | 4 +- StandaloneMmPkg/Include/Guid/MmCoreData.h | 2 +- StandaloneMmPkg/StandaloneMmPkg.ci.yaml | 9 ++++ UnitTestFrameworkPkg/PrivateInclude/Library/UnitTestBootLib.h | 2 +- UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h | 2 +- UnitTestFrameworkPkg/UnitTestFrameworkPkg.ci.yaml | 29 +++++++---- 45 files changed, 209 insertions(+), 91 deletions(-) -- 2.28.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89833): https://edk2.groups.io/g/devel/message/89833 Mute This Topic: https://groups.io/mt/91166946/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hello Michael, Thanks for taking care of this. However, I strongly feel that automation is increasing my workload here rather than the other way around, for dubious benefits, so as a package maintainer, I am inclined to disable the spell checks altogether for all the packages I am in charge of. On Tue, 17 May 2022 at 18:01, <mikuback@linux.microsoft.com> wrote: > > From: Michael Kubacki <michael.kubacki@microsoft.com> > > The SpellCheck plugin began reporting new typos that were previously > missed. This is impacting edk2 pull requests from being completed. > > A change in the cspell plugin or some other upstream component may > have caused them to appear now. This patch series mitigates the > issue by fixing legitimate spelling issues and adding new ignored > words if appropriate. > > Fixes in ArmPkg were deferred due to the number of reported issues > in the package. The maintainers should follow up by reviewing the > extended word list for SpellCheck in ArmPkg.ci.yaml and determining > the best resolution for each item. > > That follow up task for ArmPkg was filed in the following BZ: > https://bugzilla.tianocore.org/show_bug.cgi?id=3929 > > Cc: Alexei Fedorov <Alexei.Fedorov@arm.com> > Cc: Ankit Sinha <ankit.sinha@intel.com> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Guomin Jiang <guomin.jiang@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Michael Kubacki <mikuback@linux.microsoft.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Sami Mujawar <sami.mujawar@arm.com> > Cc: Sean Brogan <sean.brogan@microsoft.com> > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > Cc: Wei6 Xu <wei6.xu@intel.com> > Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> > > Michael Kubacki (8): > PrmPkg: Fix new typos reported > StandaloneMmPkg: Fix new typos reported > DynamicTablesPkg: Fix new typos reported > UnitTestFrameworkPkg: Fix new typos reported > FatPkg: Fix new typos reported > FmpDevicePkg: Fix new typos reported > ArmPkg: Ignore new typos reported > ArmVirtPkg: Add new ignored spelling errors > > DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 2 +- > DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCmn600LibArm/SsdtCmn600Generator.c | 2 +- > DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c | 2 +- > DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.c | 2 +- > DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlParser.c | 2 +- > DynamicTablesPkg/Library/Common/AmlLib/Tree/AmlNode.c | 2 +- > DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepo.c | 4 +- > DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.c | 2 +- > DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c | 2 +- > FatPkg/EnhancedFatDxe/FileSpace.c | 2 +- > FatPkg/EnhancedFatDxe/ReadWrite.c | 2 +- > FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 2 +- > PrmPkg/Application/PrmInfo/PrmInfo.c | 2 +- > PrmPkg/Library/DxePrmModuleDiscoveryLib/DxePrmModuleDiscoveryLib.c | 2 +- > PrmPkg/PrmLoaderDxe/PrmLoaderDxe.c | 2 +- > PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestImage.c | 2 +- > PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestProtocol.c | 2 +- > StandaloneMmPkg/Core/Dependency.c | 2 +- > StandaloneMmPkg/Core/Dispatcher.c | 18 +++---- > UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.c | 18 +++---- > UnitTestFrameworkPkg/Library/UnitTestBootLibNull/UnitTestBootLibNull.c | 2 +- > UnitTestFrameworkPkg/Library/UnitTestBootLibUsbClass/UnitTestBootLibUsbClass.c | 2 +- > UnitTestFrameworkPkg/Library/UnitTestLib/Log.c | 2 +- > UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLib.c | 2 +- > UnitTestFrameworkPkg/Test/UnitTest/Sample/SampleUnitTest/SampleUnitTest.c | 2 +- > ArmPkg/ArmPkg.ci.yaml | 55 +++++++++++++++++++- > ArmVirtPkg/ArmVirtPkg.ci.yaml | 25 +++++---- > DynamicTablesPkg/DynamicTablesPkg.ci.yaml | 31 ++++++++--- > DynamicTablesPkg/Include/Library/DynamicPlatRepoLib.h | 4 +- > DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.h | 2 +- > DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.h | 2 +- > DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.h | 2 +- > DynamicTablesPkg/Readme.md | 4 +- > FatPkg/EnhancedFatDxe/Fat.h | 2 +- > FatPkg/FatPkg.ci.yaml | 22 ++++++-- > FmpDevicePkg/FmpDevicePkg.ci.yaml | 4 ++ > PrmPkg/Include/Library/PrmModuleDiscoveryLib.h | 2 +- > PrmPkg/PrmLoaderDxe/PrmAcpiTable.h | 4 +- > PrmPkg/PrmPkg.ci.yaml | 9 ++++ > PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTest.h | 4 +- > StandaloneMmPkg/Include/Guid/MmCoreData.h | 2 +- > StandaloneMmPkg/StandaloneMmPkg.ci.yaml | 9 ++++ > UnitTestFrameworkPkg/PrivateInclude/Library/UnitTestBootLib.h | 2 +- > UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h | 2 +- > UnitTestFrameworkPkg/UnitTestFrameworkPkg.ci.yaml | 29 +++++++---- > 45 files changed, 209 insertions(+), 91 deletions(-) > > -- > 2.28.0.windows.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89844): https://edk2.groups.io/g/devel/message/89844 Mute This Topic: https://groups.io/mt/91166946/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Ard, I think that's a reasonable approach. We could also consider locking onto a specific cspell version to decrease the likelihood of this sporadically appearing in the future. In this case, I would prefer not to make the decision to disable spell check entirely on behalf of various package maintainers though. I'm just trying to keep the status quo from unblocking other changes. Do you think that's something you or others could add as a change on top of this series? Thanks, Michael On 5/17/2022 12:13 PM, Ard Biesheuvel wrote: > Hello Michael, > > Thanks for taking care of this. However, I strongly feel that > automation is increasing my workload here rather than the other way > around, for dubious benefits, so as a package maintainer, I am > inclined to disable the spell checks altogether for all the packages I > am in charge of. > > > On Tue, 17 May 2022 at 18:01, <mikuback@linux.microsoft.com> wrote: >> >> From: Michael Kubacki <michael.kubacki@microsoft.com> >> >> The SpellCheck plugin began reporting new typos that were previously >> missed. This is impacting edk2 pull requests from being completed. >> >> A change in the cspell plugin or some other upstream component may >> have caused them to appear now. This patch series mitigates the >> issue by fixing legitimate spelling issues and adding new ignored >> words if appropriate. >> >> Fixes in ArmPkg were deferred due to the number of reported issues >> in the package. The maintainers should follow up by reviewing the >> extended word list for SpellCheck in ArmPkg.ci.yaml and determining >> the best resolution for each item. >> >> That follow up task for ArmPkg was filed in the following BZ: >> https://bugzilla.tianocore.org/show_bug.cgi?id=3929 >> >> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com> >> Cc: Ankit Sinha <ankit.sinha@intel.com> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> >> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Guomin Jiang <guomin.jiang@intel.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Leif Lindholm <quic_llindhol@quicinc.com> >> Cc: Liming Gao <gaoliming@byosoft.com.cn> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> Cc: Michael Kubacki <mikuback@linux.microsoft.com> >> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Sami Mujawar <sami.mujawar@arm.com> >> Cc: Sean Brogan <sean.brogan@microsoft.com> >> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> >> Cc: Wei6 Xu <wei6.xu@intel.com> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> >> >> Michael Kubacki (8): >> PrmPkg: Fix new typos reported >> StandaloneMmPkg: Fix new typos reported >> DynamicTablesPkg: Fix new typos reported >> UnitTestFrameworkPkg: Fix new typos reported >> FatPkg: Fix new typos reported >> FmpDevicePkg: Fix new typos reported >> ArmPkg: Ignore new typos reported >> ArmVirtPkg: Add new ignored spelling errors >> >> DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 2 +- >> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCmn600LibArm/SsdtCmn600Generator.c | 2 +- >> DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c | 2 +- >> DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.c | 2 +- >> DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlParser.c | 2 +- >> DynamicTablesPkg/Library/Common/AmlLib/Tree/AmlNode.c | 2 +- >> DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepo.c | 4 +- >> DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.c | 2 +- >> DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c | 2 +- >> FatPkg/EnhancedFatDxe/FileSpace.c | 2 +- >> FatPkg/EnhancedFatDxe/ReadWrite.c | 2 +- >> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 2 +- >> PrmPkg/Application/PrmInfo/PrmInfo.c | 2 +- >> PrmPkg/Library/DxePrmModuleDiscoveryLib/DxePrmModuleDiscoveryLib.c | 2 +- >> PrmPkg/PrmLoaderDxe/PrmLoaderDxe.c | 2 +- >> PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestImage.c | 2 +- >> PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTestProtocol.c | 2 +- >> StandaloneMmPkg/Core/Dependency.c | 2 +- >> StandaloneMmPkg/Core/Dispatcher.c | 18 +++---- >> UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.c | 18 +++---- >> UnitTestFrameworkPkg/Library/UnitTestBootLibNull/UnitTestBootLibNull.c | 2 +- >> UnitTestFrameworkPkg/Library/UnitTestBootLibUsbClass/UnitTestBootLibUsbClass.c | 2 +- >> UnitTestFrameworkPkg/Library/UnitTestLib/Log.c | 2 +- >> UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLib.c | 2 +- >> UnitTestFrameworkPkg/Test/UnitTest/Sample/SampleUnitTest/SampleUnitTest.c | 2 +- >> ArmPkg/ArmPkg.ci.yaml | 55 +++++++++++++++++++- >> ArmVirtPkg/ArmVirtPkg.ci.yaml | 25 +++++---- >> DynamicTablesPkg/DynamicTablesPkg.ci.yaml | 31 ++++++++--- >> DynamicTablesPkg/Include/Library/DynamicPlatRepoLib.h | 4 +- >> DynamicTablesPkg/Library/Common/AmlLib/Parser/AmlMethodParser.h | 2 +- >> DynamicTablesPkg/Library/FdtHwInfoParserLib/Gic/ArmGicDispatcher.h | 2 +- >> DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.h | 2 +- >> DynamicTablesPkg/Readme.md | 4 +- >> FatPkg/EnhancedFatDxe/Fat.h | 2 +- >> FatPkg/FatPkg.ci.yaml | 22 ++++++-- >> FmpDevicePkg/FmpDevicePkg.ci.yaml | 4 ++ >> PrmPkg/Include/Library/PrmModuleDiscoveryLib.h | 2 +- >> PrmPkg/PrmLoaderDxe/PrmAcpiTable.h | 4 +- >> PrmPkg/PrmPkg.ci.yaml | 9 ++++ >> PrmPkg/Test/UnitTest/Library/UefiBootServicesTableLibUnitTest/UefiBootServicesTableLibUnitTest.h | 4 +- >> StandaloneMmPkg/Include/Guid/MmCoreData.h | 2 +- >> StandaloneMmPkg/StandaloneMmPkg.ci.yaml | 9 ++++ >> UnitTestFrameworkPkg/PrivateInclude/Library/UnitTestBootLib.h | 2 +- >> UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h | 2 +- >> UnitTestFrameworkPkg/UnitTestFrameworkPkg.ci.yaml | 29 +++++++---- >> 45 files changed, 209 insertions(+), 91 deletions(-) >> >> -- >> 2.28.0.windows.1 >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89846): https://edk2.groups.io/g/devel/message/89846 Mute This Topic: https://groups.io/mt/91166946/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 17 May 2022 at 18:25, Michael Kubacki <mikuback@linux.microsoft.com> wrote: > > Hi Ard, > > I think that's a reasonable approach. > > We could also consider locking onto a specific cspell version to > decrease the likelihood of this sporadically appearing in the future. > > In this case, I would prefer not to make the decision to disable spell > check entirely on behalf of various package maintainers though. I'm just > trying to keep the status quo from unblocking other changes. > > Do you think that's something you or others could add as a change on top > of this series? > I guess that could wait until after the stable tag. However, your current series adds words such as "addressig" and "characteritics" to the ignorelist of ArmPkg, and that is something I do have a problem with: this is just busywork, as there is no confusion whatsoever about the meaning of the texts in question, nobody is proposing changes to the code that contains these typos, and adding typos to these ignorelists is clearly not the correct solution. CI and automation are fine, but here, they are just creating problems, resulting in pointless churn, and impeding the stable tag process. Please, just turn it off. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89851): https://edk2.groups.io/g/devel/message/89851 Mute This Topic: https://groups.io/mt/91166946/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. Thanks, Michael On 5/17/2022 1:31 PM, Ard Biesheuvel wrote: > On Tue, 17 May 2022 at 18:25, Michael Kubacki > <mikuback@linux.microsoft.com> wrote: >> >> Hi Ard, >> >> I think that's a reasonable approach. >> >> We could also consider locking onto a specific cspell version to >> decrease the likelihood of this sporadically appearing in the future. >> >> In this case, I would prefer not to make the decision to disable spell >> check entirely on behalf of various package maintainers though. I'm just >> trying to keep the status quo from unblocking other changes. >> >> Do you think that's something you or others could add as a change on top >> of this series? >> > > I guess that could wait until after the stable tag. However, your > current series adds words such as "addressig" and "characteritics" to > the ignorelist of ArmPkg, and that is something I do have a problem > with: this is just busywork, as there is no confusion whatsoever about > the meaning of the texts in question, nobody is proposing changes to > the code that contains these typos, and adding typos to these > ignorelists is clearly not the correct solution. > > CI and automation are fine, but here, they are just creating problems, > resulting in pointless churn, and impeding the stable tag process. > Please, just turn it off. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89853): https://edk2.groups.io/g/devel/message/89853 Mute This Topic: https://groups.io/mt/91166946/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 17 May 2022 at 21:32, Michael Kubacki <mikuback@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 (#89854): https://edk2.groups.io/g/devel/message/89854 Mute This Topic: https://groups.io/mt/91166946/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
> <mikuback@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 (#89855): https://edk2.groups.io/g/devel/message/89855
Mute This Topic: https://groups.io/mt/91166946/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <mikuback@linux.microsoft.com> > 发送时间: 2022年5月18日 7:51 > 收件人: devel@edk2.groups.io; ardb@kernel.org > 抄送: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ankit Sinha > <ankit.sinha@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Bret > Barkelew <Bret.Barkelew@microsoft.com>; Gerd Hoffmann > <kraxel@redhat.com>; Guomin Jiang <guomin.jiang@intel.com>; Jiewen Yao > <jiewen.yao@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Liming > Gao <gaoliming@byosoft.com.cn>; Michael D Kinney > <michael.d.kinney@intel.com>; Nate DeSimone > <nathaniel.l.desimone@intel.com>; Ray Ni <ray.ni@intel.com>; Sami > Mujawar <sami.mujawar@arm.com>; Sean Brogan > <sean.brogan@microsoft.com>; Wei6 Xu <wei6.xu@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 > > <mikuback@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 (#89869): https://edk2.groups.io/g/devel/message/89869 Mute This Topic: https://groups.io/mt/91177798/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 <mikuback@linux.microsoft.com> >> 发送时间: 2022年5月18日 7:51 >> 收件人: devel@edk2.groups.io; ardb@kernel.org >> 抄送: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ankit Sinha >> <ankit.sinha@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Bret >> Barkelew <Bret.Barkelew@microsoft.com>; Gerd Hoffmann >> <kraxel@redhat.com>; Guomin Jiang <guomin.jiang@intel.com>; Jiewen Yao >> <jiewen.yao@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Liming >> Gao <gaoliming@byosoft.com.cn>; Michael D Kinney >> <michael.d.kinney@intel.com>; Nate DeSimone >> <nathaniel.l.desimone@intel.com>; Ray Ni <ray.ni@intel.com>; Sami >> Mujawar <sami.mujawar@arm.com>; Sean Brogan >> <sean.brogan@microsoft.com>; Wei6 Xu <wei6.xu@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 >>> <mikuback@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 (#89873): https://edk2.groups.io/g/devel/message/89873 Mute This Topic: https://groups.io/mt/91177798/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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; gaoliming@byosoft.com.cn; ardb@kernel.org > 抄送: 'Alexei Fedorov' <Alexei.Fedorov@arm.com>; 'Ankit Sinha' > <ankit.sinha@intel.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; > 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Gerd Hoffmann' > <kraxel@redhat.com>; 'Guomin Jiang' <guomin.jiang@intel.com>; 'Jiewen > Yao' <jiewen.yao@intel.com>; 'Leif Lindholm' <quic_llindhol@quicinc.com>; > 'Michael D Kinney' <michael.d.kinney@intel.com>; 'Nate DeSimone' > <nathaniel.l.desimone@intel.com>; 'Ray Ni' <ray.ni@intel.com>; 'Sami > Mujawar' <sami.mujawar@arm.com>; 'Sean Brogan' > <sean.brogan@microsoft.com>; 'Wei6 Xu' <wei6.xu@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 <mikuback@linux.microsoft.com> > >> 发送时间: 2022年5月18日 7:51 > >> 收件人: devel@edk2.groups.io; ardb@kernel.org > >> 抄送: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ankit Sinha > >> <ankit.sinha@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; > Bret > >> Barkelew <Bret.Barkelew@microsoft.com>; Gerd Hoffmann > >> <kraxel@redhat.com>; Guomin Jiang <guomin.jiang@intel.com>; Jiewen > Yao > >> <jiewen.yao@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; > Liming > >> Gao <gaoliming@byosoft.com.cn>; Michael D Kinney > >> <michael.d.kinney@intel.com>; Nate DeSimone > >> <nathaniel.l.desimone@intel.com>; Ray Ni <ray.ni@intel.com>; Sami > >> Mujawar <sami.mujawar@arm.com>; Sean Brogan > >> <sean.brogan@microsoft.com>; Wei6 Xu <wei6.xu@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 > >>> <mikuback@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 (#89882): https://edk2.groups.io/g/devel/message/89882 Mute This Topic: https://groups.io/mt/91181009/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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; gaoliming@byosoft.com.cn; ardb@kernel.org >> 抄送: 'Alexei Fedorov' <Alexei.Fedorov@arm.com>; 'Ankit Sinha' >> <ankit.sinha@intel.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; >> 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Gerd Hoffmann' >> <kraxel@redhat.com>; 'Guomin Jiang' <guomin.jiang@intel.com>; 'Jiewen >> Yao' <jiewen.yao@intel.com>; 'Leif Lindholm' <quic_llindhol@quicinc.com>; >> 'Michael D Kinney' <michael.d.kinney@intel.com>; 'Nate DeSimone' >> <nathaniel.l.desimone@intel.com>; 'Ray Ni' <ray.ni@intel.com>; 'Sami >> Mujawar' <sami.mujawar@arm.com>; 'Sean Brogan' >> <sean.brogan@microsoft.com>; 'Wei6 Xu' <wei6.xu@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 <mikuback@linux.microsoft.com> >>>> 发送时间: 2022年5月18日 7:51 >>>> 收件人: devel@edk2.groups.io; ardb@kernel.org >>>> 抄送: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ankit Sinha >>>> <ankit.sinha@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; >> Bret >>>> Barkelew <Bret.Barkelew@microsoft.com>; Gerd Hoffmann >>>> <kraxel@redhat.com>; Guomin Jiang <guomin.jiang@intel.com>; Jiewen >> Yao >>>> <jiewen.yao@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; >> Liming >>>> Gao <gaoliming@byosoft.com.cn>; Michael D Kinney >>>> <michael.d.kinney@intel.com>; Nate DeSimone >>>> <nathaniel.l.desimone@intel.com>; Ray Ni <ray.ni@intel.com>; Sami >>>> Mujawar <sami.mujawar@arm.com>; Sean Brogan >>>> <sean.brogan@microsoft.com>; Wei6 Xu <wei6.xu@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 >>>>> <mikuback@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 (#89887): https://edk2.groups.io/g/devel/message/89887 Mute This Topic: https://groups.io/mt/91181009/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
For the changes in CI , Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>. Create PR https://github.com/tianocore/edk2/pull/2906 to merge it for this stable tag. Thanks Liming > -----邮件原件----- > 发件人: Michael Kubacki <mikuback@linux.microsoft.com> > 发送时间: 2022年5月18日 22:52 > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn; ardb@kernel.org > 抄送: 'Alexei Fedorov' <Alexei.Fedorov@arm.com>; 'Ankit Sinha' > <ankit.sinha@intel.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; > 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Gerd Hoffmann' > <kraxel@redhat.com>; 'Guomin Jiang' <guomin.jiang@intel.com>; 'Jiewen > Yao' <jiewen.yao@intel.com>; 'Leif Lindholm' <quic_llindhol@quicinc.com>; > 'Michael D Kinney' <michael.d.kinney@intel.com>; 'Nate DeSimone' > <nathaniel.l.desimone@intel.com>; 'Ray Ni' <ray.ni@intel.com>; 'Sami > Mujawar' <sami.mujawar@arm.com>; 'Sean Brogan' > <sean.brogan@microsoft.com>; 'Wei6 Xu' <wei6.xu@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; gaoliming@byosoft.com.cn; > ardb@kernel.org > >> 抄送: 'Alexei Fedorov' <Alexei.Fedorov@arm.com>; 'Ankit Sinha' > >> <ankit.sinha@intel.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; > >> 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Gerd Hoffmann' > >> <kraxel@redhat.com>; 'Guomin Jiang' <guomin.jiang@intel.com>; 'Jiewen > >> Yao' <jiewen.yao@intel.com>; 'Leif Lindholm' > <quic_llindhol@quicinc.com>; > >> 'Michael D Kinney' <michael.d.kinney@intel.com>; 'Nate DeSimone' > >> <nathaniel.l.desimone@intel.com>; 'Ray Ni' <ray.ni@intel.com>; 'Sami > >> Mujawar' <sami.mujawar@arm.com>; 'Sean Brogan' > >> <sean.brogan@microsoft.com>; 'Wei6 Xu' <wei6.xu@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 <mikuback@linux.microsoft.com> > >>>> 发送时间: 2022年5月18日 7:51 > >>>> 收件人: devel@edk2.groups.io; ardb@kernel.org > >>>> 抄送: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ankit Sinha > >>>> <ankit.sinha@intel.com>; Ard Biesheuvel > <ardb+tianocore@kernel.org>; > >> Bret > >>>> Barkelew <Bret.Barkelew@microsoft.com>; Gerd Hoffmann > >>>> <kraxel@redhat.com>; Guomin Jiang <guomin.jiang@intel.com>; Jiewen > >> Yao > >>>> <jiewen.yao@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; > >> Liming > >>>> Gao <gaoliming@byosoft.com.cn>; Michael D Kinney > >>>> <michael.d.kinney@intel.com>; Nate DeSimone > >>>> <nathaniel.l.desimone@intel.com>; Ray Ni <ray.ni@intel.com>; Sami > >>>> Mujawar <sami.mujawar@arm.com>; Sean Brogan > >>>> <sean.brogan@microsoft.com>; Wei6 Xu <wei6.xu@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 > >>>>> <mikuback@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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.