Update --compile_one_dependency heuristics for header-only libraries

Previously, if a header was part of a header_only library :a, then
`--compile_one_dependency` would prefer to choose :b which depends on `:a`, rather
than `:a` itself.
After this change, it will generally prefer `:a`, unless parse_headers is turned
off specifically in this package (in which case it is likely that a.h can only
be understood in the context of `b.cc`).

RELNOTES: `--compile_one_dependency` selects header-only `cc_library`s in more cases
PiperOrigin-RevId: 656460540
Change-Id: Iba70dddd1610a78d71c05274d77d479c84efade7

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",

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

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'])");