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.

Sign up for GitHub

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

Jump to bottom

cmake: Disable discovery of Homebrew libraries for dependencies #11299

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

cmake: Disable discovery of Homebrew libraries for dependencies #11299

wants to merge 2 commits into from

Conversation

PatTheMav
Copy link
Member

Description

Adds the default installation location of Homebrew packages to list of ignored paths for CMake to resolve dependencies on macOS.

Motivation and Context

macOS builds should only use dependencies built via the obs-deps build scripts. Default variants of the same dependencies are not compatible with our app packaging requirements and thus will create issues when creating the app bundle.

This is specifically an issue when MbedTLS is installed via Homebrew which ships a CMake package config by default and is picked up by our code ever since we switched to prefer CMake packages.

How Has This Been Tested?

Tested on macOS 15 with MbedTLS installed via Homebrew and observed that CMake will only pick up MbedTLS provided by obs-deps.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@PatTheMav
cmake: Disable discovery of Homebrew libraries for dependencies
c98c122 
macOS builds should only use dependencies built via the obs-deps
build scripts. Default variants of the same dependencies are not
compatible with our app packaging requirements and thus will create
issues when creating the app bundle.

This is specifically an issue when MbedTLS is installed via Homebrew
which ships a CMake package config by default and is picked up
by our code ever since we switched to prefer CMake packages.
@PatTheMav PatTheMav added Bug Fix Non-breaking change which fixes an issue Seeking Testers Build artifacts on CI macOS Affects macOS labels Sep 18, 2024
@jcm93
Copy link
Contributor

jcm93 commented Sep 18, 2024 •
edited
Loading

This is still pulling in some homebrew libraries, for example libluajit is still pulled from /opt/homebrew/Cellar if it exists there. The --debug-find output reveals this:

CMake Debug Log at cmake/finders/FindLuajit.cmake:95 (find_library):
  find_library called with the following settings:

    VAR: Luajit_LIBRARY
    NAMES: "luajit"
           "luajit-51"
           "luajit-5.1"
           "lua51"
    Documentation: Luajit location
    Framework
      Only Search Frameworks: 0
      Search Frameworks Last: 0
      Search Frameworks First: 1
    AppBundle
      Only Search AppBundle: 0
      Search AppBundle Last: 0
      Search AppBundle First: 1
    CMAKE_FIND_USE_CMAKE_PATH: 1
    CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH: 1
    CMAKE_FIND_USE_SYSTEM_ENVIRONMENT_PATH: 1
    CMAKE_FIND_USE_CMAKE_SYSTEM_PATH: 1
    CMAKE_FIND_USE_INSTALL_PREFIX: 1

  find_library considered the following locations:

    /Users/xxx/code/obs-studio/.deps/obs-deps-2024-09-05-universal/lib/libluajit(\.tbd|\.dylib|\.so|\.a)
    /Users/xxx/code/obs-studio/.deps/obs-deps-2024-09-05-universal/libluajit(\.tbd|\.dylib|\.so|\.a)
    /Users/xxx/code/obs-studio/.deps/obs-deps-qt6-2024-09-05-universal/lib/libluajit(\.tbd|\.dylib|\.so|\.a)
    /Users/xxx/code/obs-studio/.deps/obs-deps-qt6-2024-09-05-universal/libluajit(\.tbd|\.dylib|\.so|\.a)

  The item was found at

    /opt/homebrew/Cellar/luajit/2.1.1725453128/lib/libluajit.dylib

I'll see if I can dig into it any more... perhaps /opt/homebrew is still in the SYSTEM_ENVIRONMENT_PATH or CMAKE_SYSTEM_PATH and so it's still searched?

@PatTheMav
Copy link
Member Author

This is still pulling in some homebrew libraries, for example libluajit is still pulled from /opt/homebrew/Cellar if it exists there. The --debug-find output reveals this:

CMake Debug Log at cmake/finders/FindLuajit.cmake:95 (find_library):
  find_library called with the following settings:

    VAR: Luajit_LIBRARY
    NAMES: "luajit"
           "luajit-51"
           "luajit-5.1"
           "lua51"
    Documentation: Luajit location
    Framework
      Only Search Frameworks: 0
      Search Frameworks Last: 0
      Search Frameworks First: 1
    AppBundle
      Only Search AppBundle: 0
      Search AppBundle Last: 0
      Search AppBundle First: 1
    CMAKE_FIND_USE_CMAKE_PATH: 1
    CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH: 1
    CMAKE_FIND_USE_SYSTEM_ENVIRONMENT_PATH: 1
    CMAKE_FIND_USE_CMAKE_SYSTEM_PATH: 1
    CMAKE_FIND_USE_INSTALL_PREFIX: 1

  find_library considered the following locations:

    /Users/xxx/code/obs-studio/.deps/obs-deps-2024-09-05-universal/lib/libluajit(\.tbd|\.dylib|\.so|\.a)
    /Users/xxx/code/obs-studio/.deps/obs-deps-2024-09-05-universal/libluajit(\.tbd|\.dylib|\.so|\.a)
    /Users/xxx/code/obs-studio/.deps/obs-deps-qt6-2024-09-05-universal/lib/libluajit(\.tbd|\.dylib|\.so|\.a)
    /Users/xxx/code/obs-studio/.deps/obs-deps-qt6-2024-09-05-universal/libluajit(\.tbd|\.dylib|\.so|\.a)

  The item was found at

    /opt/homebrew/Cellar/luajit/2.1.1725453128/lib/libluajit.dylib

