| Message ID | 20210812084501.2883780-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran On Thu, 12 Aug 2021 at 09:45, Kieran Bingham <kieran.bingham@ideasonboard.com> 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. Thanks! David > > 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@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: > # > -- > 2.30.2 >
Hi David, On Thu, Aug 12, 2021 at 10:01:26AM +0100, David Plowman wrote: > Hi Kieran > > On Thu, 12 Aug 2021 at 09:45, Kieran Bingham > <kieran.bingham@ideasonboard.com> 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. Paul > > > > > 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@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: > > # > > -- > > 2.30.2 > >
Hello, On Thu, Aug 12, 2021 at 07:07:24PM +0900, paul.elder@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. 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. > > > 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@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: > > > #
On 12/08/2021 13:13, Laurent Pinchart wrote: > Hello, > > On Thu, Aug 12, 2021 at 07:07:24PM +0900, paul.elder@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@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: >>>> # >
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@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@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: > >>>> #
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@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@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: >>>>>> # >
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: #
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. 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@ideasonboard.com> --- .clang-format | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)