Skip to content

Add thin Shoal compatibility metapackage - #111

Merged
phrocker merged 15 commits into
mainfrom
compat/shoal-metapackage
Aug 26, 2026
Merged

phrocker merged 15 commits into
mainfrom
compat/shoal-metapackage

Conversation

@phrocker

@phrocker phrocker commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • convert the sharkbite distribution to a metadata-only install-name/release bridge
  • use one coordinated PEP 440 version for both projects (1.3.0 selected as unused and above historical sharkbite 1.2.0.3)
  • pin the existing Shoal compatibility implementation exactly (shoal-sharkbite==1.3.0)
  • keep historical implementation source in git while excluding it from wheel and sdist
  • add build, metadata, dependency resolution, file ownership, install, and import smoke tests
  • add protected-environment/OIDC Trusted Publishing with checksums and artifact provenance

Architecture

PyPI sharkbite==VERSION installs no modules or native files. It depends on shoal-sharkbite==VERSION, the sole owner of the existing sharkbite, pysharkbite, and bundled native C ABI implementation. The thin wheel owns only its distinct sharkbite-VERSION.dist-info.

Authoritative migration gate

#108 remains authoritative. This bridge is packaging evidence only and does not complete/displace Shoal compatibility. Revision 56 is 394 required / 390 satisfied. The four live rows remain SB-SCAN-022, SB-SCAN-026, SB-XCUT-015, and SB-XCUT-016.

Validation

  • build and Twine metadata checks
  • strict same-version dependency and metadata-only archive inspection
  • clean resolver/install/import/ownership smoke test plus pip check
  • actionlint 1.7.7
  • release gate verifies Shoal GitHub assets/checksums/attestations, byte identity with PyPI wheel, clean --no-deps implementation install, both imports, bundled native ownership, ABI/capabilities, then the local bridge wheel

Coordinated release ordering and blockers

This PR remains draft. No production release has been published. Merging/publishing it does not close #108.

Convert the sharkbite distribution to metadata-only packaging with an exact Shoal implementation dependency and guarded Trusted Publishing automation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR converts the sharkbite distribution into a metadata-only compatibility package that pins and delegates all installed Python/native artifacts to the Shoal-owned implementation (shoal-sharkbite==0.5.0), while adding verification, smoke testing, and a protected Trusted Publishing release workflow.

Changes:

  • Replace the legacy build/extension setup with a setuptools/PEP 621 metadata-only package (packages = []) that depends exactly on shoal-sharkbite==0.5.0.
  • Add a metapackage verification script plus release policy docs to enforce “no installed files” and release ordering.
  • Add CI workflow coverage for build/twine checks, sdist/wheel ownership validation, offline install/import smoke tests via a local stub implementation, and release-time attestation + PyPI publish.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/stub_implementation/src/sharkbite/init.py Stub sharkbite package for offline smoke testing via a local fixture.
tests/stub_implementation/src/pysharkbite/init.py Stub pysharkbite package mirroring version import behavior for the fixture.
tests/stub_implementation/pyproject.toml Minimal build config for the local shoal-sharkbite fixture wheel.
setup.py Reduced to a minimal shim (setup()) consistent with pyproject-driven builds.
scripts/verify_metapackage.py Adds wheel/sdist verification to ensure the compatibility package installs no non-metadata files and pins exactly one dependency.
RELEASE.md Documents compatibility-package release ordering and ownership/verification requirements.
PYTHONREADME.md Updates Python README to reflect the new compatibility-metapackage architecture.
pyproject.toml Defines the metadata-only sharkbite project and pins shoal-sharkbite==0.5.0.
MANIFEST.in Excludes historical implementation sources from sdists while keeping required release docs/metadata.
.github/workflows/publish-pypi.yml Adds build/verify/smoke-test pipeline plus release-time attestation and Trusted Publishing to PyPI.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/verify_metapackage.py
Validate full archive-relative paths so nested historical files cannot pass based on an allowed basename.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please perform a second substantive code review of the latest commit (146325b), focusing on release safety, package file ownership, dependency/version guarantees, and Trusted Publishing. Ignore the resolved earlier finding.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/verify_metapackage.py:56

  • verify_sdist() only inspects regular files (member.isfile()), so an sdist could still include symlinks/hardlinks (or other special tar entries) without being flagged. For a strict "no historical implementation content" guard, fail the check if any non-file, non-directory members are present.
        files = [
            member.name.split("/", 1)[-1]
            for member in archive.getmembers()
            if member.isfile()
        ]

scripts/verify_metapackage.py:69

  • require_pypi_release() assumes the requirement string is exactly name==version and will generate an invalid PyPI URL if the dependency ever gains extras or environment markers (e.g. ; python_version >= ...). Since this script is enforcing an "exact pin" contract, explicitly reject markers/extras and normalize whitespace before constructing the URL.
def require_pypi_release(requirement: str) -> None:
    name, version = requirement.split("==", 1)
    url = f"https://pypi.org/pypi/{name}/{version}/json"

Upload verified distributions and SHA-256 checksums to the intentional GitHub release after Trusted Publishing succeeds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please perform a second substantive code review of the latest commit (146325b), focusing on release safety, package file ownership, dependency/version guarantees, and Trusted Publishing. Igno...

Second review of 146325b (earlier resolved sdist-basename finding excluded). No blocking defect found in the intended design; the items below are ordered by release-safety impact.

Dependency/version guarantees

  1. verify_metapackage.py accepts any Requires-Dist that merely starts with shoal-sharkbite==. That passes for a conditional pin such as shoal-sharkbite==0.5.0; python_version < "3.10", or an extras-qualified/whitespace variant. A marker would silently produce installs of sharkbite with no implementation at all — exactly the failure mode this package exists to prevent. Suggest normalizing and comparing the full Requires-Dist value to an expected literal (and rejecting any ; marker or bracketed extra).
  2. require_pypi_release() treats presence of urls as sufficient. A yanked shoal-sharkbite==0.5.0 still returns files; resolvers will refuse it for a pinned dependency, so the publish gate would pass while every downstream pip install sharkbite fails. Recommend asserting not all(f.get("yanked") for f in payload["urls"]) (and, ideally, that at least one wheel is present).
  3. requirement.split("==", 1) is done on the raw metadata string; combined with (1), a marker/extra would be passed into the PyPI URL. Parsing after the strict validation in (1) fixes both.
  4. The pinned versions 1.3.0 and 0.5.0 are duplicated in pyproject.toml, RELEASE.md, and hardcoded three times in the smoke-test heredoc. Drift here is silent: a version bump that misses the workflow yields a green run asserting the old pin. Deriving both from pyproject.toml metadata (the tag-validation step already reads it via tomllib) removes the class of error.

Package file ownership

  1. The wheel guard rejects members via ".dist-info/" not in name, i.e. a substring test on the whole path. It does not assert that exactly one dist-info directory exists nor that it is named sharkbite-<version>.dist-info, so a nested path containing that substring, or a second dist-info, would pass. Prefer name.startswith(f"{expected_dist_info}/").
  2. verify_sdist() filters on member.isfile(), so symlink/hardlink members are skipped entirely — a LINKTYPE/SYMTYPE entry pointing outside the tree would not be flagged. Recommend failing on any non-regular member (also protects consumers extracting the sdist).
  3. Nothing asserts the wheel is py3-none-any or that it declares no entry points/scripts. A sharkbite-1.3.0.data/scripts/... payload is currently caught by (5)'s filter only incidentally; an explicit tag/entry-point assertion makes the "installs nothing" contract enforced rather than implied.

Release safety / Trusted Publishing

  1. The publish job downloads release-artifacts and copies *.whl/*.tar.gz into publish-dist without running sha256sum -c SHA256SUMS. The checksums are generated and attested but never verified at the point of use; adding the check makes the artifact hand-off between jobs self-validating and costs one line.
  2. The publish job has no actions/setup-python, so python scripts/verify_metapackage.py runs against whatever interpreter the runner image provides. Pinning it with setup-python matches the build job and prevents an image change from turning the last pre-upload gate into a hard failure (or, worse, a skipped one).
  3. Trusted Publishing setup itself looks correct: id-token: write is scoped to the attest/publish jobs only, the protected pypi environment gates upload, both publish paths are github.event_name == 'release', the tag is validated against the project version before build, and all third-party actions are SHA-pinned. workflow_dispatch/push cannot reach publish. No long-lived token is referenced.
  4. Minor: README.md is in ALLOWED_SDIST_FILES but is not referenced by MANIFEST.in or readme; harmless, but it widens the allowlist without a reason.

Items 1, 2, 5, 6, and 8 are the ones I would fix before the release blockers are cleared. Say the word and I will implement them in this PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/verify_metapackage.py:74

  • require_pypi_release() only converts HTTPError into an AssertionError. Transient network failures (DNS, timeouts, TLS issues) raise URLError and will currently bubble up as an unhelpful stack trace in CI. Catch URLError too and raise a clear assertion so the failure mode is actionable.
    try:
        with urllib.request.urlopen(url, timeout=30) as response:
            payload = json.load(response)
    except urllib.error.HTTPError as error:
        raise AssertionError(f"{requirement} is not published on PyPI") from error

scripts/verify_metapackage.py:56

  • verify_sdist() only inspects members where member.isfile() is true, so symlink/hardlink/other non-regular tar entries are ignored and could slip into the sdist without being detected. Since this script is acting as a release gate to prevent shipping historical implementation files, it should explicitly reject any non-file/non-directory members before applying the allowlist.
    with tarfile.open(path, "r:*") as archive:
        files = [
            member.name.split("/", 1)[-1]
            for member in archive.getmembers()
            if member.isfile()
        ]

Clarify that the package is only an install-name bridge and that issue 108 remains open until all four live Accumulo rows pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@phrocker phrocker mentioned this pull request Aug 21, 2026
9 tasks
@phrocker
phrocker requested a lite review from Copilot August 21, 2026 15:13
@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please review the latest migration-gate clarification against issue #108. Confirm this remains only an install-name/release bridge and does not claim completion or duplicate the Shoal compatibility implementation.

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please review the latest migration-gate clarification against issue #108. Confirm this remains only an install-name/release bridge and does not claim completion or duplicate the Shoal compati...

Confirmed. d2c40a1's additions to PYTHONREADME.md, RELEASE.md, and pyproject.toml (Migration URL) state this is packaging/install-name evidence only, that the implementation already lives in Shoal, and that #108 stays open until its four live Accumulo rows pass — consistent with issue #108's current revision-56 state (394 required / 390 satisfied / 4 remaining, all live-Accumulo). No claim of completion or duplication of the Shoal implementation is made anywhere in the diff.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comment thread scripts/verify_metapackage.py Outdated
Use 1.3.0 for both distributions, enforce exact same-version metadata, and verify Shoal release hashes, provenance, native ABI, ownership, and pip health before publishing the bridge.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@phrocker
phrocker requested a lite review from Copilot August 21, 2026 15:42
@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please review commit 0234aa3 against the merged same-version operator contract in shoal-oss#219. Focus on exact VERSION equality, release ordering, artifact/provenance verification, native ownership, and ensuring the bridge owns only its distinct dist-info. Ignore suppressed comments.

@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please perform the next substantive review of commit 52fb475. Verify the four latest release-safety fixes and look for any new correctness, dependency/version, artifact integrity, metadata-only ownership, Trusted Publishing, or CI defects. Ignore suppressed/minimized comments. If no new unsuppressed findings remain, state that explicitly.

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please perform the next substantive review of commit 52fb475. Verify the four latest release-safety fixes and look for any new correctness, dependency/version, artifact integrity, metadata-on...

Reviewed 52fb475. All four fixes verified:

  • The added --pattern "shoal_sharkbite-${version}.tar.gz" correctly targets the sdist without overlapping the existing wheel pattern (sdist filenames have no trailing -* segment), so the checksum/attestation steps now operate on both artifacts.
  • require_pypi_release() now requires a non-yanked wheel; confirmed against no-artifacts, yanked-wheel, sdist-only, and non-yanked-wheel payloads — only the last passes.
  • verify_sdist() correctly rejects non-regular members while still permitting directory entries.
  • metadata_from_wheel() correctly rejects non-py3-none-any tags and entry_points.txt; confirmed a real build still passes (twine check + verifier both PASSED).

No new correctness, dependency/version, artifact-integrity, ownership, Trusted Publishing, or CI defects found in this commit.

Make iterator and validation headers self-contained for current Ubuntu compilers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bb63dd55-8d79-4fb2-b809-6d865e062cd8
@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please review current HEAD 2edf8c2. The only changes after the prior clean round are direct <cstdint> includes in three legacy headers to fix Ubuntu CI. Check for any new unsuppressed correctness, build, release-safety, dependency/version, ownership, or workflow findings; ignore suppressed/minimized comments. State explicitly if none remain.

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please review current HEAD 2edf8c2. The only changes after the prior clean round are direct <cstdint> includes in three legacy headers to fix Ubuntu CI. Check for any new unsuppressed corre...

Reviewed 2edf8c2. Confirmed the diff is exactly the three <cstdint> includes (IterInfo.h, PythonIterInfo.h, inputvalidation.h), each needed since the headers use uint32_t/fixed-width types without including it. Compiled the three headers standalone with g++ -std=c++17 — no new warnings or errors beyond a pre-existing, unrelated std::binary_function deprecation notice. No workflow, pyproject.toml, or verify_metapackage.py files are touched in this commit, so all prior release-safety fixes remain intact. No new unsuppressed correctness, build, release-safety, dependency/version, ownership, or workflow findings.

Marc Parisi and others added 2 commits August 21, 2026 13:48
The compatibility distribution is metadata-only, so preserve native compilation for changes to historical C++ surfaces without gating packaging-only commits on unrelated compiler debt.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bb63dd55-8d79-4fb2-b809-6d865e062cd8
Clarify that metadata-only releases use the packaging workflow while legacy native CI remains scoped to native source changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bb63dd55-8d79-4fb2-b809-6d865e062cd8
@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please perform the final substantive review of current HEAD 2d8c802. Review the complete PR, including the legacy native workflow path scoping and release-policy clarification. Check for any new unsuppressed correctness, CI coverage, release-safety, dependency/version, artifact-integrity, metadata-only ownership, or Trusted Publishing findings. Ignore suppressed/minimized comments and state explicitly if none remain.

Co-authored-by: phrocker <1781585+phrocker@users.noreply.github.com>

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please perform the final substantive review of current HEAD 2d8c802. Review the complete PR, including the legacy native workflow path scoping and release-policy clarification. Check for any ...

Reviewed 2d8c802 in full. Found one real regression: b228c93 ("Scope legacy native CI to native changes") inadvertently reverted the <cstdint> includes added in b2a9e44/2edf8c2 for IterInfo.h, PythonIterInfo.h, inputvalidation.h, and Uri.h. Confirmed with a direct g++ build that this reintroduces uint32_t/uint16_t "not declared" compile errors — the exact Ubuntu CI failure those earlier commits fixed. Fixed in 38e2eb5 by restoring the four includes; recompiled headers standalone (only a pre-existing, unrelated std::binary_function deprecation warning remains).

Everything else checks out:

  • ccpp.yml path scoping (include/**, src/**, CMakeLists.txt, etc.) correctly limits the legacy native workflow to native-source changes and is valid YAML.
  • RELEASE.md's new clarification accurately describes the CI boundary between publish-pypi.yml (packaging) and ccpp.yml (native).
  • Python packaging side unaffected: rebuilt sdist/wheel, twine check and verify_metapackage.py (dependency pin, metadata-only ownership, sdist/wheel guards) all still pass.
  • Trusted Publishing, artifact/provenance verification, and same-version dependency guarantees are unchanged from the prior clean review of 52fb475/2edf8c2.

No other new unsuppressed correctness, CI coverage, release-safety, dependency/version, artifact-integrity, ownership, or Trusted Publishing findings remain.

@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please perform one final review of exact HEAD 38e2eb5 after your restored <cstdint> includes. Check the complete PR for any new unsuppressed findings; ignore suppressed/minimized comments. Do not modify the branch unless you find a substantive defect. State explicitly if the round is clean.

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please perform one final review of exact HEAD 38e2eb5 after your restored <cstdint> includes. Check the complete PR for any new unsuppressed findings; ignore suppressed/minimized comments. ...

Reviewed exact HEAD 38e2eb5. Confirmed the diff from 2d8c802 is only the four restored <cstdint> includes (IterInfo.h, PythonIterInfo.h, inputvalidation.h, Uri.h) — compiled each standalone with g++ -std=c++17, no errors (only the pre-existing unrelated std::binary_function deprecation notice). Rebuilt the package, twine check and scripts/verify_metapackage.py both pass against the fresh wheel/sdist (dependency pin, metadata-only ownership, sdist/wheel guards all intact). ccpp.yml and publish-pypi.yml are still valid YAML with no changes in this commit. Round is clean — no new unsuppressed findings. No branch changes made.

Co-authored-by: phrocker <1781585+phrocker@users.noreply.github.com>
Remove accidentally committed egg-info output and ignore future local distribution metadata and build artifacts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bb63dd55-8d79-4fb2-b809-6d865e062cd8
@phrocker

Copy link
Copy Markdown
Owner Author

@copilot please review exact HEAD b262099 after removal of the accidentally committed generated sharkbite.egg-info files and addition of dist/ / *.egg-info/ ignore rules. Review the complete PR for any new unsuppressed findings, ignore suppressed/minimized comments, do not modify the branch unless a substantive defect exists, and state explicitly if the round is clean.

Copilot AI commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@copilot please review exact HEAD b262099 after removal of the accidentally committed generated sharkbite.egg-info files and addition of dist/ / *.egg-info/ ignore rules. Review the complete PR ...

Reviewed exact HEAD b262099. Confirmed the diff from 38e2eb5 only removes the accidentally-committed sharkbite.egg-info/* files and adds dist/ and *.egg-info/ to .gitignore. Ran a fresh python -m build from a clean tree — git status --porcelain remains empty afterward, confirming the generated egg-info/build directories are now properly ignored and won't be re-committed. twine check and scripts/verify_metapackage.py both still pass on the rebuilt wheel/sdist (dependency pin, metadata-only ownership, sdist/wheel guards all intact). No other files changed in this commit, so all prior release-safety fixes remain in place. Round is clean — no new unsuppressed findings. No branch changes made.

@phrocker
phrocker merged commit 5ec93ac into main Aug 26, 2026
5 checks passed
@phrocker
phrocker deleted the compat/shoal-metapackage branch August 26, 2026 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rewrite sharkbite using shoal

3 participants