Repository navigation
feat(settings): support explicit per-version wheel build numbers #1391
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 all commits
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import contextlib | ||||||||||||||||||||
| import json | ||||||||||||||||||||
| import logging | ||||||||||||||||||||
| import os | ||||||||||||||||||||
|
|
@@ -8,13 +9,17 @@ | |||||||||||||||||||
| import re | ||||||||||||||||||||
| import subprocess | ||||||||||||||||||||
| import sys | ||||||||||||||||||||
| import tempfile | ||||||||||||||||||||
| import typing | ||||||||||||||||||||
| from email.parser import Parser | ||||||||||||||||||||
| from io import TextIOWrapper | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from packaging.requirements import Requirement | ||||||||||||||||||||
| from packaging.tags import Tag, parse_tag | ||||||||||||||||||||
| from packaging.utils import BuildTag, NormalizedName, parse_wheel_filename | ||||||||||||||||||||
| from packaging.version import Version | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from . import dependencies, external_commands, metrics, resolver | ||||||||||||||||||||
| from . import dependencies, external_commands, metrics, resolver, wheels | ||||||||||||||||||||
| from .requirements_file import RequirementType | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if typing.TYPE_CHECKING: | ||||||||||||||||||||
|
|
@@ -37,6 +42,53 @@ | |||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| class _InstallationWheelView: | ||||||||||||||||||||
| """Reuse links within one worker, refreshing selection before each install.""" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__(self, work_dir: pathlib.Path) -> None: | ||||||||||||||||||||
| self._temporary = tempfile.TemporaryDirectory( | ||||||||||||||||||||
| prefix="install-wheels-", dir=work_dir | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| self.path = pathlib.Path(self._temporary.name) | ||||||||||||||||||||
| self._links: dict[str, pathlib.Path] = {} | ||||||||||||||||||||
| self._identities: dict[ | ||||||||||||||||||||
| str, tuple[NormalizedName, Version, BuildTag, frozenset[Tag]] | ||||||||||||||||||||
| ] = {} | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def update(self, ctx: context.WorkContext) -> None: | ||||||||||||||||||||
| """Refresh changed links without caching settings or hook results.""" | ||||||||||||||||||||
| desired: dict[str, pathlib.Path] = {} | ||||||||||||||||||||
| seen: set[str] = set() | ||||||||||||||||||||
| for source in (ctx.wheels_downloads, ctx.wheels_prebuilt): | ||||||||||||||||||||
| for path in sorted(source.glob("*.whl")): | ||||||||||||||||||||
| seen.add(path.name) | ||||||||||||||||||||
| identity = self._identities.get(path.name) | ||||||||||||||||||||
| if identity is None: | ||||||||||||||||||||
| identity = parse_wheel_filename(path.name) | ||||||||||||||||||||
| self._identities[path.name] = identity | ||||||||||||||||||||
| name, version, actual_tag, wheel_tags = identity | ||||||||||||||||||||
| pbi = ctx.package_build_info(name) | ||||||||||||||||||||
| if not pbi.pre_built and pbi.get_build_number(version) is not None: | ||||||||||||||||||||
| expected = wheels.get_build_tag( | ||||||||||||||||||||
| ctx=ctx, | ||||||||||||||||||||
| req=Requirement(name), | ||||||||||||||||||||
| version=version, | ||||||||||||||||||||
| wheel_tags=wheel_tags, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| if actual_tag != expected: | ||||||||||||||||||||
| continue | ||||||||||||||||||||
| desired.setdefault(path.name, path) | ||||||||||||||||||||
| for filename, target in self._links.items(): | ||||||||||||||||||||
| if desired.get(filename) != target: | ||||||||||||||||||||
| (self.path / filename).unlink() | ||||||||||||||||||||
| for filename, target in desired.items(): | ||||||||||||||||||||
| if self._links.get(filename) != target: | ||||||||||||||||||||
| (self.path / filename).symlink_to(target) | ||||||||||||||||||||
| self._links = desired | ||||||||||||||||||||
| for filename in self._identities.keys() - seen: | ||||||||||||||||||||
| del self._identities[filename] | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| class MissingDependency(Exception): # noqa: N818 | ||||||||||||||||||||
| def __init__( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
|
|
@@ -212,6 +264,30 @@ def _createenv(self) -> None: | |||||||||||||||||||
| ) | ||||||||||||||||||||
| logger.info("created build environment in %s", self.path) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @contextlib.contextmanager | ||||||||||||||||||||
| def _installation_wheel_args(self, explicit: bool) -> typing.Iterator[list[str]]: | ||||||||||||||||||||
| """Expose permitted local builds, or preserve legacy source arguments.""" | ||||||||||||||||||||
| if not explicit: | ||||||||||||||||||||
| yield self._ctx.pip_wheel_server_args | ||||||||||||||||||||
| return | ||||||||||||||||||||
|
|
||||||||||||||||||||
| view: _InstallationWheelView | None = getattr( | ||||||||||||||||||||
| self._ctx._installation_wheel_views, "view", None | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| if view is None: | ||||||||||||||||||||
| logger.info( | ||||||||||||||||||||
| "Explicit build numbers in loaded settings enable local-only " | ||||||||||||||||||||
| "dependency installation for all packages, including unrelated " | ||||||||||||||||||||
| "versions. --wheel-server-url is not used for installation; " | ||||||||||||||||||||
| "all required wheels must be available locally." | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| view = _InstallationWheelView(self._ctx.work_dir) | ||||||||||||||||||||
| self._ctx._installation_wheel_views.view = view | ||||||||||||||||||||
| view.update(self._ctx) | ||||||||||||||||||||
| # uv treats build tags as preferences, not constraints. Keep other | ||||||||||||||||||||
| # builds out of its candidate set, including its installed/cache state. | ||||||||||||||||||||
| yield ["--no-index", "--offline", "--find-links", str(view.path), "--reinstall"] | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def install(self, reqs: typing.Iterable[Requirement]) -> None: | ||||||||||||||||||||
| if not reqs: | ||||||||||||||||||||
| return | ||||||||||||||||||||
|
|
@@ -227,20 +303,48 @@ def install(self, reqs: typing.Iterable[Requirement]) -> None: | |||||||||||||||||||
| ":all:", | ||||||||||||||||||||
| ] | ||||||||||||||||||||
| cmd.extend(self._ctx.pip_constraint_args) | ||||||||||||||||||||
| cmd.extend(self._ctx.pip_wheel_server_args) | ||||||||||||||||||||
| cmd.extend(str(req) for req in reqs) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| self.run( | ||||||||||||||||||||
| cmd, | ||||||||||||||||||||
| cwd=str(self.path.parent), | ||||||||||||||||||||
| network_isolation=False, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| explicit = self._ctx.settings.has_explicit_build_numbers | ||||||||||||||||||||
| with self._installation_wheel_args(explicit) as source_args: | ||||||||||||||||||||
| cmd.extend(source_args) | ||||||||||||||||||||
| cmd.extend(str(req) for req in reqs) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| self.run( | ||||||||||||||||||||
| cmd, | ||||||||||||||||||||
| cwd=str(self.path.parent), | ||||||||||||||||||||
| network_isolation=False, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| if explicit: | ||||||||||||||||||||
| self._validate_installed_build_numbers() | ||||||||||||||||||||
| logger.info( | ||||||||||||||||||||
| "installed dependencies %s into build environment in %s", | ||||||||||||||||||||
| reqs, | ||||||||||||||||||||
| self.path, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def _validate_installed_build_numbers(self) -> None: | ||||||||||||||||||||
|
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. [Nit] — The new validation re-implements the existing The inline One small helper returning
Suggested change
Member
Author
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. See my reply on the installation policy. I’m leaning toward deferring exact dependency-installation enforcement, which would remove
Member
Author
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. Implemented the shared |
||||||||||||||||||||
| """Fail before compilation if direct references bypass the filtered source.""" | ||||||||||||||||||||
| for name, raw_version, raw_wheel in self._installed_distributions(): | ||||||||||||||||||||
| version = Version(raw_version) | ||||||||||||||||||||
| pbi = self._ctx.package_build_info(name) | ||||||||||||||||||||
| if pbi.pre_built or pbi.get_build_number(version) is None: | ||||||||||||||||||||
| continue | ||||||||||||||||||||
| metadata = Parser().parsestr(raw_wheel or "") | ||||||||||||||||||||
| tags = frozenset( | ||||||||||||||||||||
| tag for value in metadata.get_all("Tag", []) for tag in parse_tag(value) | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| expected = wheels.get_build_tag( | ||||||||||||||||||||
| ctx=self._ctx, req=Requirement(name), version=version, wheel_tags=tags | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| assert expected # Explicit counters, including zero, are non-empty. | ||||||||||||||||||||
| expected_text = f"{expected[0]}{expected[1]}" | ||||||||||||||||||||
| match = re.fullmatch(r"([0-9]+)(.*)", metadata.get("Build") or "") | ||||||||||||||||||||
| actual: BuildTag = (int(match[1]), match[2]) if match else () | ||||||||||||||||||||
| if not tags or actual != expected: | ||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||
| f"{name}=={version}: installed wheel build {metadata.get('Build')!r} " | ||||||||||||||||||||
| f"does not match explicit build {expected_text!r}" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_sysconfig_path(self, name: str) -> str: | ||||||||||||||||||||
| """Get ``sysconfig.get_path(name)`` in environment""" | ||||||||||||||||||||
| if not name.isidentifier(): | ||||||||||||||||||||
|
|
@@ -255,18 +359,25 @@ def get_sysconfig_path(self, name: str) -> str: | |||||||||||||||||||
|
|
||||||||||||||||||||
| def get_distributions(self) -> typing.Mapping[str, Version]: | ||||||||||||||||||||
| """Get a mapping of installed package names and their versions in the build env""" | ||||||||||||||||||||
| lines = [ | ||||||||||||||||||||
| "import sys,json", | ||||||||||||||||||||
| "from importlib.metadata import distributions", | ||||||||||||||||||||
| "json.dump({d.name: d.version for d in distributions()}, sys.stdout)", | ||||||||||||||||||||
| ] | ||||||||||||||||||||
| result = self.run([str(self.python), "-c", "; ".join(lines)]) | ||||||||||||||||||||
| mapping = { | ||||||||||||||||||||
| name: Version(version) | ||||||||||||||||||||
| for name, version, _ in self._installed_distributions() | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return dict(sorted(mapping.items())) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def _installed_distributions(self) -> list[tuple[str, str, str | None]]: | ||||||||||||||||||||
| """Read distribution identities and wheel metadata in the build env.""" | ||||||||||||||||||||
| script = ( | ||||||||||||||||||||
| "import json,sys; from importlib.metadata import distributions; " | ||||||||||||||||||||
| "json.dump([(d.name, d.version, d.read_text('WHEEL')) " | ||||||||||||||||||||
| "for d in distributions()], sys.stdout)" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| result = self.run([str(self.python), "-c", script]) | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| mapping = json.loads(result.strip()) | ||||||||||||||||||||
| except Exception: | ||||||||||||||||||||
| return typing.cast(list[tuple[str, str, str | None]], json.loads(result)) | ||||||||||||||||||||
| except json.JSONDecodeError: | ||||||||||||||||||||
| logger.exception("failed to de-serialize JSON: %s", result) | ||||||||||||||||||||
| raise | ||||||||||||||||||||
| return {name: Version(version) for name, version in sorted(mapping.items())} | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| @metrics.timeit(description="prepare build environment") | ||||||||||||||||||||
|
|
||||||||||||||||||||
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.
[Nit] —
docs/customization.md(line 454,build_tag_hooknotes) andsrc/fromager/build_environment.py(lines 232, 305)The hook is now sometimes called with a fake, bare requirement — but the docs still imply it always gets the real one.
The new install-time filtering and post-install validation call
wheels.get_build_tag(req=Requirement(name), ...)— a requirement reconstructed from just the package name, no extras, no marker, no URL. Meanwhile the build and cache paths pass the real requirement from the resolve graph.Why that matters: a
build_tag_hookthat looks at anything beyond the name (say,req.extrasorreq.marker) will compute a different expected tag during install-time filtering than during the actual build — so wheels get filtered out or installs rejected for no visible reason. The docs say the hook must be "deterministic and independent of wheel contents, build environment, or ELF metadata" — which a hook readingreq.extrastechnically satisfies today, yet it would still break under this PR.The bare requirement is inherent to the design (those code paths iterate all local wheels / all installed dists, so there is no real requirement to thread through). Fine — but then the tightened contract should be written down. One extra bullet in the "Important notes" list:
wheel_tags: during dependency-installation filtering, Fromager re-evaluates the hook with a bare requirement containing just the name.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.
This also ties into my reply on the installation policy. Deferring exact dependency-installation enforcement would remove these installation-time hook calls with bare requirements. If we retain enforcement, this hook-contract question still needs addressing.
Uh oh!
There was an error while loading. Please reload this page.
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’ve documented the narrower contract for explicit-counter installation checks: the suffix must use only the name, version, variant, and
wheel_tags, within the existing compatibility-tag restrictions. Those calls lack the original extras, markers, specifier, and URL.This remains a design question in #1392: accept that restriction for enforcement, or preserve the original requirement context. The generation/cache-only option would avoid adding these installation-time hook calls.