Obey --experimental_inprocess_symlink_creation in RunfilesTreeUpdater.

The bulk of this CL is just plumbing the flag from the configuration into the RunfilesSupplier implementations. The functional changes are in RunfilesTreeUpdater; compare with the preexisting SymlinkTreeStrategy implementation.

Additionally, I've fixed some variable and method names to exactly match the flags defined in CoreOptions; the code is less confusing and easier to refactor that way.

The motivation behind this change is to investigate the feasibility of defaulting to the in-process implementation and deleting the out-of-process one, which has known issues (see #4327).

PiperOrigin-RevId: 558112830
Change-Id: I4c509be665776013ffc2d483694df515d5ab32f2

Author: tjgq

src/main/java/com/google/devtools/build/lib/actions/BUILD

@@ -488,10 +488,12 @@ java_library(
     srcs = ["RunfilesSupplier.java"],
     deps = [
         ":artifacts",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/net/starlark/java/eval",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 

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

@@ -18,11 +18,13 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
 import java.util.Map;
+import javax.annotation.Nullable;
 
 /** A {@link RunfilesSupplier} implementation for composing multiple instances. */
 public final class CompositeRunfilesSupplier implements RunfilesSupplier {
@@ -101,19 +103,21 @@ public ImmutableList<Artifact> getManifests() {
   }
 
   @Override
-  public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
+  @Nullable
+  public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) {
     for (RunfilesSupplier supplier : suppliers) {
-      if (supplier.isBuildRunfileLinks(runfilesDir)) {
-        return true;
+      RunfileSymlinksMode mode = supplier.getRunfileSymlinksMode(runfilesDir);
+      if (mode != null) {
+        return mode;
       }
     }
-    return false;
+    return null;
   }
 
   @Override
-  public boolean isRunfileLinksEnabled(PathFragment runfilesDir) {
+  public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
     for (RunfilesSupplier supplier : suppliers) {
-      if (supplier.isRunfileLinksEnabled(runfilesDir)) {
+      if (supplier.isBuildRunfileLinks(runfilesDir)) {
         return true;
       }
     }

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

@@ -17,12 +17,14 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
 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.skyframe.serialization.autocodec.SerializationConstant;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Map;
+import javax.annotation.Nullable;
 
 /** Empty implementation of RunfilesSupplier */
 public final class EmptyRunfilesSupplier implements RunfilesSupplier {
@@ -53,12 +55,13 @@ public ImmutableList<Artifact> getManifests() {
   }
 
   @Override
-  public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
-    return false;
+  @Nullable
+  public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) {
+    return null;
   }
 
   @Override
-  public boolean isRunfileLinksEnabled(PathFragment runfilesDir) {
+  public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
     return false;
   }
 

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

@@ -17,9 +17,11 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Map;
+import javax.annotation.Nullable;
 import net.starlark.java.eval.StarlarkValue;
 
 /** Convenience wrapper around runfiles allowing lazy expansion. */
@@ -30,37 +32,35 @@
 // they are exposed through ctx.resolve_tools[2], for example.
 public interface RunfilesSupplier extends StarlarkValue {
 
-  /** @return the contained artifacts */
+  /** Returns the contained artifacts. */
   NestedSet<Artifact> getArtifacts();
 
-  /** @return the runfiles' root directories. */
+  /** Returns the runfiles' root directories. */
   ImmutableSet<PathFragment> getRunfilesDirs();
 
-  /**
-   * Returns mappings from runfiles directories to artifact mappings in that directory.
-   *
-   * @return runfiles' mappings
-   */
+  /** Returns mappings from runfiles directories to artifact mappings in that directory. */
   ImmutableMap<PathFragment, Map<PathFragment, Artifact>> getMappings();
 
-  /** @return the runfiles manifest artifacts, if any. */
+  /** Returns the runfiles manifest artifacts, if any. */
   ImmutableList<Artifact> getManifests();
 
   /**
-   * Returns whether for a given {@code runfilesDir} the runfile symlinks are materialized during
-   * build. Also returns {@code false} if the runfiles supplier doesn't know about the directory.
+   * Returns the {@link RunfileSymlinksMode} for the given {@code runfilesDir}, or {@code null} if
+   * the {@link RunfilesSupplier} doesn't know about the directory.
    *
    * @param runfilesDir runfiles directory relative to the exec root
    */
-  boolean isBuildRunfileLinks(PathFragment runfilesDir);
+  @Nullable
+  RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir);
 
   /**
-   * Returns whether it's allowed to create runfile symlinks in the {@code runfilesDir}. Also
-   * returns {@code false} if the runfiles supplier doesn't know about the directory.
+   * Returns whether the runfile symlinks should be materialized during the build for the given
+   * {@code runfilesDir}, or {@code false} if the {@link RunfilesSupplier} doesn't know about the
+   * directory.
    *
    * @param runfilesDir runfiles directory relative to the exec root
    */
-  boolean isRunfileLinksEnabled(PathFragment runfilesDir);
+  boolean isBuildRunfileLinks(PathFragment runfilesDir);
 
   /**
    * Returns a {@link RunfilesSupplier} identical to this one, but with the given runfiles

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1669,6 +1669,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//src/main/java/com/google/devtools/common/options",
         "//src/main/java/net/starlark/java/annot",
         "//src/main/java/net/starlark/java/eval",
         "//src/main/protobuf:failure_details_java_proto",

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
 import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -87,8 +88,8 @@ public final class RunfilesSupport implements RunfilesSupplier {
   private final Artifact repoMappingManifest;
   private final Artifact runfilesMiddleman;
   private final Artifact owningExecutable;
+  private final RunfileSymlinksMode runfileSymlinksMode;
   private final boolean buildRunfileLinks;
-  private final boolean runfilesEnabled;
   private final CommandLine args;
   private final ActionEnvironment actionEnvironment;
 
@@ -106,7 +107,9 @@ private static RunfilesSupport create(
       CommandLine args,
       ActionEnvironment actionEnvironment) {
     Artifact owningExecutable = Preconditions.checkNotNull(executable);
-    boolean createManifest = ruleContext.getConfiguration().buildRunfilesManifests();
+    RunfileSymlinksMode runfileSymlinksMode =
+        ruleContext.getConfiguration().getRunfileSymlinksMode();
+    boolean buildRunfileManifests = ruleContext.getConfiguration().buildRunfileManifests();
     boolean buildRunfileLinks = ruleContext.getConfiguration().buildRunfileLinks();
 
     // Adding run_under target to the runfiles manifest so it would become part
@@ -131,7 +134,7 @@ private static RunfilesSupport create(
 
     Artifact runfilesInputManifest;
     Artifact runfilesManifest;
-    if (createManifest) {
+    if (buildRunfileManifests) {
       runfilesInputManifest = createRunfilesInputManifestArtifact(ruleContext, owningExecutable);
       runfilesManifest =
           createRunfilesAction(
@@ -144,17 +147,15 @@ private static RunfilesSupport create(
         createRunfilesMiddleman(
             ruleContext, owningExecutable, runfiles, runfilesManifest, repoMappingManifest);
 
-    boolean runfilesEnabled = ruleContext.getConfiguration().runfilesEnabled();
-
     return new RunfilesSupport(
         runfiles,
         runfilesInputManifest,
         runfilesManifest,
         repoMappingManifest,
         runfilesMiddleman,
         owningExecutable,
+        runfileSymlinksMode,
         buildRunfileLinks,
-        runfilesEnabled,
         args,
         actionEnvironment);
   }
@@ -166,8 +167,8 @@ private RunfilesSupport(
       Artifact repoMappingManifest,
       Artifact runfilesMiddleman,
       Artifact owningExecutable,
+      RunfileSymlinksMode runfileSymlinksMode,
       boolean buildRunfileLinks,
-      boolean runfilesEnabled,
       CommandLine args,
       ActionEnvironment actionEnvironment) {
     this.runfiles = runfiles;
@@ -176,8 +177,8 @@ private RunfilesSupport(
     this.repoMappingManifest = repoMappingManifest;
     this.runfilesMiddleman = runfilesMiddleman;
     this.owningExecutable = owningExecutable;
+    this.runfileSymlinksMode = runfileSymlinksMode;
     this.buildRunfileLinks = buildRunfileLinks;
-    this.runfilesEnabled = runfilesEnabled;
     this.args = args;
     this.actionEnvironment = actionEnvironment;
   }
@@ -194,27 +195,33 @@ public PathFragment getRunfilesDirectoryExecPath() {
   }
 
   /**
-   * Returns {@code true} if runfile symlinks should be materialized when building an executable.
-   *
-   * <p>Also see {@link #isRunfilesEnabled()}.
+   * Same as {@link #getRunfileSymlinksMode(PathFragment)} with {@link
+   * #getRunfilesDirectoryExecPath} as the implied argument.
    */
-  public boolean isBuildRunfileLinks() {
-    return buildRunfileLinks;
+  public RunfileSymlinksMode getRunfileSymlinksMode() {
+    return runfileSymlinksMode;
   }
 
   @Override
-  public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
-    return buildRunfileLinks && runfilesDir.equals(getRunfilesDirectoryExecPath());
+  @Nullable
+  public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) {
+    if (runfilesDir.equals(getRunfilesDirectoryExecPath())) {
+      return runfileSymlinksMode;
+    }
+    return null;
   }
 
   /**
-   * Returns {@code true} if runfile symlinks are enabled.
-   *
-   * <p>This option differs from {@link #isBuildRunfileLinks()} in that if {@code false} it also
-   * disables runfile symlinks creation during run/test.
+   * Same as {@link #isBuildRunfileLinks(PathFragment)} with {@link #getRunfilesDirectoryExecPath}
+   * as the implied argument.
    */
-  public boolean isRunfilesEnabled() {
-    return runfilesEnabled;
+  public boolean isBuildRunfileLinks() {
+    return buildRunfileLinks;
+  }
+
+  @Override
+  public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
+    return buildRunfileLinks && runfilesDir.equals(getRunfilesDirectoryExecPath());
   }
 
   public Runfiles getRunfiles() {
@@ -552,11 +559,6 @@ public ImmutableList<Artifact> getManifests() {
     return ImmutableList.of();
   }
 
-  @Override
-  public boolean isRunfileLinksEnabled(PathFragment runfilesDir) {
-    return runfilesEnabled && runfilesDir.equals(getRunfilesDirectoryExecPath());
-  }
-
   @Override
   public RunfilesSupplier withOverriddenRunfilesDir(PathFragment newRunfilesDir) {
     return newRunfilesDir.equals(getRunfilesDirectoryExecPath())
@@ -566,7 +568,7 @@ public RunfilesSupplier withOverriddenRunfilesDir(PathFragment newRunfilesDir) {
             runfiles,
             /* manifest= */ null,
             repoMappingManifest,
-            buildRunfileLinks,
-            runfilesEnabled);
+            runfileSymlinksMode,
+            buildRunfileLinks);
   }
 }