Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Copy link
Copy Markdown
Contributor

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.

"""
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

            assert abs_path

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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")
Expand Down
Loading