Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jan 4, 2022

@merwok
Copy link
Member

merwok commented Jan 4, 2022

Can you make a separate ticket for this? Thanks!


class TestPackages(unittest.TestCase):

def test_packages_hash(self):
Copy link
Member

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).

Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

@ned-deily ned-deily Jan 5, 2022

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants