py_binary and oci_image don't play nice when mediated via pkg_tar

[bcsgh]

I don't know that this is a bug, of who's bug it is, but this seems like a good place to start trying to make thins better. (Feel free to point me at someone else.)

tl;dr; when pkg_tar is wrapped in the most obvious way around a py_binary so that it can be stuffed in an oci_image, files from the local workspace end up in a different location than the wrapper script that py_binary generates expect them to be in. (Non local files might as well, but things started working for me before I ever observed that.) I make no claim to it being correct, but the following commit seems to resolve the problem by inserting the workspace_name into the a suitable spot in the paths that will used in the tar.

https://github.com/bcsgh/rules_pkg/commit/8adcd2a83dc5a3fd642573cc46af16a774631732

I expect that would be a breaking change for more than a few users. But then again, the existing behavior is wacky enough that it's possible that nobody has anything that works depending on it. (Or maybe not.)

[bcsgh]

Note: seems the relevant code is somewhat new: https://github.com/bazelbuild/rules_pkg/pull/754

[storm-of-fists]

Something interesting here is I put a git_override in my MODULE.bazel

bazel_dep(name = "rules_pkg")
git_override(
    module_name = "rules_pkg",
    remote = "https://github.com/bazelbuild/rules_pkg",
    commit = "994a1f5b94f5baa4904d9487622c2908fe44d2d1",
)

so that it would pull in the changes from that pr above, but im still having issues with certain files not being found. Here is how I build the image up.

py_binary(
    name = "sadie_bot",
    srcs = ["sadie_bot.py"],
    tags = [
        "manual",
        "no-sandbox",
    ],
    deps = [
        "@external_py//discord_py",
    ],
)

pkg_tar(
    name = "sadie_bot_pkg",
    srcs = [
        ":sadie_bot",
    ],
    include_runfiles = True,
)

oci_image(
    name = "sadie_bot_image",
    base = "@python_base",
    entrypoint = ["./sadie_bot"],
    tars = [":sadie_bot_pkg"],

)

And here is the output when I try to run up the container. It seems to enter the script just fine, but we don't seem to be replicating the runfiles exactly.

(.venv) [clatham@clatham-dev sadie_bot]$ docker compose up
[+] Running 1/0
 ✔ Container sadie_bot-sadie_bot-1  Recreated                         0.0s 
Attaching to sadie_bot-sadie_bot-1
sadie_bot-sadie_bot-1  | Traceback (most recent call last):
sadie_bot-sadie_bot-1  |   File "//./sadie_bot", line 559, in <module>
sadie_bot-sadie_bot-1  |     Main()
sadie_bot-sadie_bot-1  |   File "//./sadie_bot", line 490, in Main
sadie_bot-sadie_bot-1  |     assert os.path.exists(main_filename), \
sadie_bot-sadie_bot-1  | AssertionError: Cannot exec() '/./sadie_bot.runfiles/_main/projects/friend_zone/sadie_bot/sadie_bot.py': file not found.

We seem to be missing the _main directory inside the .runfiles directory.

[aiuto]

Is oci_image relevant here? What I mean, is can you repro your issue this way

bazel build //some:foo.tar
mkdir /tmp/test
cp bazel-bin/some/foo.tar /tmp/test
cd /tmp/test
tar xf foo.tar
path/to/pybin_wrapper_script

[bcsgh]

