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.

Sign up for GitHub

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

Add tool executables (from FilesToRunProvider) to action inputs. #10110

Closed
wants to merge 5 commits into from
Closed

Add tool executables (from FilesToRunProvider) to action inputs. #10110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca0e703
Add tool executables (from FilesToRunProvider) to action inputs.
jmillikin Oct 26, 2019
854a9df
Use a batch file on Windows in the test.
jmillikin Oct 27, 2019
91d16c6
Add executable to filesToRun in FilesToRunProvider constructor.
jmillikin Nov 2, 2019
0ab74a5
Move injection of executable into RuleConfiguredTargetBuilder
jmillikin Nov 5, 2019
46fd2ce
Check ruleContext.getRule().getRuleClassObject().isSkylark()
jmillikin Nov 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ private CommandHelper(
Artifact executableArtifact = tool.getExecutable();
// If the label has an executable artifact add that to the multimaps.
if (executableArtifact != null) {
resolvedToolsBuilder.add(executableArtifact);
mapGet(tempLabelMap, label).add(executableArtifact);
// Also send the runfiles when running remotely.
toolsRunfilesBuilder.add(tool.getRunfilesSupplier());
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public FilesToRunProvider(
NestedSet<Artifact> filesToRun,
@Nullable RunfilesSupport runfilesSupport,
@Nullable Artifact executable) {
if (executable != null) {
filesToRun = NestedSetBuilder.fromNestedSet(filesToRun).add(executable).build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
this.filesToRun = filesToRun;
this.runfilesSupport = runfilesSupport;
this.executable = executable;
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,10 +799,6 @@ public Builder addTool(Artifact tool) {
* source code).
*/
public Builder addTool(FilesToRunProvider tool) {
Artifact executable = tool.getExecutable();
if (executable != null) {
addTool(executable);
}
addTransitiveTools(tool.getFilesToRun());
addRunfilesSupplier(tool.getRunfilesSupplier());
return this;
Expand Down