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

Bad installed_wheels dir when a wheel's top_level.txt dirname collides with sources #2160

Closed
thejcannon opened this issue Jun 20, 2023 · 3 comments · Fixed by #2165
Closed
Assignees
Labels

Comments

@thejcannon
Copy link
Contributor

Steps to reproduce:

Here's a pex (renamed to .zip for GitHub's sake) local.zip. It has a top_level.txt of top_level. Rename to local.whl

Then, next to that file:

$ mkdir -p sources/top_level/
$ echo 'print("hello")' > sources/top_level/mymain.py
$ pex --output-file binary.pex --venv prepend --requirements-pex local.pex --entry-point top_level.mymain $'--sources-directory=sources' --layout packed
$ ./binary/__main__.py
hello
$ ls ~/.pex/installed_wheels/a571cccbc564d1e7da4efe2416aca4bb049256ab4d0b30553f9d4de8207fb283/top_level-0.0.0-cp38-cp38-linux_x86_64.whl/top_level/
__init__.py  mymain.py  __pycache__

mymain.py is not part of the wheel, and shouldn't be in there.

@huonw huonw added the bug label Jun 26, 2023
@jsirois jsirois self-assigned this Jul 1, 2023
@jsirois
Copy link
Member

jsirois commented Jul 1, 2023

There is a bit of chaff in the example. The key bit is --venv and not also --venv-site-packages-copies (for example, layout is irrelevant) - use --venv-site-packages-copiesand all is well.

With just --venv you see:

$ ./binary.pex
hello
$ ls -l ~/.pex/venvs/s/fe373d35/venv/lib/python3.8/site-packages/
total 12
-rw-r--r-- 1 jsirois jsirois  230 Jun 30 18:14 PEX_EXTRA_SYS_PATH.pth
lrwxrwxrwx 1 jsirois jsirois  152 Jun 30 18:14 top_level -> ../../../../../../installed_wheels/a571cccbc564d1e7da4efe2416aca4bb049256ab4d0b30553f9d4de8207fb283/top_level-0.0.0-cp38-cp38-linux_x86_64.whl/top_level
drwxr-xr-x 2 jsirois jsirois 4096 Jun 30 18:14 top_level-0.0.0.dist-info

And that should be enough to see the issue. Now, for 3rdparty deps, this sort of collision (namespace package) is handled via a pex-ns-pkgs.pth you don't see here:
https://github.com/pantsbuild/pex/blob/c2fa8130d3dfb38f19f550a5bb0a3c69b7459f0d/pex/venv/installer.py#L459-L495
https://github.com/pantsbuild/pex/blob/c2fa8130d3dfb38f19f550a5bb0a3c69b7459f0d/pex/venv/installer.py#L514-L530

So, to handle this case where the namespace package is split across --sources-directory sources and 3rdaprty deps, the handling above will need to be de-localized.

@jsirois
Copy link
Member

jsirois commented Jul 1, 2023

The issue you're going to run into here though @thejcannon that Pex can't help you with is your example is not properly ns-packaged. The sources are (lack of __init__.py) but the local.zip does not cooperate - it has an empty __init__.py; so with the fix you'll get an error finding top_level.mymain (or not - vagaries of sys.path order dictate). I'll proceed with the Pex fix, but you'll have some work to do on your example or in Pants depending the source of the bad ns-package mix.

This was referenced Jul 1, 2023
jsirois added a commit to jsirois/pex that referenced this issue Jul 2, 2023
Previously namespace packages were handled only amongst 3rd-party deps.
Any ns-package also split across PEX user sources would lead to one of
the 3rd-party dep members of the namespace package getting its cached
wheel install contaminated with the PEX user source that was a member of
the ns-package.

Fixes pex-tool#2160
@jsirois
Copy link
Member

jsirois commented Jul 2, 2023

I used this example, but with the ns-package issue fixed.

jsirois added a commit that referenced this issue Jul 2, 2023
Previously namespace packages were handled only amongst 3rd-party deps.
Any ns-package also split across PEX user sources would lead to one of
the 3rd-party dep members of the namespace package getting its cached
wheel install contaminated with the PEX user source that was a member of
the ns-package.

Fixes #2160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants