-
Notifications
You must be signed in to change notification settings - Fork 541
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: remove 'site-packages' from install dir prefix #1766
Conversation
d64838d
to
643ea7a
Compare
643ea7a
to
f579aa2
Compare
I am not sure if changing this won't have any unintended consequences. I've linked a few tickets here:
Probably there are more things, but these are the only that come to my head right now. |
+1 to |
re: backwards compatibility concerns: Yeah, these are valid concerns. I'm fine with putting this behind an environment variable to control behavior. This at least removes Matt having to carry a patch indefinitely until we can fix it and he can upgrade to Bazel 7. Then the envvar can just be a no-op and we can drop it. Unfortunately, due to support for Bazel 6.x, our options for addressing the problem of the long PYTHONPATH env var are pretty limited because we can't change the binary generation or bootstrapping in that version of Bazel. With Bazel 7, its a much more solvable problem, as we can now just modify the rules and bootstrap directly (patches welcome). Unfortunately, Matt's situation needs to support Bazel 6.x The only other idea we've been able to come up with is to hide the deps/PyInfo from the binary by having a generated site.py-based target capture it and perform path manipulations. I don't think anyone has tried to implement it yet, though.
Can you explain why it relies on it? There aren't any promises about the unpacked layout of the downloaded packages. The targets are the unit of abstraction.
What draft is this referring to? |
lol, oops, typo. I meant "we" as in the rules_python owners. Patches welcome, too, of course. |
Thanks for the context, I'm fine with this behaviour being gated under a feature flag. |
@mattem, should this PR be continued? |
Just adding a note that I think this will break anyone using the |
Feel free to reopen PR later. Closing to clean things up. |
Removes the
site-packages
path segment in unpacked external dependency installs.This additional path segment causes issues when dealing with very large dependency trees, so much so that the value of
PYTHONPATH
exceeds the OS limit of what can be set.This manifests due to the Python binary launcher adding workspace roots automatically to sys path, in addition to the
site-packages
segmented path, ie thePYTHONPATH
var will end up with both (from printing thePYTHONPATH
inexamples/pip_parse
in rules_python.Before:
After:
In large graphs, this increase in env var length results in a catastrophic failure:
While this behaviour is possible to toggle via
--experimental_python_import_all_repositories
, there are a large number of dependencies that expect these roots to be available (runfiles for example iirc).