-
Notifications
You must be signed in to change notification settings - Fork 53
feat(wheels): add configurable build tag hook for wheel filenames #1273
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 |
|---|---|---|
|
|
@@ -485,11 +485,25 @@ def _is_wheel_built( | |
| wheel_server_urls=wheel_server_urls, | ||
| ) | ||
| logger.info("found candidate wheel %s", url) | ||
| pbi = wkctx.package_build_info(req) | ||
| build_tag_from_settings = pbi.build_tag(resolved_version) | ||
| build_tag = build_tag_from_settings if build_tag_from_settings else (0, "") | ||
| wheel_basename = downloads.extract_filename_from_url(url) | ||
| _, _, build_tag_from_name, _ = parse_wheel_filename(wheel_basename) | ||
| _, _, build_tag_from_name, wheel_tags = parse_wheel_filename(wheel_basename) | ||
|
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. url, wheel_basename, build_tag_from_name, and wheel_tags are assigned inside the try block and used after the except. The except block returns None, so the code is technically correct, but the flow is fragile. An else clause on the try would make the intent explicit and survive future refactoring more safely: |
||
| except Exception: | ||
| logger.debug( | ||
| "could not locate prebuilt wheel %s-%s on %s", | ||
| dist_name, | ||
| resolved_version, | ||
| wheel_server_urls, | ||
| exc_info=True, | ||
| ) | ||
| logger.info("could not locate prebuilt wheel") | ||
| return None | ||
| else: | ||
| # Compute expected build tag in the else clause so hook | ||
| # validation errors propagate instead of being swallowed. | ||
| expected_tag = wheels.get_build_tag( | ||
| ctx=wkctx, req=req, version=resolved_version, wheel_tags=wheel_tags | ||
| ) | ||
| build_tag = expected_tag if expected_tag else (0, "") | ||
| existing_build_tag = build_tag_from_name if build_tag_from_name else (0, "") | ||
| if ( | ||
| existing_build_tag[0] > build_tag[0] | ||
|
|
@@ -513,21 +527,10 @@ def _is_wheel_built( | |
| wheel_filename = None | ||
|
|
||
| if not wheel_filename: | ||
| # if the found wheel was on an external server, then download it | ||
| logger.info("downloading wheel from %s", url) | ||
| wheel_filename = wheels.download_wheel(req, url, wkctx.wheels_downloads) | ||
|
|
||
| return wheel_filename | ||
| except Exception: | ||
| logger.debug( | ||
| "could not locate prebuilt wheel %s-%s on %s", | ||
| dist_name, | ||
| resolved_version, | ||
| wheel_server_urls, | ||
| exc_info=True, | ||
| ) | ||
| logger.info("could not locate prebuilt wheel") | ||
| return None | ||
|
|
||
|
|
||
| def _build_parallel( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| import logging | ||
| import os | ||
| import pathlib | ||
| import re | ||
| import shutil | ||
| import sys | ||
| import tempfile | ||
|
|
@@ -38,12 +39,67 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| _BUILD_TAG_SEGMENT_RE = re.compile(r"^[a-zA-Z0-9.]+$") | ||
|
|
||
| FROMAGER_BUILD_SETTINGS = "fromager-build-settings" | ||
| FROMAGER_ELF_PROVIDES = "fromager-elf-provides.txt" | ||
| FROMAGER_ELF_REQUIRES = "fromager-elf-requires.txt" | ||
| FROMAGER_BUILD_REQ_PREFIX = "fromager" | ||
|
|
||
|
|
||
| def _validate_build_tag_segments(segments: list[str]) -> None: | ||
| """Validate that each segment matches ``[a-zA-Z0-9.]``.""" | ||
| for seg in segments: | ||
| if not isinstance(seg, str): | ||
| raise ValueError( | ||
| f"build_tag_hook must return strings, got {type(seg).__name__}" | ||
| ) | ||
| if not _BUILD_TAG_SEGMENT_RE.match(seg): | ||
| raise ValueError( | ||
| f"build tag hook returned invalid segment {seg!r}: " | ||
| "each segment must match [a-zA-Z0-9.]" | ||
| ) | ||
|
|
||
|
|
||
| def get_build_tag( | ||
| *, | ||
| ctx: context.WorkContext, | ||
| req: Requirement, | ||
| version: Version, | ||
| wheel_tags: frozenset[Tag], | ||
| ) -> BuildTag: | ||
| """Compute the full build tag including any hook-provided suffix. | ||
|
|
||
| Calls ``pbi.build_tag(version)`` for the numeric base, then invokes | ||
| the configured ``build_tag_hook`` (if any) to append environment | ||
| suffix segments. | ||
|
|
||
| .. versionadded:: 0.93.0 | ||
| """ | ||
| pbi = ctx.package_build_info(req) | ||
| base_tag = pbi.build_tag(version) | ||
| if not base_tag: | ||
| return base_tag | ||
|
|
||
| hook = ctx.settings.build_tag_hook | ||
| if hook is None: | ||
| return base_tag | ||
|
|
||
| raw = hook(ctx=ctx, req=req, version=version, wheel_tags=wheel_tags) | ||
| if isinstance(raw, str | bytes): | ||
|
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. Can we add a test to verify bytes are also rejected? Currently, we only have test for |
||
| raise ValueError( | ||
| "build_tag_hook must return a sequence of strings, not a single string" | ||
| ) | ||
| segments = list(raw) | ||
| _validate_build_tag_segments(segments) | ||
|
|
||
| if not segments: | ||
| return base_tag | ||
|
|
||
| suffix = base_tag[1] + "_" + "_".join(segments) | ||
| return (base_tag[0], suffix) | ||
|
|
||
|
|
||
| def _log_existing_sboms( | ||
| req: Requirement, | ||
| dist_info_dir: pathlib.Path, | ||
|
|
@@ -264,8 +320,11 @@ def add_extra_metadata_to_wheels( | |
| ) | ||
| sbom.write_sbom(sbom=sbom_doc, dist_info_dir=dist_info_dir) | ||
|
|
||
| build_tag_from_settings = pbi.build_tag(version) | ||
| build_tag = build_tag_from_settings if build_tag_from_settings else (0, "") | ||
| build_tag = get_build_tag( | ||
| ctx=ctx, req=req, version=version, wheel_tags=wheel_tags | ||
| ) | ||
| if not build_tag: | ||
| build_tag = (0, "") | ||
|
|
||
| cmd = [ | ||
| "wheel", | ||
|
|
||
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.
Is there a reason why we are removing this log line?
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.
My mistake. Will restore the changelog debug logging.