SkyframeLoadingPhaseRunner: implement --compile_one_dependency.

Also fix a bug in the LoadingPhaseRunner - we weren't printing an error for
failed targets, duh!

--
MOS_MIGRATED_REVID=114310591

3 files changed, 69 insertions, 10 deletions

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 85766dac93..7caddf6329 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
@@ -17,6 +17,7 @@ import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildType;
@@ -37,12 +38,11 @@ import java.util.List;
 /**
  * Implementation of --compile_one_dependency.
  */
-final class CompileOneDependencyTransformer {
+public final class CompileOneDependencyTransformer {
+  private final TargetProvider targetProvider;
 
-  private final PackageManager pkgManager;
-
-  public CompileOneDependencyTransformer(PackageManager pkgManager) {
-    this.pkgManager = pkgManager;
+  public CompileOneDependencyTransformer(TargetProvider targetProvider) {
+    this.targetProvider = targetProvider;
   }
 
   /**
@@ -100,7 +100,7 @@ final class CompileOneDependencyTransformer {
       try {
         // The call to getSrcTargets here can be removed in favor of the
         // rule.getLabels() call below once we update "srcs" for all rules.
-        if (SrcTargetUtil.getSrcTargets(eventHandler, rule, pkgManager).contains(target)) {
+        if (SrcTargetUtil.getSrcTargets(eventHandler, rule, targetProvider).contains(target)) {
           if (rule.getRuleClassObject().isPreferredDependency(target.getName())) {
             return rule;
           } else if (fallbackRule == null) {
@@ -148,10 +148,11 @@ final class CompileOneDependencyTransformer {
 
     try {
       // If the rule has source targets, return it.
-      if (!SrcTargetUtil.getSrcTargets(eventHandler, result, pkgManager).isEmpty()) {
+      if (!SrcTargetUtil.getSrcTargets(eventHandler, result, targetProvider).isEmpty()) {
         return result;
       }
     } catch (NoSuchThingException e) {
+      eventHandler.handle(Event.error(e.getMessage()));
       throw new TargetParsingException(
           "Couldn't find dependency on target '" + target.getLabel() + "'");
     } catch (InterruptedException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index 0f6476f577..ea30c0584c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -22,10 +22,13 @@ import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.pkgcache.CompileOneDependencyTransformer;
 import com.google.devtools.build.lib.pkgcache.FilteringPolicies;
 import com.google.devtools.build.lib.pkgcache.LoadingOptions;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
+import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
+import com.google.devtools.build.lib.skyframe.EnvironmentBackedRecursivePackageProvider.MissingDepException;
 import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue.TargetPatternList;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternSkyKeyOrException;
@@ -209,11 +212,20 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       }
     }
 
+    ResolvedTargets<Target> result = builder.build();
     if (compileOneDependency) {
-      // TODO(ulfjack): Add support for compile_one_dependency before hooking this up.
-      throw new UnsupportedOperationException();
+      TargetProvider targetProvider = new EnvironmentBackedRecursivePackageProvider(env);
+      try {
+        return new CompileOneDependencyTransformer(targetProvider)
+            .transformCompileOneDependency(env.getListener(), result);
+      } catch (MissingDepException e) {
+        return null;
+      } catch (TargetParsingException e) {
+        env.getListener().handle(Event.error(e.getMessage()));
+        return ResolvedTargets.failed();
+      }
     }
-    return builder.build();
+    return result;
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index e56117e454..1e0b234d89 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -550,6 +550,52 @@ public class LoadingPhaseRunnerTest {
     tester.assertContainsEventWithFrequency("invalid value in 'bash_version' attribute", 1);
   }
 
+  @Test
+  public void testCompileOneDependency() throws Exception {
+    tester.addFile("base/BUILD",
+        "cc_library(name = 'hello', srcs = ['hello.cc'])");
+    tester.useLoadingOptions("--compile_one_dependency");
+    LoadingResult loadingResult = assertNoErrors(tester.load("base/hello.cc"));
+    assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//base:hello"));
+  }
+
+  @Test
+  public void testCompileOneDependencyNonExistentSource() throws Exception {
+    tester.addFile("base/BUILD",
+        "cc_library(name = 'hello', srcs = ['hello.cc', '//bad:bad.cc'])");
+    tester.useLoadingOptions("--compile_one_dependency");
+    try {
+      tester.load("base/hello.cc");
+      fail();
+    } catch (TargetParsingException expected) {
+      tester.assertContainsError("no such package 'bad'");
+    }
+  }
+
+  @Test
+  public void testCompileOneDependencyNonExistentSourceKeepGoing() throws Exception {
+    tester.addFile("base/BUILD",
+        "cc_library(name = 'hello', srcs = ['hello.cc', '//bad:bad.cc'])");
+    tester.useLoadingOptions("--compile_one_dependency");
+    if (runsLoadingPhase()) {
+      // The LegacyLoadingPhaseRunner throws an exception if it can't load any of the sources in the
+      // same rule as the source we're looking for even with --keep_going.
+      // In general, we probably want --compile_one_dependency to be compatible with --keep_going
+      // for consistency, but it's unclear if this is actually a problem for anyone. The most common
+      // use case for compile_one_dependency is to iterate quickly on a single file, without
+      // --keep_going.
+      try {
+        tester.load("base/hello.cc");
+        fail();
+      } catch (TargetParsingException expected) {
+        tester.assertContainsError("no such package 'bad'");
+      }
+    } else {
+      LoadingResult loadingResult = tester.loadKeepGoing("base/hello.cc");
+      assertThat(loadingResult.hasTargetPatternError()).isTrue();
+    }
+  }
+
   private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception {
     try {
       tester.load(targetPattern);