Skip to content

Conversation

@woodruffw
Copy link
Member

@woodruffw woodruffw commented Apr 30, 2024

WIP, needs more tests.

Summary of changes:

  • Updates _make_package to accept an attestations parameter, which receives the list of attestations for the distribution (from _split_inputs).
  • When --attestations is set and attestations is nonempty, the supplied attestations are passed into the constructed PackageFile.
  • Internally, PackageFile reads each attestation as JSON and compounds it onto a JSON array to send as the attestations field during upload, per PEP 740.

See #1094.

Signed-off-by: William Woodruff <william@yossarian.net>
data["gpg_signature"] = self.gpg_signature

if self.attestations is not None:
data["attestations"] = json.dumps(self.attestations)
Copy link
Member

Choose a reason for hiding this comment

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

I'm 99% certain I remember that this ends up in a multipart/form-data body. Does PyPI want this to have a content type? Also, does this need to be conditioned on index server? I don't expect any of the third party ones to support this any time soon

Copy link
Member Author

Choose a reason for hiding this comment

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

In the current draft, we expect it to be in that multipart/form-data body: https://peps.python.org/pep-0740/#upload-endpoint-changes. No content-type marker is expected.

(I have no objection to changing this to require a content-type like the main content field, if you think that would be clearer! I'd have to tweak the PEP in that case.)

Also, does this need to be conditioned on index server? I don't expect any of the third party ones to support this any time soon

Everything is currently flagged behind --attestations which is off by default, so in principle this shouldn't need to be conditioned on the index server. That being said I could additionally add that check, although I think third party indices/mirrors may eventually want to support these as well and may be surprised by twine pre-filtering them 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't know. I'm more worried about people enabling this against a not-PyPI index that won't ignore the field and complaining about the errors/failures to upload

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm more worried about people enabling this against a not-PyPI index that won't ignore the field and complaining about the errors/failures to upload

Ah yeah, hadn't thought of that. Support on non-PyPI indices is a distant idea anyways so I'll flag this off 🙂

Copy link
Member

Choose a reason for hiding this comment

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

We can always add another option to enable it on a per-index basis later. But yeah, I just dread every 3rd party index issue we get because it is always a nightmare of the index doing something that violates a specification

woodruffw added 4 commits May 1, 2024 11:38
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
@woodruffw woodruffw marked this pull request as ready for review May 1, 2024 18:33
@woodruffw
Copy link
Member Author

Should be good for another look! I've rounded out the coverage as well.

@sigmavirus24 sigmavirus24 merged commit 0ec5d18 into pypa:main May 1, 2024
@woodruffw woodruffw deleted the ww/attestations-attach branch May 1, 2024 23:05
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.

2 participants