First cut at runfiles support in pkg_* rules#605
Merged
aiuto merged 13 commits intobazelbuild:mainfrom Sep 15, 2022
Merged
Conversation
|
So I've tried this with the most recent rules_python to package a Python binary runfiles, and here's the problem I'm encountering: there's
So here's a suggestion: perhaps this should take |
Collaborator
Author
|
That's going to have to be a second pass.
When we start doing symlinks instead of files, it's not obvious what is
correct:
- create symlink and hope the file is there
- copy the file to the expected place and create the link
- copy the target to the location of the symlink and make it a real file.
The first choice is probably wrong.
If the second choice, what if the expected place is outside the workspace?
The third one seems right, and yet still wrong.
It's going to take some poking about to cover all the issues.
…On Tue, Sep 13, 2022 at 8:10 PM depthwise ***@***.***> wrote:
So I've tried this with the most recent rules_python to package a Python
binary runfiles, and here's the problem I'm encountering: there's
DefaultInfo.default_runfiles.root_symlinks that rules_pkg doesn't seem
take into account. This breaks py_binary in 2 ways:
1. bazel_tools/tools/python/py3wrapper.sh is missing (although it is
present under external, just not directly at root)
2. Python bootstrap expects the various external PIP modules to be in
runfiles root (they are symlinked when building in bazel), but because
these are, well, root_symlinks, they're missing too.
So here's a suggestion: perhaps this should take root_symlinks into
account? I know this property is undocumented, but it is very much a thing.
—
Reply to this email directly, view it on GitHub
<#605 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXHHHBYFRBCC7GIG2TVZ2TV6EJXLANCNFSM54UOACZA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
nacl
approved these changes
Sep 14, 2022
Collaborator
nacl
left a comment
nacl
left a comment
There was a problem hiding this comment.
LGTM all things considered, but I think the commit message heading could be made more specific and call out exactly what happened.
BTW, it looks like pkg_zip doesn't even support include_runfiles as an attribute, so that may need to be added in and tested.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Add include_runfiles support to the manifest builder used by pkg_tar and pkg_zip. This pulls in runfiles in the same layout as bazel does. This is not a great layout, but it is understandable. For example, an executable with a
datadep might look likeNote the duplication of an_executable. That could be expensive, so we will eventually want a way to suppress that.
Next steps:
pkg_tarandpkg_zip.include_runfilestopkg_files.See #153 for overall design and features to be implemented.