Without actually trying that (I'm ooo right now) I think so. My debugging involved extracting a listing from the tar file. The open question for me is what the layout is supposed to be, which is where OCI enters the picture.

[aiuto]

The only thing that really matters is if you untar a file and expect to run it with a single command line invocation (that is, the wrapper script) it runs. I don't know enough about docker to have much of an opinion on what is going on here. Have you asked in the rules_oci project?

On Tue, Jan 2, 2024 at 2:32 PM bcsgh @.***> wrote:

Without actually trying that (I'm ooo right now) I think so. My debugging involved extracting a listing from the tar file. The open question for me is what the layout is supposed to be, which is where OCI enters the picture.

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_pkg/issues/803#issuecomment-1874456303, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHGTGQ23TDPEEJXY5TTYMROEHAVCNFSM6AAAAABBH72KXSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZUGQ2TMMZQGM . You are receiving this because you commented.Message ID: @.***>

[bcsgh]

My educated guess us that docker/oci isn't at fault here beyond (correctly) placing things beyond access of the untared stuff. (I mostly mentioned it because it's part of the motivating case and I'm not sure that it isn't relevant.)

BTW: another options that might simplify things a lot would be to switch from using File.short_path to File.path and then figure out what prefix to strip off, likely based on something like ctx.bin_dir or ctx.genfiles_dir.

[bcsgh]

Side note: there are some interesting similarities between this issue and another one in a different project: https://github.com/bazelbuild/rules_closure/pull/597

The solution might not be the same across both, but if I had to guess they might be mutually informative.

[aiuto]

It is very likely related. The change that calls the workspace "_main" rather than the WORKSPACE(name="x"), and thus gives different paths between --bzlmod and non-bzlmod build broke a lot of assumptions.

On Wed, Jan 3, 2024 at 1:24 PM bcsgh @.***> wrote:

Side note: there are some interesting similarities between this issue and another one in a different project: bazelbuild/rules_closure#597 https://github.com/bazelbuild/rules_closure/pull/597

The solution might not be the same across both, but if I had to guess they might be mutually informative.

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_pkg/issues/803#issuecomment-1875785642, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHAF2P4JYUDFVA64MI3YMWO43AVCNFSM6AAAAABBH72KXSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZVG44DKNRUGI . You are receiving this because you commented.Message ID: @.***>

[ewianda]

Seeing the same error. The following patch works for us.

--- a/pkg/private/pkg_files.bzl
+++ b/pkg/private/pkg_files.bzl
@@ -69,6 +69,7 @@
         "content_map": "in/out The content_map we are building up",
         "file_deps": "in/out list of file Depsets represented in the map",
         "label": "ctx.label",
+        "workspace_name": "ctx.workspace_name",

         # Behaviors
         "allow_duplicates_with_different_content": "bool: don't fail when you double mapped files",
@@ -116,6 +117,7 @@
         content_map = dict(),
         file_deps = list(),
         label = label,
+        workspace_name = ctx.workspace_name,
         allow_duplicates_with_different_content = allow_duplicates_with_different_content,
         strip_prefix = strip_prefix,
         include_runfiles = include_runfiles,
@@ -429,7 +431,7 @@
             base_file = the_executable or all_files[0]
             base_file_path = mapping_context.path_mapper(
                 dest_path(base_file, data_path, data_path_without_prefix))
-            base_path = base_file_path + ".runfiles"
+            base_path = base_file_path + ".runfiles/" + mapping_context.workspace_name

[bcsgh]

So what's next on this?

[aiuto]

I'm not sure. I've been distracted by other things and not had a chance to look at it.

On Sun, Jan 21, 2024 at 4:01 PM bcsgh @.***> wrote:

So what's next on this?

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_pkg/issues/803#issuecomment-1902762206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHCMVNRKYPYSFER6CI3YPV6Z3AVCNFSM6AAAAABBH72KXSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSG43DEMRQGY . You are receiving this because you commented.Message ID: @.***>

[Naraharirahul]

Seeing the same error. The following patch works for us.

--- a/pkg/private/pkg_files.bzl
+++ b/pkg/private/pkg_files.bzl
@@ -69,6 +69,7 @@
         "content_map": "in/out The content_map we are building up",
         "file_deps": "in/out list of file Depsets represented in the map",
         "label": "ctx.label",
+        "workspace_name": "ctx.workspace_name",

         # Behaviors
         "allow_duplicates_with_different_content": "bool: don't fail when you double mapped files",
@@ -116,6 +117,7 @@
         content_map = dict(),
         file_deps = list(),
         label = label,
+        workspace_name = ctx.workspace_name,
         allow_duplicates_with_different_content = allow_duplicates_with_different_content,
         strip_prefix = strip_prefix,
         include_runfiles = include_runfiles,
@@ -429,7 +431,7 @@
             base_file = the_executable or all_files[0]
             base_file_path = mapping_context.path_mapper(
                 dest_path(base_file, data_path, data_path_without_prefix))
-            base_path = base_file_path + ".runfiles"
+            base_path = base_file_path + ".runfiles/" + mapping_context.workspace_name

So, I came across the same issue and this patch did fix the path by adding workspace name, but I see in v0.10.0, the external folder is no longer available. It just extracts the dependencies inside .runfiles folder. Is there something which would create the external folder too?

[aiuto]

What "external folder". Do you mean @external_py? The rules_pkg version should change that. A different version of rules_python might.

I don't think this is a problem we can solve unilaterally in the packaging side. It depends on the combination of bazel version, if bzlmod is enabled or not, and rules_python version. When I get back from vacation and new role ramp-up, some time in March, I can build out a matrix of things people might do, vs. current behavior, and come up with a new behavior that will generally work for most reasonable setups.

[lalten]

I'm in the same situation: trying to migrate to rules_oci and running into difficulties with rules_pkg.

The ctx.workspace part is definitely needed to make Bazel's language stubs find their runfiles.

The change that calls the workspace "_main" rather than the WORKSPACE(name="x"), and thus gives different paths between --bzlmod and non-bzlmod build broke a lot of assumptions.

From a runfiles finding perspective, nothing has changed between workspace and bzlmod. You may find inspiration in https://github.com/lalten/rules_appimage/blob/bffa8ee991a6cae5c82ed3f180ad3ff4b2bf4867/appimage/private/runfiles.bzl#L38-L39

[lalten]

Here's @ewianda's patch again, updated for release 0.10.1:

--- pkg/private/pkg_files.bzl
+++ pkg/private/pkg_files.bzl
@@ -69,6 +69,7 @@ _MappingContext = provider(
         "content_map": "in/out The content_map we are building up",
         "file_deps": "in/out list of file Depsets represented in the map",
         "label": "ctx.label",
+        "workspace_name": "ctx.workspace_name",

         # Behaviors
         "allow_duplicates_with_different_content": "bool: don't fail when you double mapped files",
@@ -116,6 +117,7 @@ def create_mapping_context_from_ctx(
         content_map = dict(),
         file_deps = list(),
         label = label,
+        workspace_name = ctx.workspace_name,
         allow_duplicates_with_different_content = allow_duplicates_with_different_content,
         strip_prefix = strip_prefix,
         include_runfiles = include_runfiles,
@@ -428,7 +430,7 @@ def add_from_default_info(
             # the first file of DefaultInfo.files should be the right target.
             base_file = the_executable or all_files[0]
             base_file_path = dest_path(base_file, data_path, data_path_without_prefix)
-            base_path = base_file_path + ".runfiles"
+            base_path = base_file_path + ".runfiles/" + mapping_context.workspace_name

             for rf in runfiles.files.to_list():
                 d_path = mapping_context.path_mapper(base_path + "/" + rf.short_path)

[ewianda]

Can close. Fixed in https://github.com/bazelbuild/rules_pkg/pull/856