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

formula: set CMAKE_PROGRAM_PATH to use shims #18742

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Nov 9, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

We probably need to keep CMAKE_PREFIX_PATH environment variable as it is cleanest way of handling nested and indirect cmake usage.

See https://cmake.org/cmake/help/latest/command/find_program.html for details on search order,

  • CMAKE_PROGRAM_PATH cache variable is 2nd
  • CMAKE_PREFIX_PATH environment variable is 3rd
  • PATH is 5th

An alternative could be using more specific alternatives to CMAKE_PREFIX_PATH. There are a number of things the variable impacts, e.g.

  • find_package - probably main use we want. We could consider scanning deps for CMake files and using <PackageName>_DIR environment variables instead though may need to be careful to avoid overriding CLT and make sure case-sensitive name.
  • find_program - I would guess we mainly want to find these via PATH
  • find_library - we set CMAKE_LIBRARY_PATH/CMAKE_FRAMEWORK_PATH already so may cover many cases
  • find_file - ? CMAKE_INCLUDE_PATH could cover some of this
  • find_path - ? CMAKE_INCLUDE_PATH could cover some of this

Comment on lines +1826 to +1827
def std_cmake_args(install_prefix: prefix, install_libdir: "lib", find_framework: "LAST",
program_path: ENV["PATH"]&.split(File::PATH_SEPARATOR))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def std_cmake_args(install_prefix: prefix, install_libdir: "lib", find_framework: "LAST",
program_path: ENV["PATH"]&.split(File::PATH_SEPARATOR))
def std_cmake_args(install_prefix: prefix, install_libdir: "lib", find_framework: "LAST")

I suggest just using the contents of ENV["PATH"] unconditionally unless we know there are formulae that need to set this.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense. Have been thinking we probably want some more CMake handling like this

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.

3 participants