From ca0e7036ab69673bb6eec3aa45d3e765d9176608 Mon Sep 17 00:00:00 2001 From: John Millikin Date: Sat, 26 Oct 2019 19:46:17 +0900 Subject: [PATCH 1/5] Add tool executables (from FilesToRunProvider) to action inputs. --- .../build/lib/analysis/CommandHelper.java | 1 + .../lib/analysis/actions/SpawnAction.java | 4 ++ src/test/shell/bazel/bazel_rules_test.sh | 55 +++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java index 9d5b6fb9bd94b4..eae3e1c27ec712 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java @@ -197,6 +197,7 @@ 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()); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 7bf508863b6e2e..05373eaf0bf08c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -799,6 +799,10 @@ 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; diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh index ca94833b237cb0..3fe7059fff2266 100755 --- a/src/test/shell/bazel/bazel_rules_test.sh +++ b/src/test/shell/bazel/bazel_rules_test.sh @@ -524,4 +524,59 @@ EOF expect_log "Public or private visibility labels (e.g. //visibility:public or //visibility:private) cannot be used in combination with other labels" } +function test_executable_without_default_files() { + mkdir pkg + cat >pkg/BUILD <<'EOF' +load(":rules.bzl", "bin_rule", "out_rule") +bin_rule(name = "hello_bin") +out_rule(name = "hello_out") + +genrule( + name = "hello_gen", + tools = [":hello_bin"], + outs = ["hello_gen.txt"], + cmd = "$(location :hello_bin) $@", +) +EOF + + cat >pkg/rules.bzl <<'EOF' +def _bin_rule(ctx): + out_sh = ctx.actions.declare_file(ctx.attr.name + ".sh") + ctx.actions.write( + output = out_sh, + content = "#!/bin/sh\necho 'hello world' > $@", + is_executable = True, + ) + return DefaultInfo( + files = depset(direct = []), + executable = out_sh, + ) + +def _out_rule(ctx): + out = ctx.actions.declare_file(ctx.attr.name + ".txt") + ctx.actions.run( + executable = ctx.executable._hello_bin, + outputs = [out], + arguments = [out.path], + mnemonic = "HelloOut", + ) + return DefaultInfo( + files = depset(direct = [out]), + ) + +bin_rule = rule(_bin_rule, executable = True) +out_rule = rule(_out_rule, attrs = { + "_hello_bin": attr.label( + default = ":hello_bin", + executable = True, + cfg = "host", + ), +}) +EOF + + bazel build //pkg:hello_out //pkg:hello_gen >$TEST_log 2>&1 || fail "Should build" + assert_contains "hello world" bazel-bin/pkg/hello_out.txt + assert_contains "hello world" bazel-bin/pkg/hello_gen.txt +} + run_suite "rules test" From 854a9df72c7454f7f3ee755357895e1b60cc6efe Mon Sep 17 00:00:00 2001 From: John Millikin Date: Sun, 27 Oct 2019 11:32:53 +0900 Subject: [PATCH 2/5] Use a batch file on Windows in the test. --- src/test/shell/bazel/bazel_rules_test.sh | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh index 3fe7059fff2266..ef70918a41992d 100755 --- a/src/test/shell/bazel/bazel_rules_test.sh +++ b/src/test/shell/bazel/bazel_rules_test.sh @@ -539,12 +539,26 @@ genrule( ) EOF - cat >pkg/rules.bzl <<'EOF' + # On Windows this file needs to be acceptable by CreateProcessW(), rather + # than a Bourne script. + if "$is_windows"; then + cat >pkg/rules.bzl <<'EOF' +_SCRIPT_EXT = ".bat" +_SCRIPT_CONTENT = "@ECHO OFF\necho hello world > %1" +EOF + else + cat >pkg/rules.bzl <<'EOF' +_SCRIPT_EXT = ".sh" +_SCRIPT_CONTENT = "#!/bin/sh\necho 'hello world' > $@" +EOF + fi + + cat >>pkg/rules.bzl <<'EOF' def _bin_rule(ctx): - out_sh = ctx.actions.declare_file(ctx.attr.name + ".sh") + out_sh = ctx.actions.declare_file(ctx.attr.name + _SCRIPT_EXT) ctx.actions.write( output = out_sh, - content = "#!/bin/sh\necho 'hello world' > $@", + content = _SCRIPT_CONTENT, is_executable = True, ) return DefaultInfo( From 91d16c64943a9ec3d1d47254dced5bab33386a0d Mon Sep 17 00:00:00 2001 From: John Millikin Date: Sat, 2 Nov 2019 22:07:31 +0900 Subject: [PATCH 3/5] Add executable to filesToRun in FilesToRunProvider constructor. --- .../com/google/devtools/build/lib/analysis/CommandHelper.java | 1 - .../devtools/build/lib/analysis/FilesToRunProvider.java | 3 +++ .../devtools/build/lib/analysis/actions/SpawnAction.java | 4 ---- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java index eae3e1c27ec712..9d5b6fb9bd94b4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java @@ -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()); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java index cdffce4f518dbc..5e368061d9be65 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java @@ -44,6 +44,9 @@ public FilesToRunProvider( NestedSet filesToRun, @Nullable RunfilesSupport runfilesSupport, @Nullable Artifact executable) { + if (executable != null) { + filesToRun = NestedSetBuilder.fromNestedSet(filesToRun).add(executable).build(); + } this.filesToRun = filesToRun; this.runfilesSupport = runfilesSupport; this.executable = executable; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 05373eaf0bf08c..7bf508863b6e2e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -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; From 0ab74a54f9c18d5888edb7d5952cc89d0a3dccd4 Mon Sep 17 00:00:00 2001 From: John Millikin Date: Tue, 5 Nov 2019 18:05:09 +0900 Subject: [PATCH 4/5] Move injection of executable into RuleConfiguredTargetBuilder --- .../google/devtools/build/lib/analysis/FilesToRunProvider.java | 3 --- .../build/lib/analysis/RuleConfiguredTargetBuilder.java | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java index 5e368061d9be65..cdffce4f518dbc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/FilesToRunProvider.java @@ -44,9 +44,6 @@ public FilesToRunProvider( NestedSet filesToRun, @Nullable RunfilesSupport runfilesSupport, @Nullable Artifact executable) { - if (executable != null) { - filesToRun = NestedSetBuilder.fromNestedSet(filesToRun).add(executable).build(); - } this.filesToRun = filesToRun; this.runfilesSupport = runfilesSupport; this.executable = executable; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index d5baacfc6a20bb..e08c4fc3ba1df7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -325,6 +325,9 @@ private NestedSet buildFilesToRun( NestedSet runfilesMiddlemen, NestedSet filesToBuild) { filesToRunBuilder.addTransitive(filesToBuild); filesToRunBuilder.addTransitive(runfilesMiddlemen); + if (executable != null) { + filesToRunBuilder.add(executable); + } return filesToRunBuilder.build(); } From 46fd2ce35eb17df92861a390e381e90634fa23d8 Mon Sep 17 00:00:00 2001 From: John Millikin Date: Tue, 5 Nov 2019 19:40:02 +0900 Subject: [PATCH 5/5] Check ruleContext.getRule().getRuleClassObject().isSkylark() --- .../build/lib/analysis/RuleConfiguredTargetBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index e08c4fc3ba1df7..463b0a70856f3f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -325,7 +325,7 @@ private NestedSet buildFilesToRun( NestedSet runfilesMiddlemen, NestedSet filesToBuild) { filesToRunBuilder.addTransitive(filesToBuild); filesToRunBuilder.addTransitive(runfilesMiddlemen); - if (executable != null) { + if (executable != null && ruleContext.getRule().getRuleClassObject().isSkylark()) { filesToRunBuilder.add(executable); } return filesToRunBuilder.build();