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

[Kieran Bingham]

Quoting Tomi Valkeinen (2022-05-10 10:22:57)
> 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.

Ok - I can go with that. But the line fix on the headers probably goes
in still ;-)

I think that's enough for me to keep going with the integrations.

--
Kieran


> 
>   Tomi