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

Runfiles are not included in pkg_tar. #392

Closed
kangnak opened this issue Jul 27, 2021 · 12 comments · Fixed by #398
Closed

Runfiles are not included in pkg_tar. #392

kangnak opened this issue Jul 27, 2021 · 12 comments · Fixed by #398

Comments

@kangnak
Copy link

kangnak commented Jul 27, 2021

It seems include_runfiles = True in pkg_tar() doesn't work after the PR, at least with py_binary. The resulting tar file doesn't include runfiles. And the release version 0.5.0 has the same issue.

@aiuto
Copy link
Collaborator

aiuto commented Jul 27, 2021

Can someone help me out by describing the expected behavior? I am not going to have a lot of time explore the old behavior. If, for example, you had a package (my_app) with a py_binary (my_prog), with a data file (foo), would you expect it to appear in the tar file as

./my_prog
./foo

or as

./my_prog
./my_prog.runfiles/my_app/foo

or as something else?

aiuto added a commit to aiuto/rules_pkg that referenced this issue Jul 28, 2021
This was split out from bazelbuild#376 to make that easier.

I want to get this in place before adding a fix for bazelbuild#392.
The UTF8 tests can wait.
@kangnak
Copy link
Author

kangnak commented Jul 28, 2021

The default behaviour from the version 0.4.0 is to flatten all files by removing directories:

./my_prog
./foo

This is suboptimal and doesn't work in many reasons. So we have to use remap_paths and strip_pefix to keep the directory structure:

pkg_tar(
    name = "my_prog_pkg",
    srcs = [":my_prog"],
    include_runfiles = True,
    remap_paths = {
        "my_app/my_prog.py": "my_prog.runfiles/<repo name>/my_app/my_prog.py",
        "my_app/my_prog": "my_prog",
        "my_app": "my_prog.runfiles/<repo name>/my_app",
        "external": "my_prog.runfiles",
    },
    strip_prefix = "/",
)

So I hope include_runfiles = True should not flatten files in the first place. However this might affect other people who are using the current default behaviour.

@aiuto
Copy link
Collaborator

aiuto commented Jul 28, 2021

It sounds like what you want is to mimic the runfiles layout that Bazel produces.
That should be a distinct option, but it insufficient for the only choice, since it is unique to Bazel and does not match the expectations of any other packaging.

I have a PR ready that mimics the old behavior. That can go in after I add tests.

For the mimic bazel case, what I would do is move the functionality to a new rule, so it can be reused by all pkg_* rules.

pkg_expand_runfiles(
    name = "my_prog_runfiles",
    srcs = [":my_prog"],
)
pkg_tar(
    name = "my_prog_tar",
    srcs = ":my_prog_runfiles",
)
pkg_zip(
    name = "my_prog_zip",
    srcs = ":my_prog_runfiles",
)

@kangnak
Copy link
Author

kangnak commented Jul 28, 2021

Your PR(pkg_expand_runfiles) sounds good to me. Thanks for your works!

aiuto added a commit to aiuto/rules_pkg that referenced this issue Aug 3, 2021
Fixes bazelbuild#392

This does not address the deeper issues with include_runfiles not
being convenient to use. It just gets back to parity with the legacy
feature.

Also fix: Do not complain on duplicates that are safe.
Addresses part of bazelbuild#252
@aiuto aiuto closed this as completed in #398 Aug 9, 2021
aiuto added a commit that referenced this issue Aug 9, 2021
Fixes #392

This does not address the deeper issues with include_runfiles not being convenient to use. It just gets back to parity with the legacy feature.

Also:
- Do not complain on duplicates that are safe. Addresses part of #252
- run buildifier over the changed files, which created a little noise.
@limdor
Copy link
Contributor

limdor commented Aug 13, 2021

@aiuto we cannot update to 0.5.0 because of this issue, do you have a plan on when 0.6.0 would be created? Is there any ticket that I could follow? Thanks

@aiuto
Copy link
Collaborator

aiuto commented Aug 13, 2021 via email

@limdor
Copy link
Contributor

