Add MUE_COMPILE_USE_SYSTEM_OPUS cmake option to help prevent library conflicts #22243
Conversation
Chrisimx
force-pushed
the
add-use-system-opus-cmake-option
branch
3 times, most recently
from
35aa39a
to
1c5ef4a
Compare
|
I assume there's a problem with the pipeline because other PRs fail with the same error. |
|
Yes, there are. Not your fault |
|
I was wondering if it's possible to use The idea would be that if system opus is being used and is found, then you create an alias target pointing to the imported PkgConfig::OPUS target; and otherwise, you would use the The advantage would be that you only need to pass this target to MODULE_LINK (from where it will eventually be used for I'm not completely sure whether that works immediately, and it might need some tweaking, but if it does work that would be quite nice. |
cbjeukendrup
requested review from
igorkorsukov
and removed request for
Jojo-Schmitz
|
@cbjeukendrup I think it's definitely a good idea and makes things a little nicer. I tried to make it work but it would always not compile because it can't find the header files when I just add the target to MODULE_LINK and not add anything to MODULE_INCLUDE: I tried first implementing everything you mentioned in another branch of my fork in this commit: Chrisimx@1831ec9 MuseScore/src/framework/audio/thirdparty/opus/CMakeLists.txt Lines 177 to 184 in 76c26f3 |
This creates a simple way for using the system library version of opus which solves the problem of at same time dynamically linking to system libopus.so through libMuseSamplerCore.so and statically linking to a different version in an elegant way. Using this option can prevent conflicts arising because of this leading to hard-to-diagnose bugs/crashes (e.g. musescore#21838)
Chrisimx
force-pushed
the
add-use-system-opus-cmake-option
branch
from
1c5ef4a
to
8089082
Compare
|
I commented on Chrisimx@1831ec9 that it contains a typo; maybe that is causing problems. Also, note that |
|
@cbjeukendrup I've updated the branch to fix the typo. Unfortunately, it still does not work. Isn't "opus" the name of the target that is created by the CMakeLists.txt in the thirdparty/opus directory here: I also tried instead creating an alias target pointing to it, so it would report if this target doesn't exist (and not think it's a lib name or path). This has shown that it correctly recognises the target. And I also think the right public include directory is added by the opus CMakeLists.txt: MuseScore/src/framework/audio/thirdparty/opus/CMakeLists.txt Lines 177 to 184 in 3b1e984 Finally, I've printed the INCLUDE_DIRECTORIES property of the opus target. It contained a path to the thidparty/opus/include dir but only with a CMake generator expression (BUILD_INTERFACE and INSTALL_INTERFACE in it). Therefore, I used target_include_directories to just added a simple path to the opus target for testing and it all the same didn't work. |
These options were added in musescore#19795 and musescore#22243
These options were added in musescore#19795 and musescore#22243
These options were added in musescore#19795 and musescore#22243
This PR creates a simple way for using the system library version of opus which solves the problem of at same time dynamically linking to system libopus.so through libMuseSamplerCore.so and statically linking to a different version in an elegant way. Using this option can prevent conflicts arising because of this leading to hard-to-diagnose bugs/crashes (e.g. #21838)
As has been shown with #21838 using two different versions of libopus can lead and has led to hard-to-diagnose problems as explained in #21838 (comment)
As there is API and ABI compatibility between different versions of libopus, there is nothing preventing us from both times linking to the same library instead of different versions by both times linking to the system opus lib.
This is often a more sensible option as it resolves the following issue: In the current state, each time a new opus sys lib is packaged the musescore packager would have to update/replace the libopus under src/framework/audio/thirdparty/opus with the system version and release a new package. While building they also need to make sure that the compile options always match to avoid crashes/bugs/breakage.
Relying on such exact timing and precision and always updating the opus and musescore package together is unfeasible, at the very least unnecessary and will likely lead to breakage on other distros (other than Arch Linux) as well in the future when they update their opus system library. Therefore, I propose adding the cmake variable to
MUE_COMPILE_USE_SYSTEM_OPUSto make the job of package maintainers of MuseScore easier and more failsafe.Resolves: #21838 (the issue is closed but there was no elegant and sustainable solution brought into musescore; this PR aims to be this)