inconsistent handling of DefaultInfo runfiles in multiple rules (sh_*, java_binary,...)

[dmivankov]

Description of the problem / feature request:

Setting data attribute of sh_binary adds file to runfiles if it is a filegroup but not if it is a simple DefaultInfo output (files=[..], but no runfiles=).
For sh_library file is added to runfiles in both cases and then it can be propagated to sh_binary using that sh_library.

I'd expect sh_binary to collect runfiles from it's own and its sh_library targets consistently, either collect all with simple DefaultInfo, or collect none with simple DefaultInfo

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

script.sh

find -name "data.txt*"

data.txt

123

rule.bzl

def _copy_file_impl(ctx):
    out = ctx.actions.declare_file("%s-copy" % ctx.file.src.short_path)
    ctx.actions.run_shell(
        inputs = [ctx.file.src],
        command = "cp \"{path}\" \"{out}\"".format(path = ctx.file.src.path, out = out.path),
        outputs = [out],
    )
    return DefaultInfo(files = depset([out])) # note that we don't specify ", runfies = ctx.runfiles(files = [out])"
copy_file = rule(
    attrs = {
        "src": attr.label(
            mandatory = True,
            allow_single_file=True
        ),
    },
    implementation = _copy_file_impl,
)

BUILD

load("//:rule.bzl", "copy_file")

filegroup(
	name = "group",
	srcs = [":data.txt"],
)

copy_file( # in real applications this could be a more complex rule
	name = "copy",
	src = ":data.txt",
)

sh_binary(
	name = "script_ok",
	srcs = [":script.sh"],
	data = [":group"],
)

sh_binary(
	name = "script_bad",
	srcs = [":script.sh"],
	data = [":copy"],
)

sh_library(
	name = "workaround",
	data = [":copy"],
)

sh_binary(
	name = "script_workaround",
	srcs = [":script.sh"],
	deps = [":workaround"],
        # note that we can't use $(rootpath :copy) in attributes here unless we keep add data=[":copy"] too
)

Testing

$ bazel run :script_ok 2>/dev/null
./data.txt
$ bazel run :script_bad 2>/dev/null
$ bazel run :script_workaround 2>/dev/null
./data.txt-copy

### What operating system are you running Bazel on?

NixOS

### What's the output of `bazel info release`?

release 3.3.1- (@non-git)

### If `bazel info release` returns "development version" or "(@non-git)", tell us how you built Bazel.

From NixOS nixpkgs collection

[divanorama]

Also noticed comment reporting same thing in 2018 #1147 (comment)
With a potential fix merged in #6352

[divanorama]

Also #4519
And a clue in #6783
So sh_library collects data to runfiles explicitly

bazel/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShLibrary.java

Line 39 in d1c0d20

.addAll(ruleContext.getPrerequisiteArtifacts("data", Mode.DONT_CHECK).list())

which probably doesn't happen with machinery used by sh_binary/sh_test

[divanorama]

So fixing sh_binary/sh_test to promote data to runfiles should be fine per https://docs.bazel.build/versions/master/be/common-definitions.html#common.data

Targets named in the data attribute will appear in the *.runfiles area of this rule, if it has one. This may include data files needed by a binary or library, or other programs needed by it.

cc_binary and py_binary don't include "data" attribute into runfiles too, didn't check cc_library/py_library yet

[divanorama]

cc_library & py_library unlike sh_library do not add "data" to runfiles (when it is DefaultInfo without runfiles)

There may be a use-case for data-dependency that has runfiles != files though.

https://docs.bazel.build/versions/master/skylark/lib/DefaultInfo.html#parameters

files: objects representing the default outputs to build when this target is specified on the bazel command line
By default it is all predeclared outputs.

Few notable things that would be nice to have here

• point out that declared outputs (either via ctx.declare_ or rule(outputs=)) don't have to be put into DefaultInfo and can be accessed both from cli and from rules (didn't fully check it yet)
• clarify that actually default outputs aren't only for cli, they also matter for ctx.file(s) https://docs.bazel.build/versions/master/skylark/lib/ctx.html#file

runfiles: files that this target needs when run (via the run command or as a tool dependency)

Here it doesn't look like that it applies to a case where a rule generates a "data" file output, "data" file isn't a tool or runnable

Unclear part with "data" is

Targets named in the data attribute will appear in the *.runfiles area

What is a "target" here? For a target with single file output and no other providers, looks quite logical that this file should be in runfiles. Probably all target outputs as well as runfiles should be used, and for deps/tools case it may be different.

And another thing is that filegroup promotes srcs (aka outputs) as data runfiles
https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/filegroup/Filegroup.java#L96

So, should any rules taking data parameter put all files from data to runfiles?

[dmivankov]

java_binary also doesn't add data= to runfiles

[brandjon]

Dup in #15043, also probably dup in #1147.

[brandjon]

Dup #15043 also has associated PR #15052.

[uri-canva]

Can we rename this issue to reflect the fact it applies to a lot of different rules? Also #15052 looks like it resolves this, can anyone who had the original issue confirm?

[fmeum]

Yes, this is fixed by the PR you linked.

[dmivankov]

Looks like it gets resolved for me too, #15052 is included in 6.0.0 release
Updated issue title, feel free to tune it more

[fmeum]

@dmivankov I will close this as it is fixed by 6f6d4cc, which is in Bazel 6. Please let me know if this is still an issue in some cases.