cmake: Fix compiler flags from FindWayland and FindPipeWire #6205
Conversation
|
The fix is correct insofar as I'd prefer if the variable is renamed as well, to avoid the confusion introduced by the fix ("definitions" being set as compiler options). |
|
I've renamed WAYLAND_DEFINITIONS as requested and also added a commit to rename PIPEWIRE_DEFINITIONS as all your comments also apply to that one. Question: |
|
Multiple commits is fine, but I'd suggest splitting the changes between one commit for Wayland and another for PipeWire. |
|
Hi, sorry, just force pushed the commits again after some fiddling with git. It's just as you've proposed: One commit makes the changes for pipewire. The other one fixes the compilation with wayland and also renames the wayland variable. :) |
|
Minor nit to pick: One commit explicitly mentions Wayland, the other doesn't. I'd suggest renaming the commit to explicitly call out that it changes something related to PipeWire. |
|
Done :) |
There was a problem hiding this comment.
Minor nitpicks on the commit messages.
Use a capitalized word immediately after the commit message prefix. Punctuate paragraphs in the commit message body correctly.
The first commit doesn't seem to have anything to do directly with libobs, so mentioning it is confusing. The changes are just in our CMake finders, so I don't think it makes sense to mention libobs, unless you can elaborate?
|
One more try... Regarding the first commit: CMake is perfectly happy with the current state of FindWayland. The error manifests later during libobs-* compilation when gcc needs to interpret cmake's output. If you still don't like it, please propose an alternative to get it merged. |
If WAYLAND_DEFINITIONS is set to "-I/usr/include/wayland", setting its value as INTERFACE_COMPILE_DEFINITION leads to CMake emitting "-D-I/usr/include/wayland" in the compiler flags. This breaks the compilation of targets that call find_package(Wayland), such as libobs and libobs-opengl, in gcc. To avoid this, rename WAYLAND_DEFINITIONS to WAYLAND_COMPILE_FLAGS to reflect that it contains ready-to-use compiler flags. Use WAYLAND_COMPILE_FLAGS as INTERFACE_COMPILE_OPTIONS so it just gets appended without adding the superfluous "-D".
PIPEWIRE_DEFINITIONS contains ready-to-use compile flags. If used with INTERFACE_COMPILE_DEFINITIONS, this can lead to a compile error. Rename the variable to PIPEWIRE_COMPILE_FLAGS to be clear about that and prevent accidental misuse.
RytoEX
changed the title
${WAYLAND_DEFINITIONS} is set to "-I/usr/include/wayland" on my system
Setting it as INTERFACE_COMPILE_DEFINITIONS leads to CMake emitting "-D-I/usr/include/wayland" in the compiler flags
this breaks the compilation
Instead add WAYLAND_DEFINITIONS as compile option so it just gets appended without adding the superflours "-D"
Description
Motivation and Context
I was trying to compile OBS for the first time using openSUSE Tumbleweed. I was able to set the dependencies and cmake up, but during compilation GCC throws an error:
This is caused by cmake emitting
cd /home/user/Programming/OBS/obs-studio/build/libobs && /usr/bin/cc -D-I/usr/include/wayland -DENABLE_DARRAY_TYPE_TEST [...]-D makes gcc wait for a macro, but instead the include flag follows immediately.
To fix it we just have to tell cmake to not interpret WAYLAND_DEFINITIONS as macro definition, but only append it to the compiler flags. (This is inline with what is done in FindPipeWire.cmake.)
This issue was likely introduced by commit df94b02 from PR #5155
P.S.: First day of working with OBS and first patch. I've tried following all contributor guidelines, but please tell me if I could've done better. :)
How Has This Been Tested?
With this commit I am able to compile OBS using openSUSE Tumbleweed on a Thinkpad P14s.
Snapshot 20220322 with CMake 3.22.2
Snapshot 20220323 with CMake 3.22.3
I'm able to launch the compiled binary on X11 and Wayland using KDE 5.24.3, do some simple screen capture using XSHM and Pipewire, record my desktop and click through menus.
Types of changes
Checklist: