From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id E41A99412A5 for ; Fri, 17 Nov 2023 09:08:41 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=obWhsULMJ/kjeUur9oTPYpvbpNvclQuiz9Ey5PNVQ5s=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700212120; v=1; b=urYYw/I4MtNg4iMuOZnQcniXdYxBjeKPm5D1nuabhj/3XwFevdlphjCJJZrsWNB5lDyoddda 5nPLAqk2vP2Z2EsrX2x0g44eqYwdu9lsapOxoo1s5pAqADzAOScjS21WiIwlBNhEFoIcieXUq6C IbQYwVzDWCH8UpAwzI4Gn8Q0= X-Received: by 127.0.0.2 with SMTP id CeDpYY7687511xFgucndxQVg; Fri, 17 Nov 2023 01:08:40 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.8158.1700212119963140344 for ; Fri, 17 Nov 2023 01:08:40 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-192-naPEsB22MVS4C8WYoTPhEA-1; Fri, 17 Nov 2023 04:08:35 -0500 X-MC-Unique: naPEsB22MVS4C8WYoTPhEA-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 213AA811E7E; Fri, 17 Nov 2023 09:08:35 +0000 (UTC) X-Received: from [10.39.193.21] (unknown [10.39.193.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 91B152166B2A; Fri, 17 Nov 2023 09:08:33 +0000 (UTC) Message-ID: <59767c3d-f79a-15fc-b498-3aa829ce3450@redhat.com> Date: Fri, 17 Nov 2023 10:08:32 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] edk2 uncrustify update (73.0.8)? To: devel@edk2.groups.io, pedro.falcato@gmail.com Cc: Rebecca Cran , mikuback@linux.microsoft.com, Michael Kinney , Andrew Fish , Marcin Juszkiewicz , "Leif Lindholm (Quic)" References: <0107c96b-849a-db48-194b-1a4c1f3b0c78@redhat.com> <23dd696a-52a1-4c26-bfb6-5b5587325c42@linux.microsoft.com> <30e9b11c-ab39-4266-8981-5242542b625d@bsdio.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: fbhB6Ep0dhyLGB9vtkV3bAFxx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="urYYw/I4"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/16/23 09:29, Pedro Falcato wrote: > On Tue, Nov 14, 2023 at 3:01 PM Laszlo Ersek wrote: >> >> On 11/13/23 22:33, Pedro Falcato wrote: >>> On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran wrote: >>>> >>>> On 11/13/2023 1:08 PM, Michael Kubacki wrote: >>>>> Yes. I just did it. It is relatively minor and impacts expected code >>>>> areas. >>>>> >>>>> https://github.com/tianocore/edk2/pull/5043/files >>>> >>>> >>>> Could you update .git-blame-ignore-revs please? >>> >>> You can't do that until the merge is done, since we use >>> rebase-and-merge for tianocore (and rebases do not keep stable commit >>> hashes). >>> But I would plead that this should not get merged in general :/ >> > > Laszlo! > > Sorry for the delay. > >> Seeing the cumulative diff in that PR, do you have specific >> counter-arguments? > > Well, my counter-argument is that formatting is becoming a topic of > its own. I used to be very pro-formatter but if this leads to either > 1) eyesore or 2) weird churn every now and then, > I feel like we should reconsider the current approach. I feel like all > formatting (manual or automated) is fine as long as it's: > 1) Visually consistent with the codebase's style > 2) Not horrendous to look at That was what we did first, for several years. It wasn't working well. An edk2 coding style document had always existed, but it had sufficient corner cases that formatting had *always* been a topic of its own. In particular, point (2) "not horrendous to look at" is impossible to define. - edk2 mainly uses a Windows-like coding style, which is immediately horrendous to anyone coming from a Linux background, at first - edk2 uses extremely long lines in some modules (200+ characters). I personally use 80 character lines (for good reason -- not the greatest eyesight, so I need to use a relatively large font, and I like to place two columns of source code on my screen side-by-side, for diffing or comparing otherwise). I've received multiple "buy a larger monitor" or "use two monitors at the same time", and those don't work for me either. Some people are super productive with 30" monitors, I'm not. Therefore, "long lines or not" can never be decided by democracy, only by decree. uncrustify is decree. - somewhat as a consequence of my avoidance of long lines, I tended to break up long function calls earlier / more frequently than other developers. In order to conserve *vertical* screen real estate, I used a "condensed" multi-line argument style for function calls, in OvmfPkg and ArmVirtPkg. (The official multi-line style is more wasteful.) That wasn't working for other project participants. Uncrustify now does not permit my preferred (condensed) style, but at least there are no more debates about it. Your premise is that those two points form a stable foundation, but they don't. We tried. > and switching back and forth because 'magic indentation tool' says so > just seems silly to me. I don't perceive this update as switching back and forth; it seems like a minor tweak. >> The diff is trivial, IMO. You mentioned "error prone" and "much churn", >> which are very valid concerns, but they don't seem to apply here. We can >> review a diff of this size (especially if it's split up on Pkg >> boundaries), and the github view indicates the change is only in >> whitespace amount. >> >> The change in >> "OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c" >> is a net win; the current formatting is really distracting. >> >> Furthermore, this diff actually highlights some inexplicable syntax in >> "EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c": those backslashes (not >> parts of any macro definition) are an eyesore. They should be fixed >> regardless of re-uncrustification. >> >> The version N vs. N+1 concern shouldn't be one; the authoritative >> version is what the YAML file in edk2 says. > > Well, I fear it's not that simple. EDK2's uncrustify has historically > been a PITA, I've had to convince people to give it a try, I myself > don't even know how I installed it (IIRC, some weird random > combination of unzip + the NuGet...). (1) The following file in edk2: .pytool/Plugin/UncrustifyCheck/Readme.md highlights the "Project Mu Uncrustify fork source code and documentation": https://dev.azure.com/projectmu/Uncrustify (This file is easy to find in edk2 just by grepping for "uncrustify".) (2) The "Repos" link in the sidebar of that page leads to: https://dev.azure.com/projectmu/_git/Uncrustify A Clone button appears then, with the project URL being https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify (3) This project can be cloned, and built on Linux very easily. It has a top-level README.md file with instructions. It needs "cmake" and "make". (4) The actual version to check out and build -- in order to match the github CI -- is captured in edk2's file .pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml (5) A bit of digging in edk2's .pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py explains that the edk2 uncrustify config file to use with the binary from (3) and (4) is .pytool/Plugin/UncrustifyCheck/uncrustify.cfg plus further useful command line options. (6) Based on all that, I run uncrustify locally as follows: uncrustify \ -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \ -F file-list \ --replace \ --no-backup \ --if-changed I dislike downloading binaries and/or containers just like many others. Luckily, in this case, it's easy to build a local binary from source, and to run it. > Getting everyone on the same version would already be hard even if the > software was easy to install. I agree the documentation does not target a native, local build. Most users (i.e., most developers) seem to prefer something that "just works", be that a remotely build binary, or a container. That's probably why the documentation explains that kind of usage. I've not suggested extending the documentation with something like the above bullet list because I consider my preference (i.e., for a local build) to be "niche". Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111343): https://edk2.groups.io/g/devel/message/111343 Mute This Topic: https://groups.io/mt/102559740/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-