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

CMake: Allow disabling Wayland support with USE_WAYLAND_WSI #11537

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

akien-mga
Copy link
Contributor

Closes #11536.

CMakeLists.txt Outdated
@@ -134,7 +134,7 @@ option(USE_FFMPEG "Build with FFMPEG support" ${USE_FFMPEG})
option(USE_SYSTEM_SNAPPY "Dynamically link against system snappy" ${USE_SYSTEM_SNAPPY})
option(USE_SYSTEM_FFMPEG "Dynamically link against system FFMPEG" ${USE_SYSTEM_FFMPEG})
option(USE_SYSTEM_LIBZIP "Dynamically link against system libzip" ${USE_SYSTEM_LIBZIP})
option(USE_WAYLAND_WSI "Set to ON to require Wayland support for Vulkan" ${USE_WAYLAND_WSI})
option(USE_WAYLAND_WSI "Set to ON to require Wayland support for Vulkan" ON)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what ${USE_WAYLAND_WSI} would resolve to if users don't specify anything - my intention was that Wayland support is tested by default as it used to be.

But indeed I see now that the patch would as the possible scenarios are now:

  • USE_WAYLAND_WSI=ON (default): Test for Wayland, if found, ok (like before)
  • USE_WAYLAND_WSI=ON (default): Test for Wayland, if not found, fatal error (like before)
  • USE_WAYLAND_WSI=OFF: Don't test for Wayland (new)

So we no longer have the previous scenario which was:

  • USE_WAYLAND_WSI=OFF (default): Test for Wayland, if not found, continue silently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish CMake had a "on/off/auto" switch like autotools (and it's not often that I wish for autotools-like things :P).

@hrydgard hrydgard added this to the v1.8.0 milestone Nov 6, 2018
@hrydgard
Copy link
Owner

hrydgard commented Nov 6, 2018

Alright, I think this should be fine.

Let's wait for the buildbot to go green before merge.

@akien-mga
Copy link
Contributor Author

I'll just tweak the hint string for USE_WAYLAND_WSI then to be more explicit.

@akien-mga
Copy link
Contributor Author

Yeah CI fails, and any build against older SDL2 or without Wayland libraries would fail too, that's probably not a good change as is.

Alternatively I could drop the fatal error, so that the scenarios are:

  • USE_WAYLAND_WSI=ON (default): Test for Wayland, if found, ok (like before)
  • USE_WAYLAND_WSI=ON (default): Test for Wayland, if not found, continue silently without Wayland support (like before without defining USE_WAYLAND_WSI=ON)
  • USE_WAYLAND_WSI=OFF: Don't test for Wayland (new)

@akien-mga
Copy link
Contributor Author

BTW should I move the option up near USING_X11_VULKAN in the # :: Environments section?

This change means that USE_WAYLAND_WSI=ON no longer triggers a
fatal error if Wayland libraries are missing though, it will just
show a message and continue building without Wayand WSI support.

Closes hrydgard#11536.
@akien-mga akien-mga force-pushed the cmake-wayland-opt-out branch from 3160828 to 3bc89f3 Compare November 6, 2018 11:09
@akien-mga
Copy link
Contributor Author

I've pushed an updated version which does what I describe in #11537 (comment).

@hrydgard
Copy link
Owner

hrydgard commented Nov 6, 2018

Seems ok.

@hrydgard hrydgard merged commit a96e792 into hrydgard:master Nov 6, 2018
@akien-mga akien-mga deleted the cmake-wayland-opt-out branch November 6, 2018 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants