Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Track empty directories in tree artifacts #16015

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] Track empty directories in tree artifacts #16015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeChildArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeEmptyDirectoryArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.cache.ActionCache;
Expand Down Expand Up @@ -337,13 +339,19 @@ private static CachedOutputMetadata loadCachedOutputMetadata(
localTreeMetadata != null
&& !localTreeMetadata.equals(TreeArtifactValue.MISSING_TREE_ARTIFACT);

Map<TreeFileArtifact, FileArtifactValue> childValues = new HashMap<>();
Map<TreeChildArtifact, FileArtifactValue> childValues = new HashMap<>();
// Load remote child file metadata from cache.
cachedTreeMetadata
.childValues()
.forEach(
(key, value) ->
childValues.put(TreeFileArtifact.createTreeOutput(parent, key), value));
(key, value) -> {
if (value.getType().isDirectory()) {
childValues.put(TreeEmptyDirectoryArtifact.create(parent, key), value);
} else {
childValues.put(TreeFileArtifact.createTreeOutput(parent, key), value);
}
}
);
// Or add local one.
if (localTreeMetadataExists) {
localTreeMetadata.getChildValues().forEach(childValues::put);
Expand Down
11 changes: 3 additions & 8 deletions src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,17 +105,13 @@ public PathFragment getExecPath() {
/**
* Expands middleman and tree artifacts in a sequence of {@link ActionInput}s.
*
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
* is true, a tree artifact will be included in the constructed list when it expands into zero
* file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
* included.
* <p>The constructed list never contains tree or middleman artifacts.
*
* <p>Non-middleman, non-tree artifacts are returned untouched.
*/
public static List<ActionInput> expandArtifacts(
NestedSet<? extends ActionInput> inputs,
ArtifactExpander artifactExpander,
boolean keepEmptyTreeArtifacts) {
ArtifactExpander artifactExpander) {
List<ActionInput> result = new ArrayList<>();
List<Artifact> containedArtifacts = new ArrayList<>();
for (ActionInput input : inputs.toList()) {
Expand All @@ -125,8 +121,7 @@ public static List<ActionInput> expandArtifacts(
}
containedArtifacts.add((Artifact) input);
}
Artifact.addExpandedArtifacts(
containedArtifacts, result, artifactExpander, keepEmptyTreeArtifacts);
Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander);
return result;
}

Expand Down
11 changes: 6 additions & 5 deletions src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeChildArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.bugreport.BugReporter;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
Expand Down Expand Up @@ -175,9 +176,9 @@ private int getIndex(String execPathString) {
@Nullable
@Override
public FileArtifactValue getMetadata(ActionInput input) {
if (input instanceof TreeFileArtifact) {
TreeFileArtifact treeFileArtifact = (TreeFileArtifact) input;
int treeIndex = getIndex(treeFileArtifact.getParent().getExecPathString());
if (input instanceof TreeChildArtifact) {
TreeChildArtifact treeChildArtifact = (TreeChildArtifact) input;
int treeIndex = getIndex(treeChildArtifact.getParent().getExecPathString());
if (treeIndex != -1) {
checkArgument(
values[treeIndex] instanceof TrieArtifact,
Expand All @@ -186,7 +187,7 @@ public FileArtifactValue getMetadata(ActionInput input) {
return ((TrieArtifact) values[treeIndex])
.treeArtifactValue
.getChildValues()
.get(treeFileArtifact);
.get(treeChildArtifact);
}
}
int index = getIndex(input.getExecPathString());
Expand Down Expand Up @@ -262,7 +263,7 @@ public ActionInput getInput(String execPathString) {

// We must return an entry from the map since a duplicate would not have the generating action
// key set.
Map.Entry<TreeFileArtifact, ?> entry = tree.findChildEntryByExecPath(execPath);
Map.Entry<TreeChildArtifact, ?> entry = tree.findChildEntryByExecPath(execPath);
return entry != null ? entry.getKey() : null;
}

Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeChildArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -56,21 +57,21 @@
*/
public interface ActionTemplate<T extends Action> extends ActionAnalysisMetadata {
/**
* Given a set of input TreeFileArtifacts resolved at execution time, returns a list of expanded
* Given a set of input TreeChildArtifacts resolved at execution time, returns a list of expanded
* actions to be executed.
*
* <p>Each of the expanded actions' outputs must be a {@link TreeFileArtifact} owned by {@code
* artifactOwner} with a parent in {@link #getOutputs}. This is generally satisfied by calling
* {@link TreeFileArtifact#createTemplateExpansionOutput}.
*
* @param inputTreeFileArtifacts the set of {@link TreeFileArtifact}s inside {@link
* @param inputTreeChildArtifacts the set of {@link TreeChildArtifact}s inside {@link
* #getInputTreeArtifact}
* @param artifactOwner the {@link ArtifactOwner} of the generated output {@link
* TreeFileArtifact}s
* @return a list of expanded {@link Action}s to execute
*/
ImmutableList<T> generateActionsForInputArtifacts(
ImmutableSet<TreeFileArtifact> inputTreeFileArtifacts, ActionLookupKey artifactOwner)
ImmutableSet<TreeChildArtifact> inputTreeChildArtifacts, ActionLookupKey artifactOwner)
throws ActionExecutionException;

/** Returns the input TreeArtifact. */
Expand Down
158 changes: 108 additions & 50 deletions src/main/java/com/google/devtools/build/lib/actions/Artifact.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,6 +1268,43 @@ public ArchivedTreeArtifact deserialize(
}
}

public static abstract class TreeChildArtifact extends DerivedArtifact {

protected final SpecialArtifact parent;
protected final PathFragment parentRelativePath;

private TreeChildArtifact(
SpecialArtifact parent, PathFragment parentRelativePath, Object owner) {
super(parent.getRoot(), parent.getExecPath().getRelative(parentRelativePath), owner);
Preconditions.checkArgument(
parent.isTreeArtifact(),
"The parent of TreeFileArtifact (parent-relative path: %s) is not a TreeArtifact: %s",
parentRelativePath,
parent);
Preconditions.checkArgument(
!parentRelativePath.containsUplevelReferences() && !parentRelativePath.isAbsolute(),
"%s is not a proper normalized relative path",
parentRelativePath);
this.parent = parent;
this.parentRelativePath = parentRelativePath;
}

@Override
public SpecialArtifact getParent() {
return parent;
}

@Override
public PathFragment getParentRelativePath() {
return parentRelativePath;
}

@Override
public String getTreeRelativePathString() {
return parentRelativePath.getPathString();
}
}

/**
* A special kind of artifact that represents a concrete file created at execution time under its
* associated parent TreeArtifact.
Expand All @@ -1291,9 +1328,7 @@ public ArchivedTreeArtifact deserialize(
* return {@code false}.
* </ol>
*/
public static final class TreeFileArtifact extends DerivedArtifact {
private final SpecialArtifact parent;
private final PathFragment parentRelativePath;
public static final class TreeFileArtifact extends TreeChildArtifact {

/**
* Creates a {@link TreeFileArtifact} representing a child of the given parent tree artifact.
Expand Down Expand Up @@ -1358,33 +1393,7 @@ public static TreeFileArtifact createTemplateExpansionOutput(

private TreeFileArtifact(
SpecialArtifact parent, PathFragment parentRelativePath, Object owner) {
super(parent.getRoot(), parent.getExecPath().getRelative(parentRelativePath), owner);
Preconditions.checkArgument(
parent.isTreeArtifact(),
"The parent of TreeFileArtifact (parent-relative path: %s) is not a TreeArtifact: %s",
parentRelativePath,
parent);
Preconditions.checkArgument(
!parentRelativePath.containsUplevelReferences() && !parentRelativePath.isAbsolute(),
"%s is not a proper normalized relative path",
parentRelativePath);
this.parent = parent;
this.parentRelativePath = parentRelativePath;
}

@Override
public SpecialArtifact getParent() {
return parent;
}

@Override
public PathFragment getParentRelativePath() {
return parentRelativePath;
}

@Override
public String getTreeRelativePathString() {
return parentRelativePath.getPathString();
super(parent, parentRelativePath, owner);
}

@Override
Expand Down Expand Up @@ -1424,6 +1433,68 @@ public TreeFileArtifact deserialize(DeserializationContext context, CodedInputSt
}
}

public static final class TreeEmptyDirectoryArtifact extends TreeChildArtifact {

public static TreeEmptyDirectoryArtifact create(
SpecialArtifact parent, PathFragment parentRelativePath) {
Preconditions.checkArgument(
parent.hasGeneratingActionKey(),
"%s has no generating action key (parent owner: %s, parent relative path: %s)",
parent,
parent.getArtifactOwner(),
parentRelativePath);
return new TreeEmptyDirectoryArtifact(parent, parentRelativePath,
parent.getGeneratingActionKey());
}

public static TreeEmptyDirectoryArtifact create(
SpecialArtifact parent, String parentRelativePath) {
return create(parent, PathFragment.create(parentRelativePath));
}

private TreeEmptyDirectoryArtifact(
SpecialArtifact parent, PathFragment parentRelativePath, Object owner) {
super(parent, parentRelativePath, owner);
}

@Override
public boolean isDirectory() {
return true;
}

@Override
public boolean isChildOfDeclaredDirectory() {
return true;
}
}

@SuppressWarnings("unused") // Used by reflection.
private static final class TreeEmptyDirectoryArtifactCodec implements ObjectCodec<TreeEmptyDirectoryArtifact> {

@Override
public Class<TreeEmptyDirectoryArtifact> getEncodedClass() {
return TreeEmptyDirectoryArtifact.class;
}

@Override
public void serialize(
SerializationContext context, TreeEmptyDirectoryArtifact obj, CodedOutputStream codedOut)
throws SerializationException, IOException {
context.serialize(obj.parent, codedOut);
context.serialize(obj.parentRelativePath, codedOut);
context.serialize(getGeneratingActionKeyForSerialization(obj, context), codedOut);
}

@Override
public TreeEmptyDirectoryArtifact deserialize(DeserializationContext context, CodedInputStream codedIn)
throws SerializationException, IOException {
SpecialArtifact parent = context.deserialize(codedIn);
PathFragment parentRelativePath = context.deserialize(codedIn);
Object generatingActionKey = context.deserialize(codedIn);
return new TreeEmptyDirectoryArtifact(parent, parentRelativePath, generatingActionKey);
}
}

// ---------------------------------------------------------------------------
// Static methods to assist in working with Artifacts

Expand Down Expand Up @@ -1524,38 +1595,29 @@ public static String joinRootRelativePaths(String delimiter, Iterable<Artifact>
* Adds a collection of artifacts to a given collection, with middleman actions and tree artifacts
* expanded once.
*
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
* is true, a tree artifact will be included in the constructed list when it expands into zero
* file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
* included.
* <p>The constructed list never contains tree or middleman artifacts.
*/
static void addExpandedArtifacts(
Iterable<Artifact> artifacts,
Collection<? super Artifact> output,
ArtifactExpander artifactExpander,
boolean keepEmptyTreeArtifacts) {
addExpandedArtifacts(
artifacts, output, Functions.identity(), artifactExpander, keepEmptyTreeArtifacts);
ArtifactExpander artifactExpander) {
addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander);
}

/**
* Converts a collection of artifacts into the outputs computed by outputFormatter and adds them
* to a given collection. Middleman artifacts and tree artifacts are expanded once.
*
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
* is true, a tree artifact will be included in the constructed list when it expands into zero
* file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
* included.
* <p>The constructed list never contains tree or middleman artifacts.
*/
private static <E> void addExpandedArtifacts(
Iterable<? extends Artifact> artifacts,
Collection<? super E> output,
Function<? super Artifact, E> outputFormatter,
ArtifactExpander artifactExpander,
boolean keepEmptyTreeArtifacts) {
ArtifactExpander artifactExpander) {
for (Artifact artifact : artifacts) {
if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) {
expandArtifact(artifact, output, outputFormatter, artifactExpander, keepEmptyTreeArtifacts);
expandArtifact(artifact, output, outputFormatter, artifactExpander);
} else {
output.add(outputFormatter.apply(artifact));
}
Expand All @@ -1566,17 +1628,13 @@ private static <E> void expandArtifact(
Artifact middleman,
Collection<? super E> output,
Function<? super Artifact, E> outputFormatter,
ArtifactExpander artifactExpander,
boolean keepEmptyTreeArtifacts) {
ArtifactExpander artifactExpander) {
Preconditions.checkArgument(middleman.isMiddlemanArtifact() || middleman.isTreeArtifact());
List<Artifact> artifacts = new ArrayList<>();
artifactExpander.expand(middleman, artifacts);
for (Artifact artifact : artifacts) {
output.add(outputFormatter.apply(artifact));
}
if (keepEmptyTreeArtifacts && middleman.isTreeArtifact() && artifacts.isEmpty()) {
output.add(outputFormatter.apply(middleman));
}
}

/**
Expand Down
Loading