I'll see if I can dig into it any more... perhaps /opt/homebrew is still in the SYSTEM_ENVIRONMENT_PATH or CMAKE_SYSTEM_PATH and so it's still searched?

Can you try with it changed to CMAKE_IGNORE_PATH?

@jcm93
Copy link
Contributor

jcm93 commented Sep 18, 2024

No change with CMAKE_IGNORE_PATH instead of CMAKE_IGNORE_PREFIX_PATH.

@jcm93
Copy link
Contributor

jcm93 commented Sep 18, 2024

From https://cmake.org/cmake/help/latest/variable/CMAKE_IGNORE_PATH.html:

The listed directories do not apply recursively, so any subdirectories to be ignored must also be explicitly listed.

So maybe CMAKE_IGNORE_PATH will work if we explicitly enumerate /opt/homebrew/Cellar/lib, /opt/homebrew/lib, etc.

@PatTheMav
Copy link
Member Author

From https://cmake.org/cmake/help/latest/variable/CMAKE_IGNORE_PATH.html:

The listed directories do not apply recursively, so any subdirectories to be ignored must also be explicitly listed.

So maybe CMAKE_IGNORE_PATH will work if we explicitly enumerate /opt/homebrew/Cellar/lib, /opt/homebrew/lib, etc.

That does make it sound like CMAKE_IGNORE_PREFIX_PATH is more correct:

CMAKE_IGNORE_PREFIX_PATH provides a more appropriate way to ignore a whole search prefix.

@jcm93
Copy link
Contributor

jcm93 commented Sep 18, 2024

Hmm, for the prefix path, it also says:

The prefixes are also ignored by the Config mode of the find_package() command (Module mode is unaffected)

Aren't OBS's CMake finders in the module mode? So find_package will ignore this?

@PatTheMav
Copy link
Member Author

Hmm, for the prefix path, it also says:

The prefixes are also ignored by the Config mode of the find_package() command (Module mode is unaffected)

Aren't OBS's CMake finders in the module mode? So find_package will ignore this?

To me it sounds like the find_package call in find modules is unaffected by this, but we commonly only use that one to find pkg-config - the dependencies themselves are resolved via find_library and find_path and their docs state that the variable is used.

@PatTheMav
cmake: Change library discovery order in LuaJIT find module
07ca0c9 
To ensure that the find module prefers the variant of LuaJIT shipped
as part of obs-deps, it's necessary to give the name as shipped in
obs-deps the highest precedence.

This ensures that the "find_library" call will discover the less
version-specific variant of the library possibly installed via
Homebrew.
@jcm93
Copy link
Contributor

jcm93 commented Sep 20, 2024

Per a CMake maintainer, /opt/homebrew is considered a separate installation prefix to /opt/homebrew/Cellar. So I think we also need to separately add /opt/homebrew/Cellar to the ignore prefix path list, and perhaps instead of /opt/homebrew, /opt/homebrew/lib?

@jcm93
Copy link
Contributor

jcm93 commented Sep 20, 2024

It seems like if we exclude all of the following:

list(APPEND CMAKE_IGNORE_PREFIX_PATH "/opt/homebrew/lib" "/usr/local/lib" "/opt/homebrew" "/usr/local")

Then CMake successfully stops finding any homebrew versions of MbedTLS's config on my system.

The extra exclusions seem to be necessary because cmFindPackageCommand::SearchPrefix is appending "/lib/cmake/MbedTLS" to all prefixes for MbedTLS after checking against ignored prefixes, so if /usr/local/ is anywhere in our system environment paths, it can end up searching /usr/local/lib/cmake/MbedTLS, and finding it, even though /usr/local/lib is specifically excluded.

jcm93
jcm93 reviewed Sep 20, 2024
View reviewed changes
cmake/macos/defaults.cmake
@@ -36,6 +36,8 @@ set(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE)
set(CMAKE_INSTALL_RPATH_USE_LINK_PATH FALSE)
# Use common bundle-relative RPATH for installed targets
set(CMAKE_INSTALL_RPATH "@executable_path/../Frameworks")
# Ignore any dependent packages installed via Homebrew
list(APPEND CMAKE_IGNORE_PREFIX_PATH "/opt/homebrew" "/usr/local/lib")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
list(APPEND CMAKE_IGNORE_PREFIX_PATH "/opt/homebrew" "/usr/local/lib")
list(APPEND CMAKE_IGNORE_PREFIX_PATH "/opt/homebrew" "/opt/homebrew/lib" "/usr/local" "/usr/local/lib")

Per previous comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcm93 jcm93 jcm93 left review comments

Assignees
No one assigned
Labels
Bug Fix Non-breaking change which fixes an issue macOS Affects macOS Seeking Testers Build artifacts on CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PatTheMav @jcm93