From 3c2ad0f26b479cfb2b73206f76602c8b13ead140 Mon Sep 17 00:00:00 2001 From: Lalatendu Mohanty Date: Sun, 12 Apr 2026 15:34:29 -0400 Subject: [PATCH] docs: add docstrings and comments to complex functions Add docstrings to functions and inline comments to non-obvious code patterns across various files. Only targets genuinely complex logic that would be hard for new contributors to understand. Closes: #1046 Co-Authored-By: Claude Opus 4.6 Signed-off-by: Lalatendu Mohanty --- src/fromager/bootstrapper.py | 10 +++++++++- src/fromager/dependencies.py | 10 +++++++++- src/fromager/dependency_graph.py | 5 +++++ src/fromager/external_commands.py | 8 +++++++- src/fromager/finders.py | 24 ++++++++++++++++++++++++ src/fromager/pyproject.py | 6 ++++++ src/fromager/requirements_file.py | 2 ++ src/fromager/resolver.py | 12 +++++++++++- src/fromager/sources.py | 13 +++++++++++++ src/fromager/vendor_rust.py | 2 +- src/fromager/wheels.py | 13 +++++++++++++ 11 files changed, 100 insertions(+), 5 deletions(-) diff --git a/src/fromager/bootstrapper.py b/src/fromager/bootstrapper.py index 1dff6de0..d582352c 100644 --- a/src/fromager/bootstrapper.py +++ b/src/fromager/bootstrapper.py @@ -1078,6 +1078,9 @@ def _look_for_existing_wheel( if not wheel_filename: return None, None + # Re-validate build tag from the actual wheel metadata because + # finders.find_wheel matches by filename prefix, which may not + # enforce an exact build tag match. _, _, build_tag, _ = wheels.extract_info_from_wheel_file(req, wheel_filename) if expected_build_tag and expected_build_tag != build_tag: logger.info( @@ -1181,7 +1184,12 @@ def _unpack_metadata_from_wheel( return None def _resolve_version_from_git_url(self, req: Requirement) -> tuple[str, Version]: - "Return path to the cloned git repository and the package version." + """Resolve source path and version from a ``git+`` URL. + + Parses the URL for an ``@ref`` version hint. If the ref is a valid + version, reuses an existing clone when possible. Otherwise, clones + the repo and extracts the version from package metadata. + """ if not req.url: raise ValueError(f"unable to resolve from URL with no URL in {req}") diff --git a/src/fromager/dependencies.py b/src/fromager/dependencies.py index aa2e8d96..e7964e4d 100644 --- a/src/fromager/dependencies.py +++ b/src/fromager/dependencies.py @@ -81,6 +81,8 @@ def _filter_requirements( for r in requirements: if not isinstance(r, Requirement): r = Requirement(r) + # Use the parent's extras for marker evaluation because PEP 508 + # markers like `extra == "foo"` refer to the parent's install extras. if requirements_file.evaluate_marker(req, r, req.extras): requires.add(r) else: @@ -529,7 +531,13 @@ def get_build_backend_hook_caller( build_env: build_environment.BuildEnvironment, log_filename: str | None = None, ) -> pyproject_hooks.BuildBackendHookCaller: - """Create pyproject_hooks build backend caller""" + """Create a ``BuildBackendHookCaller`` with merged environment variables. + + Environment variables are merged from three sources, where later + sources override earlier ones: hook-provided ``extra_environ``, + package-specific ``override_environ``, and ``build_env`` virtualenv + variables. + """ def _run_hook_with_extra_environ( cmd: typing.Sequence[str], diff --git a/src/fromager/dependency_graph.py b/src/fromager/dependency_graph.py index 8f05be99..b4612867 100644 --- a/src/fromager/dependency_graph.py +++ b/src/fromager/dependency_graph.py @@ -169,6 +169,8 @@ def _traverse_install_requirements( start_edges: list[DependencyEdge], visited: set[str], ) -> typing.Iterable[DependencyEdge]: + # Depth-first walk of install requirement edges, skipping nodes + # already visited to avoid loops in the dependency graph. for edge in start_edges: if edge.key in visited: continue @@ -220,6 +222,9 @@ def from_dict( graph_dict: dict[str, dict[str, typing.Any]], ) -> DependencyGraph: graph = cls() + # Stack-based DFS to reconstruct graph from serialized dict, + # skipping nodes already visited to avoid processing shared + # dependencies twice. stack = [ROOT] visited = set() while stack: diff --git a/src/fromager/external_commands.py b/src/fromager/external_commands.py index e190bcb5..6c4a2607 100644 --- a/src/fromager/external_commands.py +++ b/src/fromager/external_commands.py @@ -58,7 +58,13 @@ def run( log_filename: str | None = None, stdin: TextIOWrapper | None = None, ) -> str: - """Call the subprocess while logging output""" + """Run a subprocess with optional network isolation and structured logging. + + Captures output to a log file or in-memory pipe and prefixes each + line with the current package name for easier searching. Raises + ``NetworkIsolationError`` instead of ``CalledProcessError`` when the + failure output indicates a network access problem. + """ if extra_environ is None: extra_environ = {} env = os.environ.copy() diff --git a/src/fromager/finders.py b/src/fromager/finders.py index 80ffa8da..83ddcca5 100644 --- a/src/fromager/finders.py +++ b/src/fromager/finders.py @@ -40,6 +40,14 @@ def find_sdist( req: Requirement, dist_version: str, ) -> pathlib.Path | None: + """Find an sdist archive in downloads_dir for the given requirement. + + First checks for a plugin-provided exact filename. If no plugin match, + tries four naming conventions (underscore-normalized, canonical, original, + and dotted) with case-insensitive comparison to handle inconsistent + sdist naming across the Python ecosystem. Case-insensitive globbing + is not available before Python 3.12. + """ sdist_file_name = overrides.find_and_invoke( req.name, "expected_source_archive_name", @@ -95,6 +103,13 @@ def find_wheel( dist_version: str, build_tag: BuildTag = (), ) -> pathlib.Path | None: + """Find a wheel file in downloads_dir for the given requirement. + + Tries four naming conventions (PEP 427 transformed, canonical, original, + and dotted), each suffixed with the build tag when present. Uses + case-insensitive ``startswith`` matching rather than exact base comparison + because wheel filenames include platform/Python tags after the version. + """ filename_prefix = _dist_name_to_filename(req.name) canonical_name = canonicalize_name(req.name) # if build tag is 0 then we can ignore to handle non tagged wheels for backward compatibility @@ -140,6 +155,15 @@ def find_source_dir( req: Requirement, dist_version: str, ) -> pathlib.Path | None: + """Find the unpacked source directory for a requirement. + + Tries three strategies in order: + 1. Plugin override providing the exact directory name + 2. Derive from the sdist archive name (strip extension), assuming the + unpacked layout is ``//`` (double-nested) + 3. Fall back to four naming conventions with case-insensitive matching, + returning ``//`` (same double-nested convention) + """ sdir_name_func = overrides.find_override_method( req.name, "expected_source_directory_name" ) diff --git a/src/fromager/pyproject.py b/src/fromager/pyproject.py index 2277bc29..5148f1a4 100644 --- a/src/fromager/pyproject.py +++ b/src/fromager/pyproject.py @@ -93,6 +93,12 @@ def _default_build_system(self, doc: tomlkit.TOMLDocument) -> TomlDict: return build_system def _update_build_requires(self, build_system: TomlDict) -> None: + """Merge, remove, and deduplicate build requirements. + + Starts with setuptools as a default, adds the existing requirements, + removes unwanted ones, then applies updates. Only writes back if + the resulting set differs from the original. + """ old_requires = build_system[BUILD_REQUIRES] # always include setuptools req_map: dict[NormalizedName, list[Requirement]] = { diff --git a/src/fromager/requirements_file.py b/src/fromager/requirements_file.py index 4aede78b..a588d20a 100644 --- a/src/fromager/requirements_file.py +++ b/src/fromager/requirements_file.py @@ -65,6 +65,8 @@ def evaluate_marker( if not extras: marker_envs = [default_env] else: + # Evaluate once per extra because PEP 508 markers like `extra == "foo"` + # can only match one extra value at a time. marker_envs = [default_env.copy() | {"extra": e} for e in extras] for marker_env in marker_envs: diff --git a/src/fromager/resolver.py b/src/fromager/resolver.py index 2ee09115..908cbcf3 100644 --- a/src/fromager/resolver.py +++ b/src/fromager/resolver.py @@ -222,7 +222,13 @@ def get_project_from_pypi( *, override_download_url: str | None = None, ) -> Candidates: - """Return candidates created from the project name and extras.""" + """Fetch and filter package candidates from a PyPI-compatible server. + + Filters the project's package list by: project status, filename + validity, package type (sdist/wheel), yanked status, Python version + compatibility, and platform tags. Can substitute + ``override_download_url`` into each candidate's URL. + """ found_candidates: set[str] = set() ignored_candidates: set[str] = set() logger.debug("%s: getting available versions from %s", project, sdist_server_url) @@ -806,6 +812,10 @@ def cache_key(self) -> str: raise NotImplementedError("GenericProvider does not implement caching") def find_candidates(self, identifier: typing.Any) -> Candidates: + # Accepts three input formats from _version_source: + # 1. Candidate objects (used directly) + # 2. (url, Version) tuples + # 3. (url, str) tuples (version parsed via _match_function) candidates: list[Candidate] = [] version: Version | None for item in self._version_source(identifier): diff --git a/src/fromager/sources.py b/src/fromager/sources.py index 73fec612..8bea4b76 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -295,6 +295,12 @@ def download_url( url: str, destination_filename: str | None = None, ) -> pathlib.Path: + """Download a URL to destination_dir, returning the local path. + + Returns immediately if the file already exists. Downloads to a + temporary file first and renames it on success to avoid leaving + partial files. Retries on transient network errors. + """ basename = ( destination_filename if destination_filename @@ -500,6 +506,13 @@ def prepare_source( source_filename: pathlib.Path, version: Version, ) -> pathlib.Path: + """Unpack and prepare source for building. + + Git URL sources skip the plugin system and are prepared directly. + Non-git sources go through ``find_and_invoke`` which may call a + package-specific override. The plugin may return a ``Path`` or a + ``(Path, bool)`` tuple; both forms are handled. + """ if req.url: logger.info( "preparing source cloned from %s into %s, ignoring any plugins", diff --git a/src/fromager/vendor_rust.py b/src/fromager/vendor_rust.py index 8039538c..d88268a5 100644 --- a/src/fromager/vendor_rust.py +++ b/src/fromager/vendor_rust.py @@ -56,7 +56,7 @@ def _cargo_shrink(crate_dir: pathlib.Path) -> None: for filename in crate_dir.glob(pattern): logger.debug("removing '%s'", filename.relative_to(crate_dir.parent)) filename.unlink() - filename.touch() # create empty file + filename.touch() # keep empty placeholder so cargo doesn't error on missing files removed_files.append(str(filename.relative_to(crate_dir))) # update checksums diff --git a/src/fromager/wheels.py b/src/fromager/wheels.py index fa13d8a2..5aaf20b5 100644 --- a/src/fromager/wheels.py +++ b/src/fromager/wheels.py @@ -139,6 +139,13 @@ def _extra_metadata_elfdeps( def extract_info_from_wheel_file( req: Requirement, wheel_file: pathlib.Path ) -> tuple[str, Version, BuildTag, frozenset[Tag]]: + """Extract metadata from a wheel filename. + + Returns the **verbatim** dist name (not normalized) because the + dist-info directory inside the wheel uses the original casing + (e.g. ``MarkupSafe``, not ``markupsafe``). Uses ``parse_wheel_filename`` + for validation and to extract version, build tag, and platform tags. + """ # parse_wheel_filename normalizes the dist name, however the dist-info # directory uses the verbatim distribution name from the wheel file. # Packages with upper case names like "MarkupSafe" are affected. @@ -174,6 +181,12 @@ def add_extra_metadata_to_wheels( sdist_root_dir: pathlib.Path, wheel_file: pathlib.Path, ) -> pathlib.Path: + """Unpack a wheel, inject extra metadata, and repack with a build tag. + + Unpacks the wheel, validates dist-info, adds plugin metadata, build + settings, requirement files, ELF dependency info (Linux only), and + an SBOM. Repacks with ``wheel pack`` and deletes the original. + """ pbi = ctx.package_build_info(req) dist_name, dist_version, _, wheel_tags = extract_info_from_wheel_file( req, wheel_file