Skip to content

Commit

Permalink
Automated rollback of commit 52c0597.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Crash due to failed assertion, leading to issue 184944522.

*** Original change description ***

BEP reports analysis failure for targets with artifact conflicts.

Artifact conflicts are detected through a non-skyframe evaluation that is not
capable of identifying a top-level target or aspect that failed as a result of the
artifact conflict. We do perform a skyframe evaluation to identify which targets
and aspects to actually evaluate however, and *this* evaluation is now used to
report artifact conflicts in BEP.

RELNOTES: None.
PiperOrigin-RevId: 367682388
  • Loading branch information
michaeledgar authored and copybara-github committed Apr 9, 2021
1 parent c3f20f0 commit a41d418
Show file tree
Hide file tree
Showing 9 changed files with 54 additions and 424 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
* lead to an error if both actions were executed in the same build.
*/
public class ArtifactPrefixConflictException extends Exception implements DetailedException {
private final Label firstOwner;

public ArtifactPrefixConflictException(
PathFragment firstPath, PathFragment secondPath, Label firstOwner, Label secondOwner) {
super(
Expand All @@ -37,11 +35,6 @@ public ArtifactPrefixConflictException(
+ "These actions cannot be simultaneously present; please rename one of the output "
+ "files or build just one of them",
firstPath, firstOwner, secondPath, secondOwner));
this.firstOwner = firstOwner;
}

public Label getFirstOwner() {
return firstOwner;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.buildeventstream.BuildEvent;
import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
Expand All @@ -30,7 +28,6 @@
import com.google.devtools.build.lib.causes.Cause;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.skyframe.AspectValueKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import java.util.Collection;
import javax.annotation.Nullable;
Expand All @@ -40,34 +37,13 @@
* target cannot be completed because of an error in one of its dependencies.
*/
public class AnalysisFailureEvent implements BuildEvent {
@Nullable private final AspectValueKey failedAspect;
private final ConfiguredTargetKey failedTarget;
private final BuildEventId configuration;
private final NestedSet<Cause> rootCauses;

public AnalysisFailureEvent(
ActionLookupKey failedTarget, BuildEventId configuration, NestedSet<Cause> rootCauses) {
Preconditions.checkArgument(
failedTarget instanceof ConfiguredTargetKey || failedTarget instanceof AspectValueKey);
if (failedTarget instanceof ConfiguredTargetKey) {
this.failedAspect = null;
this.failedTarget = (ConfiguredTargetKey) failedTarget;
} else {
this.failedAspect = (AspectValueKey) failedTarget;
this.failedTarget = failedAspect.getBaseConfiguredTargetKey();
}
if (configuration != null) {
this.configuration = configuration;
} else {
this.configuration = NullConfiguration.INSTANCE.getEventId();
}
this.rootCauses = rootCauses;
}

public AnalysisFailureEvent(
AspectValueKey failedAspect, BuildEventId configuration, NestedSet<Cause> rootCauses) {
this.failedAspect = failedAspect;
this.failedTarget = failedAspect.getBaseConfiguredTargetKey();
ConfiguredTargetKey failedTarget, BuildEventId configuration, NestedSet<Cause> rootCauses) {
this.failedTarget = failedTarget;
if (configuration != null) {
this.configuration = configuration;
} else {
Expand All @@ -79,7 +55,6 @@ public AnalysisFailureEvent(
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("failedAspect", failedAspect)
.add("failedTarget", failedTarget)
.add("configuration", configuration)
.add("legacyFailureReason", getLegacyFailureReason())
Expand Down Expand Up @@ -111,12 +86,7 @@ public NestedSet<Cause> getRootCauses() {

@Override
public BuildEventId getEventId() {
if (failedAspect == null) {
return BuildEventIdUtil.targetCompleted(failedTarget.getLabel(), configuration);
} else {
return BuildEventIdUtil.aspectCompleted(
failedTarget.getLabel(), configuration, failedAspect.getAspectName());
}
return BuildEventIdUtil.targetCompleted(failedTarget.getLabel(), configuration);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public String extractTag(SkyKey skyKey) {
return Label.print(((ConfiguredTargetKey) skyKey.argument()).getLabel());
}

static class ActionConflictFunctionException extends SkyFunctionException {
private static class ActionConflictFunctionException extends SkyFunctionException {
ActionConflictFunctionException(ConflictException e) {
super(e, Transience.PERSISTENT);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,15 +199,14 @@ static class ConflictException extends Exception {
this.apce = e;
}

IllegalStateException rethrowTyped()
throws ActionConflictException, ArtifactPrefixConflictException {
void rethrowTyped() throws ActionConflictException, ArtifactPrefixConflictException {
if (ace == null) {
throw Preconditions.checkNotNull(apce);
}
if (apce == null) {
throw Preconditions.checkNotNull(ace);
}
throw new IllegalStateException("malformed ConflictException has no well-typed cause");
throw new IllegalStateException();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,8 @@ public abstract class AspectValueKey implements ActionLookupKey {
private static final Interner<StarlarkAspectLoadingKey> starlarkAspectKeyInterner =
BlazeInterners.newWeakInterner();

/**
* Gets the name of the aspect that would be returned by the corresponding value's {@code
* aspectValue.getAspect().getAspectClass().getName()}, if the value could be produced.
*
* <p>Only needed for reporting errors in BEP when the key's AspectValue fails evaluation.
*/
public abstract String getAspectName();

public abstract String getDescription();

@Nullable
abstract BuildConfigurationValue.Key getAspectConfigurationKey();

/** Returns the key for the base configured target for this aspect. */
public abstract ConfiguredTargetKey getBaseConfiguredTargetKey();

public static AspectKey createAspectKey(
Label label,
@Nullable BuildConfiguration baseConfiguration,
Expand Down Expand Up @@ -135,10 +121,6 @@ public SkyFunctionName functionName() {
return SkyFunctions.ASPECT;
}

@Override
public String getAspectName() {
return aspectDescriptor.getDescription();
}

@Override
public Label getLabel() {
Expand Down Expand Up @@ -192,13 +174,11 @@ public String getDescription() {
* base target's configuration.
*/
@Nullable
@Override
BuildConfigurationValue.Key getAspectConfigurationKey() {
return aspectConfigurationKey;
}

/** Returns the key for the base configured target for this aspect. */
@Override
public ConfiguredTargetKey getBaseConfiguredTargetKey() {
return baseConfiguredTargetKey;
}
Expand Down Expand Up @@ -331,11 +311,6 @@ Label getStarlarkFileLabel() {
return starlarkFileLabel;
}

@Override
public String getAspectName() {
return String.format("%s%%%s", starlarkFileLabel, starlarkValueName);
}

@Override
public Label getLabel() {
return targetLabel;
Expand All @@ -347,18 +322,6 @@ public String getDescription() {
return String.format("%s%%%s of %s", starlarkFileLabel, starlarkValueName, targetLabel);
}

@Nullable
@Override
BuildConfigurationValue.Key getAspectConfigurationKey() {
return aspectConfigurationKey;
}

/** Returns the key for the base configured target for this aspect. */
@Override
public ConfiguredTargetKey getBaseConfiguredTargetKey() {
return baseConfiguredTargetKey;
}

@Override
public int hashCode() {
// We use the hash code caching strategy employed by java.lang.String. There are three subtle
Expand Down
Loading

0 comments on commit a41d418

Please sign in to comment.