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

Submitted by Jussi Kukkonen on May 23, 2017, 8:05 a.m.

Details

Message ID 20170523080517.12723-1-jussi.kukkonen@intel.com
State New
Headers show
Series "configure.ac: Add --with-wayland-scanner-path" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Jussi Kukkonen May 23, 2017, 8:05 a.m.
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.

AC_PATH_PROG is no longer used.

Also add a AC_SUBST-call (it seems previously the pkg-config value was
never substituted 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(-)

Patch hide | download patch | download mbox

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"],
+            [wayland_scanner="auto"])
+if test x$wayland_scanner = xauto; then
+        PKG_CHECK_MODULES([WAYLAND_SCANNER],
+                          [wayland-scanner],
+                          [wayland_scanner=`$PKG_CONFIG --variable=wayland_scanner wayland-scanner`],
+                          [AC_MSG_ERROR([wayland-scanner was not found and --with-wayland-scanner-path was not used])])
+fi
+AC_SUBST(wayland_scanner)
 
 AC_ARG_ENABLE(systemd_notify,
               AS_HELP_STRING([--enable-systemd-notify],

Comments

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.


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


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

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

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])])


> +                          [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.
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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
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@sardemff7.net 
> <mailto:sardemff7%2Bwayland@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,
On 23 May 2017 at 14:51, Quentin Glidic <sardemff7+wayland@sardemff7.net>
wrote:
>>
>>  >> 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.

You are correct and I was confused.

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

I think that is not warranted.
* My issue was that every wayland-scanner user was doing something
different, often in a way that required patching in Yocto
* Weston was fine for us but Pekka gave me feedback that lead me to believe
the weston method was not the best solution either

I did my best to try and resolve both issues. It looks like my attempt was
not successful but there should be no reason to say I "want to break"
something.


> If you want to make a patch, it should not introduce a regression.

I think _if_ the goal is to use the same scanner lookup in e.g. mesa and
weston, some use case in one or the other is just bound to have 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.


I'd be happy with you making a patch -- I don't think I could make a patch
that met your requirements especially WRT this "build-pkg-config" setup.

Thanks again,
 Jussi
On Wed, 24 May 2017 11:32:21 +0300
Jussi Kukkonen <jussi.kukkonen@intel.com> wrote:

> On 23 May 2017 at 14:51, Quentin Glidic <sardemff7+wayland@sardemff7.net>
> wrote:

> > So you want to break the simple (cross-compiling) cases to fix
> > something that doesn’t need fixing?
> 
> I think that is not warranted.
> * My issue was that every wayland-scanner user was doing something
> different, often in a way that required patching in Yocto
> * Weston was fine for us but Pekka gave me feedback that lead me to
> believe the weston method was not the best solution either
> 
> I did my best to try and resolve both issues. It looks like my
> attempt was not successful but there should be no reason to say I
> "want to break" something.

Hi,

I'm with Jussi here.

> > If you want to make a patch, it should not introduce a regression.  
> 
> I think _if_ the goal is to use the same scanner lookup in e.g. mesa
> and weston, some use case in one or the other is just bound to have a
> regression.

Quentin, do you see any way unifying the usage of wayland-scanner
without regressing at least one user?

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

Is anyone actually using wayland-scanner.m4?

I did not find any evidence of Weston using it. Certainly no-one is
using wayland-scanner.mk anymore because of the structure or
wayland-protocols and the need to search for XML files in both local
and system paths. Since wayland-scanner.m4 has a reference to
wayland-scanner.mk, I doubt anyone is using wayland-scanner.m4 either.

We should make a plan to get rid of wayland-scanner.mk and
wayland-scanner.m4, I do not think they are fixable.

> > If you’re not comfortable making one, I’ll do it by this week-end
> > so you can test it in yours.

Did anything come out of that?

There is also the sysroot patch from Tomek Bury that no-one has
commented on yet.
https://patchwork.freedesktop.org/patch/165478/


Thanks,
pq
On 7/18/17 3:58 PM, Pekka Paalanen wrote:
> On Wed, 24 May 2017 11:32:21 +0300
> Jussi Kukkonen <jussi.kukkonen@intel.com> wrote:
> 
>> On 23 May 2017 at 14:51, Quentin Glidic <sardemff7+wayland@sardemff7.net>
>> wrote:
> 
>>> So you want to break the simple (cross-compiling) cases to fix
>>> something that doesn’t need fixing?
>>
>> I think that is not warranted.
>> * My issue was that every wayland-scanner user was doing something
>> different, often in a way that required patching in Yocto
>> * Weston was fine for us but Pekka gave me feedback that lead me to
>> believe the weston method was not the best solution either
>>
>> I did my best to try and resolve both issues. It looks like my
>> attempt was not successful but there should be no reason to say I
>> "want to break" something.
> 
> Hi,
> 
> I'm with Jussi here.
> 
>>> If you want to make a patch, it should not introduce a regression.
>>
>> I think _if_ the goal is to use the same scanner lookup in e.g. mesa
>> and weston, some use case in one or the other is just bound to have a
>> regression.
> 
> Quentin, do you see any way unifying the usage of wayland-scanner
> without regressing at least one user?

Yes, we can just do what we do now in Weston, *adding* the Yocto style.
IOW, we want to support, without passing any variable:
- native and cross-compiling as a system package
   just wayland-scanner from (native) path is good
- native in a custom prefix (system may be different version)
   prefer the pkg-config file
- cross-compiling in a custom prefix (system may be different version)
   we need to check for the native (custom prefix) pkg-config here

In all three cases, checking for the version would be much better. We 
could even use the native system scanner in some cases if the version 
matches, avoiding another build in cross-compiling case.

In pseudo-code, here is what I suggest:

get_wayland_version()

# macro WL_PROG_SCANNER([version])
if $cross_compiling; then
     get_native_pkg_config_scanner_path()
else
     get_pkg_config_scanner_path()
fi
if scanner == ""; then
     get_PATH_scanner_path()
     if scanner == "" || ! check_scanner_version(); then
         error()
     fi
fi
# macro end

You can call "pkg-config --variable=wayland_scanner 'wayland-scanner = 
"version'" directly to let pkg-config do the version check.

That would support all use cases listed above, with no variable. And we 
can still add the configure switch for very specific corner cases that 
might exist.


>>> 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.
> 
> Is anyone actually using wayland-scanner.m4?
> 
> I did not find any evidence of Weston using it. Certainly no-one is
> using wayland-scanner.mk anymore because of the structure or
> wayland-protocols and the need to search for XML files in both local
> and system paths. Since wayland-scanner.m4 has a reference to
> wayland-scanner.mk, I doubt anyone is using wayland-scanner.m4 either.
> 
> We should make a plan to get rid of wayland-scanner.mk and
> wayland-scanner.m4, I do not think they are fixable.

We can deprecate the current macro while adding new ones. Or have a new 
file. It doesn’t matter much, it’s just a (autoreconf) build time dep on 
the newer Wayland.
I see no issue with wayland-scanner.m4 not being used. We can make *new* 
versions use it, it’s adding API/ABI.


>>> If you’re not comfortable making one, I’ll do it by this week-end
>>> so you can test it in yours.
> 
> Did anything come out of that?

We need to agree on the “good” way to do the checks first, but if you 
feel better doing so with a real patch as a basis, I can do it.


> There is also the sysroot patch from Tomek Bury that no-one has
> commented on yet.
> https://patchwork.freedesktop.org/patch/165478/

Ack.
On Thu, 20 Jul 2017 12:21:27 +0200
Quentin Glidic <sardemff7+wayland@sardemff7.net> wrote:

> On 7/18/17 3:58 PM, Pekka Paalanen wrote:
> > On Wed, 24 May 2017 11:32:21 +0300
> > Jussi Kukkonen <jussi.kukkonen@intel.com> wrote:
> >   
> >> On 23 May 2017 at 14:51, Quentin Glidic <sardemff7+wayland@sardemff7.net>
> >> wrote:  
> >   
> >>> So you want to break the simple (cross-compiling) cases to fix
> >>> something that doesn’t need fixing?  
> >>
> >> I think that is not warranted.
> >> * My issue was that every wayland-scanner user was doing something
> >> different, often in a way that required patching in Yocto
> >> * Weston was fine for us but Pekka gave me feedback that lead me to
> >> believe the weston method was not the best solution either
> >>
> >> I did my best to try and resolve both issues. It looks like my
> >> attempt was not successful but there should be no reason to say I
> >> "want to break" something.  
> > 
> > Hi,
> > 
> > I'm with Jussi here.
> >   
> >>> If you want to make a patch, it should not introduce a regression.  
> >>
> >> I think _if_ the goal is to use the same scanner lookup in e.g. mesa
> >> and weston, some use case in one or the other is just bound to have a
> >> regression.  
> > 
> > Quentin, do you see any way unifying the usage of wayland-scanner
> > without regressing at least one user?  
> 
> Yes, we can just do what we do now in Weston, *adding* the Yocto style.
> IOW, we want to support, without passing any variable:
> - native and cross-compiling as a system package
>    just wayland-scanner from (native) path is good
> - native in a custom prefix (system may be different version)
>    prefer the pkg-config file
> - cross-compiling in a custom prefix (system may be different version)
>    we need to check for the native (custom prefix) pkg-config here

Why should custom prefix vs. non-custom prefix be handled differently?
How do you tell them apart when there is no strict standard on the file
system layout?

And the default prefix in autotools is actually neither.

> In all three cases, checking for the version would be much better. We 
> could even use the native system scanner in some cases if the version 
> matches, avoiding another build in cross-compiling case.

Ensuring the version is not wrong is nice, but skipping a step on
cross-compiling based on it is not a good idea. It means that for some
people the cross-build would work, and for some it would require
additional building - which is easy to overlook by recipe writers, so
would likely become a build failure for people who need the "extra"
step.

E.g. a Yocto build depending on the host system tools (as opposed to
build tools built/downloaded by Yocto itself) is frowned upon, because
the host system should not affect how the actual build works. And it's
a very easy mistake to do.

> 
> In pseudo-code, here is what I suggest:
> 
> get_wayland_version()
> 
> # macro WL_PROG_SCANNER([version])
> if $cross_compiling; then
>      get_native_pkg_config_scanner_path()

How would get_native_pkg_config_scanner_path() work? What kind of setup
does it need from the user (or e.g. Yocto)?

> else
>      get_pkg_config_scanner_path()
> fi
> if scanner == ""; then

How could 'scanner' ever be empty here? Do you imply doing some "can it
run" check?

>      get_PATH_scanner_path()
>      if scanner == "" || ! check_scanner_version(); then
>          error()
>      fi
> fi
> # macro end
> 
> You can call "pkg-config --variable=wayland_scanner 'wayland-scanner = 
> "version'" directly to let pkg-config do the version check.
> 
> That would support all use cases listed above, with no variable. And we 
> can still add the configure switch for very specific corner cases that 
> might exist.

If the two details I pointed out are not a problem, that sounds fine to
me.

Any currently used build setup must have both PATH and pkg-config
set up to find the new scanner instead of the system scanner, since we
have projects preferring PATH and projects preferring pkg-config
variable. (Yet, I have a feeling this is not actually the case, since
less correct setups have worked by accident.)

> >>> 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.  
> > 
> > Is anyone actually using wayland-scanner.m4?
> > 
> > I did not find any evidence of Weston using it. Certainly no-one is
> > using wayland-scanner.mk anymore because of the structure or
> > wayland-protocols and the need to search for XML files in both local
> > and system paths. Since wayland-scanner.m4 has a reference to
> > wayland-scanner.mk, I doubt anyone is using wayland-scanner.m4 either.
> > 
> > We should make a plan to get rid of wayland-scanner.mk and
> > wayland-scanner.m4, I do not think they are fixable.  
> 
> We can deprecate the current macro while adding new ones. Or have a new 
> file. It doesn’t matter much, it’s just a (autoreconf) build time dep on 
> the newer Wayland.
> I see no issue with wayland-scanner.m4 not being used. We can make *new* 
> versions use it, it’s adding API/ABI.

So the autoconf macro you are proposing would only find
wayland-scanner, right? I suppose that might work.

The problem with the old macro is that it tries to do three things
together: find wayland-scanner, find the XML file directory, and set up
make rules. Particularly the make rules part is problematic since it
assumes a single directory layout, which does not work even for weston.

> >>> If you’re not comfortable making one, I’ll do it by this week-end
> >>> so you can test it in yours.  
> > 
> > Did anything come out of that?  
> 
> We need to agree on the “good” way to do the checks first, but if you 
> feel better doing so with a real patch as a basis, I can do it.

What you outlined above sounds ok, but the devil is in the details. :-)

If you'd like to send a patch, please CC Jussi Kukkonen, Tomek Bury,
and anyone else you can find who was interested in cross-builds. And
probably also Emil Velikov for Mesa.

> > There is also the sysroot patch from Tomek Bury that no-one has
> > commented on yet.
> > https://patchwork.freedesktop.org/patch/165478/  
> 
> Ack.

Thanks for checking that one.


Thanks,
pq