-
-
Notifications
You must be signed in to change notification settings - Fork 570
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
feat: allow populating binary's venv site-packages with symlinks #2617
Open
rickeylev
wants to merge
8
commits into
bazel-contrib:main
Choose a base branch
from
rickeylev:venv.site.packages
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
01be3e2
wip: libs in venv site packages
rickeylev 0ed66f8
add docs for features.bzl
rickeylev 62207c3
add versionadded info to features docs
rickeylev f2fad71
Merge branch 'main' of https://github.com/bazelbuild/rules_python int…
rickeylev 67872b7
address review comments and finish some todos
rickeylev d4603a4
add other repo for testing with workspace
rickeylev 2f5b354
Merge branch 'main' of https://github.com/bazelbuild/rules_python int…
rickeylev 8e9c731
Merge branch 'main' of https://github.com/bazel-contrib/rules_python …
rickeylev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,8 @@ def _PyInfo_init( | |
direct_original_sources = depset(), | ||
transitive_original_sources = depset(), | ||
direct_pyi_files = depset(), | ||
transitive_pyi_files = depset()): | ||
transitive_pyi_files = depset(), | ||
site_packages_symlinks = depset()): | ||
_check_arg_type("transitive_sources", "depset", transitive_sources) | ||
|
||
# Verify it's postorder compatible, but retain is original ordering. | ||
|
@@ -70,6 +71,7 @@ def _PyInfo_init( | |
"has_py2_only_sources": has_py2_only_sources, | ||
"has_py3_only_sources": has_py2_only_sources, | ||
"imports": imports, | ||
"site_packages_symlinks": site_packages_symlinks, | ||
"transitive_implicit_pyc_files": transitive_implicit_pyc_files, | ||
"transitive_implicit_pyc_source_files": transitive_implicit_pyc_source_files, | ||
"transitive_original_sources": transitive_original_sources, | ||
|
@@ -140,6 +142,31 @@ A depset of import path strings to be added to the `PYTHONPATH` of executable | |
Python targets. These are accumulated from the transitive `deps`. | ||
The order of the depset is not guaranteed and may be changed in the future. It | ||
is recommended to use `default` order (the default). | ||
""", | ||
"site_packages_symlinks": """ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a note that this is still unstable and may be removed/changed in between minor version releases of |
||
:type: depset[tuple[str | None, str]] | ||
|
||
A depset with `topological` ordering. | ||
|
||
Tuples of `(runfiles_path, site_packages_path)`. Where | ||
* `runfiles_path` is a runfiles-root relative path. It is the path that | ||
has the code to make importable. If `None` or empty string, then it means | ||
to not create a site packages directory with the `site_packages_path` | ||
name. | ||
* `site_packages_path` is a path relative to the site-packages directory of | ||
the venv for whatever creates the venv (typically py_binary). It makes | ||
the code in `runfiles_path` available for import. Note that this | ||
is created as a "raw" symlink (via `declare_symlink`). | ||
|
||
:::{tip} | ||
The topological ordering means dependencies earlier and closer to the consumer | ||
have precedence. This allows e.g. a binary to add dependencies that override | ||
values from further way dependencies, such as forcing symlinks to point to | ||
specific paths or preventing symlinks from being created. | ||
::: | ||
|
||
:::{versionadded} VERSION_NEXT_FEATURE | ||
::: | ||
""", | ||
"transitive_implicit_pyc_files": """ | ||
:type: depset[File] | ||
|
@@ -266,6 +293,7 @@ def PyInfoBuilder(): | |
transitive_pyc_files = builders.DepsetBuilder(), | ||
transitive_pyi_files = builders.DepsetBuilder(), | ||
transitive_sources = builders.DepsetBuilder(), | ||
site_packages_symlinks = builders.DepsetBuilder(order = "topological"), | ||
) | ||
return self | ||
|
||
|
@@ -351,6 +379,7 @@ def _PyInfoBuilder_merge_all(self, transitive, *, direct = []): | |
self.transitive_original_sources.add(info.transitive_original_sources) | ||
self.transitive_pyc_files.add(info.transitive_pyc_files) | ||
self.transitive_pyi_files.add(info.transitive_pyi_files) | ||
self.site_packages_symlinks.add(info.site_packages_symlinks) | ||
|
||
return self | ||
|
||
|
@@ -400,6 +429,7 @@ def _PyInfoBuilder_build(self): | |
transitive_original_sources = self.transitive_original_sources.build(), | ||
transitive_pyc_files = self.transitive_pyc_files.build(), | ||
transitive_pyi_files = self.transitive_pyi_files.build(), | ||
site_packages_symlinks = self.site_packages_symlinks.build(), | ||
) | ||
else: | ||
kwargs = {} | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 error out here?
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.
Hm. Yeah. I can't imagine why one would want it on sys.path twice.
Even in the case where there are conflicts creating the symlinks, the later entry on sys.path from the imports attr wouldn't help -- sys.path is coming first. I suppose it would help for namespace packages but, if they're namespace packages, then the files probably wouldn't be conflicting?
Yeah, I'm having a hard time seeing the value in both being set. I'll change it to fail.
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.
Done