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

Don't use absolut paths to macOS frameworks #57

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

daschuer
Copy link
Contributor

This fixes #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

Check for working C compiler: /Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc - skipped
2023-07-28T06:28:15.3963980Z ##[warning]/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk/System/Library/Frameworks/CoreFoundation.framework/Headers/CFBase.h:45:6: warning: '__BIG_ENDIAN__' is not defined, evaluates to 0 [-Wundef]

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.

@rbdannenberg
Copy link
Contributor

Some comments:

  1. Absolute paths are a basic design decision in CMake (I assume to eliminate frequent ambiguity when system tools use inconsistent hints and search algorithms), so I don't understand this change: Isn't this building with one path and linking with another?
  2. Explanation said "portmidi requires a framework from ... 12.4" -- this can't be right, but I think maybe you mean a particular portmidi library build that you want to use for some reason with 13.2 has 12.4 wired in.
  3. CMake has specific built-in find_library support for Apple frameworks. This change implies that there is a general design error in the way CMake supports using frameworks.
  4. Using -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.
  5. In the changed 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?
  6. I'm still learning new features of CMake (and CMake is much different from the CMake I started using with Portmidi), but I think 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.

@rbdannenberg rbdannenberg merged commit 45d6a40 into PortMidi:master Jul 28, 2023
3 checks passed
@daschuer
Copy link
Contributor Author

Ok maybe it helps to share my understanding I am on Linux so I am not an Expert and might be not fully correct.

  1. Yes, the absolute paths are generated by cmake during the configure run and creates a more traceable build process which is good in general.

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.

  1. Yes, the particular versions are from the Mixxx CI.

  2. Yes, my guess is that this is legacy.

  3. Yes.

  4. Yes, we can consider that. I have not touched it here, because I did not really understand the Threads library issue.

  5. 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.

@rbdannenberg
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't link to macOS frameworks with an absolute path
2 participants