search

scikit-hep / pyhf

pure-Python HistFactory implementation with tensors and autodiff
https://pyhf.readthedocs.io/
Apache License 2.0
283 stars 83 forks source link

ci: Add GitHub artifact attestations to package distribution #2473

Closed matthewfeickert closed 4 months ago

matthewfeickert commented 5 months ago

Description

Checklist Before Requesting Reviewer

Before Merging

For the PR Assignees:

* Add generation of GitHub artifact attestations to built sdist and wheel
  before upload.
  c.f.:
   - https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/
   - https://docs.github.com/en/actions/security-guides/using-artifact-attestations-to-establish-provenance-for-builds
* Add verification of artifact attestation before publishing to PyPI
  using the 'gh attestation verify' CLI API, added in v2.49.0.
   - c.f. https://github.com/cli/cli/releases/tag/v2.49.0
matthewfeickert commented 5 months ago

Currently blocked by https://github.com/actions/runner-images/issues/9784, without updating ubuntu-latest to ubuntu-24.04.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.21%. Comparing base (ecd0613) to head (1b0191c). Report is 29 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2473 +/- ## ======================================= Coverage 98.21% 98.21% ======================================= Files 69 69 Lines 4543 4543 Branches 804 804 ======================================= Hits 4462 4462 Misses 48 48 Partials 33 33 ``` | [Flag](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | Coverage Δ | | |---|---|---| | [contrib](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `97.79% <ø> (ø)` | | | [doctest](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `98.08% <ø> (ø)` | | | [unittests-3.10](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `96.23% <ø> (ø)` | | | [unittests-3.11](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `96.23% <ø> (ø)` | | | [unittests-3.12](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `96.23% <ø> (ø)` | | | [unittests-3.8](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `96.25% <ø> (ø)` | | | [unittests-3.9](https://app.codecov.io/gh/scikit-hep/pyhf/pull/2473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep) | `96.27% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scikit-hep#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

matthewfeickert commented 4 months ago

GitHub CLI is updated to v2.49.2 in https://github.com/actions/runner-images/issues/9784 and so should work now.

matthewfeickert commented 4 months ago

So it seems that the most important takeaway is described in https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/#a-more-secure-future, and while it seems that this should mean that the number of attestations generated isn't relevant as everything will be machine generated, it seems that it might(?) be useful to keep https://github.com/scikit-hep/pyhf/attestations containing mostly only releases.

@woodruffw @di, not to bug you with things that go beyond https://github.com/sigstore/sigstore-python, but is this the correct take (that we should only be publishing attestations for releases)?

matthewfeickert commented 4 months ago

Also cc @sethmlarson given very helpful feedback on https://github.com/scientific-python/summit-2024/issues/9#issuecomment-2107536124.

woodruffw commented 4 months ago

Thanks for the ping @matthewfeickert!

IMO, the number of attestations generated is not currently relevant: these attestations don't appear on PyPI and are "generic" in nature (i.e. don't express a specific intent to build, release, etc.), so having multiple of them doesn't accomplish much at the moment.

However, to take a step back: we're building PEP 740 on the same foundations (Sigstore + sigstore-python), and it will have support for multiple attestations, e.g. for distinguishing publish, release, and also third-party counter-attestations. That PEP uses the same underlying technologies as GitHub's Attestations feature but requires attestations to be uploaded to PyPI as well, which means we'll be building it into the standard PyPA tooling.

That's still a little out and not directly related to what you asked though 😅. But the TL;DR: IMO one GitHub attestation is fine, and in the future PEP 740 will also allow your tools to (automatically) load multiple attestations per-file onto PyPI.

matthewfeickert commented 4 months ago

we're building PEP 740 on the same foundations (Sigstore + sigstore-python), and it will have support for multiple attestations, e.g. for distinguishing publish, release, and also third-party counter-attestations. That PEP uses the same underlying technologies as GitHub's Attestations feature but requires attestations to be uploaded to PyPI as well, which means we'll be building it into the standard PyPA tooling.

Amazing! Thank you very much for all of this work and this is actually quite useful to know about. :+1: I also now have some reading to do. :) https://peps.python.org/pep-0740/

But the TL;DR: IMO one GitHub attestation is fine, and in the future PEP 740 will also allow your tools to (automatically) load multiple attestations per-file onto PyPI.

Okay thanks this is useful. My question was more in the context of the fact that https://github.com/scikit-hep/pyhf/attestations is a log of all attestations, and if I was to remove the if guards that I place here it would generate attestations for each run of the CI, which just seems like it would be noisy. So while not a problem I'll leave the if guards in and do similar things as I propagate this out across other Scikit-HEP libraries and to Scientific Python libraries. :+1:

matthewfeickert commented 4 months ago

@meeseeksdev backport to release/v0.7.x