Skip to content
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

First cut at runfiles support in pkg_* rules #605

Merged
merged 13 commits into from
Sep 15, 2022
Merged

Conversation

aiuto
Copy link
Collaborator

@aiuto aiuto commented Jul 26, 2022

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 data dep might look like

foo/an_exectuable
foo/an_executable.runfiles/foo/an_excutable
foo/an_executable.runfiles/foo/data.txt

Note the duplication of an_executable. That could be expensive, so we will eventually want a way to suppress that.

Next steps:

  • Add e2e tests for pkg_tar and pkg_zip.
  • Add include_runfiles to pkg_files.

See #153 for overall design and features to be implemented.

@aiuto aiuto requested a review from nacl as a code owner July 26, 2022 04:06
@aiuto aiuto marked this pull request as draft July 26, 2022 04:06
@aiuto aiuto added this to the runfiles milestone Jul 26, 2022
@aiuto aiuto marked this pull request as ready for review July 26, 2022 16:18
@depthwise
Copy link

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.

@aiuto
Copy link
Collaborator Author

aiuto commented Sep 14, 2022 via email

Copy link
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pkg/private/pkg_files.bzl Show resolved Hide resolved
pkg/private/pkg_files.bzl Show resolved Hide resolved
@aiuto aiuto merged commit 2188879 into bazelbuild:main Sep 15, 2022
@aiuto aiuto deleted the runfiles branch September 15, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants