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

[Kieran Bingham]

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


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

--
Kieran



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