[libcamera-devel] [PATCH] libcamera: Move internal headers to src/libcamera/include/libcamera/

[Laurent Pinchart]

Hi Kieran,

On Tue, May 12, 2020 at 08:58:06AM +0100, Kieran Bingham wrote:
> On 11/05/2020 23:20, Laurent Pinchart wrote:
> > The libcamera internal headers are located in src/libcamera/include/.
> > The directory is added to the compiler headers search path with a meson
> > include_directories() directive, and internal headers are included with
> > (e.g. for the internal semaphore.h header)
> > 
> >   #include "semaphore.h"
> > 
> > All was well, until libcxx decided to implement the C++20
> > synchronization library. The __threading_support header gained a
> > 
> >   #include <semaphore.h>
> > 
> > to include the pthread's semaphore support. As include_directories()
> > adds src/libcamera/include/ to the compiler search path with -I, the
> > internal semaphore.h is included instead of the pthread version.
> > Needless to say, the compiler isn't happy.
> > 
> > Thread options have been considered to fix this issue:
> > 
> > - Use -iquote instead of -I. The -iquote option instructs gcc to only
> >   consider the header search path for headers included with the ""
> >   version. Meson unfortunately doesn't support this option.
> > 
> > - Rename the internal semaphore.h header. This was deemed to be the
> >   beginning of a long whack-a-mole game, where namespace clashes with
> >   system libraries would appear over time (possibly dependent on
> >   particular system configurations) and would need to be constantly
> >   fixed.
> > 
> > - Rename the src/libcamera/include/ internal headers directory to
> >   src/libcamera/include/libcamera/. This causes lots of churn in all the
> >   existing source files through the all project.
> > 
> > The first option would be best, but isn't available to us due to missing
> > support in meson. Even if -iquote support was added, we would need to
> > fix the problem before a new version of meson containing the required
> > support would be released.
> 
> Have you reported this to meson?

I've discussed it with Jussi Pakkanen on their IRC channel. He wasn't
aware of -iquote (and to be fair, I wasn't either until a couple of days
ago :-)). He thought solving the namespace clash itself was better, as
otherwise other issues could appear in the future. I'm certainly
cautious here as I want a future-proof solution, but I think -iquote
would provide it.

-iquote isn't something Jussi seemed to rule out (although he said it
would probably not work with the Microsoft compiler, but that's not an
issue for us), but not something he was considering as a priority
either. As we have to fix this before -iquote support is available
anyway, I didn't try to push too hard.

> > The third option is thus the only practical solution available. Bite the
> > bullet, and do it.
> 
> Ouch, I want to find a way to disagree, because -iquote is what we
> /want/ but if meson is forcing our hand .. then ... ;-(
> 
> Sooo much redundant use of specifying the path ...
> 
> Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 
> > While at it, should we pick a different directory name for internal
> > headers, such as libcamera-internal (or hopefully something shorter) to
> > allow identically named public and private headers ? Or would that be
> > considered bad practice ?
> 
> I would be worried about having the same internal and external file
> name. It would get confusing.
> 
> I guess it could be said that being explicit about #include
> "libcamera/internal/pixelformat.h" would be different from #include
> "libcamera/pixelformat.h" ... but we're definitely getting into an ouch
> territory there...

Agreed. "libcamera/internal/foo.h" is an interesting name though, as it
would be very explicit that the header is internal. It could be achieved
with

	mv src/libcamera/include src/libcamera/internal

and -Isrc/ but that potentially opens the door to more namespace clashes
(for instance src/gstreamer/foo.h could get included by mistake instead
of /usr/include/gstreamer/foo.h - this particular example is bogus as
the system location is /usr/include/gstreamer-1.0/gst/, but you get the
idea). Having

	mv src/libcamera/include src/libcamera/include/libcamera/internal

isn't something I would be looking forward to
(src/libcamera/include/libcamera/ is painful enough). We could however
move the headers to include/libcamera/internal/, I'm not sure to like it
much, but that may be because I'm used to the current location in
src/libcamera/.

> To better consider that ... what purposes would we have for having the
> same header expose a different interface internally than externally?
> 
> (i.e. potentially answering my own question, could we expect an internal
> pixelformat.h to provide V4L2 pixelformats as well?, I think not in that
> instance, but maybe there are other possible scenarios like that...)

For classes that will be extended with a D-pointer, the definition of
the internal D class could go to "libcamera/internal/foo.h" for a public
class defined in <libcamera/foo.h>. Some of the D classes will be
completely internal to one libcamera class, and thus defined in the
corresponding .cpp file, but others may need to be accessible from
multiple internal classes. Qt solves this with

	foo.cpp
	foo.h
	foo_p.h

all located in the src directory. Only foo.h is installed.

Let's keep brainstorming this a little bit. For the purpose of this
discussion, s/internal/private/ in directory names could be an option.

-- 
Regards,

Laurent Pinchart