Fix a subtle bug involving action conflicts with extra actions, fix another silly bug, and clean up action conflict handling a fair amount:
1. Add an additional conflict-filtering step after top-level artifacts have been computed (as distinct from top-level configured targets and aspects). This will filter out such artifacts that depend on targets that have action conflicts, preventing those actions from executing.
2. Fix a silly bug where if there was an action conflict, we would filter out all top-level aspects from the set to build, even if they were error-free, because we were checking the wrong type of key.
3. Remove all action conflict checking from the execution phase. It is up to the analysis phase to filter out all targets that depend on such conflicts. Having this belt-and-suspenders style is error-prone.
4. Move action conflict checking into analysis-phase-proper objects. As part of that, move the flag --experimental_strict_conflict_checks to AnalysisOptions. Its prior location actually meant that its stricter action conflict checking would not start on the first build it was specified for: it would have to wait for the subsequent build. There's still a bug here, in that a build with no conflicts if the flag is off might not detect conflicts with the flag on until the server has to analyze a new configured target, but I'll leave it to felly@ to decide how important that is.
I considered unifying the conflict-filtering steps, but decided against it, because I didn't like the idea of computing extra action artifacts for targets that we wouldn't try to build in the end because they depended on action conflicts. Keeping the semantics that only error-free targets can have their extra-action artifacts built seemed like the right call, which means we have to filter twice. I don't expect any performance impact because the Skyframe nodes from the first evaluation will still be there, and this codepath is only hit on rare occasions anyway.
There is talk of deprecating extra actions. If they are ever removed, then potentially this second filtering step could also be removed. Left a TODO for cparsons@ around that.
PiperOrigin-RevId: 306296831
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java
index 8b03eb1..1667c98 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java
@@ -13,37 +13,37 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.Streams.stream;
+import static com.google.common.base.Preconditions.checkArgument;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.Interner;
import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.skyframe.AbstractSkyKey;
import com.google.devtools.build.skyframe.SkyFunctionName;
-import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
/**
* A marker for an {@link ActionLookupValue} which is known to be transitively error-free from
* action conflict issues.
*/
-class ActionLookupConflictFindingValue implements SkyValue {
+public class ActionLookupConflictFindingValue implements SkyValue {
@AutoCodec
static final ActionLookupConflictFindingValue INSTANCE = new ActionLookupConflictFindingValue();
private ActionLookupConflictFindingValue() {}
- public static ImmutableList<SkyKey> keys(Iterable<ActionLookupValue.ActionLookupKey> lookupKeys) {
- return stream(lookupKeys).map(Key::create).collect(toImmutableList());
- }
-
public static Key key(ActionLookupValue.ActionLookupKey lookupKey) {
return Key.create(lookupKey);
}
+ public static Key key(Artifact artifact) {
+ checkArgument(artifact instanceof Artifact.DerivedArtifact, artifact);
+ return ActionLookupConflictFindingValue.key(
+ ((Artifact.DerivedArtifact) artifact).getGeneratingActionKey().getActionLookupKey());
+ }
+
@AutoCodec.VisibleForSerialization
@AutoCodec
static class Key extends AbstractSkyKey<ActionLookupValue.ActionLookupKey> {
