macOS static build support #4163

daschuer · 2021-07-31T14:18:12Z

This is the collection of changes that are required to build MacOs statically with VCPKG support.
This also still builds dynamically with the current environment.

…for the other targets as well

…PRIME relationship

Be-ing · 2021-07-31T14:21:54Z

CMakeLists.txt

+    find_package(FFTW REQUIRED)
+    find_package(SndFile REQUIRED)
+    find_package(sord CONFIG REQUIRED)
+    find_package(SQLite3 REQUIRED)
+    find_library(SAMPLERATE_LIBRARY samplerate REQUIRED)
+    target_link_libraries(mixxx-lib PRIVATE
+        FFTW::FFTW
+        SndFile::sndfile
+        sord::sord
+        ${SQLite3_LIBRARIES}
+        ${SAMPLERATE_LIBRARY}
+    )


Aren't these handled elsewhere in CMakeLists.txt?

I have tried to get rid of some of them but these are the remaining bits.

coveralls · 2021-07-31T14:50:22Z

Pull Request Test Coverage Report for Build 1101583044

0 of 0 changed or added relevant lines in 0 files are covered.
36 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.003%) to 26.023%

Files with Coverage Reduction	New Missed Lines	%
src/engine/cachingreader/cachingreaderworker.cpp	2	63.48%
src/engine/readaheadmanager.cpp	2	88.79%
src/controllers/scripting/controllerscriptenginebase.cpp	32	40.74%

Totals
Change from base Build 1100644322:	-0.003%
Covered Lines:	20016
Relevant Lines:	76916

💛 - Coveralls

cmake/modules/FindChromaprint.cmake

cmake/modules/FindOpusFile.cmake

Be-ing · 2021-07-31T15:14:00Z

cmake/modules/Findlilv.cmake

@@ -84,5 +84,15 @@ if(lilv_FOUND)
        INTERFACE_COMPILE_OPTIONS "${PC_lilv_CFLAGS_OTHER}"
        INTERFACE_INCLUDE_DIRECTORIES "${lilv_INCLUDE_DIR}"
    )
+    get_target_property(LILV_TYPE lilv::lilv TYPE)
+    if(LILV_TYPE STREQUAL "STATIC_LIBRARY")
+      find_package(lv2 CONFIG REQUIRED)


Suggested change

find_package(lv2 CONFIG REQUIRED)

find_package(lv2)

Are you sure CONFIG must be forced?

It is taken from here:
https://github.com/microsoft/vcpkg/blob/261c458af6e3eed5d099144aff95d2b5035f656b/ports/lilv/CMakeLists.txt#L4
and it works.

These libraries do not ship CMake config modules. Those come from vcpkg. So I think CONFIG REQUIRED can only work if the static lilv library comes from vcpkg.

The config module for lilv in vcpkg should already search for these libraries...
https://github.com/microsoft/vcpkg/blob/261c458af6e3eed5d099144aff95d2b5035f656b/ports/lilv/CMakeLists.txt#L84

Maybe the issue is that some are linked PRIVATE?
https://github.com/microsoft/vcpkg/blob/261c458af6e3eed5d099144aff95d2b5035f656b/ports/lilv/CMakeLists.txt#L54

Why shouldn't we rely on that?

Because it is to be considered an implementation detail that might change at any time.

Example: All crate dependencies in Rust are private. If I need a dependency myself I have to declare it explicitly. Cargo then will figure out if a single, shared version could be used or if multiple different versions are needed.

We should use PUBLIC only for our own, static libs that are just an intermediate build artifact and no deployment unit.

I disagree. Downstream applications should not be responsible for manually linking all the transient dependencies of their dependencies. IMO that indicates a bug in either the library's build system or the vcpkg port.

Then this was a misunderstanding. Of course, not manually link unless we use transient dependencies elsewhere!

Be-ing · 2021-07-31T16:33:07Z

CMakeLists.txt

