[libcamera-devel] [PATCH v10 4/7] Add Python bindings

[Tomi Valkeinen]

On 10/05/2022 12:08, Kieran Bingham wrote:
> Quoting Tomi Valkeinen (2022-05-10 07:04:58)
>> On 09/05/2022 23:56, Kieran Bingham wrote:
>>
>>> I'd like to add the following here while / when merging:
>>>
>>> ================================================================================
>>>
>>> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
>>> index c983da3a94f1..09646faacd5a 100644
>>> --- a/src/py/libcamera/pymain.cpp
>>> +++ b/src/py/libcamera/pymain.cpp
>>> @@ -17,6 +17,7 @@
>>>    #include <unistd.h>
>>>
>>>    #include <libcamera/base/log.h>
>>> +
>>>    #include <libcamera/libcamera.h>
>>>
>>>    #include <pybind11/functional.h>
>>> @@ -138,6 +139,14 @@ static void handleRequestCompleted(Request *req)
>>>    void init_pyenums(py::module &m);
>>>    void init_pyenums_generated(py::module &m);
>>>
>>> +/*
>>> + * The python module is not expected to be directly compatible with our usual
>>> + * code style, and clang-format can't easily handle the specific styles used
>>> + * with chaining here. Disable clang-format for the remainder of this module.
>>> + */
>>> +
>>> +// clang-format off
>>> +
>>>    PYBIND11_MODULE(_libcamera, m)
>>>    {
>>>           init_pyenums(m);
>>> ================================================================================
>>>
>>> Any objections, or modifications to the comment?
>>
>> Looks good to me.
>>
>> I'd really like to use clang-format, though. It almost looks to me that
>> clang-format bugs here. E.g. first clang-format formats to this:
>>
>>          pyCamera
>>                  .def(
>>                          "start", [](Camera &self, py::dict controls) {
>>                                  return 0;
>>                          },
>>                          py::arg("controls") = py::dict());
>>
>> I have no idea why it wants to add the line feed after .def. But I
>> remove the py::arg, and format:
>>
>>          pyCamera
>>                  .def(
>>                          "start", [](Camera &self, py::dict controls) {
>>                                  return 0;
>>                          });
>>
>> So no change. Then I first manually remove the line feed, i.e.
>> .def("start", ... and then format:
>>
>>          pyCamera
>>                  .def("start", [](Camera &self, py::dict controls) {
>>                          return 0;
>>                  });
>>
>> Ta-da! Now it's fine. But now, adding back the py::arg, and format, it
>> goes back to the first case.
>>
>> I've been fighting with clang-format before, and I have never managed it
>> to format the code quite properly. Looking at the formatted pymain.cpp,
>> I see two issues: the first is the extra line split like above, the
>> second is that it likes to create very long lines for lambdas. Neither
>> of those look like intended usual libcamera coding style, but rather
>> bugs or misconfigured rules.
> 
> If you can find a way to satiate clang-format then that's probably
> preferred too - but I don't want to break the way you've styled these
> indents, and I think they make sense, given the modelling used by the
> pybind.

pybind11 project has its own .clang-format file, but even that formats 
the bindings badly. I just can't see why clang-format wants to do this:

      .def(
          "add_buffer",
          [](Request &self, const Stream *stream, FrameBuffer *buffer) {
              return self.addBuffer(stream, buffer);
          },
          py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */

None of the lines are close to the 80 char limit, but still clang-format 
insist on line break there.

> Would you prefer me to leave the clang-format on and we can ignore the
> checkstyle warnings here ? Or perhaps try to improve in the future?
> 
> Or we could also try to improve in the future and then re-enable
> clang-format?

Well, as I said, I'd definitely want to use clang-format, but until 
something happens which makes it possible, I think disabling 
clang-format is fine.

However, it does seem to have a practical issue. I still use 
clang-format to manually format pieces of the code, and then manually 
correct it if clang-format gets it wrong. With "clang-format off" that 
manual formatting is not possible anymore, as clang-format does not 
format anything after "clang-format off".

So I'd prefer to leave the "clang-format off" out for now and ignore the 
warnings.

  Tomi