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

[Kieran Bingham]

On 12/08/2021 14:16, Laurent Pinchart wrote:
> 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.

Agreed.


>>  - 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 ?

I tried using cpp as a preprocesor, but it's not happy - there are too
many issues to abuse cpp as a generic preprocessor with this file.

Annoyingly, it does 'work' but it generates lots of warnings and errors
due to # comments being treated as preprocessor directives and not
parsed correctly.

I've got an RFC to post that will let us keep multiple versions of the
.clang-format file as .clang-format-7, .clang-format-12, and the
checkstyle will symlink the latest supported version.

Not sure if I'm entirely fond of that either yet ... buy hence an RFC.


>>> 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.

Essentially that's what I've implemented so lets see how that goes.


>>>>>> 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:
>>>>>>  #
>