-  target_link_libraries(mixxx-lib PRIVATE ${COREFOUNDATION_LIBRARY})
+  if(QT5_TYPE STREQUAL "STATIC_LIBRARY")
+    find_package(FFTW REQUIRED)
+    find_package(SndFile REQUIRED)


already linked below

Be-ing · 2021-07-31T16:34:06Z

CMakeLists.txt

+  if(QT5_TYPE STREQUAL "STATIC_LIBRARY")
+    find_package(FFTW REQUIRED)
+    find_package(SndFile REQUIRED)
+    find_package(sord CONFIG REQUIRED)


handled by Findlilv.cmake

CMakeLists.txt

Be-ing · 2021-07-31T17:47:54Z

Windows build failed???

 [763/870] Automatic MOC for target mixxx-test
FAILED: mixxx-test_autogen/timestamp 
cmd.exe /C "cd /D D:\a\mixxx\mixxx\build && "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe" -E cmake_autogen D:/a/mixxx/mixxx/build/CMakeFiles/mixxx-test_autogen.dir/AutogenInfo.json RelWithDebInfo && "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe" -E touch D:/a/mixxx/mixxx/build/mixxx-test_autogen/timestamp && "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe" -E cmake_transform_depfile Ninja gccdepfile D:/a/mixxx/mixxx D:/a/mixxx/mixxx D:/a/mixxx/mixxx/build D:/a/mixxx/mixxx/build D:/a/mixxx/mixxx/build/mixxx-test_autogen/deps D:/a/mixxx/mixxx/build/CMakeFiles/d/fd30099103cc38c9d949ff52ba949bf29cca78df1ae23494c1a9032d94f5cfbc.d"

Be-ing · 2021-07-31T18:01:43Z

CMakeLists.txt

+    find_package(FFTW REQUIRED)
+    find_package(SndFile REQUIRED)
+    find_package(sord CONFIG REQUIRED)
+    find_library(SAMPLERATE_LIBRARY samplerate REQUIRED)


I'm confused... Mixxx doesn't use libsamplerate?

rubberband uses libsamplerate

...and Rubberband also needs FFTW, depending on the build config. Rubberband 1.9.2 comes with its own, optimized FFT code that doesn't require external dependencies.

Be-ing · 2021-07-31T18:02:15Z

CMakeLists.txt

+
+if(APPLE)
+  if(QT5_TYPE STREQUAL "STATIC_LIBRARY")
+    find_package(FFTW REQUIRED)


What is this needed for?

Chromaprint and Rubberband, unfortunately setting INTERFACE_LINK_LIBRARIES is not sufficient.

Co-authored-by: Be <be.0@gmx.com>

cmake/modules/IsStaticLibrary.cmake

daschuer · 2021-08-01T18:07:56Z

This is mergeable now. There was a bug in the static library detection. This is fixed now with a new module.

CMakeLists.txt

cmake/modules/FindKeyFinder.cmake

cmake/modules/FindOpusFile.cmake

Be-ing · 2021-08-01T21:06:07Z

cmake/modules/Findlilv.cmake

@@ -84,5 +86,12 @@ if(lilv_FOUND)
        INTERFACE_COMPILE_OPTIONS "${PC_lilv_CFLAGS_OTHER}"
        INTERFACE_INCLUDE_DIRECTORIES "${lilv_INCLUDE_DIR}"
    )
+    is_static_library(lilv_IS_STATIC lilv::lilv)
+    if(lilv_IS_STATIC)
+      find_package(sord CONFIG REQUIRED)


Suggested change

find_package(sord CONFIG REQUIRED)

find_package(sord REQUIRED)

As already mentioned, this is how it is called in the lilv CMakeList.txt both place need to be use the same method.
See: https://github.com/microsoft/vcpkg/blob/261c458af6e3eed5d099144aff95d2b5035f656b/ports/lilv/CMakeLists.txt#L6

This means this CMake module can only work with vcpkg because the upstream library does not ship a CMake config module.

@Holzhaus what are your thoughts on this?

