Skip to content
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.

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

Vendor truststore #12107

Merged
merged 6 commits into from Sep 26, 2023
Merged

Vendor truststore #12107

merged 6 commits into from Sep 26, 2023

Conversation

sethmlarson
Copy link
Contributor

Spun off of #11647, this PR vendors truststore but doesn't change the default behavior of pip. The functionality behind truststore still requires using the --use-feature=truststore flag.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

One nit, lgtm.

src/pip/_internal/cli/req_command.py Outdated Show resolved Hide resolved
@sethmlarson
Copy link
Contributor Author

Thanks @uranusjr for the quick reviews! Pinging @pfmoore for whether this is acceptable to merge for the 23.2 window.

@uranusjr
Copy link
Member

Hmm, seems like the vendoring internals need some update to account for truststore not working on lower versions. Not sure what exactly needs to be done; may take a look later.

@pfmoore
Copy link
Member

pfmoore commented Jun 27, 2023

Hmm, seems like the vendoring internals need some update to account for truststore not working on lower versions. Not sure what exactly needs to be done; may take a look later.

The first problem is going to be that we will require Python 3.10+ to do a pip revendoring, as truststore has requires_python>=3.10. While that's not exactly a major ask, it is a hidden constraint that I'm somewhat uncomfortable with. But we should be explicit if we make that choice.

The second issue is that if truststore uses Python 3.10+ syntax, it won't compile on older Pythons, meaning that the step of compiling the installed code when installing pip will fail. And even if we skip compiling, this will be a time-bomb for anyone who later chooses to do compileall on their site-packages. I haven't checked whether truststore does use Python 3.10+ syntax, but even if it doesn't, I'm not comfortable with taking the risk that it will stay that way, because they won't have tests to catch any new syntax getting introduced. Our requirements for vendored packages has always been that they support all versions of Python that pip supports. I'm sorry, but my initial view is that we shouldn't be changing that requirement in a rush "because we want to get truststore into 23.2".

My view is that this is a good change, and we should work towards getting it into pip. But we should not target any specific release. Let's work on this and on #11647 (for that one, pinning down the transition process for making truststore the default) at whatever pace is they need, and release them when they are ready.

@sethmlarson
Copy link
Contributor Author

sethmlarson commented Aug 21, 2023

I've made Python 3.10+ explicitly required for the vendoring task in this commit: f16b7c2

In order to guarantee compatibility with compileall I've added a CI job to Truststore which runs compileall on Python 3.7: sethmlarson/truststore#108

I've also added a step which runs pip debug to our integration test suite for pip.

@sethmlarson
Copy link
Contributor Author

Finally I've added 3f0f4d2 which allows Truststore to not import successfully on old Python versions. There were a handful of ways I tried setting this up, this one seemed the least intrusive to the rest of the debug code? Happy to restructure it a different way if that would be better.

Otherwise I believe this is ready for another round of review. Please take this comment into consideration as well as there have been some changes outside this PR to provide more assurances long-term about the adoption of Truststore.

@sethmlarson
Copy link
Contributor Author

I've upgraded Truststore to 0.8.0 in bff1e6a

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Seems clean enough to me

@sethmlarson
Copy link
Contributor Author

Since it's been a bit since this was approved, wanted to check in that this would still be considered for the next release or if there was anything outstanding for me to do?

@pfmoore
Copy link
Member

pfmoore commented Sep 25, 2023

Apparently making my review comment as "resolved" wasn't enough so I've just re-approved. Someone needs to merge this is the main thing. I'll leave that to someone else to do, as I don't really have the time right now to follow up if any issues arise.

@pfmoore pfmoore added this to the 23.3 milestone Sep 25, 2023
@pfmoore
Copy link
Member

pfmoore commented Sep 25, 2023

I've added this to the 23.3 milestone, as I don't think there's any reason to delay it beyond that - other than "we forgot", which hopefully adding it to the milestone will address 😉

@uranusjr
Copy link
Member

I’m going to pull the trigger since reverting this should be easy anyway…

@uranusjr uranusjr merged commit ed113ff into pypa:main Sep 26, 2023
26 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants