Add tool executables (from FilesToRunProvider) to action inputs.

[jmillikin]

Fixes #7390

Summary: rules are allowed to set DefaultInfo(files = depset(), executable = ...). If the resulting targets are used as tool inputs to another target, the executable will not be added to the downstream targets' action inputs. This manifests as either an execvp() error (from ctx.actions.run()) or a Bash error in genrules.

This PR fixes that so the use case of non-default executables works as expected. This is useful for building scripts with complex runfiles, where bazel run //:something and bazel build //:something && bazel-bin/something cannot be supported by the same output file.

cc @ccate @jmmv

[irengrig]

/cc @lberki I am not sure about the team label here

[lberki]

Well, that's an interesting one. I didn't expect that we allow the creation of a FilesToRunProvider where the executable is not in getFiles().

Since we use FilesToRunProvider in many places other than CommandHelper, I think this pull request only fixes a part of the problem. I see two viable solutions:

1. Make it an error to create a FilesToRunProvider where the executable is not in files. This would be an incompatible change and it appears that there are places where this is used.
2. Just silently add the executable to FilesToRunProvider#getFiles() when it's not already there. This would cause a minor amount of RAM wastage.

I think the best approach is to do something halfway between: do (1) for call sites from Java and (2) for call sites from Starlark.

@laurentlb , WDYT?

[benjaminp]

A similar situation happens with the presence of the executable in the runfiles. That case is handled with an implementation similar to what lberki suggests here. Broken native rules receive an IllegalStateException. Starlark rules typically implicitly have the executable added to their runfiles or receive a Starlark-level error in some more obscure cases (at least since #6607 fixed the Java-level crash leaking through).

[jmillikin]

New commit added to implement "Just silently add the executable to FilesToRunProvider#getFiles() when it's not already there." -- there seems to be only one place in the code where FilesToRunProvider can get constructed with that condition, and I couldn't figure out how to tell whether the caller is from Starlark or Java.

[lberki]

This will create an extra NestedSet instance for every call site, most importantly, in the constructor of FileConfiguredTarget, which means an extra object for each input file, which is probably unacceptable overhead.

How about special-casing at least #fromSingleExecutableArtifact() so that this overhead is not present?

Given that the only other call site is in RuleConfiguredTargetBuilder#build(), I'd prefer putting logic there because, well, that place already has logic. In addition to being a personal preference, you also have more information, notably, ruleContext.getRule().getRuleClassObject().isSkylark(), which you can use to determine whether this is a Starlark rule and thus adding the binary to the nested set is necessary.

Even better is changing SkylarkRuleConfiguredTargetUtil#addSimpleProviders() -- that place has control over both the executable and filesToBuildfields ofRuleConfiguredTargetBuilderand is specific to Starlark rules, so you can just add the "executable is forcibly added tofilesToRun`" logic there.

[jmillikin]

Given that the only other call site is in RuleConfiguredTargetBuilder#build(), I'd prefer putting logic there because, well, that place already has logic.

Done.

[lberki]

Added a comment -- TL;DR: I think it's better to tweak SkylarkRuleConfiguredTargetUtil#addSimpleProviders() because that's specific to Starlark.

What I'm slightly unhappy about is that you probably can't just assert whether the executable is in filesToRun for Java rules because you'd need to iterate over the contents of the latter nested set just for error checking, which sounds wasteful.

[jmillikin]

addSimpleProviders() seems to be used only for constructing filesToBuild, but this change is about adding the executable to filesToRun.

I've expanded RuleConfiguredTargetBuilder::build() (via buildFilesToRun()) to inject the executable. It's applied to all rules, since as you note it's difficult to figure out whether an Artifact is present in a NestedSet without iterating over it.

[jmillikin]

Gentle ping @lberki

[philwo]

@lberki As requested, here's your ping. :) Please let me know if I can import and cherry-pick this into Bazel 1.2.0.

[lberki]

I'm sorry for the delay. I've had a little too many issues that landed on my desk in the last few days.