Skip to content

deps: bump protobuf-specs #1276

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

deps: bump protobuf-specs #1276

wants to merge 8 commits into from

Conversation

woodruffw
Copy link
Member

Very WIP. This includes underlying changes to the proto models themselves, so I need to go through various internal usages and correct them.

On the plus side, this makes our typechecking of each proto model's internals much more correct.

@woodruffw
deps: bump protobuf-specs
a9f8052 
Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw marked this pull request as draft January 10, 2025 22:05
@woodruffw
Copy link
Member Author

Switching to betterproto's Pydantic models produces generally better-typed results, but it looks like enums are strictly worse:

sigstore/_internal/trust.py:96: error: Argument 1 to <set> has incompatible type "int"; expected "PublicKeyDetails"  [arg-type]
sigstore/_internal/trust.py:97: error: Argument 2 to <set> has incompatible type "int"; expected "PublicKeyDetails"  [arg-type]
sigstore/_internal/trust.py:98: error: Argument 3 to <set> has incompatible type "int"; expected "PublicKeyDetails"  [arg-type]
sigstore/_internal/trust.py:99: error: Argument 4 to <set> has incompatible type "int"; expected "PublicKeyDetails"  [arg-type]
sigstore/_internal/trust.py:103: error: Dict entry 0 has incompatible type "int": "SHA256"; expected "PublicKeyDetails": "HashAlgorithm"  [dict-item]
sigstore/_internal/trust.py:104: error: Dict entry 1 has incompatible type "int": "SHA384"; expected "PublicKeyDetails": "HashAlgorithm"  [dict-item]
sigstore/_internal/trust.py:105: error: Dict entry 2 has incompatible type "int": "SHA512"; expected "PublicKeyDetails": "HashAlgorithm"  [dict-item]
sigstore/models.py:670: error: Incompatible types in assignment (expression has type "dict[str, MessageSignature]", variable has type "MessageSignature | Envelope")  [assignment]
sigstore/models.py:672: error: Incompatible types in assignment (expression has type "dict[str, sigstore_protobuf_specs.io.intoto.Envelope]", variable has type "MessageSignature | sigstore.dsse.Envelope")  [assignment]
sigstore/models.py:674: error: Argument after ** must be a mapping, not "MessageSignature | Envelope"  [arg-type]
Found 10 errors in 2 files (checked [28](https://github.com/sigstore/sigstore-python/actions/runs/12772251322/job/35601345407?pr=1276#step:5:29) source files)
make: *** [Makefile:66: lint] Error 1

I'll look into whether this can be fixed upstream.

@woodruffw woodruffw mentioned this pull request Jan 29, 2025
Closed
This was referenced Feb 24, 2025
Merged
Open
@woodruffw woodruffw mentioned this pull request Apr 4, 2025
Closed
@jku jku mentioned this pull request Apr 11, 2025
Open
@jku
Copy link
Member

jku commented Apr 16, 2025 •
edited
Loading

Switching to betterproto's Pydantic models produces generally better-typed results, but it looks like enums are strictly worse:

....
I'll look into whether this can be fixed upstream.

Any updates on this? Working around seems to not be awful: jku@7028110

  • avoid explicitly settings the class variable annotations since that seems to be what upsets mypy when set[_PublicKeyDetails] is used
  • Use Any in the dict that gets unpacked into the bundle

I've tried to read through the protobuf-specs changes and I don't see anything obviously missing between 0.3.2 and 0.3.5. (it's possible though, the betterproto update manages to hide the actual changes in massive docstring changes...).

There is of course newer protobufs already but maybe it's easier to handle this step by step?

@jku
Copy link
Member

jku commented Apr 17, 2025 •
edited
Loading

There is of course newer protobufs already but maybe it's easier to handle this step by step?

I can do the 0.4.1 bump separately: there is a bit of work to get the fancier SigningConfig to work but I have most of it in a branch already.

@woodruffw
Copy link
Member Author

Any updates on this? Working around seems to not be awful: jku@7028110

Sorry, it's just been blocked on my time 😅 -- your change looks good, I'll cherry-pick that in!

woodruffw
woodruffw commented Apr 18, 2025
View reviewed changes
sigstore/_cli.py
Comment on lines -716 to -718
signature = base64.b64encode(
result._inner.message_signature.signature
).decode()
Copy link
Member Author

Choose a reason for hiding this comment

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

NB: This was previously just wrong for DSSE outputs, but we didn't notice since nothing really uses the --signature flag anymore.

@woodruffw
Copy link
Member Author

Local tests are now OK, but the conformance suite is failing for unclear reasons. My best guess however is that this is potentially a consequence of us mixing the sigstore-python and conformance environments, and the two no longer agreeing about sigstore-protobuf-specs' APIs.

@jku
Copy link
Member

jku commented Apr 22, 2025

Local tests are now OK, but the conformance suite is failing for unclear reasons. My best guess however is that this is potentially a consequence of us mixing the sigstore-python and conformance environments, and the two no longer agreeing about sigstore-protobuf-specs' APIs.

sigstore/sigstore-conformance#187 should fix that issue

@jku
Copy link
Member

jku commented Apr 22, 2025 •
edited
Loading

sigstore/sigstore-conformance#187 should fix that issue

Now merged: you should be able to test this by bumping the sigstore-conformance hash in this PR...

EDIT: I tested bumping the conformance hash in https://github.com/jku/sigstore-python/actions/runs/14600225714/job/40956304385 : looks good. We could do a conformance release to get this fixed (there's nothing else in it though).

jku added a commit to sigstore/sigstore-conformance that referenced this pull request Apr 22, 2025
@jku
Workaround betterproto/pydantic issues
e86b47e 
This seems to work for now.. See also
sigstore/sigstore-python#1276

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
Copy link
Member

jku commented Apr 22, 2025 •
edited
Loading

I tested bumping the conformance hash in https://github.com/jku/sigstore-python/actions/runs/14600225714/job/40956304385 : looks good. We could do a conformance release to get this fixed (there's nothing else in it though).

Actually the install log still looks wrong:

  • neither the conformance action nor our sigstore-python install uses a virtual env so they still end up mixing dependencies here
  • I really thought conformance action did this already... I guess I can try adding that

@jku jku mentioned this pull request Apr 22, 2025
Merged
@jku
Copy link
Member

jku commented Apr 22, 2025 •
edited
Loading

Okay this finally does look good: https://github.com/jku/sigstore-python/actions/runs/14600769194/job/40958111997 -- need one more PR in sigstore-conformance: sigstore/sigstore-conformance#204 (to make sure the action also installs dependencies in a virtualenv and not in the main python environment)

This PR is ready to merge from my POV as long as there's a conformance release or we start using a unreleased conformance

@jku jku mentioned this pull request Apr 25, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@woodruffw @jku