-
-
Notifications
You must be signed in to change notification settings - Fork 634
fix(venv): symlink shared libraries directly #3331
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(venv): symlink shared libraries directly #3331
Conversation
9b137a5
to
f044f34
Compare
f044f34
to
438076b
Compare
@hartikainen Give this a try? I tried this with your repro and things appear to work (I don't have a gpu on my machine, so the jax call to get gpu devices failed, but otherwise everything imported successfully). This commit has the patch to apply: https://github.com/bazel-contrib/rules_python/commit/add0c262ed6cabe50df9266157ac74aca384f3f9.patch Otherwise, this is almost ready, just need to do some cleanup for CI. |
…into fix.symlink.shared.libs.directly
PTAL. Cleaned up and ready |
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.
Should we add a CHANGELOG item to mark that this has been fixed?
@gemini-code-assist /review |
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.
Code Review
This pull request addresses an issue with shared library loading in venvs by introducing a mechanism to symlink them directly, which is a solid approach to resolving the problem with $ORIGIN
resolution. The changes are well-structured and include comprehensive tests for the new logic, which is excellent.
I've identified a critical issue: some leftover debugging code that will cause build failures. Additionally, there's a high-severity issue regarding platform-specific logic that needs to be addressed for correctness on Windows, and a minor typo in a new test file. Once these points are addressed, the PR will be in great shape.
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.
PTAL
Very nice! Thank you. |
Very nice indeed! I just tested this from main...hartikainen:rules_python:venv-site-packages-investigation and this work as expected. Thank you so much @rickeylev and @aignas 🙌 |
It seems
$ORIGIN
resolves prior to symlink resolution. This makes it resolvedifferently depending on if the directory or file itself is symlinked.
To fix, special case shared libraries and have them symlinked directly. Since an
explicit file is the target,
VenvSymlinkEntry.link_to_file
is added to hold theFile object that will be linked to.
An unfortunate side-effect of this logic is any package with
lib*.so
fileswill be more expensive to build (depset flattened at analysis time, more files
symlinked), but it beats not working at all. Optimizing that can be done in
another change.
Tests added to generate libraries that look like what something from PyPI
does. Manually verified a case using jax and jax plugins.
Fixes #3228