-
Notifications
You must be signed in to change notification settings - Fork 306
fix(pathfinder): remove dead/misleading error handling in platform loaders #2239
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -165,17 +165,19 @@ def load_with_system_search(desc: LibDescriptor) -> LoadedDL | None: | |
| A LoadedDL object if successful, None if the library cannot be loaded | ||
|
|
||
| Raises: | ||
| RuntimeError: If the library is loaded but no expected symbol is found | ||
| OSError: If the library is loaded but its absolute path cannot be | ||
| resolved via dlinfo (surfaced from abs_path_for_dynamic_library). | ||
| """ | ||
| for soname in _candidate_sonames(desc): | ||
| try: | ||
| handle = _load_lib(desc, soname) | ||
| except OSError: | ||
| pass | ||
| else: | ||
| # abs_path_for_dynamic_library never returns None: it returns a | ||
| # resolved path or raises OSError. Let that error surface rather | ||
| # than masking it, consistent with the deterministic-loader policy. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment explains behavior elsewhere. It could easily drift. Please remove. |
||
| abs_path = abs_path_for_dynamic_library(desc.name, handle) | ||
| if abs_path is None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will make the expectation explicit and obvious, without risking drift. |
||
| raise RuntimeError(f"No expected symbol for libname={desc.name!r}") | ||
| return LoadedDL(abs_path, False, handle._handle, "system-search") | ||
| return None | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,11 +69,10 @@ def add_dll_directory(dll_abs_path: str) -> None: | |
| dirpath = os.path.dirname(dll_abs_path) | ||
| assert os.path.isdir(dirpath), dll_abs_path | ||
|
|
||
| # Add the DLL directory to the search path | ||
| result = kernel32.AddDllDirectory(dirpath) | ||
| if not result: | ||
| # Fallback: just update PATH if AddDllDirectory fails | ||
| pass | ||
| # Add the DLL directory to the native search path. AddDllDirectory only | ||
| # affects the LOAD_LIBRARY_SEARCH_USER_DIRS search; PATH is updated | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a remark: I'm not sure if there is actually any situation where PATH is still searched on Windows. Modern Python versions disabled that AFAIK, but I don't know the exact version details. I.e. the change here looks good as-is for the purpose of this PR. |
||
| # unconditionally below to also cover legacy dependent-DLL resolution. | ||
| kernel32.AddDllDirectory(dirpath) | ||
|
|
||
| # Update PATH as a fallback for dependent DLL resolution | ||
| curr_path = os.environ.get("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.
I'd rather delete the entire Raises section: this is documenting behavior elsewhere / risks drift. It's not a public API. agents will figure this out anyway, and might even get distracted by this part of the docstring.