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

[Thierry Reding]

On Mon, May 19, 2014 at 11:59:51AM +0300, Pekka Paalanen wrote:
> On Sun, 18 May 2014 00:39:16 +0200 Thierry Reding <thierry.reding at gmail.com> wrote:
[...]
> >      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?

Correct.

> >      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.

I guess simple here is very subjective. This doesn't matter all that
much if you build wayland/weston, but if for example there were a dozen
packages that need to run wayland-scanner, then you'd need to specify
the path to it explicitly a dozen times. If, however, it was looked up
in the PATH then you would only need to make sure it's available in the
path. Once.

To complicate matters furthermore it's entirely possible that every
package had come up with its own way to set the path to wayland-scanner.

> 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.

But if people have an outdated version of libwayland installed, then
surely the .pc file will also be outdated, won't it?

> >      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.

I don't think that's really an option. If, as proposed earlier, we allow
cross builds to ship wayland-scanner, then having the .pc file take
priority will cause the cross-built version of wayland-scanner to be
picked up. The reason is that wayland-scanner.pc will be installed in
${prefix}/lib/pkgconfig, along with any other wayland-*.pc. In order to
pick up the necessary include paths and libraries, people will need to
set PKG_CONFIG_PATH or PKG_CONFIG_LIBDIR appropriately. But doing so
will pick up wayland-scanner.pc at the same time.

> >      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.

If PATH contains an outdated wayland-scanner, then the it's likely that
wayland-scanner.pc is also outdated. The only case where the above could
really happen is if a distribution package has wayland installed in the
standard location. If users now install a custom version in a non-
standard location, they will already need to be careful to properly set
up the environment (by pointing PKG_CONFIG_PATH or PKG_CONFIG_LIBDIR at
the non-standard location). Requiring that PATH be set appropriately
doesn't seem to me like much of an additional burden. In fact I think
that makes it even nicely symmetric with the pkg-config setup.

> > 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.

Unless something causes these rules to need modification, in which case
all projects now need to adapt manually. I think if at least weston used
that snippet there'd be enough regular testing to make sure nothing
breaks. Otherwise if it's really causing too much grief to be useful by
any project and if it can't be fixed, then perhaps a better option would
be to remove it altogether.

> 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?

I'm pretty sure that it could be made to work. However I think it makes
it more difficult than necessary. Using AC_PROG_PATH already gives you
two ways of overriding the automatic value:

	AC_PROG_PATH([WAYLAND_SCANNER], [wayland-scanner])

Will check both WAYLAND_SCANNER and ac_cv_path_WAYLAND_SCANNER before
trying to lookup an executable on the path.

And like I said, having wayland-scanner.pc take priority will break
cross-compilation unless you explicitly override using AC_ARG_VAR.

There's also the matter of consistency. Other tools (gcc, pkg-config,
...) used during the build are also looked up on the path, so following
the principle of least suprise I'd expect any build tools to follow the
same conventions.

That still leaves open the question of how to handle stale versions of
the wayland-scanner binary. Like I said, I think telling people to add
it to the PATH wouldn't be much of a burden. For many users that might
already be the case anyway. For instance I have a set of scripts to
cross-build distributions from scratch (similar in spirit to buildroot
and friends) where tools such as wayland-scanner get installed to a
prefix that's always first in PATH. When I do native builds out of git
I usually install to /usr/local/stow/$PACKAGE and let stow manage the
symlink hierarchy in /usr/local for me. The distribution that I use has
/usr/local/bin in PATH (so that it takes precedence over /usr/bin), so
any binaries that I install manually override whatever the distribution
installed.

If the exact version of wayland-scanner really matters, perhaps a better
solution would be to check for the required version at configure time.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140519/835124fc/attachment-0001.sig>