Configure coverage and runfiles for sh_library InstrumentedFilesProvider is already configured for sh_binary and sh_test, it's confusing for this to not be configured the same way. This means that you might get coverage for binaries depended on via a data dependency, even if that dependency is indirect (e.g. an sh_library rule whose sources run the binary provides the data dependency). sh_library should also gather runfiles from transitive dependencies in the same way as sh_test and sh_binary. RELNOTES: Configure coverage and runfiles for sh_library. PiperOrigin-RevId: 319092861
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java index e8d85d1..d29ddee 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java
@@ -29,11 +29,9 @@ import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction.LaunchInfo; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector; -import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector.InstrumentationSpec; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; -import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.PathFragment; @@ -97,7 +95,7 @@ .addNativeDeclaredProvider( InstrumentedFilesCollector.collectTransitive( ruleContext, - new InstrumentationSpec(FileTypeSet.ANY_FILE, "srcs", "deps", "data"), + ShCoverage.INSTRUMENTATION_SPEC, /* reportedToActualSources= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER))) .build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShCoverage.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShCoverage.java new file mode 100644 index 0000000..0d2958d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShCoverage.java
@@ -0,0 +1,26 @@ +// Copyright 2020 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.bazel.rules.sh; + +import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector.InstrumentationSpec; +import com.google.devtools.build.lib.util.FileTypeSet; + +/** Common logic for coverage for sh_* rules. */ +final class ShCoverage { + + private ShCoverage() {} + + public static final InstrumentationSpec INSTRUMENTATION_SPEC = + new InstrumentationSpec(FileTypeSet.ANY_FILE, "srcs", "deps", "data"); +}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShLibrary.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShLibrary.java index b36002a..a37d3ac 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShLibrary.java
@@ -22,8 +22,10 @@ import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.TransitionMode; +import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; /** * Implementation for the sh_library rule. @@ -39,13 +41,21 @@ .addAll(ruleContext.getPrerequisiteArtifacts("deps", TransitionMode.TARGET).list()) .addAll(ruleContext.getPrerequisiteArtifacts("data", TransitionMode.DONT_CHECK).list()) .build(); - Runfiles runfiles = new Runfiles.Builder( - ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles()) - .addTransitiveArtifacts(filesToBuild) - .build(); + Runfiles runfiles = + new Runfiles.Builder( + ruleContext.getWorkspaceName(), + ruleContext.getConfiguration().legacyExternalRunfiles()) + .addTransitiveArtifacts(filesToBuild) + .addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES) + .build(); return new RuleConfiguredTargetBuilder(ruleContext) .setFilesToBuild(filesToBuild) .addProvider(RunfilesProvider.class, RunfilesProvider.simple(runfiles)) + .addNativeDeclaredProvider( + InstrumentedFilesCollector.collectTransitive( + ruleContext, + ShCoverage.INSTRUMENTATION_SPEC, + /* reportedToActualSources= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER))) .build(); } }
diff --git a/src/test/shell/bazel/bazel_coverage_sh_test.sh b/src/test/shell/bazel/bazel_coverage_sh_test.sh index bd7b67a..517f0b6 100755 --- a/src/test/shell/bazel/bazel_coverage_sh_test.sh +++ b/src/test/shell/bazel/bazel_coverage_sh_test.sh
@@ -72,12 +72,26 @@ echo "$coverage_file_path" } -function test_sh_test_coverage() { +function set_up_sh_test_coverage() { cat <<EOF > BUILD sh_test( name = "orange-sh", srcs = ["orange-test.sh"], - data = ["//java/com/google/orange:orange-bin"] + data = ["//java/com/google/orange:orange-bin"], +) + +sh_test( + name = "orange-sh-indirect", + srcs = ["orange-test.sh"], + deps = [":orange-sh-lib"], +) + +# This doesn't test all the combinations, it only exercises +# a deps dependency from sh_test and a data dependency from +# sh_library, but they use the same InstrumentedFilesSpec. +sh_library( + name = "orange-sh-lib", + data = ["//java/com/google/orange:orange-bin"], ) EOF cat <<EOF > orange-test.sh @@ -127,11 +141,7 @@ } EOF - bazel coverage --test_output=all //:orange-sh &>$TEST_log || fail "Coverage for //:orange-sh failed" - - local coverage_file_path="$( get_coverage_file_path_from_test_log )" - - cat <<EOF > result.dat + cat <<EOF > expected.dat SF:java/com/google/orange/orangeBin.java FN:3,com/google/orange/orangeBin::<init> ()V FN:5,com/google/orange/orangeBin::main ([Ljava/lang/String;)V @@ -160,8 +170,22 @@ LF:3 end_of_record EOF - diff result.dat "$coverage_file_path" >> $TEST_log - cmp result.dat "$coverage_file_path" || fail "Coverage output file is different than the expected file" +} + +function test_sh_test_coverage() { + set_up_sh_test_coverage + bazel coverage --test_output=all //:orange-sh &>$TEST_log || fail "Coverage for //:orange-sh failed" + local coverage_file_path="$( get_coverage_file_path_from_test_log )" + diff expected.dat "$coverage_file_path" >> $TEST_log + cmp expected.dat "$coverage_file_path" || fail "Coverage output file is different than the expected file for data dep of sh_binary" +} + +function test_sh_test_coverage_indirect() { + set_up_sh_test_coverage + bazel coverage --test_output=all //:orange-sh-indirect &>$TEST_log || fail "Coverage for //:orange-sh-indirect failed" + coverage_file_path="$( get_coverage_file_path_from_test_log )" + diff expected.dat "$coverage_file_path" >> $TEST_log + cmp expected.dat "$coverage_file_path" || fail "Coverage output file is different than the expected file for data dep of sh_library" } function test_sh_test_coverage_cc_binary() {