[PATCH weston v2 7/8] Split the modules and include files between weston and libweston

[Pekka Paalanen]

On Thu, 2 Jun 2016 11:06:58 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> On 01/06/2016 20:43, Giulio Camuffo wrote:
> > The backends are now installed in lib/libweston-0, and the include
> > files that will be used by libweston in include/libweston-0. The other
> > modules and weston-specific include files are kept in the old paths.
> > A new load_weston_module() is added to load plugins in the old path,

Hi,

you forgot to rename the function in the commit message.

Also load_weston_plugin() should be called wet_load_plugin(), since it
is a new non-lib function and exported.

> > which is not part of libweston, but weston only and defined in main.c.
> > To allow that to be used by out of tree weston plugins, the function
> > is declared in a new weston.h, installed in include/weston.
> >
> > The -0 in the paths is the abi version of libweston, and it will be
> > used by the libweston .so too. When the abi change the number will
> > be increased.

Somehow odd sentence...

> >
> > Signed-off-by: Giulio Camuffo <giuliocamuffo at gmail.com>
> > ---
> >  
> 
> Oops, forgot to put two comments on this one. But I maintain my Rb with 
> those fixed.
> 
> 
> >
> > v2: - don't remove MODULEDIR
> >     - keep systemd-notify with the main.c side
> >     - rename the new load function to load_weston_plugin
> >
> >  Makefile.am            | 24 +++++++++++++++---------
> >  configure.ac           |  2 ++
> >  ivi-shell/ivi-layout.c |  3 ++-
> >  src/compositor.c       |  2 +-
> >  src/main.c             | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  src/weston.h           |  3 +++
> >  src/weston.pc.in       |  2 +-
> >  7 files changed, 70 insertions(+), 13 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 2f81ec0..e90b3ba 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > [snip]
> > @@ -213,8 +216,8 @@ pkgconfig_DATA = src/weston.pc
> >  wayland_sessiondir = $(datadir)/wayland-sessions
> >  dist_wayland_session_DATA = src/weston.desktop
> >
> > -westonincludedir = $(includedir)/weston
> > -westoninclude_HEADERS =				\
> > +libwestonincludedir = $(includedir)/libweston-${LIBWESTON_ABI_VERSION}
> > +libwestoninclude_HEADERS =				\
> >  	src/version.h				\
> >  	src/compositor.h			\
> >  	src/compositor-drm.h			\
> > @@ -229,13 +232,16 @@ westoninclude_HEADERS =				\
> >  	shared/zalloc.h				\
> >  	shared/platform.h
> >
> > +westonincludedir = $(includedir)/weston
> > +westoninclude_HEADERS = src/weston.h
> > +  
> 
> You are installing weston.h here, but it should be done in the creation 
> patch. So this patch should include a line removing weston.h from the 
> previous list (the now-libweston list).

Hmm, it should probably have been installed when it was first added to
the tree. Anyway, here or the next patch is fine by me.

> > [snip]
> > diff --git a/src/weston.pc.in b/src/weston.pc.in
> > index c560eb3..f2ffc9e 100644
> > --- a/src/weston.pc.in
> > +++ b/src/weston.pc.in
> > @@ -9,4 +9,4 @@ Name: Weston Plugin API
> >  Description: Header files for Weston plugin development
> >  Version: @WESTON_VERSION@
> >  Requires.private: wayland-server pixman-1 xkbcommon
> > -Cflags: -I${includedir}
> > +Cflags: -I${includedir}/weston -I{includedir}/libweston- at LIBWESTON_ABI_VERSION@
> >  
> 
> So weston.h is not in a different directory than compositor.h. You must 
> change the #include to use <>.

s/not/now/?

Fixing the #include styles will be fun, also when we create a new
directory for weston stuff to separate it better from libweston stuff
in the source tree. For installed headers that split already exists for
external users. I don't mind fixing it as a follow-up. There is(?) some
room for a mistake to prefer installed headers over in-tree headers
with <> I think, so we need to be careful with CPPFLAGS.

Apart from all the comments, looking good to me, so I'd say
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
at this point, and also because I didn't yet test how the installation
actually looks like.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160602/92b24f73/attachment.sig>