fix: improve libclang discovery on macOS and Linux - #67
Draft
henryiii wants to merge 1 commit into
Draft
Conversation
…TH on Linux Addresses findings 3-5 from the code review in #59: - Linux: LIBCLANG_PATH is now consulted before raising when no /usr/lib*/llvm-* directory is found; llvm_dir-derived include paths are skipped when llvm_dir is unknown. - macOS: honor LIBCLANG_PATH, and fall back to the Command Line Tools location when Xcode.app is absent (both libclang and the SDK dir). - macOS: SDK selection is now deterministic, preferring MacOSX.sdk and otherwise the newest version, instead of os.walk ordering. Assisted-by: ClaudeCode:claude-fable-5
There was a problem hiding this comment.
Pull request overview
Improves read_args() libclang discovery to better support non-standard setups on Linux and macOS (especially Command Line Tools-only macOS installs), and adds unit tests to cover the new discovery branches.
Changes:
- Honor
LIBCLANG_PATHon macOS (with validation) and add CLT (/Library/Developer/CommandLineTools) fallbacks for both libclang and SDK selection. - Honor
LIBCLANG_PATHon Linux even when no/usr/lib*/llvm-*installation is discoverable, and avoid deriving include paths from an unknownllvm_dir. - Add focused unit tests that mock platform detection, filesystem probes, and SDK directory enumeration to cover the new branches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pybind11_mkdoc/mkdoc_lib.py |
Updates macOS/Linux libclang discovery logic, adds deterministic SDK selection, and refactors version-parsing helper. |
tests/read_args_test.py |
Adds unit tests to validate new discovery behavior across Linux and macOS branches via monkeypatching/mocking. |
Suppressed comments (1)
pybind11_mkdoc/mkdoc_lib.py:650
- On Linux, LIBCLANG_PATH is now honored, but the path is not validated before calling cindex.Config.set_library_file(). If the env var points to a non-existent file (or a directory), the failure will surface later as a libclang load error that’s harder to diagnose. Windows/Darwin already validate the file path, so Linux should do the same for consistency and clearer errors.
if "LIBCLANG_PATH" in os.environ:
libclang_file = os.environ["LIBCLANG_PATH"]
elif llvm_dir is not None:
libclang_file = os.path.join(llvm_dir, "lib", "libclang.so.1")
else:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+583
to
+587
| if not os.path.isfile(library_file): | ||
| msg = ( | ||
| "Failed to find libclang.dylib! Set the LIBCLANG_PATH environment variable to provide a path to it." | ||
| ) | ||
| raise FileNotFoundError(msg) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Addresses findings 3, 4, and 5 from the code review in #59 (the issue has more items, so this does not close it).
LIBCLANG_PATHis now honored even when no/usr/lib*/llvm-*directory exists; before, aFileNotFoundErrorwas raised that told the user to set the variable it never read. Include paths derived fromllvm_dirare skipped when it is unknown.LIBCLANG_PATHis honored, and the Command Line Tools location (/Library/Developer/CommandLineTools) is used as a fallback for both libclang and the SDK when Xcode.app is absent.MacOSX.sdk, else the newest version by numeric sort, instead of the firstos.walkentry.The
cindex.Config.loadedguards from #60 are preserved. Unit tests mock the discovery branches; on a CLT-only arm64 Mac the previously failing suite now passes without any workaround.