[PATCH v3] configure.ac: Change in build system to use the path from pkg-config for wayland-scanner.

[Pekka Paalanen]

On Sun, 18 May 2014 00:39:16 +0200
Thierry Reding <thierry.reding at gmail.com> wrote:

> On Fri, May 16, 2014 at 03:43:47PM +0530, Srivardhan Hebbar wrote:
> > This is a fix to the bug https://bugs.freedesktop.org/show_bug.cgi?id=78688.
> > 
> > Signed-off-by: Srivardhan Hebbar <sri.hebbar at samsung.com>
> > ---
> >  configure.ac |   14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 031a26f..47a38cd 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -38,6 +38,8 @@ AC_ARG_VAR([WESTON_NATIVE_BACKEND],
> >             [Set the native backend to use, if Weston is not running under Wayland nor X11. @<:@default=drm-backend.so@:>@])
> >  AC_ARG_VAR([WESTON_SHELL_CLIENT],
> >             [Set the default desktop shell client to load if none is specified in weston.ini. @<:@default=weston-desktop-shell@:>@])
> > +AC_ARG_VAR([WAYLAND_SCANNER],
> > +           [Set the name of wayland-scanner file with full path. E.g: /usr/bin/wayland-scanner. By default obtain the path from pkg-config])
> >  
> >  PKG_PROG_PKG_CONFIG()
> >  
> > @@ -498,12 +500,14 @@ if test "x$have_lcms" = xyes; then
> >  fi
> >  AM_CONDITIONAL(HAVE_LCMS, [test "x$have_lcms" = xyes])
> >  
> > -AC_PATH_PROG([wayland_scanner], [wayland-scanner])
> > -if test x$wayland_scanner = x; then
> > -	AC_MSG_ERROR([wayland-scanner is needed to compile weston])
> > +if test "x$WAYLAND_SCANNER" = "x"; then
> > +	WAYLAND_SCANNER=`$PKG_CONFIG --variable=wayland_scanner wayland-scanner`
> >  fi
> > -
> > -PKG_CHECK_MODULES(WAYLAND_SCANNER, wayland-scanner)
> > +AC_MSG_CHECKING([for wayland-scanner])
> > +AS_IF(AS_EXECUTABLE_P([${WAYLAND_SCANNER}]),
> > +	[AC_MSG_RESULT([found])],[AC_MSG_RESULT([not found])]
> > +	[AC_MSG_ERROR([wayland-scanner is needed to compile weston])])
> > +AC_SUBST(wayland_scanner,[${WAYLAND_SCANNER}])

Hi,

IMO, apart from the last AC_SUBST, the patch looks good to me. I think
AC_ARG_VAR implies AC_SUBST, so the makefiles could just use
WAYLAND_SCANNER instead of subst'ing yet another variable for it.

The patch is an easy quick fix for real problems, I think.

> This is all getting really confusing. There are two scenarios that I
> think need to be considered here:
> 
>   1) Native builds: Typically wayland will build wayland-scanner and use
>      that in its own build. wayland-scanner will then be installed along
>      with wayland-scanner.pc and weston can use that to obtain the path
>      to the wayland-scanner binary. This is easy.

Yup.

>   2) Cross builds: In this case wayland cannot use wayland-scanner from
>      its own build because it will be built for the host and therefore
>      unable to run on the build host. Currently I think the recommended
>      way to build this is by specifying --disable-scanner at configure
>      time and make sure wayland-scanner is available in the path.

Huh, indeed it does. I thought wayland already had some automagic
automake variable you could set to force it use a specific binary as
$wayland_scanner, but maybe not then.

>      One issue here is that if --disable-scanner is specified, then the
>      wayland-scanner binary is neither built nor installed. That doesn't
>      have to be a problem if everything is cross-built. However I can
>      imagine some cases where having the wayland-scanner binary
>      available on the host could be nice. Currently that's impossible to
>      do for cross builds. So I think it would be nice if we could modify
>      the wayland build process to be able to still build the scanner but
>      not use it during the build process. I have some ideas on how that
>      could work, I'll see if I can cast them into a patch.

You are speaking in autotools parlance, so that host is the foreign
architecture, and build is the build machine, right?

Sure, a good idea.

>      Another issue that I'm hitting currently is that since I'm building
>      wayland with --disable-scanner, wayland-scanner.pc will not be
>      available for the cross build, which makes weston choke. This patch
>      would seem to fix that by only requiring wayland-scanner.pc if the
>      binary can't be found otherwise. That's a good thing, but on the
>      downside I'm now required to pass the absolute path to the scanner
>      binary to configure. But I already have that binary on the path to
>      build wayland. So for consistency it would be nice if weston could

It was using PATH originally, I wanted it to have to be set explicitly,
because that is the simplest way.

Finding wayland-scanner from PATH has the downside, that people
occasionally have an old version of libwayland installed from
distribution, which makes the wrong wayland-scanner to be found by PATH.

If we by default use the .pc, these problems would almost never happen,
so I'm not that attached to specifying the full path explicitly.

>      be made to behave the same way. I think a more appropriate sequence
>      would be:
> 
> 	AC_PATH_PROG([wayland_scanner], [wayland-scanner])
> 	if test "x$wayland_scanner" = "x"; then
> 		wayland_scanner=`$PKG_CONFIG --variable=wayland_scanner wayland-scanner`
> 	fi

I'd really like to have the .pc file take priority, because of the
above.

> 
>      Perhaps a sensible thing to do would be to allow the user to still
>      override the path default with a full path to the scanner. That
>      could either be done with AC_ARG_VAR as proposed in this patch or
>      alternatively AC_PATH_PROG already lets the user override it using
>      the ac_cv_path_wayland_scanner cache variable.
> 
> For native builds the above will work equally well: in the typical case
> where wayland is installed in the standard location, wayland-scanner
> would end up in /usr/bin which is usually on the path, therefore
> AC_PATH_PROG should find it. If wayland is installed in a non-standard
> location, then AC_PATH_PROG will not find it (unless the user has made
> sure to put it in the path explicitly), in which case pkg-config can
> still pick it up.

Unless the standard PATH contains an outdated wayland-scanner.

> For cross builds the only thing required will be to preinstall a native
> version of wayland-scanner and make it available on the path. Looking it
> up via pkg-config is not an option since it will either not be available
> (if wayland was built with --disable-scanner) or it will be a binary
> that can't be executed on the build host.

Yes.

> I think it would further be useful to make all of that logic available
> via an m4 snippet (like what we already have in wayland-scanner.m4) so
> that all packages that need wayland-scanner handle this consistently.
> weston should use that snippet rather than implement its own version.

Actually that .m4 snippet has caused a lot of grief. Now, most projects
just don't bother with it. It is much easier and more flexible to just
copy the few lines that the wayland-scanner build rules need.

Finding wayland-scanner is slightly different though, because build
rules you may want to customize, but finding a binary not so much.

To summarize, I'd prefer this search order:

1. AC_ARG_VAR if set, otherwise
2. .pc file, with fallback to
3. searching the PATH

Would that be ok?

Adding AC_ARG_VAR to libwayland would be nice, too, but the PATH
problem does not really exist there because if wayland-scanner is
built, the build will use the built version explicitly, so only
--disable-scanner builds could find a wrong wayland-scanner.


Thanks,
pq