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: USE_WAYLAND_WSI doesn't allow disabling Wayland support #11536

Closed
akien-mga opened this issue Nov 6, 2018 · 3 comments

Comments

@akien-mga
Copy link
Contributor

commented Nov 6, 2018

What happens?

Building PPSSPPSDL on Linux with Wayland installed but USE_WAYLAND_WSI=OFF still links against Wayland (if installed on the system). The option seems to be only used to force check that wayland support will be enabled here:

ppsspp/CMakeLists.txt

Lines 149 to 151 in f81dd83

if(USE_WAYLAND_WSI AND NOT WAYLAND_FOUND)
message(FATAL_ERROR "Could not find libwayland, but USE_WAYLAND_WSI was enabled. Failing.")
endif()

What should happen?

I'd want the option to define whether Wayland should be used or not, and not simply whether its absence is a fatal error.

As such I'd propose something like the following diff. Opening an issue for feedback instead of a PR for now, as the current behaviour might be intended - I'm just suggesting to change it.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4c15cba0d..9129a6659 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -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)
 option(USE_ADDRESS_SANITIZER "Use Clang memory sanitizer" ${USE_ADDRESS_SANITIZER})
 
 if(UNIX AND NOT (APPLE OR ANDROID) AND VULKAN)
@@ -145,13 +145,14 @@ if(UNIX AND NOT (APPLE OR ANDROID) AND VULKAN)
 		message("NOT using X11 for Vulkan")
 	endif()
 	# add_definitions(-DVK_USE_PLATFORM_XCB_KHR)
-	find_package(Wayland)
-	if(USE_WAYLAND_WSI AND NOT WAYLAND_FOUND)
-		message(FATAL_ERROR "Could not find libwayland, but USE_WAYLAND_WSI was enabled. Failing.")
-	endif()
-	if(WAYLAND_FOUND)
-		include_directories(${WAYLAND_INCLUDE_DIR})
-		add_definitions(-DVK_USE_PLATFORM_WAYLAND_KHR)
+	if(USE_WAYLAND_WSI)
+		find_package(Wayland)
+		if(NOT WAYLAND_FOUND)
+			message(FATAL_ERROR "Could not find libwayland, but USE_WAYLAND_WSI was enabled. Failing.")
+		else()
+			include_directories(${WAYLAND_INCLUDE_DIR})
+			add_definitions(-DVK_USE_PLATFORM_WAYLAND_KHR)
+		endif()
 	endif()
 endif()
 

What hardware, operating system, and PPSSPP version? On desktop, GPU matters for graphical issues.

Mageia 7 x86_64 (Linux), PPSSPP 1.7.2/master.

@akien-mga

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

BTW Wayland is considered "found" even if wayland-egl is missing, but that might be on purpose. Just mentioning it in case it's a bug in FindWayland.cmake:

-- Found WAYLAND_CLIENT: /usr/lib/libwayland-client.so  
-- Found WAYLAND_SERVER: /usr/lib/libwayland-server.so  
-- Could NOT find WAYLAND_EGL (missing: WAYLAND_EGL_LIBRARIES) 
-- Found WAYLAND_CURSOR: /usr/lib/libwayland-cursor.so  
-- Found WAYLAND: /usr/lib/libwayland-client.so;/usr/lib/libwayland-server.so;WAYLAND_EGL_LIBRARIES-NOTFOUND;/usr/lib/libwayland-cursor.so
@hrydgard

This comment has been minimized.

Copy link
Owner

commented Nov 6, 2018

Your change looks fine to me, feel free to make a pull request.

That could very well be a bug in FindWayland.cmake. Feel free to also fix that if you like :)

@akien-mga

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Had a quick look at FindWayland.cmake, but I don't see an easy way to improve the script. It will mark WAYLAND_FOUND as true if those variables are defined: https://github.com/hrydgard/ppsspp/blob/master/cmake/Modules/FindWayland.cmake#L56

And it seems to consider that /usr/lib/libwayland-client.so;/usr/lib/libwayland-server.so;WAYLAND_EGL_LIBRARIES-NOTFOUND;/usr/lib/libwayland-cursor.so is a proper definition.

All in all I'd say let's not fix what's not broken -- if some users start complaining about not being able to build because they had a partial wayland dev setup, it can be investigated further.

akien-mga added a commit to akien-mga/ppsspp that referenced this issue Nov 6, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.