[weston] xkbcommon library is not optional.

[Bryce Harrington]

On Fri, Oct 16, 2015 at 12:10:57PM +0300, Pekka Paalanen wrote:
> On Fri, 16 Oct 2015 10:27:56 +0200
> Hardening <rdp.effort at gmail.com> wrote:
> 
> > Le 16/10/2015 03:28, Bryce Harrington a écrit :
> > > On Tue, Oct 13, 2015 at 01:59:13PM +0200, Joaquim Duran wrote:
> > >> Hello,
> > >>
> > >> When configuring the Weston project, it is possible to disable (don't
> > >> include) the library libxkbcommon. To compile Weston successfully,
> > >> even if the option --disable-xkbcommon is specified, the library must
> > >> be installed because the file src/compositor.h requires it.
> > > 
> > > Joaquim, good find.
> > > 
> > > From the comments in configure.ac:
> > > 
> > >           AS_HELP_STRING([--disable-xkbcommon], [Disable libxkbcommon
> > >                   support: This is only useful in environments
> > >                   where you do not have a hardware keyboard. If
> > >                   libxkbcommon support is disabled clients will not
> > >                   be sent a keymap and and must know how to
> > >                   interpret the keycode sent for any key event.]),,
> > > 
> > > So it sounds like this is a special case that is intended to work.
> > > 
> > > The header include was from commit 855028fe three years ago, while the
> > > --disable-xkbcommon was added by 382ff46f just two years ago.  That
> > > makes me think that your situation was simply overlooked.
> > > 
> > > If that's true, then presumably the fix would involve peppering
> > > compositor.* and other files with tests like,
> > > 
> > > #ifdef ENABLE_XKBCOMMON
> > > ...
> > > #endif
> > > 
> > > For example it looks like the weston_xkb_info struct would need to
> > > either be removed or stubbed out, and users of it be disabled.  There
> > > are also some weston calls that use xkb_keymap, xkb_rule_names,
> > > etc. structures for params that'd need addressed.
> > > 
> > > On the face of it, seems like it could be a fair amount of work, and a
> > > bit invasive, but maybe there's some tricks to make it simpler
> > > (typedeffing the unsupported struct args and so on.)
> > > 
> > > Alternatively, --disable-xkbcommon could be dropped.  However I get the
> > > sense it actually solves a real world need, and functional regression is
> > > rarely a good idea.
> > > 
> > 
> > I'm to fix the bug. Anyway it's almost sure that there's no real usage
> > of this flag as otherwise weston does not compile...
> 
> I would be suprised if it didn't work at the time it was added. I
> suspect we broke it later, and no-one noticed.

That was my initial assumption too, but reviewing the patches and the
order things landed, I didn't find any evidence that this is the case.
Is it possible it simply did not get tested on a system with xkbcommon
removed?

If it did work originally and subsequently broke, I'd be +1 on dropping
it and avoiding the clutter in the code.  It would help if we knew if
anyone was actually using/needing the option.
 
> I recall some discussions about some obscure embedded platform where
> xkbcommon, even when it's tiny, was deemed as waste of space,
> especially along with the xkeyboard-config database (even though you
> don't even need the whole database). Or maybe it was about wasting
> memory. Can't recall.
> 
> Hrm, now that I actually test --disable-xkbcommon, Weston *does* build.
> All I get during build is:
> 
>   CC       src/weston-input.o
> /home/pq/git/weston/src/input.c: In function ‘weston_seat_init_keyboard’:
> /home/pq/git/weston/src/input.c:2226:1: warning: label ‘err’ defined but not used [-Wunused-label]
> /home/pq/git/weston/src/input.c: In function ‘weston_keyboard_reset_state’:
> /home/pq/git/weston/src/input.c:2238:20: warning: unused variable ‘state’ [-Wunused-variable]
> /home/pq/git/weston/src/input.c: At top level:
> /home/pq/git/weston/src/input.c:555:1: warning: ‘weston_xkb_info_destroy’ used but never defined [enabled by default]
> /home/pq/git/weston/src/input.c:989:1: warning: ‘run_modifier_bindings’ defined but not used [-Wunused-function]
> 
> And no other warning or error.
> 
> I do not get a build failure, because libxkbcommon is installed in my
> system, and so I don't need additional -I flags to find its headers.
> When we build modules (backends, plugins, anything), missing symbols
> are not fatal because the parent executable is providing some we must
> not error on.
> 
> At least x11-backend.so will fail to load doe to missing
> xkbcommon symbols. Makes me wonder if there is some other backend that
> doesn't fail so.
> 
> So it's possible someone is or was actually getting away with this,
> having xkbcommon installed for the build but not at runtime.
> 
> We can remove --disable-xkbcommon, but be prepared having to revert and
> fix that properly later.

It would be worth attempting to contact the original author before going
down that road, just in case.

Bryce

> Thanks,
> pq

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBViC/KCNf5bQRqqqnAQh3yA/9FxJUmCMtD+vooAdFHiuCchEsqP3EP+cu
> 2iSFrSAeGY/W1G6NuZtNuhuC+ZDOW5vYA+6EQ2o8rO9RbdCpOoo37WY3LLDHx48t
> Tn/9wRPnnu3Yt3ghSJAhdKx6QbdHDwRUmYtRWyRjXOJNd3B7MesyolgUbohMLhJK
> CZKOr1ekpVwaeYYHRXza3w1Hva8BgB/n8JN8q2JZ8PKwOx6ve4g52yI4inVaTEXN
> tAxnaIrcPNYb3lZMmG7FzDlxuetd81dgb42A8VpNqUuT923Nuy1995uy+uF1+MjA
> XFnwrW6oUhOFFzu6RJUsgD/aACIORf6fcVJOIofcHSUhU23cAlOmqrCRWLFqhFjm
> WR2S/4Z145/xKTsR3ecaLF0NsLNuxT2yj1FsXL/PXOXZWk1j4gkn+YebYtUCrtW7
> aRwdYWQMzjgxMXjlGdk65ANqSD8hR/e97aZG/myDa7Jpz15Hvz8+KvU4jBO8TIcn
> 4g0n08UUpbpfTm4jzclqnPhc66Tpzx7XVWfxeZMBeHACcJ8EzkXGWMOdcQ934Lmy
> 5+KtXqG5uovjw6Fj3E22VRukTdaZcKQ+n6nZ4p9yJDgcVYtD7XpO0uv4dEA1vOQK
> bd3O3SgQAlsWqnDLEGEJDm5s31ICXImy+/kY/ZxZ53r1+OTIzfVmkxUyBUih3NDH
> WlSQR0vDqvk=
> =nrOL
> -----END PGP SIGNATURE-----


> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel