[PATCH][weston] configure.ac: Add --with-wayland-scanner-path
Quentin Glidic
sardemff7+wayland at sardemff7.net
Tue May 23 11:51:47 UTC 2017
On 5/23/17 1:21 PM, Jussi Kukkonen wrote:
> Thanks for feedback,
>
> Anything I didn't comment on I think I agree with -- the
> wayland-scanner.m4 proposal I'll have to consider but I'd like to be
> _really_ sure of what most wayland-scanner users want before going that way.
If we cannot find a way that works for everyone, then we failed at
defining what wayland-scanner is and how it should be used.
> Jussi
>
>
> On 23 May 2017 at 11:56, Quentin Glidic <sardemff7+wayland at sardemff7.net
> <mailto:sardemff7%2Bwayland at sardemff7.net>> wrote:
> >
> > On 5/23/17 10:05 AM, Jussi Kukkonen wrote:
> >>
> >> Modify wayland-scanner lookup: Use the path given by pkg-config
> >> but offer an option to override the path with
> >> "--with-wayland-scanner-path=PATH". The latter is useful for
> >> cross-compile situations.
> >
> >
> > I would rather use --with-wayland-scanner.
>
> Name was chosen to match "--with-xserver-path" that already existed.
> "--with-wayland-scanner" works for me too.
Nevermind, let’s go with -path.
> >> AC_PATH_PROG is no longer used.
> >
> > I disagree with that, but you can at least make it work for
> cross-compiling (to an incompatible host) *without* the need to add
> --with-wayland-scanner-path in most cases.
>
> If you take a look at the "Double-checking the correct way to find
> wayland-scanner" thread from last week (sorry, forgot to mention that in
> the patch), you'll see that my original proposal was based on
> AC_PATH_PROG. But pq had some comments:
>
> | The obvious caveat of Weston style is that if you have wayland-scanner
> | installed in your system, and you are building another version into a
> | $prefix, you must set PATH to find things in $prefix or they will
> | silently use the system version. This is problematic, because sometimes
> | (rarely) the scanner changes and it is assumed that the scanner matches
> | the libwayland you are building for. So, casual contributors can easily
> | shoot their foot in Weston style, while in Mesa style they get the
> | right one automatically - the one pkg-config is pointing to.
>
> https://lists.freedesktop.org/archives/wayland-devel/2017-May/034136.html
>
> So the idea was that looking in the path could accidentally lead one to
> find an installed wayland-scanner that does not match the libwayland
> we're trying to build against. So I tried to make the "casual case"
> always work safely even if it means the rest of us have to set a
> configure flag a little more often.
I read that, and I just disagree, no big deal. :-)
IMO, we must limit the extra flags to case that wouldn’t work otherwise.
The simplest cross-compiling case (with correctly named tools and in
system) must work without extra args.
> >> Also add a AC_SUBST-call (it seems previously the pkg-config value was
> >> never substituted into Makefiles).
> >
> > It was, AC_PATH_PROG() does call AC_SUBST().
>
> Sure, AC_PATH_PROG() works. As I said, it's the value _from pkg-config_
> (if wayland-scanner was not in path) that never makes it into Makefiles.
That’s not how m4 works.
AC_SUBST() is expanded at m4 time, so just having AC_PATH_PROG() will
expand AC_SUBST() and thus, the pkg-config value *will* make it to
Makefile just fine.
> >> My goal is to standardize wayland-scanner usage in a way that does
> >> not require patching when cross-compiling in Yocto. I'm sending a
> >> similar patch to mesa and will fix other projects if these two patches
> >> are well received.
> >>
> >> I did not check that wayland-scanner can actually run as pq suggested:
> >> I don't think that's typically a problem and the error on make should
> >> be fairly good in that case.
> >>
> >> Thanks,
> >> Jussi
> >>
> >> configure.ac <http://configure.ac> | 19 ++++++++++++++-----
> >> 1 file changed, 14 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/configure.ac <http://configure.ac> b/configure.ac
> <http://configure.ac>
> >> index db757f20..e17135a9 100644
> >> --- a/configure.ac <http://configure.ac>
> >> +++ b/configure.ac <http://configure.ac>
> >> @@ -653,11 +653,20 @@ if test "x$enable_lcms" != "xno"; then
> >> fi
> >> AM_CONDITIONAL(HAVE_LCMS, [test "x$enable_lcms" = xyes])
> >> -AC_PATH_PROG([wayland_scanner], [wayland-scanner])
> >> -if test x$wayland_scanner = x; then
> >> - PKG_CHECK_MODULES(WAYLAND_SCANNER, [wayland-scanner])
> >> - wayland_scanner=`$PKG_CONFIG --variable=wayland_scanner
> wayland-scanner`
> >> -fi
> >> +AC_ARG_WITH([wayland-scanner-path],
> >> + [AS_HELP_STRING([--with-wayland-scanner-path=PATH],
> >> + [Path to wayland-scanner (by default
> the path from
> >> + 'pkg-config --variable=wayland_scanner
> wayland-scanner'
> >> + is used)])],
> >> + [wayland_scanner="$withval"],
> >
> >
> > Drop that [].
> >
> >> + [wayland_scanner="auto"])
> >
> >
> > And use "with_wayland_scanner=yes" here. Then, (see post-fi comment)
> >
> >
> >> +if test x$wayland_scanner = xauto; then
> >> + PKG_CHECK_MODULES([WAYLAND_SCANNER],
> >> + [wayland-scanner],
> >> + [wayland_scanner=`$PKG_CONFIG
> --variable=wayland_scanner wayland-scanner`],
> >
> >
> > Here, $PKG_CONFIG is wrong. Specifically in cross-compiling case,
> which is what your patch is supposed to fix.
>
> I believe this code is essentially the same as it used to be (with the
> caveat that this code now runs if the option is not given, and it used
> to run if AC_PATH_PROG failed). This code path cannot work when
> cross-compiling but I would expect anyone cross-compiling to provide a
> "--with-wayland-scanner-path=PATH".
>
> I guess I could error out if $cross_compiling and
> --with-wayland-scanner-path is not given? I can't think of a case where
> that could work...
>
> > Having a way to override the value is nice, but we already had.
> > Running "./configure
> ac_cv_path_wayland_scanner=/usr/yocto/bin/wayland-scanner" should work.
> Doing nothing works in most cases.
>
> That does work for weston configure (and the AC_PATH_PROG method just
> works in yocto anyway). But as explained I was trying to avoid
> AC_PATH_PROG as suggested by pq and then ac_cv_variables no longer work...
>
> > With this patch, you’re breaking cross-compilation to incompatible
> host in the default case, to add something you can already do.
> >
> > At the very least, you should be using the *build* pkg-config, with
> something like that:
> > AC_CANONICAL_BUILD
> > AC_ARG_VAR([BUILD_PKG_CONFIG])
> > AC_PATH_PROGS([BUILD_PKG_CONFIG],
> > [${build}-pkg-config pkg-config], [${PKG_CONFIG}])
> > # We’re safe, ${PKG_CONFIG} is used elsewhere so it’s a good fallback
> > wayland_scanner=`${BUILD_PKG_CONFIG} --variable=wayland_scanner
> wayland-scanner`
> > # But using ${PKG_CONFIG} can lead to unusable executable here, we
> *must* check it works
> > wl_scanner_version=`${wayland_scanner} --version 2>/dev/null`
> > AS_IF([test "x$?" != "x0"], [AC_MSG_ERROR([${wayland_scanner} is not
> usable])])
>
> I don't think that's helpful in our case and I would be uncomfortable
> writing a patch like this that I couldn't test.
>
> For background: the yocto build time pkg-config works fine for pretty
> much everything (PKG_CONFIG_SYSROOT_DIR is used so include and library
> paths just work and ${pc_sysrootdir} could be used to find the
> buildtime sysroot in other cases) ... but it doesn't work for finding
> the build time _native_ sysroot as we need to do here. Typically this
> problem is solved by giving the path via a configure option or by using
> AC_PATH_PROG().
So you want to break the simple (cross-compiling) cases to fix something
that doesn’t need fixing?
If you want to make a patch, it should not introduce a regression.
So at this point, we should make a generic patch, ideally (IMO) in
wayland-scanner.m4, and have people test it in all their different setup
to see if it works as expected.
If you’re not comfortable making one, I’ll do it by this week-end so you
can test it in yours.
Cheers,
--
Quentin “Sardem FF7” Glidic
More information about the wayland-devel
mailing list