From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 19258C1462 for ; Mon, 15 Jan 2024 13:46:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E61131B048 for ; Mon, 15 Jan 2024 13:46:53 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (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 firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 15 Jan 2024 13:46:53 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0C6BA4434F for ; Mon, 15 Jan 2024 13:46:53 +0100 (CET) Message-ID: <906ec91f-68c8-42ba-8a26-6e12694a89e9@proxmox.com> Date: Mon, 15 Jan 2024 13:46:52 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Lukas Wagner To: Proxmox Backup Server development discussion , Thomas Lamprecht , Stefan Lendl References: <20240112144328.353398-1-s.lendl@proxmox.com> <87zfx7hrnp.fsf@gmail.com> <2479b88f-fcc4-4668-974d-fe9547ab77fc@proxmox.com> Content-Language: de-AT, en-US In-Reply-To: <2479b88f-fcc4-4668-974d-fe9547ab77fc@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.004 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [RFC v2 proxmox-backup] git-hooks: pre-commit runs cargo fmt --check X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Jan 2024 12:46:54 -0000 On 1/15/24 10:42, Thomas Lamprecht wrote: >>> Let's rather add a fmt --check test to the buildbot, then we get >>> pinged on issues and a maintainer can just commit and push a cargo >>> fmt run without that much fuzz.. >>> >> >> I agree adding a fmt check to a CI make absolute sense. As of now the >> buildbot is not triggered when sending a patch series. So the user will >> not get feedback. > > This is not triggered either when sending a patch series though?! > And it wouldn't be triggered in any way if one never uses the > `make build` target too.. > >> Guarding locally against not-formatted commits, prevents user from >> sending these patches in the first place. > > No it does not, besides that it doesn't hook into git send-email or > format-patch, it will be overridden due to interfering with > >> The intent was to ensure style compliance BEFORE sending a patch series >> and not relying on a manual feedback process to identify this. > > buildbot is not a manual feedback process, and having that sometimes > slightly bad formatted code go into the repo just does not matter AT > ALL compared to pissing of people developing, rebasing, ... > So this is not solving any real problem our development process has, > at least none that would actually advance our projects, so it won't > go in, and even if it would just warn, the current form is way to > invasive to be applied for that either – would be great if one thinks > about existing hook users when doing such changes. > >> Once a CI is ready, this could and should be done in the CI. >> > > buildbot is already running on every commit and sends out mails on > failures of actual feature commits, for those odd cases where one > forgot to run cargo fmt (or setup their editor to do so), doing it > there would be more than enough... Just some thoughts from my side, since most of what was discussed can directly applied to running unit/integration tests as well: In the end, automated checks and CI should help us to build better software and make our lives easier. Everybody makes mistakes, developers and maintainers alike. These tools should help to detect them as early as possible. If issues can be detected directly on the dev's workstation by hooks/make targets, perfect! Intuitively I'd go with (obvious enough) warnings instead of a hard failure, because the failures can be annoying at times and it's always easy enough to amend the commit if you see a warning. But it'd not good to rely on that alone, so automated systems like buildbot (or whatever CI runner we choose, as maintaing buildbot seems to be a bit of a chore, from what I've heard) are equally important. One of the main issues I see with buildbot right now is that is does not help regular, non-maintainer developers *at all*, due to the fact that it only runs for commits that were already applied by a maintainer. What I'd like to have is a CI system that also has *my* back as a developer submitting changes (posting a patch series/requesting a pull), in case I don't notice a mistake (be it styling, clippy, build failures in certain feature flag permutations, failing tests etc.), allowing me to fix it before a maintainer finds the time to review/accept/apply my patch series. This should be the long-term goal, in my opinion. As an immediate improvement, added checks on buildbot and non-obtrusive hooks (could be opt-in for the dev as well) are a great addition as well, IMO. Lukas