Fetching symlink inputs for top-level output

Top-level output files can be symlinks to non-top-level output files, such as
in the case of a cc_binary linking against a .so built in the same build.

I reuse the existing mayInsensitivelyPropagateInputs() method, which was
originally introduced for action rewinding, but seems to have exactly the
intended semantics here as well.

Unfortunately, this requires a small change to the BlazeModule API to pass in
the full analysis result (so we can access the action graph, so we can read
the aformentioned flag).

I considered using execution-phase information on whether an output is a
symlink. However, at that point it's too late! The current design only allows
pulling an output file *when its generating action runs*. It would be better if
this was changed to pull output files independently of action execution. That
would avoid a lot of problems with the current design, such as bazelbuild#10902.

Fixes bazelbuild#11532.

Change-Id: Iaf1e48895311fcf52d9e1802d53598288788a921

src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java

@@ -99,7 +99,7 @@ public interface ActionExecutionMetadata extends ActionAnalysisMetadata {
    * contents are equal to their inputs' contents, and a symlink action does not consume its inputs'
    * contents.
    *
-   * <p>This property is relevant for action rewinding.
+   * <p>This property is relevant for action rewinding and top-level output fetching.
    */
   default boolean mayInsensitivelyPropagateInputs() {
     return false;

src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java

@@ -126,7 +126,7 @@ public static AnalysisResult execute(
       }
 
       for (BlazeModule module : env.getRuntime().getBlazeModules()) {
-        module.afterAnalysis(env, request, buildOptions, analysisResult.getTargetsToBuild());
+        module.afterAnalysis(env, request, buildOptions, analysisResult);
       }
 
       reportTargets(env, analysisResult);

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -26,8 +26,10 @@
 import com.google.common.flogger.GoogleLogger;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.AnalysisResult;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
@@ -496,13 +498,13 @@ public void afterAnalysis(
       CommandEnvironment env,
       BuildRequest request,
       BuildOptions buildOptions,
-      Iterable<ConfiguredTarget> configuredTargets) {
+      AnalysisResult analysisResult) {
     if (remoteOutputsMode != null && remoteOutputsMode.downloadToplevelOutputsOnly()) {
       Preconditions.checkState(actionContextProvider != null, "actionContextProvider was null");
       boolean isTestCommand = env.getCommandName().equals("test");
       TopLevelArtifactContext artifactContext = request.getTopLevelArtifactContext();
       ImmutableSet.Builder<ActionInput> filesToDownload = ImmutableSet.builder();
-      for (ConfiguredTarget configuredTarget : configuredTargets) {
+      for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) {
         if (isTestCommand && isTestRule(configuredTarget)) {
           // When running a test download the test.log and test.xml.
           filesToDownload.addAll(getTestOutputs(configuredTarget));
@@ -511,6 +513,17 @@ public void afterAnalysis(
           filesToDownload.addAll(getRunfiles(configuredTarget));
         }
       }
+      // Also fetch all inputs for actions that propagate their inputs (symlink actions).
+      for (ActionInput actionInput : filesToDownload.build()) {
+        if (!(actionInput instanceof Artifact)) {
+          continue;
+        }
+        ActionExecutionMetadata action =
+            (ActionExecutionMetadata) analysisResult.getActionGraph().getGeneratingAction((Artifact) actionInput);
+        if (action.mayInsensitivelyPropagateInputs()) {
+          filesToDownload.addAll(action.getInputs().toList());
+        }
+      }
       actionContextProvider.setFilesToDownload(filesToDownload.build());
     }
   }

src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java

@@ -16,6 +16,7 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.eventbus.SubscriberExceptionHandler;
+import com.google.devtools.build.lib.analysis.AnalysisResult;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -282,13 +283,13 @@ public BuildOptions getDefaultBuildOptions(BlazeRuntime runtime) {
    * @param env the command environment
    * @param request the build request
    * @param buildOptions the build's top-level options
-   * @param configuredTargets the build's requested top-level targets as {@link ConfiguredTarget}s
+   * @param analysisResult the build's requested top-level targets as {@link ConfiguredTarget}s
    */
   public void afterAnalysis(
       CommandEnvironment env,
       BuildRequest request,
       BuildOptions buildOptions,
-      Iterable<ConfiguredTarget> configuredTargets)
+      AnalysisResult analysisResult)
       throws InterruptedException, ViewCreationFailedException {}
 
   /**

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -1170,6 +1170,58 @@ EOF
   || fail "Expected runfile bazel-bin/a/create_bar.sh to be downloaded"
 }
 
+function test_downloads_toplevel_symlinks() {
+  # Test that --remote_download_toplevel fetches inputs to symlink actions. In
+  # particular, cc_binary links against a symlinked imported .so file, and only
+  # the symlink is in the runfiles.
+  if [[ "$PLATFORM" == "darwin" ]]; then
+    # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
+    # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
+    # action executors in order to select the appropriate Xcode toolchain.
+    return 0
+  fi
+
+  mkdir -p a
+
+  cat > a/bar.cc <<'EOF'
+int f() {
+  return 42;
+}
+EOF
+
+  cat > a/foo.cc <<'EOF'
+extern int f();
+int main() { return f() == 42 ? 0 : 1; }
+EOF
+
+  cat > a/BUILD <<'EOF'
+cc_binary(
+  name = "foo",
+  srcs = ["foo.cc"],
+  deps = [":libbar_lib"],
+)
+
+cc_import(
+  name = "libbar_lib",
+  shared_library = ":libbar.so",
+)
+
+cc_binary(
+  name = "libbar.so",
+  srcs = ["bar.cc"],
+  linkshared = 1,
+  linkstatic = 1,
+)
+EOF
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    //a:foo || fail "Failed to build //a:foobar"
+
+  ./bazel-bin/a/foo${EXE_EXT} || fail "bazel-bin/a/foo${EXE_EXT} failed to run"
+}
+
 function test_downloads_toplevel_src_runfiles() {
   # Test that using --remote_download_toplevel with a non-generated (source)
   # runfile dependency works.