[libcamera-devel] [RFC PATCH 1/8] meson: Simplify pkg_mod.generate

[Laurent Pinchart]

Hi Kieran,

On Mon, Dec 07, 2020 at 03:31:55PM +0000, Kieran Bingham wrote:
> On 01/12/2020 17:12, Laurent Pinchart wrote:
> > On Tue, Dec 01, 2020 at 05:01:46PM +0000, Kieran Bingham wrote:
> >> On 01/12/2020 16:58, Laurent Pinchart wrote:
> >>> On Tue, Dec 01, 2020 at 04:56:56PM +0000, Kieran Bingham wrote:
> >>>> On 01/12/2020 16:50, Laurent Pinchart wrote:
> >>>>> On Mon, Nov 23, 2020 at 04:43:12PM +0000, Kieran Bingham wrote:
> >>>>>> Later versions of meson allow for the first positional argument to
> >>>>>> specificy the defaults.  Specify the libcamera library as the first
> >>>>>> argument, and remove the filebase.
> >>>>>>
> >>>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>>>> ---
> >>>>>>  meson.build | 3 +--
> >>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/meson.build b/meson.build
> >>>>>> index 55cf36e15f57..ced4afa7d726 100644
> >>>>>> --- a/meson.build
> >>>>>> +++ b/meson.build
> >>>>>> @@ -146,10 +146,9 @@ run_command('ln', '-fsT', meson.source_root(),
> >>>>>>  configure_file(output : 'config.h', configuration : config_h)
> >>>>>>  
> >>>>>>  pkg_mod = import('pkgconfig')
> >>>>>> -pkg_mod.generate(libraries : libcamera,
> >>>>>> +pkg_mod.generate(libcamera,
> >>>>>>                   version : '1.0',
> >>>>>>                   name : 'libcamera',
> >>>>>> -                 filebase : 'camera',
> >>>>>>                   description : 'Complex Camera Support Library',
> >>>>>>                   subdirs : 'libcamera')
> >>>>>>  
> >>>>>
> >>>>> The documentation states
> >>>>>
> >>>>> Since 0.46 a StaticLibrary or SharedLibrary object can optionally be
> >>>>> passed as first positional argument. If one is provided a default value
> >>>>> will be provided for all required fields of the pc file:
> >>>>>
> >>>>> - install_dir is set to pkgconfig folder in the same location than the
> >>>>>   provided library.
> >>>>> - description is set to the project's name followed by the library's
> >>>>>   name.
> >>>>> - name is set to the library's name.
> >>>>>
> >>>>>
> >>>>> Is it the "name" argument that can be dropped, not "filebase" ? filebase
> >>>>> seems to default to name, so it would be set to "libcamera", not
> >>>>> "camera". If we drop the name argument then name will default to the
> >>>>> shared library name, which is "camera", and filebase will then default
> >>>>> to name. There thus seems to be a change of behaviour, which should be
> >>>>> documented in the commit message (and verified to ensure it's correct).
> >>>>
> >>>>
> >>>> I'll have to check, but this was a patch I had stuck around for ages,
> >>>> which I created upon a recommendation from one of the meson devs.
> >>>>
> >>>> I haven't seen any change in behaviour, but I'll double check again
> >>>> later, meanwhile: an install from this build seems to show everything
> >>>> still correctly named 'libcamera' ...
> >>>>
> >>>> /tmp/libcamera-build-prefix/usr/share/libcamera
> >>>> /tmp/libcamera-build-prefix/usr/share/doc/libcamera-0.0.11
> >>>> /tmp/libcamera-build-prefix/usr/lib/x86_64-linux-gnu/libcamera.so
> >>>> /tmp/libcamera-build-prefix/usr/include/libcamera/libcamera/version.h
> >>>
> >>> I expect this patch to only affect the .pc file generation :-) Could you
> >>> diff its contents, and check if its name changes ?
> >>
> >> diff
> >> /tmp/libcamera-build-prefix/usr/lib/x86_64-linux-gnu/pkgconfig/libcamera.pc
> >> /usr/lib/x86_64-linux-gnu/pkgconfig/libcamera.pc
> >>
> >> <blank> ;-)
> > 
> > That's strange, here I have
> 
> I must have installed a 'patched' version at some point which broke my
> comparison.
> 
> Anyway, this patch is a distraction - so I've split out the meson fixes
> to a new series and dropped this patch for now.
> 
> There's no bug/issue here - it was only a recommendation from meson
> developers.
> 
> We can always revisit it later.
> 
> > diff -Nur a/usr/local/lib64/pkgconfig/camera.pc b/usr/local/lib64/pkgconfig/camera.pc
> > --- a/usr/local/lib64/pkgconfig/camera.pc       2020-12-01 17:37:07.077026434 +0200
> > +++ b/usr/local/lib64/pkgconfig/camera.pc       1970-01-01 02:00:00.000000000 +0200
> > @@ -1,9 +0,0 @@
> > -prefix=/usr/local
> > -libdir=${prefix}/lib64
> > -includedir=${prefix}/include
> > -
> > -Name: libcamera
> > -Description: Complex Camera Support Library
> > -Version: 1.0
> > -Libs: -L${libdir} -lcamera
> > -Cflags: -I${includedir}/libcamera
> > diff -Nur a/usr/local/lib64/pkgconfig/libcamera.pc b/usr/local/lib64/pkgconfig/libcamera.pc
> > --- a/usr/local/lib64/pkgconfig/libcamera.pc    1970-01-01 02:00:00.000000000 +0200
> > +++ b/usr/local/lib64/pkgconfig/libcamera.pc    2020-12-01 19:04:21.415247132 +0200
> > @@ -0,0 +1,9 @@
> > +prefix=/usr/local
> > +libdir=${prefix}/lib64
> > +includedir=${prefix}/include
> > +
> > +Name: libcamera
> > +Description: Complex Camera Support Library
> > +Version: 1.0
> > +Libs: -L${libdir} -lcamera
> > +Cflags: -I${includedir}/libcamera
> > 
> > Contents are the same, but the .pc file got renamed from camera.pc to
> > libcamera.pc. No necessarily an issue, but pkg-config will need to be
> 
> I see, I must have missed that, as the builds will have still found the
> old version.
> 
> Personally, I'd actually rather see the library specified as 'libcamera'
> to pkg-config, as that's our name though.
> 
> > run with an updated name, requiring an update to simple-cam I believe.
> > 
> > If we additionally drop the name argument, the file name is preserved,
> > but its contents change to
> > 
> > --- a/usr/local/lib64/pkgconfig/camera.pc	2020-12-01 17:37:07.077026434 +0200
> > +++ b/usr/local/lib64/pkgconfig/camera.pc	2020-12-01 19:09:47.299260873 +0200
> > @@ -2,7 +2,7 @@
> >  libdir=${prefix}/lib64
> >  includedir=${prefix}/include
> >  
> > -Name: libcamera
> > +Name: camera
> 
> Ok, so we don't want that bit though ;-)
> 
> >  Description: Complex Camera Support Library
> >  Version: 1.0
> >  Libs: -L${libdir} -lcamera
> > 
> > 
> > I'm not a pkg-config expert so I don't know if we should include the lib
> > prefix in the file name and/or the Name.
> 
> I think that's up to us isn't it ?

I think so. I would expect there are best practice rules, but maybe it's
only cargo cult programming :-)

> However I suspect most libraries do not prefix with 'lib' ... but most
> libraries do not have 'lib' in their name. (Except for the fact that
> they are libraries).
> 
> We're a bit different because we are 'libcamera' as opposed to say
> 'gtk', which is not known widely as 'libgtk', that's just it's library.

-- 
Regards,

Laurent Pinchart