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

fix: Handle single-file module pypi-deps in py_pex_binary #392

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ekfeet
Copy link

@ekfeet ekfeet commented Sep 13, 2024

Addresses the bug in #391.

Single-file modules such as six and typing-extensions does not work with the current py_pex_binary rule, since it's assumed that there exist a sub-directory within site-packages that is not dist-info.

An example of what a single-file module pypi-dep looks like:

ls bazel-reppro_pex_err/external/rules_python~~pip~pypi_311_six/site-packages/
__init__.py  six-1.16.0.dist-info  six.py

Since Distribution.load(..) takes in the site-packages directory, we only emit this part of the path, and let uniquify=True handle deduplication after _map_srcs is applied.


Changes are visible to end-users: no

Test plan

  • Manual testing; please provide instructions so we can reproduce:
    Add six to requirements and "@pypi_six//:pkg" as a dep to the py_pex_binary example; then import six in py_pex_binary's say.py. Printing the module or cowsay.cow(f"{six}") shows that the previous example and single-files modules now also work.

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2024

CLA assistant check
All committers have signed the CLA.

@mattem
Copy link
Member

mattem commented Sep 13, 2024

Thanks! Would you be able to turn the manual test case into an automated one?

@thesayyn Can you take a look?

@ekfeet
Copy link
Author

ekfeet commented Sep 16, 2024

Thanks! Would you be able to turn the manual test case into an automated one?

Looks to be a bit more work than just adding a test case, as I don't see any tests for the py-pex-binary rule. I'll leave test automation up to you guys if that's okay

@@ -159,7 +159,7 @@ def __call__(self, parser, namespace, value, option_str=None):
]

for dep in options.dependencies:
dist = Distribution.load(dep + "/../")
dist = Distribution.load(dep)
Copy link
Member

Choose a reason for hiding this comment

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

As i remember, Distribution.load has always wanted to read a dist-info, how does this work now?

Copy link
Author

@ekfeet ekfeet Sep 17, 2024

Choose a reason for hiding this comment

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

It should be functionally equivalent to the previous version I believe, but I might be missing something. _map_srcs now emits the path up to (and including) 'site-packages', instead of 'site-packages/some-package', so +/../ isn't needed anymore

EDIT: options.dependencies only contain the deps and not the dist-infos afaict. Is the options.distinfos in pex/main.py used?

@thesayyn
Copy link
Member

Could you also add a test for this?

@ekfeet ekfeet force-pushed the fix-py-pex-binary-single-module-files branch from e1c3618 to a11b203 Compare September 17, 2024 15:32
@ekfeet
Copy link
Author

ekfeet commented Sep 17, 2024

Could you also add a test for this?

Gave it a shot in a11b203

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.

4 participants