From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <l.wagner@proxmox.com>
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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <l.wagner@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Thomas Lamprecht <t.lamprecht@proxmox.com>,
Stefan Lendl <s.lendl@proxmox.com>
References: <20240112144328.353398-1-s.lendl@proxmox.com>
<e4cac481-c0ba-4092-9e2d-458e7aaac290@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
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=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