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

Fixes to CMake and version upgrade with CMake #5077

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

weirdbeardgame
Copy link
Contributor

@weirdbeardgame weirdbeardgame commented Nov 28, 2021

FindWayland.cmake: Changed incorrect use of FIND_PACKAGE_HANDLE_STANDARD_ARGS which is meant to link and set the main package to found rather then link individual libs

Added CMake 3.22.0 as max version. Added package version in CMakeLists as per standard CMP0048

Description of Changes

Changes incorrect FIND_PACKAGE_HANDLE_STANDARD_ARGS to link wayland libs as imported targes. Increases the minimum Cmake version to include 3.11 and more modern 3.22 and sets "package version" as per https://cmake.org/cmake/help/latest/policy/CMP0048.html#cmp0048

Rationale behind Changes

To use more modern CMAKE features for those with 3.22 and clear warnings related to that. To fix several incorrect linkages with wayland clearing warnings like

  The package name passed to `find_package_handle_standard_args` (WAYLAND)
  does not match the name of the calling package (Wayland).  This can lead to
  problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  cmake/FindWayland.cmake:56 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  cmake/SearchForStuff.cmake:184 (find_package)
  CMakeLists.txt:31 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

Suggested Testing Steps

See if it builds without those warnings

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

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

Gave it a spin with the Wayland API On.

  • Builds with no cmake warnings
  • Runs a game

Didn't notice anything immediately incorrect with the FindWayland script but I'm no cmake wizard.

Copy link
Member

@TellowKrinkle TellowKrinkle left a comment

Choose a reason for hiding this comment

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

Since you're adding imported targets, could you also edit pcsx2/CMakeLists.txt to stop looking at things like WAYLAND_CLIENT_FOUND and ${WAYLAND_CLIENT_LIBRARIES} and just link against Wayland::Client?

cmake/FindWayland.cmake Outdated Show resolved Hide resolved
cmake/FindWayland.cmake Outdated Show resolved Hide resolved
cmake/FindWayland.cmake Outdated Show resolved Hide resolved
cmake/FindWayland.cmake Outdated Show resolved Hide resolved
@weirdbeardgame weirdbeardgame force-pushed the cmake_fixes branch 11 times, most recently from 3e151c2 to 608be76 Compare November 30, 2021 02:21
Makes policy enabling based on minimum version work properly
Also increases minimum version to 3.11 because we actually do use 3.11 features
Copy link
Member

@TellowKrinkle TellowKrinkle left a comment

Choose a reason for hiding this comment

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

This enables many years worth of CMake policies so we should get some testing on Linux to make sure nothing broke

Also clears warning about mismatched names getting sent to FIND_PACKAGE_HANDLE_STANDARD_ARGS
@weirdbeardgame
Copy link
Contributor Author

Tested between Xorg and Wayland. Package built correctly and on basic use with loading a game seemed functional as it should be. Wayland was active as noted by the pause and resume tested resulting in the current black window glitch speed seemed fast enough for the earlier fixes to be active nearing full-speed at times. But sluggish on Nvidia compared to Xorg still.

@F0bes
Copy link
Member

F0bes commented Nov 30, 2021

gt3regression.mp4

Doesn't happen on master, both renderers, SW mode settings do not matter.
Happens with or without -DWAYLAND_API=on passed to cmake.

Ok, hold on a second. Reproducing this is a pain because savestates do have an affect.

@TellowKrinkle
Copy link
Member

Doesn't happen on master, both renderers, SW mode settings do not matter. Happens with or without -DWAYLAND_API=on passed to cmake.

Can you check which commit does it? I would expect it's the CMake minimum version one but just to make sure

@F0bes
Copy link
Member

F0bes commented Nov 30, 2021

Doesn't happen on master, both renderers, SW mode settings do not matter. Happens with or without -DWAYLAND_API=on passed to cmake.

Can you check which commit does it? I would expect it's the CMake minimum version one but just to make sure

Nevermind, I just got unlucky when testing this PR. After many attempts you can reproduce it on master.

Edit:
Yeah sorry I forgot to mention that it builds and plays a game for me.

@weirdbeardgame
Copy link
Contributor Author

As a side note. This works in the AUR environment as well

@weirdbeardgame
Copy link
Contributor Author

Anything else needed to merge this?

Copy link
Contributor

@Mrlinkwii Mrlinkwii left a comment

Choose a reason for hiding this comment

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

runing X11 and cmake 3.16.3 , the PR complied fine and worked like master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants