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

Feature request: thirdparty libs should be linkable from system installed #11572

Open
Lucretia opened this issue May 8, 2022 · 16 comments
Open
Assignees
Labels
community Issues particularly suitable for community contributors to work on dev Related to developer experience (compiling, code base, CI), rather than end user experience feature request Used to suggest improvements or new capabilities P1 Priority: High

Comments

@Lucretia
Copy link

Lucretia commented May 8, 2022

Add cmake options to use thirdparty libs

A lot of hte libs in thirdparty/* should have flags in cmake to enable the use of system packages, i.e.:

  • flac
  • fluidsynth
  • freetype
  • lame
  • opus
  • singleapp (on Gentoo it's dev-qt/qtsingleapplication)
  • stb

It tries to install headers from opus currently:

 * Detected file collision(s):
 * 
 *      /usr/include/opus/opus_types.h
 *      /usr/include/opus/opus_defines.h
 *      /usr/include/opus/opus_multistream.h
 *      /usr/include/opus/opus_custom.h
 *      /usr/include/opus/opus_projection.h
 *      /usr/include/opus/opus.h
 * 
 * Searching all installed packages for file collisions...
 * Detected file collision(s):
 * 
 *      /usr/include/opus/opus_types.h
 *      /usr/include/opus/opus_defines.h
 *      /usr/include/opus/opus_multistream.h
 *      /usr/include/opus/opus_custom.h
 *      /usr/include/opus/opus_projection.h
 *      /usr/include/opus/opus.h
 * 
 * Searching all installed packages for file collisions...
@Tantacrul Tantacrul added P1 Priority: High community Issues particularly suitable for community contributors to work on labels May 15, 2022
@Tantacrul
Copy link
Contributor

I'm assigning this a priority and Beta 1 release. Will need conversations with the devs here about when to do it.... however, any community members who would like to take it on would be appreciated!

@Tantacrul
Copy link
Contributor

We have decided to move this to 4.x.

We'll be reopening it when 4.0 is released.

@Tantacrul Tantacrul closed this as not planned Won't fix, can't repro, duplicate, stale Sep 1, 2022
@adigitoleo
Copy link

I'm interested in building 4.x, but would prefer to link to system libs (esp. freetype and opus). My distribution (linux) has a build template for 3.6 which seems to use USE_SYSTEM_FREETYPE=ON however I can't see that option anywhere in the current cmake configs. Could someone clarify the status of support for building with system libs? Also, do I understand correctly that webengine is no longer needed to build 4.x?

@cbjeukendrup
Copy link
Contributor

Webengine is no longer needed indeed. (Instead, you do need QtNetworkAuthorization.)
The status about using system libs has not changed since this issue was created; the USE_SYSTEM_FREETYPE option is currently not functional.

@cbjeukendrup cbjeukendrup changed the title [MU4 Task] Thirdparty libs should be linkable from system installed Feature request: thirdparty libs should be linkable from system installed Jul 15, 2023
@cbjeukendrup cbjeukendrup added the feature request Used to suggest improvements or new capabilities label Jul 15, 2023
@cbjeukendrup cbjeukendrup reopened this Jul 15, 2023
@cbjeukendrup
Copy link
Contributor

See also #16948

@cbjeukendrup cbjeukendrup added the dev Related to developer experience (compiling, code base, CI), rather than end user experience label Jul 15, 2023
@doronbehar
Copy link
Contributor

I tried to work on making musescore link to system fluidsynth, and I noticed that musescore uses a lot of private functions from the fluidsynth's src... When one installs FluidSynth, by default it includes only:

include
├── fluidsynth
│   ├── audio.h
│   ├── event.h
│   ├── gen.h
│   ├── ladspa.h
│   ├── log.h
│   ├── midi.h
│   ├── misc.h
│   ├── mod.h
│   ├── seqbind.h
│   ├── seq.h
│   ├── settings.h
│   ├── sfont.h
│   ├── shell.h
│   ├── synth.h
│   ├── types.h
│   ├── version.h
│   └── voice.h
└── fluidsynth.h

Here's an example of an error I got:

/build/source/src/framework/audio/internal/synthesizers/fluidsynth/sfcachedloader.h:35:10: fat
floader/fluid_sfont.h: No such file or directory
   35 | #include <sfloader/fluid_sfont.h>
After applying this rather simple patch:
diff --git i/CMakeLists.txt w/CMakeLists.txt
index 621c62126f..e984112959 100644
--- i/CMakeLists.txt
+++ w/CMakeLists.txt
@@ -123,6 +123,7 @@ option(MUE_COMPILE_USE_UNITY "Use unity build." ON)
 option(MUE_COMPILE_USE_CCACHE "Try use ccache" ON)
 option(MUE_COMPILE_USE_SHARED_LIBS_IN_DEBUG "Build shared libs if possible in debug" OFF)
 option(MUE_COMPILE_USE_SYSTEM_FREETYPE "Try use system freetype" OFF) # Important for the maintainer of Linux distributions
+option(MUE_COMPILE_USE_SYSTEM_FLUIDSYNTH "Try use system fluidsynth library" OFF) # Important for the maintainer of Linux distributions
 
 # === Debug ===
 option(MUE_ENABLE_LOGGER_DEBUGLEVEL "Enable logging debug level" ON)
diff --git i/src/framework/audio/CMakeLists.txt w/src/framework/audio/CMakeLists.txt
index 6293730784..20ca5e6625 100644
--- i/src/framework/audio/CMakeLists.txt
+++ w/src/framework/audio/CMakeLists.txt
@@ -69,7 +69,6 @@ elseif(OS_IS_WASM)
     )
 endif()
 
-add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/fluidsynth fluidsynth)
 
 set(MODULE_SRC
 
@@ -224,22 +223,35 @@ set(MODULE_SRC
     ${CMAKE_CURRENT_LIST_DIR}/internal/synthesizers/synthresolver.h
     )
 
-set(FLUIDSYNTH_DIR ${PROJECT_SOURCE_DIR}/thirdparty/fluidsynth/fluidsynth-2.1.4)
-set (FLUIDSYNTH_INC
-    ${FLUIDSYNTH_DIR}/include
-    ${FLUIDSYNTH_DIR}/src
-    ${FLUIDSYNTH_DIR}/src/external
-    ${FLUIDSYNTH_DIR}/src/utils
-    ${FLUIDSYNTH_DIR}/src/midi
-    ${FLUIDSYNTH_DIR}/src/rvoice
-    ${FLUIDSYNTH_DIR}/src/sfloader
-    ${FLUIDSYNTH_DIR}/src/bindings
-    ${FLUIDSYNTH_DIR}/src/synth
-    ${FLUIDSYNTH_DIR}/src/drivers
-    )
+if (MUE_COMPILE_USE_SYSTEM_FLUIDSYNTH)
+    find_package(PkgConfig)
+    pkg_check_modules(FLUIDSYNTH REQUIRED fluidsynth)
+    if (FLUIDSYNTH_FOUND)
+        message(STATUS "Found fluidsynth: ${FLUIDSYNTH_VERSION}")
+        message(STATUS "Found fluidsynth include dirs: ${FLUIDSYNTH_VERSION}")
+    else()
+        message(FATAL_ERROR "Set MUE_COMPILE_USE_SYSTEM_FLUIDSYNTH=ON, but system fluidsynth not found")
+    endif()
+endif()
 
+if (NOT FLUIDSYNTH_FOUND)
+    add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/fluidsynth fluidsynth)
+    set(FLUIDSYNTH_DIR ${PROJECT_SOURCE_DIR}/thirdparty/fluidsynth/fluidsynth-2.1.4)
+    set (FLUIDSYNTH_INCLUDE_DIRS
+        ${FLUIDSYNTH_DIR}/include
+        ${FLUIDSYNTH_DIR}/src
+        ${FLUIDSYNTH_DIR}/src/external
+        ${FLUIDSYNTH_DIR}/src/utils
+        ${FLUIDSYNTH_DIR}/src/midi
+        ${FLUIDSYNTH_DIR}/src/rvoice
+        ${FLUIDSYNTH_DIR}/src/sfloader
+        ${FLUIDSYNTH_DIR}/src/bindings
+        ${FLUIDSYNTH_DIR}/src/synth
+        ${FLUIDSYNTH_DIR}/src/drivers
+        )
+endif()
 set (MODULE_INCLUDE
-    ${FLUIDSYNTH_INC}
+    ${FLUIDSYNTH_INCLUDE_DIRS}
     )
 
 set(MODULE_LINK

@cbjeukendrup Are you aware of this private functions usage? I was wondering whether you'd interesting of avoiding it.

@cbjeukendrup
Copy link
Contributor

Yes, I'm aware of it (part of it was added by me recently), and if we could avoid it in a relatively painless way that would be great, but I'm not sure if that's doable. Using those defsfont functions seems the only way to simply load a soundfont without instantiating a synth.

@doronbehar
Copy link
Contributor

Using those defsfont functions seems the only way to simply load a soundfont without instantiating a synth.

Maybe upstream would agree to expose these functions?

@cbjeukendrup
Copy link
Contributor

That's certainly worth asking. Especially since FluidSynth still seems to be maintained / developed actively. I'll create an issue (or even a PR, why not) in their repo.

@doronbehar
Copy link
Contributor

Another attempt report, for flac this time: I tried to make musescore use a system's flac library, using this patch:

Patch
diff --git c/CMakeLists.txt w/CMakeLists.txt
index 621c62126f..96ca7b88b9 100644
--- c/CMakeLists.txt
+++ w/CMakeLists.txt
@@ -123,6 +123,7 @@ option(MUE_COMPILE_USE_UNITY "Use unity build." ON)
 option(MUE_COMPILE_USE_CCACHE "Try use ccache" ON)
 option(MUE_COMPILE_USE_SHARED_LIBS_IN_DEBUG "Build shared libs if possible in debug" OFF)
 option(MUE_COMPILE_USE_SYSTEM_FREETYPE "Try use system freetype" OFF) # Important for the maintainer of Linux distributions
+option(MUE_COMPILE_USE_SYSTEM_FLAC "Try use system flac" OFF) # Important for the maintainer of Linux distributions
 
 # === Debug ===
 option(MUE_ENABLE_LOGGER_DEBUGLEVEL "Enable logging debug level" ON)
diff --git c/build/module.cmake w/build/module.cmake
index 32ad9033e3..12229a7e7e 100644
--- c/build/module.cmake
+++ w/build/module.cmake
@@ -143,3 +143,5 @@ set(MODULE_LINK ${QT_LIBRARIES} ${MODULE_LINK})
 set(MODULE_LINK ${CMAKE_DL_LIBS} ${MODULE_LINK})
 
 target_link_libraries(${MODULE} PRIVATE ${MODULE_LINK} )
+
+set_target_properties(${MODULE} PROPERTIES LINK_FLAGS "${MODULE_LDFLAGS}")
diff --git c/src/framework/audio/CMakeLists.txt w/src/framework/audio/CMakeLists.txt
index 6293730784..25b06b5e0a 100644
--- c/src/framework/audio/CMakeLists.txt
+++ w/src/framework/audio/CMakeLists.txt
@@ -268,7 +268,18 @@ if (MUE_ENABLE_AUDIO_EXPORT)
 
     add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/lame lame)
     add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/opusenc opusenc)
-    add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/flac flac)
+    if (MUE_COMPILE_USE_SYSTEM_FLAC)
+        find_package(PkgConfig)
+        pkg_check_modules(FLAC REQUIRED flac)
+        if (NOT FLAC_FOUND)
+            message(FATAL_ERROR "Set MUE_COMPILE_USE_SYSTEM_FLAC=ON, but system FLAC not found, built-in will be used")
+        endif()
+    endif()
+    if (FLAC_FOUND)
+        set(MODULE_LDFLAGS "${FLAC_LDFLAGS}")
+    else()
+        add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/flac flac)
+    endif()
 
     set(MODULE_LINK ${MODULE_LINK} lame opusenc flac)
 endif()

And the build fails with:

/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: cannot find -lflac: No such file or directory

This one should be easier to solve, I'm just not familiar with the MODULE_LINK interface used here - I need to pass -L/nix/store/...-flac-1.4.3/lib to the LINK_FLAGS property of target audio target, but the MODULE_LDFLAGS I added doesn't work...

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Aug 22, 2023

MODULE_LINK is passed to target_link_libraries(${MODULE} PRIVATE ${MODULE_LINK} ), so its interface is the same as that of CMake's target_link_libraries.

The reason that the line set(MODULE_LINK ${MODULE_LINK} lame opusenc flac) does not work, must be that flac refers to nothing if MUE_COMPILE_USE_SYSTEM_FLAC is true. Instead, pkg_check_modules sets some variables like FLAC_FOUND, FLAC_LIBRARIES, FLAC_INCLUDE_DIRS. Those should be used instead of just flac (which stays meaningless to CMake and ld, as far as I can tell). In FindSndFile.cmake, there is an example of how we use pkg_check_modules.

@doronbehar
Copy link
Contributor

FLAC_LIBRARIES, FLAC_INCLUDE_DIRS. Those should be used instead of just flac (which stays meaningless to CMake and ld, as far as I can tell).

That makes sense. I started to use FLAC_LIBRARIES, set by pkg_check_modules, instead of flac. But it still fails, but differently:

/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: src/framework/audio/libaudio.a(unity_3_cxx.cxx.o):(.data.rel.ro._ZTV11FlacHandler[_ZTV11FlacHandler]+0x1b8): undefined reference to `FLAC::Encoder::Stream::seek_callback(unsigned long)'
/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: src/framework/audio/libaudio.a(unity_3_cxx.cxx.o):(.data.rel.ro._ZTV11FlacHandler[_ZTV11FlacHandler]+0x1c0): undefined reference to `FLAC::Encoder::Stream::tell_callback(unsigned long*)'
/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: src/framework/audio/libaudio.a(unity_3_cxx.cxx.o):(.data.rel.ro._ZTV11FlacHandler[_ZTV11FlacHandler]+0x1c8): undefined reference to `FLAC::Encoder::Stream::metadata_callback(FLAC__StreamMetadata const*)'
/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: src/framework/audio/libaudio.a(unity_3_cxx.cxx.o):(.data.rel.ro._ZTV11FlacHandler[_ZTV11FlacHandler]+0x1d0): undefined reference to `FLAC::Encoder::File::init(_IO_FILE*)'
/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: src/framework/audio/libaudio.a(unity_3_cxx.cxx.o):(.data.rel.ro._ZTV11FlacHandler[_ZTV11FlacHandler]+0x1d8): undefined reference to `FLAC::Encoder::File::init(char const*)'
/nix/store/fm1xq4f3g48k1mmwl6qg9jc3ipgfsk1x-binutils-2.40/bin/ld: src/framework/audio/libaudio.a(unity_3_cxx.cxx.o):(.data.rel.ro._ZTV11FlacHandler[_ZTV11FlacHandler]+0x1e0): undefined reference to `FLAC::Encoder::File::init(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'

And more... In FindSndFile.cmake, find_library is used. Do you think that is needed here as well? I wonder why using LIBSNDFILE_PKGCONF prefixed variables wasn't good enough.

@cbjeukendrup
Copy link
Contributor

I'm not sure why the output from pkg_check_modules is not used directly, but it seems to be the case in many places where pkg_check_modules is used, so perhaps that's the way to do it...
Maybe the reason is that pkg_check_modules only gives the lib dir and the filename of the library, and that then find_library is used to find the full path to the library. But I'm not sure if that's entirely true.

Perhaps that is the cause of the link failure, but that's difficult to say... In this case, I don't think we are using private functions or doing something unusual.

@Theofghy
Copy link
Contributor

Gentoo user working on building MuseScore 4.1.1 here. A prior ebuild had a patch which replaced a few third-party libraries with system libraries, which appears to be what you're looking for. As mentioned above, I did observe that MuseScore uses fluidsynth internals, so it's probably not possible to use system fluidsynth without changes there.

This patch is for v4.1.1, but should be very close to what you want (some cmake logic will be required, I assume).
musescore-4.1.1-unbundle-deps.patch.txt

@doronbehar
Copy link
Contributor

Thanks for the help @Theofghy :) Your usage of PkgConfig:: seems to help a lot, and I think that I had missing the flac++ lookup in my previous attempt, which probably also helped. However, I noticed a few issues that disallow part of your patch to be ported here to upstream:

  • Your patch relies on lame library including a lame.pc file, that is available only on Gentoo.
  • To make the opusenc part of the patch work, I needed to instruct cmake to look up for opus library (not opusenc). This is working now so it seems.
  • Your googletest part of the patch is probably working, but (without going too much into details) I had trouble keeping the default behavior of using the bundled version.

All of the changes that worked for me, are available in #19795 .

@doronbehar
Copy link
Contributor

doronbehar commented Oct 21, 2023

Just to summarize, after #19795, the following libraries will lack support for using a system version:

  • lame - requires finding a good way to find it on system - upstream lacks a .pc file.
  • fluidsynth - requires asking fluidsynth maintainers to install in their package header files that are only availble on their repo.
  • googletest - needs further debugging (won't put too much details here).
  • KDDockWidgets - latest upstream version had headers renamed - need to accommodate to that. Qt 6.5+: docks do not open after the last dock on a panel is closed #24866 lists issues with the current, outdated version bundled in the sources...
  • rtf2html - haven't tried to support system version of it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues particularly suitable for community contributors to work on dev Related to developer experience (compiling, code base, CI), rather than end user experience feature request Used to suggest improvements or new capabilities P1 Priority: High
Projects
Status: Available
Development

No branches or pull requests

8 participants