Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
10 changes: 9 additions & 1 deletion src/fromager/bootstrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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}")
Expand Down
10 changes: 9 additions & 1 deletion src/fromager/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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],
Expand Down
5 changes: 5 additions & 0 deletions src/fromager/dependency_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not that it does any harm, but the comment describes pretty self-explanatory code, while not adding any important context/information in the same fashion the other comments do. There is no (non-obvious) "why" explained.

for edge in start_edges:
if edge.key in visited:
continue
Expand Down Expand Up @@ -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.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This feels like it could have been a docstring instead, if you want to keep it in? Othervise, also feels that it does not add much value other than describing "what" code does (and we have the code to describe what it does already :)).

stack = [ROOT]
visited = set()
while stack:
Expand Down
8 changes: 7 additions & 1 deletion src/fromager/external_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
24 changes: 24 additions & 0 deletions src/fromager/finders.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 ``<base>/<base>/`` (double-nested)
3. Fall back to four naming conventions with case-insensitive matching,
returning ``<match>/<match>/`` (same double-nested convention)
"""
sdir_name_func = overrides.find_override_method(
req.name, "expected_source_directory_name"
)
Expand Down
6 changes: 6 additions & 0 deletions src/fromager/pyproject.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]] = {
Expand Down
2 changes: 2 additions & 0 deletions src/fromager/requirements_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
12 changes: 11 additions & 1 deletion src/fromager/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How about we make this a docstring instead?

# 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):
Expand Down
13 changes: 13 additions & 0 deletions src/fromager/sources.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion src/fromager/vendor_rust.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions src/fromager/wheels.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
Loading