-
Notifications
You must be signed in to change notification settings - Fork 11
pth: Add a add_pth option to py_pyenv.
#14
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
base: main
Are you sure you want to change the base?
Conversation
jvolkman
left a comment
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.
Sorry for not seeing this sooner.
pth files were actually my first approach, but I found that PyCharm didn't pick up on changes under the paths pointed to by the pth file, which is why I instead resorted to symlinking everything. If tools worked as expected, pth files would certainly be much more straightforward. That said, it seems like something that we could support optionally, as you are here.
venv.bzl
Outdated
| for dep in ctx.attr.deps: | ||
| if PyInfo not in dep: | ||
| continue | ||
| print(dep[PyInfo].imports.to_list()) |
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.
This seems like a leftover debug statement.
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.
indeed, whoops!
| install_data_file(env_path, file) | ||
| else: | ||
| install_site_file(site_packages_path, file) | ||
| if add_pth: |
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.
If we're creating a pth file, do we still need to call install_site_file? Isn't that redundant?
|
Hm, I didn't test using pycharm, I was testing using VSCode. Unfortunately, I don't have access to pycharm. |
Currently, rules_pyvenv installs all the python deps by symlinking them in site-packages. However, in bazel, the python deps are actually added to the PYTHONPATH, which means that for some deps, just symlinking into site-packages doesn't work. However, venv supports this via pth files. pth files will append the directories specified in the pth file to the PYTHONPATH, which exhibits the behavior we want. Fixes cedarai#13
|
I think the base paths should actually be relative to the runfiles of the current venv target instead of the root of the repo. This way any generated files / native extensions are also in that path. |
| def install_files(env_path: pathlib.Path, files: List[EnvFile], add_pth: bool) -> None: | ||
| site_packages_path = find_site_packages(env_path) | ||
| pth = site_packages_path / "venv.pth" | ||
| pths = set() |
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 think we want an ordered set here to be more correct
Currently, rules_pyvenv installs all the python deps by symlinking them in site-packages. However, in bazel, the python deps are actually added to the PYTHONPATH, which means that for some deps, just symlinking into site-packages doesn't work.
However, venv supports this via pth files. pth files will append the directories specified in the pth file to the PYTHONPATH, which exhibits the behavior we want.
Fixes #13