Closed daschuer closed 11 months ago
Some comments:
find_library
support for Apple frameworks. This change implies that there is a general design error in the way CMake supports using frameworks.-Wl,-framework,CoreAudio
, etc., seems much simpler than calling find_library
for each library, introducing a new variable for each framework, and using these variables in target_link_libraries
or whatever.pm_common/CMakeLists.txt
line 88, PM_NEEDED_LIBS
is defined as a list of frameworks and threads library. In line 95, the same (almost) list is repeated in target_link_libraries
-- why not use ${PM_NEEDED_LIBS}
to specify target_link_libraries
?target_link_libraries(portmidi...
could be PUBLIC
instead of PRIVATE
and then the PM_NEEDED_LIBS
would be automatically linked with Portmidi applications, eliminating the need to explicitly add PM_NEEDED_LIBS
to applications. Does that sound right ? Maybe I can try that.Thanks for any comments or replies. For now, I'll merge the changes.
Ok maybe it helps to share my understanding I am on Linux so I am not an Expert and might be not fully correct.
Apple frameworks are installed at different location depending on the Xcode version. The linker knows them when linked with the -framework and friends options. So a path lookup is not required. With CMAKE_OSX_DEPLOYMENT_TARGET you can control the minimum version and with xcode-select the maximum and than you can check at runtime for availability of the host system.
In this PR I have used the legacy "-Wl,-interfsce,..." solution. From cmake 2.24 we can use a special generator expression: https://cmake.org/cmake/help/v3.24/manual/cmake-generator-expressions.7.html#link-features
In case of linking portmid statically to an application, it needs to know the libraries portmidi is depending. This normally done as an imported target, the app can lookup itself. In this case it was passed as absolute path which was the original issue.
Yes, the particular versions are from the Mixxx CI.
Yes, my guess is that this is legacy.
Yes.
Yes, we can consider that. I have not touched it here, because I did not really understand the Threads library issue.
The cmake config mode alred works with PRIVATE. To be honest I have not 100% understand the PUBLIC thing. For my understanding it is required if the App directly calls dependent library feature through header files. Since it already works with PRIVATE without using PM_NEEDED_LIBS it seems to be already correct. PM_NEEDED_LIBS is probably used in module mode. When the link info is retrieved in a custom findPortmidi script using pkgconf. I think pkgconf also allows to store the dependen library. This allows to deprecat PM_NEEDED_LIBS. But I would not remove it to not create headache in the using projects.
Hope that was helpful.
Very helpful. Thanks. I think I will remove find_library
for frameworks from other projects too. I think finding frameworks is newer than Portmidi, but it may already be considered legacy and not the "right" way -- I couldn't find any clear advice with Google searches, so I don't know.
I think PM_NEEDED_LIBS came from me before there was any CMake package stuff and was supposed to specify linker inputs to applications in pm_test. Maybe I'll play with this later.
This fixes https://github.com/PortMidi/portmidi/issues/56
The failing issue can be seen here: https://github.com/daschuer/mixxx/actions/runs/5688916805/job/15419537786 It fails, because it is uses Xcode 13.2 while the portmidi requires a framework with absolute path from XVode 12.4
It works after using a vcpkg environment including this patch https://github.com/daschuer/mixxx/actions/runs/5689098761/job/15420010526
Note, the Ubuntu runners are failing because the Debian package does not provide the CMake files for config mode.