[pbs-devel] [RFC v2 proxmox-backup] git-hooks: pre-commit runs cargo fmt --check

[Lukas Wagner]

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