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

[Srivardhan]

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> Sent: Monday, May 19, 2014 2:30 PM
> To: Thierry Reding
> Cc: Srivardhan Hebbar; wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH v3] configure.ac: Change in build system to use the
path
> from pkg-config for wayland-scanner.
> 
> 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.

Hi,

In the Makefile.am wayland_scanner was the variable. So I used AC_SUBST to
substitute that. Will update the patch by changing in the Makefile.am also.

Thank-you,
Hebbar
> 
> 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