Ping @Holzhaus, this is all that is left on this PR.

We want the same.

The context here is different. This module might be copied into another application which doesn't use vcpkg and has its own Findsord.cmake module. Hardcoding CONFIG would break that. I don't think there's a good reason to make this code less flexible by doing that.

I don't care about writing the module to use it in other applications, sorry.

It is more about the general principle not to hardcode unnecessary assumptions. Anyway I am done bikeshedding this. If anyone really needs to change it, it would simply be deleting a single word.

I have asked about upstreaming these CMake build scripts.

Be-ing · 2021-08-01T21:37:23Z

It would be good to get manual testing of this on Windows and macOS to confirm there are no regressions.

CMakeLists.txt

Co-authored-by: Be <be.0@gmx.com>

foss- · 2021-08-03T10:36:54Z

It would be good to get manual testing of this on Windows and macOS to confirm there are no regressions.

macOS 11.5.1, 2.4-alpha-724-gfecab7c1db (HEAD)

No regressions noticeable.

Be-ing · 2021-08-04T21:32:02Z

CMakeLists.txt

  if(FDK_AAC_LIBRARY)
    message(STATUS "Found fdk-aac: ${FDK_AAC_LIBRARY}")
-    install(FILES ${FDK_AAC_LIBRARY} DESTINATION ${MIXXX_INSTALL_PREFIX}/Contents/Frameworks)
+    install(FILES ${FDK_AAC_LIBRARY} DESTINATION ${MIXXX_INSTALL_PREFIX}/Contents/Frameworks FOLLOW_SYMLINK_CHAIN)


I remember using this then reverting it when we initially added fdk-aac but I do not remember why I reverted it.

CMakeLists.txt

cmake/modules/FindOpusFile.cmake

Holzhaus · 2021-08-06T17:30:07Z

CMakeLists.txt

@@ -2056,7 +2074,7 @@ target_include_directories(mixxx-lib SYSTEM PUBLIC ${PortMidi_INCLUDE_DIRS})
 target_link_libraries(mixxx-lib PRIVATE ${PortMidi_LIBRARIES})

 # Protobuf
-if(STATIC_DEPS)
+if(NOT BUILD_SHARED_LIBS)


I suppose this caused the issue reported on here? https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/cmake.20fails.20with.20Could.20NOT.20find.20Protobuf

Fixed in #4185

This was broken by the refactoring for building with static libraries (c21aec2 mixxxdj#4163): mixxxdj#4163 (comment) https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/cmake.20fails.20with.20Could.20NOT.20find.20Protobuf

* Move Apple extra linking to the block where extra linking is defined for the other targets as well * Added additional libraries required for static linking on macOs * Remove unneeded extra Apple link dependencies for the non static case * Added Qt5::PrintSupport needed for MacOS linking * Add Support for VCPKG_OVERLAY_TRIPLETS * Improve cmake find modules for static linking * Remove obsolete STATIC_DEPS parameter * Enable LOCALECOMPARE option for static linking on macOS and fix ENGINEPRIME relationship with SQLite

This was broken by the refactoring for building with static libraries (c21aec2 mixxxdj#4163): mixxxdj#4163 (comment) https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/cmake.20fails.20with.20Could.20NOT.20find.20Protobuf

daschuer added 8 commits July 30, 2021 23:41

Move Apple extra linking to the block where extra linking is defined …

6dec4fc

…for the other targets as well

Added additional libraries required for static linking on macOs

39df289

Remove unneded extra Apple link dependencies for the non static case

e911ba8

Added Qt5::PrintSupport needed for MacOS linking

4ba416e

Add Support for VCPKG_OVERLAY_TRIPLETS

0396984

Improve find cmake files for the static case

8ee51e3

Remove obsolete STATIC_DEPS parameter

bd8d72f

Enable LOCALECOMPARE option for the static MacOs case, and fix ENGINE…

70c5d73

…PRIME relationship

github-actions bot added the build label Jul 31, 2021