limdor commented Aug 13, 2021

I will see if I can give it a try. Anyway is not really blocking us, we were in a quite old version of rules_pkg and now we will update to 0.4.x. It was more to know if the next version from head would be something like 1 week or more like 5 months.
The fact that we cannot update to 0.5 is not a big issue, just that we need to add some patches that would not be needed otherwise.
My goal is that at some point we can use rules_pkg without patches. Once we are in a post 0.5.x will be easier to see what we still miss and if we should request some features or propose some upstream.

@aiuto
Copy link
Collaborator

aiuto commented Aug 18, 2021 via email

@CareF
Copy link

CareF commented Aug 28, 2022

I'm wondering with this issue, what would be the recommended way of packaging a py_binary target? The executable of a py_binary requires the runfile layout as defined in https://github.com/bazelbuild/bazel//blob/master/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt so with or without include_runfiles = True the packaged py_binary cannot be used to extract really a executable target, it seems.

@dpetrou-continua
Copy link

It sounds like what you want is to mimic the runfiles layout that Bazel produces. That should be a distinct option, but it insufficient for the only choice, since it is unique to Bazel and does not match the expectations of any other packaging.

I have a PR ready that mimics the old behavior. That can go in after I add tests.

For the mimic bazel case, what I would do is move the functionality to a new rule, so it can be reused by all pkg_* rules.

pkg_expand_runfiles(
    name = "my_prog_runfiles",
    srcs = [":my_prog"],
)
pkg_tar(
    name = "my_prog_tar",
    srcs = ":my_prog_runfiles",
)
pkg_zip(
    name = "my_prog_zip",
    srcs = ":my_prog_runfiles",
)

I'm also having trouble using pkg_tar() because it seems that the runfiles are flattened; there's no dir structure. Perhaps I'm doing something wrong. My goal is to create a tar of a cc_binary() that I can then deploy. My program uses a configuration file that's a runfile; all of that works when doing a bazel run; but I now need to package the binary and the config file, and I would hope the actual source code wouldn't need to change in either situation. (Where the situations are: (1) bazel run and (2) untarring when building a Docker image and running the resulting container.)

My C++ code is simply:

  std::string error;
  std::unique_ptr<Runfiles> runfiles(
      Runfiles::Create(program_name, BAZEL_CURRENT_REPOSITORY, &error));
  if (runfiles == nullptr) {
    std::cerr << "Runfiles error: " << error << "\n";
    return EXIT_FAILURE;
  }

@aiuto , it seemed that your proposed pkg_expand_runfiles() would do the trick, but this doesn't seem to be available now.

If there's a better / different approach to accomplish my goal of packaging a cc binary and a data file, please let me know.

Thanks,
David

@dpetrou-continua
Copy link

Adding on to my last note in case it's useful to others:

I then decided to roll my own image:

  • Build my cc_binary.
  • Created a runfiles with no symlinks:
mkdir ./${TARGET}-runfiles
tar zcvhf - -C ./bazel-bin/${PACKAGE}/${TARGET}.runfiles . | tar zxvf - -C ./${TARGET}-runfiles
  • Made my own image. Note the setting of $RUNFILES_DIR and the removal of MANIFEST:
sudo docker build -t foo/${TARGET}:1.0 -f- . <<EOF
FROM ubuntu:jammy

COPY ./${TARGET}-runfiles /${TARGET}-runfiles
WORKDIR /${TARGET}-runfiles
ENV RUNFILES_DIR=/${TARGET}-runfiles
RUN ["/bin/rm", "MANIFEST"]

ENTRYPOINT ["${WORKSPACE}/${PACKAGE}/${TARGET}"]
EOF

This appears to work. And /probably/ would work for any language (not just C++) but I'm not certain of that. I welcome any suggestions for improvements, and I apologize if this is off topic for rules_pkg.

Thanks,
David

@aiuto
Copy link
Collaborator

aiuto commented Jun 22, 2023

Flattening the runfiles seems plain wrong.

rules_docker is slowly becoming abandonware.

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 a pull request may close this issue.

5 participants