<div dir="ltr"><div>Thanks for feedback,</div><div><br></div><div>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.</div><div><br></div><div>Jussi</div><div><br></div><div><br></div>On 23 May 2017 at 11:56, Quentin Glidic <<a href="mailto:sardemff7%2Bwayland@sardemff7.net" target="_blank">sardemff7+wayland@sardemff7.<wbr>net</a>> wrote:<br>><br>> On 5/23/17 10:05 AM, Jussi Kukkonen wrote:<br>>><br>>> Modify wayland-scanner lookup: Use the path given by pkg-config<br>>> but offer an option to override the path with<br>>> "--with-wayland-scanner-path=<wbr>PATH". The latter is useful for<br>>> cross-compile situations.<br>><br>><br>> I would rather use --with-wayland-scanner.<br><br>Name was chosen to match "--with-xserver-path" that already existed. "--with-wayland-scanner" works for me too.<br><br>>> AC_PATH_PROG is no longer used.<br>><br>> 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.<br><br>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:<div><br></div><div><span style="font-size:12.8px">| The obvious caveat of Weston style is that if you have wayland-scanner</span><br></div><div><span style="font-size:12.8px">| installed in your system, and you are building another version into a</span><br style="font-size:12.8px"><span style="font-size:12.8px">| $prefix, you must set PATH to find things in $prefix or they will</span><br style="font-size:12.8px"><span style="font-size:12.8px">| silently use the system version. This is problematic, because sometimes</span><br style="font-size:12.8px"><span style="font-size:12.8px">| (rarely) the scanner changes and it is assumed that the scanner matches</span><br style="font-size:12.8px"><span style="font-size:12.8px">| the libwayland you are building for. So, casual contributors can easily</span><br style="font-size:12.8px"><span style="font-size:12.8px">| shoot their foot in Weston style, while in Mesa style they get the</span><br style="font-size:12.8px"><span style="font-size:12.8px">| right one automatically - the one pkg-config is pointing to.</span><br><div><br></div><div><a href="https://lists.freedesktop.org/archives/wayland-devel/2017-May/034136.html">https://lists.freedesktop.org/archives/wayland-devel/2017-May/034136.html</a><br></div><div><br></div><div>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.<br> <br>>> Also add a AC_SUBST-call (it seems previously the pkg-config value was<br>>> never substituted into Makefiles).<br>><br>> It was, AC_PATH_PROG() does call AC_SUBST().<br><br>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.<br><br>>> My goal is to standardize wayland-scanner usage in a way that does<br>>> not require patching when cross-compiling in Yocto. I'm sending a<br>>> similar patch to mesa and will fix other projects if these two patches<br>>> are well received.<br>>><br>>> I did not check that wayland-scanner can actually run as pq suggested:<br>>> I don't think that's typically a problem and the error on make should<br>>> be fairly good in that case.<br>>><br>>> Thanks,<br>>>   Jussi<br>>><br>>>   <a href="http://configure.ac" target="_blank">configure.ac</a> | 19 ++++++++++++++-----<br>>>   1 file changed, 14 insertions(+), 5 deletions(-)<br>>><br>>> diff --git a/<a href="http://configure.ac" target="_blank">configure.ac</a> b/<a href="http://configure.ac" target="_blank">configure.ac</a><br>>> index db757f20..e17135a9 100644<br>>> --- a/<a href="http://configure.ac" target="_blank">configure.ac</a><br>>> +++ b/<a href="http://configure.ac" target="_blank">configure.ac</a><br>>> @@ -653,11 +653,20 @@ if test "x$enable_lcms" != "xno"; then<br>>>   fi<br>>>   AM_CONDITIONAL(HAVE_LCMS, [test "x$enable_lcms" = xyes])<br>>>   -AC_PATH_PROG([wayland_<wbr>scanner], [wayland-scanner])<br>>> -if test x$wayland_scanner = x; then<br>>> -       PKG_CHECK_MODULES(WAYLAND_<wbr>SCANNER, [wayland-scanner])<br>>> -       wayland_scanner=`$PKG_CONFIG --variable=wayland_scanner wayland-scanner`<br>>> -fi<br>>> +AC_ARG_WITH([wayland-scanner-<wbr>path],<br>>> +            [AS_HELP_STRING([--with-<wbr>wayland-scanner-path=PATH],<br>>> +                            [Path to wayland-scanner (by default the path from<br>>> +                             'pkg-config --variable=wayland_scanner wayland-scanner'<br>>> +                             is used)])],<br>>> +            [wayland_scanner="$withval"],<br>><br>><br>> Drop that [].<br>><br>>> +            [wayland_scanner="auto"])<br>><br>><br>> And use "with_wayland_scanner=yes" here. Then, (see post-fi comment)<br>><br>><br>>> +if test x$wayland_scanner = xauto; then<br>>> +        PKG_CHECK_MODULES([WAYLAND_<wbr>SCANNER],<br>>> +                          [wayland-scanner],<br>>> +                          [wayland_scanner=`$PKG_CONFIG --variable=wayland_scanner wayland-scanner`],<br>><br>><br>> Here, $PKG_CONFIG is wrong. Specifically in cross-compiling case, which is what your patch is supposed to fix.<div><br></div><div>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=<wbr>PATH".</div><div><br></div><div>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...</div><div><br></div><div>> Having a way to override the value is nice, but we already had.<br>> Running "./configure ac_cv_path_wayland_scanner=/<wbr>usr/yocto/bin/wayland-scanner" should work. Doing nothing works in most cases.<br><br>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...<div><div><br></div><div>> With this patch, you’re breaking cross-compilation to incompatible host in the default case, to add something you can already do.<br>><br>> At the very least, you should be using the *build* pkg-config, with something like that:<br>> AC_CANONICAL_BUILD<br>> AC_ARG_VAR([BUILD_PKG_CONFIG])<br>> AC_PATH_PROGS([BUILD_PKG_<wbr>CONFIG],<br>> [${build}-pkg-config pkg-config], [${PKG_CONFIG}])<br>> # We’re safe, ${PKG_CONFIG} is used elsewhere so it’s a good fallback<br>> wayland_scanner=`${BUILD_PKG_<wbr>CONFIG} --variable=wayland_scanner wayland-scanner`<br>> # But using ${PKG_CONFIG} can lead to unusable executable here, we *must* check it works<br>> wl_scanner_version=`${wayland_<wbr>scanner} --version 2>/dev/null`<br>> AS_IF([test "x$?" != "x0"], [AC_MSG_ERROR([${wayland_<wbr>scanner} is not usable])])<br><br></div><div>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. </div><div><br></div><div>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().</div><div><br></div><div><br></div><div>><br>><br>>> +                          [AC_MSG_ERROR([wayland-<wbr>scanner was not found and --with-wayland-scanner-path was not used])])<br>>> +fi<br>><br>><br>> Use AS_CASE, for "yes" (= "auto"), "no" (= error, to make --without-wayland-scanner fails) and "*" (= do nothing, consider the path is ok).<br>><br>><br>>> +AC_SUBST(wayland_scanner)<br>><br>><br>> Adding a run test would be nice, indeed, but we should have a working value.<br>><br>>>   AC_ARG_ENABLE(systemd_notify,<br>>>                 AS_HELP_STRING([--enable-<wbr>systemd-notify],<br>><br>><br>><br>> 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).<br>> The proper macro name is WAYLAND_PROG_WAYLAND_SCANNER (or WL_PROG…).<br>><br>> A nice thing would be to check the version too.<br>><br>> 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.<br>><br>> --<br>><br>> Quentin “Sardem FF7” Glidic<br>> ______________________________<wbr>_________________<br>> wayland-devel mailing list<br>> <a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.<wbr>freedesktop.org</a><br>> <a href="https://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/wayland-devel</a></div></div></div></div></div></div>