bazel-contrib / bazel-lib

Common useful functions for writing BUILD files and Starlark macros/rules
Apache License 2.0
136 stars 87 forks source link

[Bug] `tar` rule doesn't embed runfiles when `--build_runfile_manifests=false` #936

Open cerisier opened 3 weeks ago

cerisier commented 3 weeks ago

When using the tar rule along with --build_runfiles_manifests=false, runfiles are not added to the archive. I believe this is due to the way the tar rule expects the existence of the MANIFEST to deduce the runfiles directory.

https://github.com/bazel-contrib/bazel-lib/blob/408f76cb9ae1403c42014a5f2b02738cfd576507/lib/private/tar.bzl#L227-L230

and

https://github.com/bazel-contrib/bazel-lib/blob/408f76cb9ae1403c42014a5f2b02738cfd576507/lib/private/tar.bzl#L123-L126

The problem is:

I wonder if there is a reliable way to deduce the presence of runfiles, other than checking on existence of MANIFEST file. And a way to add the runfiles_dir mtree_line only if we know there will be runfiles.

I wonder if one solution wouldn't be to deduce a runfiles_dir from any src[DefaultInfo].default_info.files_to_run.executable and add an mtree_line for it even in the case where there is no runfiles at all.

This would result in having a .runfiles directory for each executable even in the rare event where there are no runfiles at all.

If that's acceptable, I can submit a PR.

cerisier commented 3 weeks ago

Maybe @fmeum would know something about that.

fmeum commented 3 weeks ago

I agree that creating a .runfiles directory for each executable is the correct approach. Runfiles are never empty as they always at least contain the executable itself. The logic that inspects runfiles_manifest can be removed.

Happy to review a PR!