Skip to content

Conversation

@fridex
Copy link

@fridex fridex commented Mar 14, 2023

@fridex
Copy link
Author

fridex commented Mar 15, 2023

Checking the failed test:

FAILED tests/functional/test_wheel.py::test_pip_wheel_ext_module_with_tmpdir_inside - AssertionError: Script returned code: 1

I'm not sure whether this PR introduces the given failure. Also, it looks like it fails only on Mac.

@webknjaz webknjaz added skip news Does not need a NEWS file entry (eg: trivial changes) and removed skip news Does not need a NEWS file entry (eg: trivial changes) labels Mar 16, 2023
@fridex fridex marked this pull request as draft March 16, 2023 21:39
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Mar 27, 2023
Copy link
Contributor

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

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

Just a very fast first review.

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 29, 2024
@fridex fridex force-pushed the provenance-url branch 8 times, most recently from ba0d648 to 0ffeffd Compare July 30, 2024 06:25
@fridex fridex force-pushed the provenance-url branch 3 times, most recently from 550e00d to 7c1954b Compare July 30, 2024 08:16
@sbidoul
Copy link
Member

sbidoul commented Jul 30, 2024

We may want to add a test that verifies that provenance_url.json is created when installing a wheel found in cache. I think there is already such a test for download_info in the install report json.

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @fridex!

@fridex fridex force-pushed the provenance-url branch 2 times, most recently from 2abed55 to 3ef0705 Compare July 31, 2024 12:45
@fridex
Copy link
Author

fridex commented Jul 31, 2024

We may want to add a test that verifies that provenance_url.json is created when installing a wheel found in cache. I think there is already such a test for download_info in the install report json.

Added tests, PTAL.

EDIT: docs CI failures do not seem to be related to this PR

@fridex fridex requested a review from sbidoul July 31, 2024 12:59
@fridex fridex changed the title Implement provenance_url.json PEP-710: implement provenance_url.json file Jul 31, 2024
@fridex fridex marked this pull request as ready for review July 31, 2024 13:00
Signed-off-by: Fridolin Pokorny <fridolin.pokorny@gmail.com>
@sbidoul
Copy link
Member

sbidoul commented Aug 1, 2024

There is still a little something that is nagging me.

This PR uses the direct_url.py model which implements the Direct URL Data Structure specification.

However, the current PEP 710 draft does not reference that specification explicitly.

So in theory there is a risk that if the Direct URL Data Structure specification evolves in the future, a change in direct_url.py could makes it inadvertently incompatible with PEP 710. To avoid this, it might be preferable either 1/ for PEP 710 to reference the Direct URL Data Structure specification, mentioning the subset of it that is allowed in provenance_url.json, or 2/ create a distinct provenance_url.py model in pip.

@fridex
Copy link
Author

fridex commented Aug 2, 2024

However, the current PEP 710 draft does not reference that specification explicitly.

So in theory there is a risk that if the Direct URL Data Structure specification evolves in the future, a change in direct_url.py could makes it inadvertently incompatible with PEP 710. To avoid this, it might be preferable either 1/ for PEP 710 to reference the Direct URL Data Structure specification, mentioning the subset of it that is allowed in provenance_url.json, or 2/ create a distinct provenance_url.py model in pip.

Added links to the Direct URL data structure in python/peps#3887. Please let me know if you want to be more specific in some specific paragraph with the Direct URL data structure link.

@ichard26 ichard26 marked this pull request as draft December 28, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants