[v6,5/5] libcamera: camera: Hide Camera::create() from the public API
diff mbox series

Message ID 20240807154410.9552-6-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Split libcamera documentation in public and internal APIs
Related show

Commit Message

Laurent Pinchart Aug. 7, 2024, 3:44 p.m. UTC 
The Camera::create() function is internal. Hide it from the public API
documentation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/camera.cpp | 1 +
 1 file changed, 1 insertion(+)

Comments

Dan Scally Aug. 7, 2024, 3:51 p.m. UTC | #1 
Hi Laurent

On 07/08/2024 16:44, Laurent Pinchart wrote:
> The Camera::create() function is internal. Hide it from the public API
> documentation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---


Oops, I missed this before. I think it's broadly fine but we'll have to be pretty vigilant to catch 
any new functions with the same problem.

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   src/libcamera/camera.cpp | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index aca466c9ba72..382a68f7bddd 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -821,6 +821,7 @@ void Camera::Private::setState(State state)
>    */
>   
>   /**
> + * \internal
>    * \brief Create a camera instance
>    * \param[in] d Camera private data
>    * \param[in] id The ID of the camera device
Nicolas Dufresne Aug. 7, 2024, 8:47 p.m. UTC | #2 
Hi,

Le mercredi 07 août 2024 à 16:51 +0100, Dan Scally a écrit :
> Hi Laurent
> 
> On 07/08/2024 16:44, Laurent Pinchart wrote:
> > The Camera::create() function is internal. Hide it from the public API
> > documentation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> 
> 
> Oops, I missed this before. I think it's broadly fine but we'll have to be pretty vigilant to catch 
> any new functions with the same problem.
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

I wonder if there is a way to catch accidental API additions in the CI ?

Nicolas

> 
> >   src/libcamera/camera.cpp | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index aca466c9ba72..382a68f7bddd 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -821,6 +821,7 @@ void Camera::Private::setState(State state)
> >    */
> >   
> >   /**
> > + * \internal
> >    * \brief Create a camera instance
> >    * \param[in] d Camera private data
> >    * \param[in] id The ID of the camera device
Kieran Bingham Aug. 8, 2024, 9:02 a.m. UTC | #3 
Quoting Nicolas Dufresne (2024-08-07 21:47:24)
> Hi,
> 
> Le mercredi 07 août 2024 à 16:51 +0100, Dan Scally a écrit :
> > Hi Laurent
> > 
> > On 07/08/2024 16:44, Laurent Pinchart wrote:
> > > The Camera::create() function is internal. Hide it from the public API
> > > documentation.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > 
> > 
> > Oops, I missed this before. I think it's broadly fine but we'll have to be pretty vigilant to catch 
> > any new functions with the same problem.
> > 
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> I wonder if there is a way to catch accidental API additions in the CI ?

We can already compare ABI/API changes between versions with the script
at ./utils/abi-compat.sh

To integrate that in CI - I would probably want to find a way to 'store'
the pre-built ABI files for each tagged/released version so it doesn't
get rebuilt for every run. But otherwise - it's just 'another build' so
maybe it's fine, especially as most of the time it should be run against
the merge base, not necessarily the latest tag release.

Then we can have the CI report an 'error' on ABI breakage, and a Warning
on ABI additions or such.

The ./utils/abi-compat.sh can already compare two commits directly so
this should be trivial to add to CI. Just depends if we want to add the
computational costs to CI. Maybe it's fine.

--
Kieran


> 
> Nicolas
> 
> > 
> > >   src/libcamera/camera.cpp | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index aca466c9ba72..382a68f7bddd 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -821,6 +821,7 @@ void Camera::Private::setState(State state)
> > >    */
> > >   
> > >   /**
> > > + * \internal
> > >    * \brief Create a camera instance
> > >    * \param[in] d Camera private data
> > >    * \param[in] id The ID of the camera device
>
Laurent Pinchart Aug. 8, 2024, 9:20 a.m. UTC | #4 
On Thu, Aug 08, 2024 at 10:02:24AM +0100, Kieran Bingham wrote:
> Quoting Nicolas Dufresne (2024-08-07 21:47:24)
> > Le mercredi 07 août 2024 à 16:51 +0100, Dan Scally a écrit :
> > > On 07/08/2024 16:44, Laurent Pinchart wrote:
> > > > The Camera::create() function is internal. Hide it from the public API
> > > > documentation.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > 
> > > 
> > > Oops, I missed this before. I think it's broadly fine but we'll have to be pretty vigilant to catch 
> > > any new functions with the same problem.
> > > 
> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > I wonder if there is a way to catch accidental API additions in the CI ?
> 
> We can already compare ABI/API changes between versions with the script
> at ./utils/abi-compat.sh
> 
> To integrate that in CI - I would probably want to find a way to 'store'
> the pre-built ABI files for each tagged/released version so it doesn't
> get rebuilt for every run. But otherwise - it's just 'another build' so
> maybe it's fine, especially as most of the time it should be run against
> the merge base, not necessarily the latest tag release.
> 
> Then we can have the CI report an 'error' on ABI breakage, and a Warning
> on ABI additions or such.
> 
> The ./utils/abi-compat.sh can already compare two commits directly so
> this should be trivial to add to CI. Just depends if we want to add the
> computational costs to CI. Maybe it's fine.

I'd like to at least try to cache the data. If it can't be done, then we
can discuss alternatives.

> > > >   src/libcamera/camera.cpp | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index aca466c9ba72..382a68f7bddd 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -821,6 +821,7 @@ void Camera::Private::setState(State state)
> > > >    */
> > > >   
> > > >   /**
> > > > + * \internal
> > > >    * \brief Create a camera instance
> > > >    * \param[in] d Camera private data
> > > >    * \param[in] id The ID of the camera device
Kieran Bingham Aug. 8, 2024, 9:45 a.m. UTC | #5 
Quoting Laurent Pinchart (2024-08-08 10:20:33)
> On Thu, Aug 08, 2024 at 10:02:24AM +0100, Kieran Bingham wrote:
> > Quoting Nicolas Dufresne (2024-08-07 21:47:24)
> > > Le mercredi 07 août 2024 à 16:51 +0100, Dan Scally a écrit :
> > > > On 07/08/2024 16:44, Laurent Pinchart wrote:
> > > > > The Camera::create() function is internal. Hide it from the public API
> > > > > documentation.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > ---
> > > > 
> > > > 
> > > > Oops, I missed this before. I think it's broadly fine but we'll have to be pretty vigilant to catch 
> > > > any new functions with the same problem.
> > > > 
> > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > 
> > > I wonder if there is a way to catch accidental API additions in the CI ?
> > 
> > We can already compare ABI/API changes between versions with the script
> > at ./utils/abi-compat.sh
> > 
> > To integrate that in CI - I would probably want to find a way to 'store'
> > the pre-built ABI files for each tagged/released version so it doesn't
> > get rebuilt for every run. But otherwise - it's just 'another build' so
> > maybe it's fine, especially as most of the time it should be run against
> > the merge base, not necessarily the latest tag release.
> > 
> > Then we can have the CI report an 'error' on ABI breakage, and a Warning
> > on ABI additions or such.
> > 
> > The ./utils/abi-compat.sh can already compare two commits directly so
> > this should be trivial to add to CI. Just depends if we want to add the
> > computational costs to CI. Maybe it's fine.
> 
> I'd like to at least try to cache the data. If it can't be done, then we
> can discuss alternatives.

Agreed, if theres' a way to store a cache in CI that perhaps deletes
objects after 'some time without being referenced' that would be
sufficient. Anything unavailable would always get rebuilt. I have no
idea what's avaialble in gitlab CI though in this regards.

./utils/abi-compat.sh will already take advantage of that and only
rebuild objects for commits that do not yet exist so something elegant
could be built if there is an existing cache infrastructure to support
it.

> 
> > > > >   src/libcamera/camera.cpp | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > > index aca466c9ba72..382a68f7bddd 100644
> > > > > --- a/src/libcamera/camera.cpp
> > > > > +++ b/src/libcamera/camera.cpp
> > > > > @@ -821,6 +821,7 @@ void Camera::Private::setState(State state)
> > > > >    */
> > > > >   
> > > > >   /**
> > > > > + * \internal
> > > > >    * \brief Create a camera instance
> > > > >    * \param[in] d Camera private data
> > > > >    * \param[in] id The ID of the camera device
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index aca466c9ba72..382a68f7bddd 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -821,6 +821,7 @@  void Camera::Private::setState(State state)
  */
 
 /**
+ * \internal
  * \brief Create a camera instance
  * \param[in] d Camera private data
  * \param[in] id The ID of the camera device