[libcamera-devel] [PATCH] clang-format: Bump minimum version requirements

[Laurent Pinchart]

Hi Kieran,

On Thu, Aug 12, 2021 at 02:06:38PM +0100, Kieran Bingham wrote:
> On 12/08/2021 13:13, Laurent Pinchart wrote:
> > On Thu, Aug 12, 2021 at 07:07:24PM +0900, paul.elder at ideasonboard.com wrote:
> >> On Thu, Aug 12, 2021 at 10:01:26AM +0100, David Plowman wrote:
> >>> On Thu, 12 Aug 2021 at 09:45, Kieran Bingham wrote:
> >>>>
> >>>> The .clang-format file uses CaseSensitive on Regex rules for Include Categories.
> >>>> Without this, it would not be possible to distinguish between QT headers which
> >>>> commence with a 'Q' verses system headers including <queue>.
> >>>>
> >>>> While we have more wider support for libcamera to run and build on older
> >>>> platforms, it should be safe to assume that developers who need to run
> >>>> checkstyle can update their clang-format to a modern version.
> >>>
> >>> Do you know the magic runes to do this? The Pi repositories are stuck
> >>> on version 9 currently, I believe.
> >>
> >> iirc the raspberry pi repos are compatible with regular debian repos? So
> >> you add sid to the apt repos:
> >>
> >> /etc/apt/sources.list:
> >> +deb https://deb.debian.org/debian/ sid main
> >> +deb-src https://deb.debian.org/debian/ sid main
> >>
> >> And make sure that sid doesn't get used by default (I have it set to
> >> bullseye) (make sure it doesn't conflict with other preferences settings
> >> in /etc/apt/preferences.d/) (I think preferences.d is recommended but I
> >> like all my preferences in one place):
> >>
> >> /etc/apt/preferences:
> >> Package: *
> >> Pin: release n=bullseye
> >> Pin-Priority: 1100
> >>
> >> And then apt update, and apt install clang-format-12.
> >> update-alternatives doesn't seem to pick it up so you might have to
> >> relink it manually.
> >>
> >> Not sure if this is best practice for raspberry pi though :/ I'm also
> >> not sure if this works alongside buster as I'm on bullseye, but it
> >> doesn't look like clang-format-12 has any conflicting dependencies with
> >> bullseye, so maybe it's fine on buster too.
> >>
> >>
> >> This is why I'm pushing against this patch tbh... it's like saying "it's
> >> okay because Ubuntu 22.04 has it". But it is a good change... idk what's
> >> best.
> 
> Well the distinction I was hoping to make in the commit message was that
> this is a dependency for /developers/ not users or compilers of the project.
> 
> It's only on those who create patches, and run checkstyle, who, as we
> already recommend installing the latest or later Doxygen, and Meson, I
> thought this would be the same category.
> 
> But it seems some distributions are harder to update.
> 
> 
> So this issue affects:
> 
>  - Developers only who run checkstyle.py

That should be all developers :-) Anything that makes checkstyle.py more
difficult to use is a bad regression I think.

>  - On a patch which modifies headers containing either <queue> or a
>    <Qt*>  header
> 
> git grep -l "<queue>"
> src/gstreamer/gstlibcamerasrc.cpp
> src/ipa/rkisp1/rkisp1.cpp
> src/libcamera/pipeline/ipu3/cio2.h
> src/libcamera/pipeline/ipu3/frames.h
> src/libcamera/pipeline/ipu3/ipu3.cpp
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> src/libcamera/pipeline/raspberrypi/rpi_stream.h
> src/libcamera/pipeline/rkisp1/rkisp1.cpp
> src/libcamera/pipeline/simple/simple.cpp
> 
> 
> If I drop the CaseSensitive: true then these files may ... very rarely
> most likely ... show up with a change in check style trying to move <queue>.
> 
> But as it's a one off... and when we get wider support for clang-format
> 12, then we can re-introduce it to stop the false move by clang.
> 
> The alternative (removing the QT header match) would result in larger
> risk of false moves by checkstyle I believe, but even those would be
> limited to changes made to QCam, which isn't receiving a high churn.

Can we condition the headers sorting rules on the clang-format version,
disabling them with clang-format < 12 ?

> > A hard dependency on clang-format >= 12 seems to harsh to me. Can we
> > make it optional ? One option would be to have multiple configuration
> > files, or to pass additional options to clang-format on the command line
> > from checkstyle.py.
> 
> Two files could work, though at the moment it's a single line diff
> between the two. I don't think command line arguments can help currently
> (other than specifying a clang-format version specific config file.)

There's a --style argument that accepts individual style rules, but I
don't know if it's applied on top of .clang-format or replaces it (it
seems to be the latter). It also appears that there's no command line
option to specify a custom style file, which is very annoying. We could
create symlinks on demand, or use another workaround.

> >>>> clang-format-12 is supported in Ubuntu 20.04 in the updates category,
> >>>> and in Debian sid. More recent versions of Ubuntu ship with
> >>>> clang-format-12 or later by default, and other distributions may of
> >>>> course vary further.
> >>>>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> ---
> >>>>  .clang-format | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/.clang-format b/.clang-format
> >>>> index ff60b928affc..df1421fe94a8 100644
> >>>> --- a/.clang-format
> >>>> +++ b/.clang-format
> >>>> @@ -1,6 +1,6 @@
> >>>>  # SPDX-License-Identifier: GPL-2.0-only
> >>>>  #
> >>>> -# clang-format configuration file. Intended for clang-format >= 7.
> >>>> +# clang-format configuration file. Intended for clang-format >= 12.
> >>>>  #
> >>>>  # For more information, see:
> >>>>  #

-- 
Regards,

Laurent Pinchart