Merge branch 'master' into fix-vs-autodetection

.bazelci/postsubmit.yml

@@ -338,6 +338,7 @@ tasks:
       - "--host_copt=-w"
       - "--test_tag_filters=-no_windows,-slow"
       - "--test_env=JAVA_HOME"
+      - "--test_env=BAZEL_VC"
       - "--test_env=TEST_INSTALL_BASE=$HOME/bazeltest_install_base"
       - "--test_env=TEST_REPOSITORY_HOME=C:/b/bazeltest_external"
     test_targets:

.bazelci/presubmit.yml

@@ -405,6 +405,7 @@ tasks:
       - "--host_copt=-w"
       - "--test_tag_filters=-no_windows,-slow"
       - "--test_env=JAVA_HOME"
+      - "--test_env=BAZEL_VC"
       - "--test_env=TEST_INSTALL_BASE=$HOME/bazeltest_install_base"
       - "--test_env=TEST_REPOSITORY_HOME=C:/b/bazeltest_external"
     test_targets:

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

@@ -14,12 +14,26 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.devtools.build.lib.actions.cache.ActionCache;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.Map.Entry;
 import javax.annotation.Nullable;
 
 /** Utility functions for {@link ActionCache}. */
 public class ActionCacheUtils {
   private ActionCacheUtils() {}
 
+  @Nullable
+  public static Entry<String, ActionCache.Entry> getCacheEntryWithKey(
+      ActionCache actionCache, Action action) {
+    for (Artifact output : action.getOutputs()) {
+      ActionCache.Entry entry = actionCache.get(output.getExecPathString());
+      if (entry != null) {
+        return new SimpleEntry<>(output.getExecPathString(), entry);
+      }
+    }
+    return null;
+  }
+
   /** Checks whether one of existing output paths is already used as a key. */
   @Nullable
   public static ActionCache.Entry getCacheEntry(ActionCache actionCache, Action action) {

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

@@ -29,7 +29,6 @@
  * are subclasses of {@link ActionLookupKeyOrProxy}. This allows callers to easily find the value
  * key, while remaining agnostic to what action lookup values actually exist.
  */
-// TODO(b/261521010): this layer of indirection is no longer needed and may be cleaned up.
 public interface ActionLookupKeyOrProxy extends ArtifactOwner {
   /**
    * Returns the {@link BuildConfigurationKey} for the configuration associated with this key, or

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

@@ -627,6 +627,12 @@ public long getExpireAtEpochMilli() {
       return -1;
     }
 
+    /**
+     * Extends the expiration time for this metadata. If it was constructed without known expiration
+     * time (i.e. expireAtEpochMilli < 0), this extension does nothing.
+     */
+    public void extendExpireAtEpochMilli(long expireAtEpochMilli) {}
+
     public boolean isAlive(Instant now) {
       return true;
     }
@@ -655,7 +661,7 @@ public final String toString() {
 
   /** A remote artifact that expires at a particular time. */
   private static final class RemoteFileArtifactValueWithExpiration extends RemoteFileArtifactValue {
-    private final long expireAtEpochMilli;
+    private long expireAtEpochMilli;
 
     private RemoteFileArtifactValueWithExpiration(
         byte[] digest,
@@ -672,6 +678,12 @@ public long getExpireAtEpochMilli() {
       return expireAtEpochMilli;
     }
 
+    @Override
+    public void extendExpireAtEpochMilli(long expireAtEpochMilli) {
+      Preconditions.checkState(expireAtEpochMilli > this.expireAtEpochMilli);
+      this.expireAtEpochMilli = expireAtEpochMilli;
+    }
+
     @Override
     public boolean isAlive(Instant now) {
       return now.toEpochMilli() < expireAtEpochMilli;

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

@@ -62,7 +62,7 @@ public final class TransitiveDependencyState {
    *
    * <p>More ideally, those properties would be conveyed via providers of those dependencies, but
    * doing so would adversely affect resting heap usage whereas {@link ConfiguredTargetAndData} is
-   * ephemeral. Distributed implementations will include these properties in an extra provider. It
+   * ephemeral. Distributed implementations will include these properties in an extra providers. It
    * won't affect memory because the underlying package won't exist on the node loading it remotely.
    *
    * <p>It's valid to obtain {@link Package}s of dependencies from this map instead of creating an

src/main/java/com/google/devtools/build/lib/analysis/producers/AttributeConfiguration.java

@@ -28,13 +28,6 @@ enum Kind {
      * target is known, it should be verified to be a {@link PackageGroup}.
      */
     VISIBILITY,
-    /**
-     * The configuration is null.
-     *
-     * <p>This is only applied when the dependency is in the same package as the parent and it is
-     * not configurable.
-     */
-    NULL_CONFIGURATION,
     /**
      * There is a single configuration.
      *
@@ -53,16 +46,13 @@ enum Kind {
 
   abstract void visibility();
 
-  abstract void nullConfiguration();
-
   abstract BuildConfigurationKey unary();
 
   abstract ImmutableMap<String, BuildConfigurationKey> split();
 
   public int count() {
     switch (kind()) {
       case VISIBILITY:
-      case NULL_CONFIGURATION:
       case UNARY:
         return 1;
       case SPLIT:
@@ -75,10 +65,6 @@ static AttributeConfiguration ofVisibility() {
     return AutoOneOf_AttributeConfiguration.visibility();
   }
 
-  static AttributeConfiguration ofNullConfiguration() {
-    return AutoOneOf_AttributeConfiguration.nullConfiguration();
-  }
-
   static AttributeConfiguration ofUnary(BuildConfigurationKey key) {
     return AutoOneOf_AttributeConfiguration.unary(key);
   }

src/main/java/com/google/devtools/build/lib/analysis/producers/ConfigConditionsProducer.java

@@ -14,7 +14,7 @@
 package com.google.devtools.build.lib.analysis.producers;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.analysis.InconsistentNullConfigException;
+import com.google.devtools.build.lib.analysis.InvalidVisibilityDependencyException;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
 import com.google.devtools.build.lib.analysis.config.ConfigConditions;
@@ -130,12 +130,17 @@ public void acceptConfiguredTargetAndDataError(ConfiguredValueCreationException
   }
 
   @Override
-  public void acceptConfiguredTargetAndDataError(InconsistentNullConfigException error) {
-    // A config label was evaluated with a null configuration. This should never happen as
-    // ConfigConditions are only present if the parent is a Rule, then always evaluated with the
-    // parent configuration.
+  public void acceptConfiguredTargetAndDataError(InvalidVisibilityDependencyException error) {
+    // After removing the rule transition from dependency resolution, a ConfiguredTargetKey in
+    // Skyframe with a null BuildConfigurationKey will only be used to request visibility
+    // dependencies. This will never be the case for ConfigConditions, which are always requested
+    // with the parent configuration. At the moment, nothing throws
+    // InvalidVisibilityDependencyException.
+    //
+    // TODO(b/261521010): update this comment once rule transitions are removed from dependency
+    // resolution.
     throw new IllegalArgumentException(
-        "ConfigCondition dependency should never be evaluated with a null configuration.", error);
+        "ConfigCondition dependency should never be marked visibility.", error);
   }
 
   private StateMachine constructConfigConditions(Tasks tasks, ExtendedEventHandler listener) {

src/main/java/com/google/devtools/build/lib/analysis/producers/ConfiguredTargetAndDataProducer.java

@@ -16,7 +16,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
-import com.google.devtools.build.lib.analysis.InconsistentNullConfigException;
+import com.google.devtools.build.lib.analysis.InvalidVisibilityDependencyException;
 import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -42,14 +42,14 @@ public final class ConfiguredTargetAndDataProducer
     implements StateMachine,
         Consumer<SkyValue>,
         StateMachine.ValueOrException2Sink<
-            ConfiguredValueCreationException, InconsistentNullConfigException> {
+            ConfiguredValueCreationException, InvalidVisibilityDependencyException> {
   /** Interface for accepting values produced by this class. */
   public interface ResultSink {
     void acceptConfiguredTargetAndData(ConfiguredTargetAndData value, int index);
 
     void acceptConfiguredTargetAndDataError(ConfiguredValueCreationException error);
 
-    void acceptConfiguredTargetAndDataError(InconsistentNullConfigException error);
+    void acceptConfiguredTargetAndDataError(InvalidVisibilityDependencyException error);
   }
 
   // -------------------- Input --------------------
@@ -86,8 +86,9 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
     tasks.lookUp(
         key.toKey(),
         ConfiguredValueCreationException.class,
-        InconsistentNullConfigException.class,
-        (ValueOrException2Sink<ConfiguredValueCreationException, InconsistentNullConfigException>)
+        InvalidVisibilityDependencyException.class,
+        (ValueOrException2Sink<
+                ConfiguredValueCreationException, InvalidVisibilityDependencyException>)
             this);
     return this::fetchConfigurationAndPackage;
   }
@@ -96,7 +97,7 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
   public void acceptValueOrException2(
       @Nullable SkyValue value,
       @Nullable ConfiguredValueCreationException error,
-      @Nullable InconsistentNullConfigException visibilityError) {
+      @Nullable InvalidVisibilityDependencyException visibilityError) {
     if (value != null) {
       var configuredTargetValue = (ConfiguredTargetValue) value;
       this.configuredTarget = configuredTargetValue.getConfiguredTarget();

src/main/java/com/google/devtools/build/lib/analysis/producers/DependencyProducer.java

@@ -30,16 +30,11 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException;
-import com.google.devtools.build.lib.causes.LoadingFailedCause;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.AttributeTransitionData;
-import com.google.devtools.build.lib.packages.NoSuchTargetException;
-import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
 import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException;
@@ -117,25 +112,6 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
           AttributeConfiguration.ofVisibility(), /* executionPlatformLabel= */ null);
     }
 
-    Target parentTarget = parameters.target();
-    if (parentTarget.getLabel().getPackageIdentifier().equals(toLabel.getPackageIdentifier())) {
-      try {
-        Target toTarget = parentTarget.getPackage().getTarget(toLabel.getName());
-        if (!toTarget.isConfigurable()) {
-          return computePrerequisites(
-              AttributeConfiguration.ofNullConfiguration(), /* executionPlatformLabel= */ null);
-        }
-      } catch (NoSuchTargetException e) {
-        parameters
-            .transitiveState()
-            .addTransitiveCause(new LoadingFailedCause(toLabel, e.getDetailedExitCode()));
-        listener.handle(
-            Event.error(
-                TargetUtils.getLocationMaybe(parentTarget),
-                TargetUtils.formatMissingEdge(parentTarget, toLabel, e, kind.getAttribute())));
-      }
-    }
-
     // The logic of `DependencyResolver.computeDependencyLabels` implies that
     // `parameters.configurationKey()` is non-null for everything that follows.
     BuildConfigurationKey configurationKey = checkNotNull(parameters.configurationKey());

src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisiteParameters.java

@@ -25,7 +25,6 @@
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import javax.annotation.Nullable;
@@ -34,7 +33,7 @@
 /** Common parameters for computing prerequisites. */
 public final class PrerequisiteParameters {
   private final ConfiguredTargetKey configuredTargetKey;
-  private final Target target;
+  @Nullable private final Rule associatedRule;
 
   private final ImmutableList<Aspect> aspects;
   private final StarlarkTransitionCache transitionCache;
@@ -45,14 +44,14 @@ public final class PrerequisiteParameters {
 
   public PrerequisiteParameters(
       ConfiguredTargetKey configuredTargetKey,
-      Target target,
+      @Nullable Rule associatedRule,
       Iterable<Aspect> aspects,
       StarlarkTransitionCache transitionCache,
       @Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
       @Nullable ConfiguredAttributeMapper attributeMap,
       TransitiveDependencyState transitiveState) {
     this.configuredTargetKey = configuredTargetKey;
-    this.target = target;
+    this.associatedRule = associatedRule;
     this.aspects = ImmutableList.copyOf(aspects);
     this.transitionCache = transitionCache;
     this.toolchainContexts = toolchainContexts;
@@ -64,13 +63,9 @@ public Label label() {
     return configuredTargetKey.getLabel();
   }
 
-  public Target target() {
-    return target;
-  }
-
   @Nullable
   public Rule associatedRule() {
-    return target.getAssociatedRule();
+    return associatedRule;
   }
 
   @Nullable
@@ -96,8 +91,12 @@ public ConfiguredAttributeMapper attributeMap() {
     return attributeMap;
   }
 
+  @Nullable
   public Location location() {
-    return target.getLocation();
+    if (associatedRule == null) {
+      return null;
+    }
+    return associatedRule.getLocation();
   }
 
   public BuildEventId eventId() {

src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisitesProducer.java

@@ -15,19 +15,16 @@
 
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.devtools.build.lib.analysis.AspectResolutionHelpers.computeAspectCollection;
-import static com.google.devtools.build.lib.analysis.producers.AttributeConfiguration.Kind.VISIBILITY;
 import static com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData.SPLIT_DEP_ORDERING;
 import static java.util.Arrays.sort;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.AspectCollection;
 import com.google.devtools.build.lib.analysis.DuplicateException;
 import com.google.devtools.build.lib.analysis.InconsistentAspectOrderException;
-import com.google.devtools.build.lib.analysis.InconsistentNullConfigException;
 import com.google.devtools.build.lib.analysis.InvalidVisibilityDependencyException;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException;
-import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.Aspect;
@@ -104,7 +101,6 @@ interface ResultSink {
   public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
     switch (configuration.kind()) {
       case VISIBILITY:
-      case NULL_CONFIGURATION:
         tasks.enqueue(
             new ConfiguredTargetAndDataProducer(
                 getPrerequisiteKey(/* configurationKey= */ null),
@@ -145,18 +141,9 @@ public void acceptConfiguredTargetAndData(ConfiguredTargetAndData value, int ind
   }
 
   @Override
-  public void acceptConfiguredTargetAndDataError(InconsistentNullConfigException error) {
+  public void acceptConfiguredTargetAndDataError(InvalidVisibilityDependencyException error) {
     hasError = true;
-    if (configuration.kind() == VISIBILITY) {
-      // The target was configurable, but used as a visibility dependency. This is invalid because
-      // only `PackageGroup`s are accepted as visibility dependencies and those are not
-      // configurable. Propagates the exception with more precise information.
-      sink.acceptPrerequisitesError(new InvalidVisibilityDependencyException(label));
-      return;
-    }
-    // `configuration.kind()` was `NULL_CONFIGURATION`. This is only used when the target is in the
-    // same package as the parent and not configurable so this should never happen.
-    throw new IllegalStateException(error);
+    sink.acceptPrerequisitesError(error);
   }
 
   @Override
@@ -170,15 +157,6 @@ private StateMachine computeConfiguredAspects(Tasks tasks, ExtendedEventHandler
       return DONE;
     }
 
-    if (configuration.kind() == VISIBILITY) {
-      // Verifies that the dependency is a `package_group`. The value is always at index 0 because
-      // the `VISIBILITY` configuration is always unary.
-      if (!(configuredTargets[0].getConfiguredTarget() instanceof PackageGroupConfiguredTarget)) {
-        sink.acceptPrerequisitesError(new InvalidVisibilityDependencyException(label));
-        return DONE;
-      }
-    }
-
     cleanupValues();
 
     AspectCollection aspects;