Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve how CMake looks for wayland-scanner #39

Merged
merged 1 commit into from May 16, 2019

Conversation

Projects
None yet
2 participants
@aperezdc
Copy link
Member

commented May 15, 2019

This improves FindWaylandScanner.cmake to allow it to find the wayland-scanner tool in any of the following ways (in order of preference):

  1. By passing -DWAYLAND_SCANNER=path/to/wayland-scanner to CMake.
  2. Looking for wayland-scanner in the current $PATH.
  3. Figuring out the location from pkg-config (as before).

This will make the package friendlier to cross-compilation, and then build systems can either prepend their locations for host tools to $PATH (covered by 2. above), or they can specify the tool path in the command line (covered by 1. above) if they need to be more specific.

The motivation for this patch is to avoid kludges like the following:

https://patchwork.ozlabs.org/comment/2172010/

Improve how CMake looks for wayland-scanner
This improves FindWaylandScanner.cmake to allow it to find the
wayland-scanner tool in any of the following ways (in order of
preference):

  1. By passing -DWAYLAND_SCANNER=path/to/wayland-scanner to CMake.
  2. Looking for wayland-scanner in the current $PATH
  3. Figuring out the location from pkg-config (as before).

This will make the package friendlier to cross-compilation, and
then build systems can either prepend their locations for host
tools to $PATH (covered by 2. above), or they can specify the
tool path in the command line (covered by 1. above) if they need
to be more specific.

The motivation for this patch is to avoid kludges like the following:

  https://patchwork.ozlabs.org/comment/2172010/

@aperezdc aperezdc requested review from zdobersek and carlosgcampos May 15, 2019

if (WAYLAND_SCANNER_PC_FOUND)
pkg_get_variable(WAYLAND_SCANNER_PC_EXECUTABLE wayland-scanner wayland_scanner)
if (WAYLAND_SCANNER_PC_EXECUTABLE)
wayland_scanner_get_version(WAYLAND_SCANNER_VERSION "${WAYLAND_SCANNER_PC_EXECUTABLE}")

This comment has been minimized.

Copy link
@carlosgcampos

carlosgcampos May 16, 2019

Contributor

In the case of pkg-config we already have the version here, we don't need to execute the scanner to get its version.

This comment has been minimized.

Copy link
@aperezdc

aperezdc May 16, 2019

Author Member

I think it's better to try and run it to extract the version, to avoid the case in which the path returned by pkg-config points to a binary which cannot be executed on the host used for compilation… which would be useless, and was precisely what was happening when cross-building with Buildroot.

In this case I think it is better execute the program and to report failure (if it fails to run) when trying to find an usable wayland-scanner at configuration time than to pretend things are fine and fail later during compilation in mysterious ways.

This comment has been minimized.

Copy link
@carlosgcampos
@aperezdc

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

I have double checked with Buildroot that having this patch applied indeed makes it pick the correct wayland-scanner from the host tools directory:

···
-- Found WAYLAND_EGL: wayland-egl;wayland-client
-- Found WAYLAND_SCANNER: /home/aperez/buildroot/output-rpi3/host/bin/wayland-scanner
···

Note how the configuration log output copied above picked …/host/bin/wayland-scanner; without this patch it would have picked …/target/usr/bin/wayland-scanner, which in this particular test build is an ARM binary and thus not executable in the x86_64 build host.

Let's merge this 👍

@aperezdc aperezdc merged commit eb05df9 into master May 16, 2019

@aperezdc aperezdc deleted the cmake-wayland-scanner branch May 16, 2019

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

I have also cherry picked this into the wpebackend-fdo-1.2 branch as commit 9f77cf3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.