From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.16982.1685721030899320398 for ; Fri, 02 Jun 2023 08:50:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=ahVgnim8; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 037F360ADF for ; Fri, 2 Jun 2023 15:50:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 67CEAC433EF for ; Fri, 2 Jun 2023 15:50:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685721029; bh=5bGIoTmtRuishuAfQUrTD1NVCM4uY3MdC+IG2ZRIspE=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ahVgnim8v1Qhquh9cGFIN+qilYLQHs5Ml+cWWjbp1UZP/rynvroqZIpPYHxvgVRmz zoAw2Fivz0aSTm5gwl31rrP2kBXmqjs0MUH/GOfwG6bYZGod04Idrc/XnY/Ct7Mbh1 v6uydVq1zFCcLhx12/BMtQLnok9oSyhj1MpK83+/p9+VTiK82Wjhdk1A7zcMcTz2oo hyVcwNe/fv3nUbvTtFixYZYTLqajUIIKs8S1DXObHc3qir3h5Xbao+ykpWCj7GuwVp w2Wm8mCXWtNdid1hi/C33Xqr5KUUu0zIwgg1wl2AzYzT29rRPpVDKMGwj2tuXDU9AC ds/zlG/EN6+oA== Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-4f6148f9679so799073e87.3 for ; Fri, 02 Jun 2023 08:50:29 -0700 (PDT) X-Gm-Message-State: AC+VfDyhV3JdwrRc3YlrB0w/Zv3md1ZXbM1SCROTHgm2L6yMFvq2hNz0 a5AzgNov2rGLG0wlQJuhFixFhBLK7pZ1rcHSE4o= X-Google-Smtp-Source: ACHHUZ5rhxkSnvCI8CNSYza4/MgD8dkzGqLybkZ7evwVYOdOn21DdvxDIW5VBTWSY9dyqo9sJsv3nVyNlWVJx+CAPsg= X-Received: by 2002:ac2:5e83:0:b0:4e8:43e2:a8 with SMTP id b3-20020ac25e83000000b004e843e200a8mr2384643lfq.8.1685721027412; Fri, 02 Jun 2023 08:50:27 -0700 (PDT) MIME-Version: 1.0 References: <20230602085136.3552790-1-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 2 Jun 2023 17:50:15 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks To: Michael Kubacki Cc: devel@edk2.groups.io, Leif Lindholm , "Kinney, Michael D" Content-Type: text/plain; charset="UTF-8" On Fri, 2 Jun 2023 at 17:26, Michael Kubacki wrote: > > Are there particular areas that could be improved to make it more usable > for you? I'm trying to find actionable improvements that can be made, if > any. > > I know it's not perfect but developers can run it with a single keyboard > shortcut and it's been useful internally for eliminating style minutia > from distracting design and correctness conversation in code reviews. > I don't disagree with that. But I just don't find it as important as other people seem to feel it is, and combined with the flaky CI infra (I just had to amend and push another PR [0] due to infra failure), I am finding that just getting changes merged is taking a substantial amount of time, which I'd prefer to spend on other things. The coding style that uncrustify imposes is overly rigid, and leaves no room at all for any discretion on the part of the maintainer. [0] https://github.com/tianocore/edk2/pull/4470 > > On 6/2/2023 4:51 AM, Ard Biesheuvel wrote: > > Uncrustify checks are too rigid, making them counter-productive: > > > > - it leads to code that is arguably harder to parse visually (e.g., > > the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit > > 429309e0c6b74792) > > Looking at commit 7f198321eec0f520373, I see positive changes like in > ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of > multi-line parameter formatting in the mCpu->RegisterInterruptHandler() > call are consistent. > > That carries on for a number of C calls with inconsistent spacing before > opening parentheses and calls such as those to the DEBUG macro which I > find easier to read separating each parameter on a dedicated line. > That may be true. But I personally don't think it is important enough to a) rigidly enforce, and b) fix up for existing code (but that ship sailed when we did the mass conversion) > In AArch64Mmu.h, I agree that preserving the (mostly) global column as > opposed to block-specific columns would be easier to vertically scan. Is > that the main issue in the file? > Yes. > > - it forces indentation-only changes to code in the vicinity of actual > > changes, making the code history more bloated than necessary (see > > commit 7f198321eec0f520373 for an example) > > That's true. People adjusted things like indentation before depending on > a given change, but it is predetermined now. Perhaps turning off column > alignment (in general or in a given package) could help reduce noise > from this. > Note that I find this aspect quite important. In my opinion, the git history *is* the code base. Everything we changed at any point in the past, including the commit logs, is part of this, and mass whitespace conversions, or spurious changes just to make the CI happy are problematic to me. For this reason alone, I should be able to override CI choices at my discretion as a maintainer. > I don't think alignment enforcement is necessary. That might also help > address some of thrash in AArch64Mmu.h. > Interesting. If there is room for configuration here, why does the existing configuration deviate from the coding style guidelines we had been using for years? AIUI, we use a private fork of uncrustify for edk2, right? Are there any problematic aspects in the existing guidelines that was difficult to automate? > > - finding out from the web UI what exactly Uncrustify objected to is not > > straight-forward. > > > > This should not be necessary. It can be run locally to produce the same > result as in CI. > > IDE-specific, but a lot of people use VS Code with the Uncrustify > extension > (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) > and that allows Uncrustify to be run against the open source file by > simply pressing the "code formatter" command. > > I usually write the code, stage or commit it and then run this command > on the file. The code is formatted and it gives a normal local diff of > exactly what Uncrustify changed. > I'm sure this all seems quite reasonable if you already bought into using uncrustify. But for a drive-by contributor, or someone like me who has been contributing code for many years based on the agreed coding style guidelines, I struggle to understand why uncrustify is a reasonable solution to the problem of inconsistent coding style. I'm all for legible code, don't get me wrong. (And Leif is much more pedantic in that sense than I am). But in my experience, the uncrustify rules are too rigid, and arbitrary (two spaces indentation here, four spaces there, etc etc) I suppose having a button in the GitHub UI that simply lets us exercise our discretion as maintainers to deviate from these rules would go a long way in addressing these concerns. But as it stands now, having my carefully crafted contributions rejected by an automated system based on guidelines that deviate from the ones that we agreed to, without any recourse, is simply not acceptable. > > So let's enable AuditMode for ArmPkg, so that interested parties can see > > the uncrustify recommendations if desired, but without preventing the > > changes from being merged. This leaves it at the discretion of the > > ArmPkg maintainers to decide which level of conformance is required. > > > > Cc: Leif Lindholm > > Cc: "Kinney, Michael D" > > Cc: Michael Kubacki > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPkg/ArmPkg.ci.yaml | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml > > index 24db7425051388cf..d3124816118944cb 100644 > > --- a/ArmPkg/ArmPkg.ci.yaml > > +++ b/ArmPkg/ArmPkg.ci.yaml > > @@ -239,5 +239,10 @@ > > ], > > > > "AdditionalIncludePaths": [] # Additional paths to spell check > > > > # (wildcards supported) > > > > + }, > > > > + > > > > + # options defined in .pytool/Plugin/UncrustifyCheck > > > > + "UncrustifyCheck": { > > > > + "AuditOnly": True > > > > } > > > > } > >