Skip to content

CMake: Allow disabling Wayland support with USE_WAYLAND_WSI#11537

Merged
hrydgard merged 1 commit intohrydgard:masterfrom
akien-mga:cmake-wayland-opt-out
Nov 6, 2018
Merged

CMake: Allow disabling Wayland support with USE_WAYLAND_WSI#11537
hrydgard merged 1 commit intohrydgard:masterfrom
akien-mga:cmake-wayland-opt-out

Conversation

@akien-mga
Copy link
Contributor

Closes #11536.

CMakeLists.txt Outdated
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