[PATCH][weston] configure.ac: Add --with-wayland-scanner-path

Tue May 23 11:21:06 UTC 2017

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.

Jussi

On 23 May 2017 at 11:56, Quentin Glidic <sardemff7+wayland 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.

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

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

>> 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 | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index db757f20..e17135a9 100644
>> --- a/configure.ac
>> +++ b/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().

>
>
>> +                          [AC_MSG_ERROR([wayland-scanner was not found
and --with-wayland-scanner-path was not used])])
>> +fi
>
>
> Use AS_CASE, for "yes" (= "auto"), "no" (= error, to make
--without-wayland-scanner fails) and "*" (= do nothing, consider the path
is ok).
>
>
>> +AC_SUBST(wayland_scanner)
>
>
> Adding a run test would be nice, indeed, but we should have a working
value.
>
>>   AC_ARG_ENABLE(systemd_notify,
>>                 AS_HELP_STRING([--enable-systemd-notify],
>
>
>
> I think this patch should be in Wayland, to wayland-scanner.m4 for other
projects to use (they would depend on a recent Wayland for "make dist" and
Git builds only, so the “extra dep” should be fine, one can always backport
the .m4 to their tree if needed).
> The proper macro name is WAYLAND_PROG_WAYLAND_SCANNER (or WL_PROG…).
>
> A nice thing would be to check the version too.
>
> Once we’ve figured out the proper algorithm to use, I’ll make a match to
create a wayland module in Meson to do the same thing.
>
> --
>
> Quentin “Sardem FF7” Glidic
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170523/720a5e53/attachment.html>