-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-46119: Add hash test for ensurepip packages #30393
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
Conversation
Can you make a separate ticket for this? Thanks! |
|
||
class TestPackages(unittest.TestCase): | ||
|
||
def test_packages_hash(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is possible that this test is executed from an environment where it cannot reach the internet so it should be marked as using the network (with the @requires_resource('network')
decorator).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to maybe move this test to the GitHub Actions target, as having it in the test suite can be a bit problematic but I would like to know what other RM think
CC: @ambv @zooba @ned-deily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this test? And, if it fails, what action should be taken? And what happens with those third-party distributors who do not use the bundled pip? In other words, is this going to cause more problems than it might solve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that same point, but noticed the code uses test.support.open_urlresource
so I expected it to do the skipping. I checked just now and it does indeed: https://github.com/kumaraditya303/cpython/blob/0c6c596ac9c935564998c5a666ea8cd9dd6ef927/Lib/test/support/__init__.py#L570
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this test? And, if it fails, what action should be taken?
@ned-deily The idea is to make sure the bundled pip and setuptools binaries are exactly what is upstream. This is because in PRs changing these files we cannot easily tell from the PR that the binary blobs don't contain malicious code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ned-deily The idea is to make sure the bundled pip and setuptools binaries are exactly what is upstream. This is because in PRs changing these files we cannot easily tell from the PR that the binary blobs don't contain malicious code.
Then the test should only run on a merge into the repo, perhaps via GitHub Actions as you suggested. And/or as part of the release process, i.e. when producing the release materials. Otherwise, attempting to run an integrity test every time someone runs the test suite seems both problematic and misleading: if we can't trust what's in the repo we've downloaded from GitHub or in the release materials we've downloaded from python.org, shouldn't there be an integrity test of the whole source package or repo? But for release materials, there already is.
So, instead of adding a test case to the normal test suite, how about adding it as a command line option to the ensurepip
module itself, then add running that option as part of the release process, and adding a GitHub action to run that option whenever any changes are merged to the wheel? That also gives downstream users the opportunity to easily run it if they so desire perhaps after running an ensurepip --update. (Side issue, ideally we should find a better way to separate out the wheels from the source repo so that they don't unnecessarily contribute to the repo's size over time.)
https://bugs.python.org/issue46119