Skip to content

Commit

Permalink
Enforce that the SkyKey returned by ActionLookupKey#getSkyKey is an A…
Browse files Browse the repository at this point in the history
…ctionLookupKey. It was only being violated for some singleton keys.

Also do some drive-by cleanups of unused variables. Step approximately -2.

--
PiperOrigin-RevId: 150668944
MOS_MIGRATED_REVID=150668944
janakdr authored and hermione521 committed Mar 21, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent fa492b9 commit 5cb6554
Showing 6 changed files with 26 additions and 37 deletions.
Original file line number Diff line number Diff line change
@@ -22,10 +22,10 @@
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;

import java.util.Map;

/**
@@ -68,14 +68,6 @@ ImmutableMap<Artifact, ActionAnalysisMetadata> getMapForConsistencyCheck() {
return generatingActionMap;
}

/**
* To be used only when setting the owners of deserialized artifacts whose owners were unknown at
* creation time -- not by other callers or values.
*/
Iterable<ActionAnalysisMetadata> getActionsForFindingArtifactOwners() {
return generatingActionMap.values();
}

@VisibleForTesting
public static SkyKey key(ActionLookupKey ownerKey) {
return ownerKey.getSkyKey();
@@ -101,13 +93,24 @@ public Label getLabel() {
*/
abstract SkyFunctionName getType();

SkyKey getSkyKeyInternal() {
return SkyKey.create(getType(), this);
}

/**
* Prefer {@link ActionLookupValue#key} to calling this method directly.
*
* <p>Subclasses may override if the value key contents should not be the key itself.
* <p>Subclasses may override {@link #getSkyKeyInternal} if the {@link SkyKey} argument should
* not be this {@link ActionLookupKey} itself.
*/
SkyKey getSkyKey() {
return SkyKey.create(getType(), this);
final SkyKey getSkyKey() {
SkyKey result = getSkyKeyInternal();
Preconditions.checkState(
result.argument() instanceof ActionLookupKey,
"Not ActionLookupKey for %s: %s",
this,
result);
return result;
}
}
}
Original file line number Diff line number Diff line change
@@ -46,9 +46,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
outputs.addAll(action.getOutputs());
}

return new CoverageReportValue(
outputs.build(),
actions);
return new CoverageReportValue(actions);
}

@Override
Original file line number Diff line number Diff line change
@@ -15,9 +15,7 @@
package com.google.devtools.build.lib.skyframe;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -26,21 +24,13 @@
* A SkyValue to store the coverage report Action and Artifacts.
*/
public class CoverageReportValue extends ActionLookupValue {
private final ImmutableSet<Artifact> coverageReportArtifacts;

// There should only ever be one CoverageReportValue value in the graph.
public static final SkyKey SKY_KEY =
SkyKey.create(SkyFunctions.COVERAGE_REPORT, "COVERAGE_REPORT");
public static final ArtifactOwner ARTIFACT_OWNER = new CoverageReportKey();
static final SkyKey SKY_KEY = SkyKey.create(SkyFunctions.COVERAGE_REPORT, ARTIFACT_OWNER);

public CoverageReportValue(ImmutableSet<Artifact> coverageReportArtifacts,
ImmutableList <ActionAnalysisMetadata> coverageReportActions) {
CoverageReportValue(ImmutableList<ActionAnalysisMetadata> coverageReportActions) {
super(coverageReportActions);
this.coverageReportArtifacts = coverageReportArtifacts;
}

public ImmutableSet<Artifact> getCoverageReportArtifacts() {
return coverageReportArtifacts;
}

private static class CoverageReportKey extends ActionLookupKey {
@@ -50,7 +40,7 @@ SkyFunctionName getType() {
}

@Override
SkyKey getSkyKey() {
SkyKey getSkyKeyInternal() {
return SKY_KEY;
}
}
Original file line number Diff line number Diff line change
@@ -30,8 +30,8 @@ public class WorkspaceStatusValue extends ActionLookupValue {
private final Artifact volatileArtifact;

// There should only ever be one BuildInfo value in the graph.
public static final SkyKey SKY_KEY = SkyKey.create(SkyFunctions.BUILD_INFO, "BUILD_INFO");
static final ArtifactOwner ARTIFACT_OWNER = new BuildInfoKey();
public static final SkyKey SKY_KEY = SkyKey.create(SkyFunctions.BUILD_INFO, ARTIFACT_OWNER);

public WorkspaceStatusValue(Artifact stableArtifact, Artifact volatileArtifact,
WorkspaceStatusAction action) {
@@ -55,7 +55,7 @@ SkyFunctionName getType() {
}

@Override
SkyKey getSkyKey() {
SkyKey getSkyKeyInternal() {
return SKY_KEY;
}
}
Original file line number Diff line number Diff line change
@@ -28,7 +28,6 @@
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -50,8 +49,8 @@
import org.junit.Before;

abstract class ArtifactFunctionTestCase {
protected static final SkyKey OWNER_KEY = SkyKey.create(SkyFunctions.ACTION_LOOKUP, "OWNER");
protected static final ActionLookupKey ALL_OWNER = new SingletonActionLookupKey();
static final ActionLookupKey ALL_OWNER = new SingletonActionLookupKey();
static final SkyKey OWNER_KEY = SkyKey.create(SkyFunctions.ACTION_LOOKUP, ALL_OWNER);

protected Predicate<PathFragment> allowedMissingInputsPredicate = Predicates.alwaysFalse();

@@ -146,7 +145,7 @@ public String extractTag(SkyKey skyKey) {

private static class SingletonActionLookupKey extends ActionLookupKey {
@Override
SkyKey getSkyKey() {
SkyKey getSkyKeyInternal() {
return OWNER_KEY;
}

Original file line number Diff line number Diff line change
@@ -97,10 +97,9 @@
* The common code that's shared between various builder tests.
*/
public abstract class TimestampBuilderTestCase extends FoundationTestCase {

private static final SkyKey OWNER_KEY = SkyKey.create(SkyFunctions.ACTION_LOOKUP, "OWNER");
protected static final ActionLookupValue.ActionLookupKey ALL_OWNER =
new SingletonActionLookupKey();
private static final SkyKey OWNER_KEY = SkyKey.create(SkyFunctions.ACTION_LOOKUP, ALL_OWNER);
protected static final Predicate<Action> ALWAYS_EXECUTE_FILTER = Predicates.alwaysTrue();
protected static final String CYCLE_MSG = "Yarrrr, there be a cycle up in here";

@@ -440,7 +439,7 @@ public void dump(PrintStream out) {

private static class SingletonActionLookupKey extends ActionLookupValue.ActionLookupKey {
@Override
SkyKey getSkyKey() {
SkyKey getSkyKeyInternal() {
return OWNER_KEY;
}

0 comments on commit 5cb6554

Please sign in to comment.