diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD b/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD index fa6e37725d5b83..89db56fb6cf688 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD @@ -23,6 +23,7 @@ java_library( ), deps = [ "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", + "//src/main/java/com/google/devtools/build/lib/analysis:config/feature_set", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/cmdline", diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java b/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java index 9c31be1d0ca2ec..5b2d0f67481be7 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.analysis.config.FeatureSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.cmdline.TargetParsingException; @@ -31,6 +32,7 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.packages.Types; import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns; import com.google.devtools.build.lib.util.FileType; import java.util.Collection; @@ -106,14 +108,20 @@ private Target transformCompileOneDependency(ExtendedEventHandler eventHandler, TargetPatterns.Code.DEPENDENCY_NOT_FOUND); } - // TODO(djasper): Check whether parse_headers is disabled and just return if not. - // If the rule has source targets, return it. + // We want a rule where some action processes the input. + // We should avoid cc_library rules that describe a set of headers but don't compile them. + + // If parse_headers is (probably) enabled, then this rule has a CppCompileHeader action. + if (hasParseHeadersHeuristic(result)) { + return result; + } + // If the rule has source targets, return it: one of those sources will parse the header. if (result.getRuleClassObject().hasAttr("srcs", BuildType.LABEL_LIST) && !RawAttributeMapper.of(result).getMergedValues("srcs", BuildType.LABEL_LIST).isEmpty()) { return result; } - // Try to find a rule in the same package that has 'result' as a dependency. + // Else, find a rule in the same package that has 'result' as a dependency. for (Rule rule : orderedRuleList) { RawAttributeMapper attributes = RawAttributeMapper.of(rule); // We don't know which path to follow for configurable attributes, so skip them. @@ -136,6 +144,29 @@ private Target transformCompileOneDependency(ExtendedEventHandler eventHandler, return result; } + boolean hasParseHeadersHeuristic(Rule rule) { + // We want to know whether the "parse_headers" toolchain feature is enabled or disabled. + // At load time we can't really know, so check for the common static configuration sources. + // (We ignore parse_headers being disabled through the toolchain & by rule implementations). + + FeatureSet mergedFeatures = rule.getPackage().getPackageArgs().features(); + RawAttributeMapper ruleAttrs = RawAttributeMapper.of(rule); + if (ruleAttrs.has("features", Types.STRING_LIST) && !ruleAttrs.isConfigurable("features")) { + FeatureSet ruleFeatures = FeatureSet.parse(ruleAttrs.get("features", Types.STRING_LIST)); + mergedFeatures = FeatureSet.merge(mergedFeatures, ruleFeatures); + } + + if (mergedFeatures.on().contains("parse_headers")) { + return true; + } + if (mergedFeatures.off().contains("parse_headers")) { + return false; + } + + // We assume parse_headers is on globally, unless disabled locally. + return true; + } + /** * Returns a list of rules in the given package sorted by BUILD file order. When * multiple rules depend on a target, we choose the first match in this list (after diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java index 6b6020280c1cbf..68db7b4a3ffede 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java @@ -546,6 +546,46 @@ public void testConfigurableCopts() throws Exception { .containsExactlyElementsIn(labels("//a:foo_select")); } + @Test + public void testHeaderOnlyLibrary() throws Exception { + // By default, we assume parse_headers is enabled (via --features + toolchain). + scratch.file( + "a/BUILD", + "cc_library(name = 'h', hdrs = ['h.h'])", + "cc_library(name = 'l', srcs = ['l.cc'], deps = [':h'])"); + assertThat(parseListCompileOneDep("a/h.h")).containsExactlyElementsIn(labels("//a:h")); + + // parse_headers explicitly disabled on the header-only target, use its reverse dep. + scratch.file( + "b/BUILD", + "cc_library(name = 'h', hdrs = ['h.h'], features = ['-parse_headers'])", + "cc_library(name = 'l', srcs = ['l.cc'], deps = [':h'])"); + assertThat(parseListCompileOneDep("b/h.h")).containsExactlyElementsIn(labels("//b:l")); + + // ... but if it has sources, the target itself is ok. + scratch.file( + "c/BUILD", + "cc_library(name = 'h', hdrs = ['h.h'], srcs = ['h.cc'], features = ['-parse_headers'])", + "cc_library(name = 'l', srcs = ['l.cc'], deps = [':h'])"); + assertThat(parseListCompileOneDep("c/h.h")).containsExactlyElementsIn(labels("//c:h")); + + // parse_headers disabled in the package + scratch.file( + "d/BUILD", + "package(features = ['-parse_headers'])", + "cc_library(name = 'h', hdrs = ['h.h'])", + "cc_library(name = 'l', srcs = ['l.cc'], deps = [':h'])"); + assertThat(parseListCompileOneDep("d/h.h")).containsExactlyElementsIn(labels("//d:l")); + + // parse_headers disabled in the package and enabled on the target, so enabled + scratch.file( + "e/BUILD", + "package(features = ['-parse_headers'])", + "cc_library(name = 'h', hdrs = ['h.h'], features = ['parse_headers'])", + "cc_library(name = 'l', srcs = ['l.cc'], deps = [':h'])"); + assertThat(parseListCompileOneDep("e/h.h")).containsExactlyElementsIn(labels("//e:h")); + } + @Test public void testFallBackToHeaderOnlyLibrary() throws Exception { scratch.file("a/BUILD", "cc_library(name = 'h', hdrs = ['a.h'], features = ['parse_headers'])");