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

Exclude pyc dirs, not include, when hashing code #2286

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Conversation

huonw
Copy link
Collaborator

@huonw huonw commented Nov 9, 2023

This fixes #2285 by fixing a minor typo in f9a9d94 / #2263 that caused the computation of a PEXes code hash to not recur into the correct directories.

I think this explains the symptoms in #2285 due to caching: if a PEX only contains files in subdirectories, the code hash will be da39a3ee5e6b4b0d3255bfef95601890afd80709 (the sha256 hash of no data) and thus potentially the venv create might be referring to an incorrectly cached installation.

@huonw
Copy link
Collaborator Author

huonw commented Nov 9, 2023

I haven't yet got my dev environment to work for running test with the Python 3.12 changes and don't have time to today, so the test is not yet confirmed to be precise (i.e. I don't know if the test fails before/passes after). The bisection script from #2285 does now give expected behaviour, and doesn't without, though.

@huonw huonw marked this pull request as ready for review November 9, 2023 08:35
@huonw huonw requested a review from jsirois November 9, 2023 08:35
Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

My time today is limited as well, but this is certainly a fix of something.

@huonw
Copy link
Collaborator Author

huonw commented Nov 10, 2023

I've got a dev environment working again 🎉

That test was wrong (testing dir_hash, bug was in pex_code_hash). I've rewritten the existing tests for dir_hash to also test pex_code_hash, which catches the bug, plus added an assertion for the hidden file handling.

@huonw huonw requested a review from jsirois November 10, 2023 01:34
@huonw huonw merged commit aadfe9c into pex-tool:main Nov 10, 2023
24 checks passed
@huonw huonw deleted the dir-2285 branch November 10, 2023 06:02
@jsirois
Copy link
Member

jsirois commented Nov 10, 2023

... haven't yet got my dev environment to work for running test with the Python 3.12 changes ...

@huonw fwiw, if you have docker, then you need no dev env and can do things just like CI:
https://github.com/pantsbuild/pex/blob/db5b5100d0d739964bfc46943bc9c8f9e1bed480/.github/workflows/ci.yml#L99-L101

Basically:

  1. (one time only) BASE_MODE=pull CACHE_MODE=pull ./dtox.sh --version
  2. ./dtox.sh ...regular tox args...

You don't need to do step 1, in which case these images will be built locally, but that is slow. If you have a decent network connection, pulling them is probably more convenient. What this nets you is the full suite of Python interpreters Pex supports - both CPython and PyPy - ready to go in the image. It also gets you a pre-seeded PyPI cache when you use ./dtox.sh -epy... -- --devpi .... You can omit the CACHE_MODE=pull if you don't use the --devpi feature of the build or if you'd rather have it build that cache up locally.

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.

venv create no longer includes --sources-directory contents when all files are nested
2 participants