-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Respect user-set CMAKE_PREFIX_PATH #61904
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
Conversation
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 114f674 (more details on the Dr. CI page):
1 failure not recognized by patterns:
1 job timed out:
🚧 1 fixed upstream failure:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
@cbalioglu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
4b9eb36
to
3374d7a
Compare
@cbalioglu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
3374d7a
to
fd0bfab
Compare
@cbalioglu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@cbalioglu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
tools/setup_helpers/cmake.py
Outdated
cmake_prefix_path = cast(str, build_options.get('CMAKE_PREFIX_PATH', None)) | ||
if cmake_prefix_path: | ||
build_options["CMAKE_PREFIX_PATH"] = f'{py_lib_path};{cmake_prefix_path}' | ||
else: | ||
build_options['CMAKE_PREFIX_PATH'] = py_lib_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this can simplify right?
cmake_prefix_path = cast(str, build_options.get('CMAKE_PREFIX_PATH', None)) | |
if cmake_prefix_path: | |
build_options["CMAKE_PREFIX_PATH"] = f'{py_lib_path};{cmake_prefix_path}' | |
else: | |
build_options['CMAKE_PREFIX_PATH'] = py_lib_path | |
cmake_prefix_path = cast(str, build_options.get('CMAKE_PREFIX_PATH', '')) | |
build_options["CMAKE_PREFIX_PATH"] = f'{py_lib_path};{cmake_prefix_path}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, technically they are equivalent. I just wanted the final value of CMAKE_PREFIX_PATH
to be more "pedantic". For someone checking out CMakeCache.txt later, seeing a CMAKE_PREFIX_PATH
that ends with a semicolon might give suspicion about whether it was intentional or whether there was a bug in the build system that overwrote/skipped something.
e69ab7f
to
114f674
Compare
@cbalioglu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
@cbalioglu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@cbalioglu merged this pull request in e711b5c. |
Summary: Fixes the case where the `CMAKE_PREFIX_PATH` variable gets silently overwritten by a user specified environment variable. Pull Request resolved: #61904 Reviewed By: walterddr, malfet Differential Revision: D29792014 Pulled By: cbalioglu fbshipit-source-id: babacc8d5a1490bff1e14247850cc00c6ba9e6be
Fixes the case where the
CMAKE_PREFIX_PATH
variable gets silently overwritten by a user specified environment variable.