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

Have RunfilesTree#getMapping return a SortedMap #25625

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

Have RunfilesTree#getMapping return a SortedMap #25625

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
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.io.Closeable;
import java.io.IOException;
import java.util.Map;
import java.util.SortedMap;
import javax.annotation.Nullable;

/** A class that groups services in the scope of the action. Like the FileOutErr object. */
Expand All @@ -67,7 +68,7 @@ public PathFragment getExecPath() {
}

@Override
public Map<PathFragment, Artifact> getMapping() {
public SortedMap<PathFragment, Artifact> getMapping() {
return wrapped.getMapping();
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/actions/RunfilesTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Map;
import java.util.SortedMap;
import javax.annotation.Nullable;

/** Lazy wrapper for a single runfiles tree. */
Expand All @@ -30,7 +30,7 @@ public interface RunfilesTree {
PathFragment getExecPath();

/** Returns the mapping from the location in the runfiles tree to the artifact that's there. */
Map<PathFragment, Artifact> getMapping();
SortedMap<PathFragment, Artifact> getMapping();

/**
* Returns artifacts the runfiles tree contain symlinks to.
Expand Down
21 changes: 7 additions & 14 deletions src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.UUID;
import java.util.function.BiConsumer;
Expand Down Expand Up @@ -345,7 +346,7 @@ static Map<PathFragment, Artifact> filterListForObscuringSymlinks(
* entries and elements that live outside the source tree. Null values represent empty input
* files.
*/
public Map<PathFragment, Artifact> getRunfilesInputs(Artifact repoMappingManifest) {
public SortedMap<PathFragment, Artifact> getRunfilesInputs(Artifact repoMappingManifest) {
return getRunfilesInputs(EnumSet.noneOf(ConflictType.class), null, repoMappingManifest);
}

Expand All @@ -358,12 +359,6 @@ public BiConsumer<ConflictType, String> eventRunfilesConflictReceiver(
case NESTED_RUNFILES_TREE -> EventKind.ERROR;
case PREFIX_CONFLICT ->
conflictPolicy == ConflictPolicy.ERROR ? EventKind.ERROR : EventKind.WARNING;
default ->
switch (conflictPolicy) {
case IGNORE -> throw new IllegalStateException();
default ->
conflictPolicy == ConflictPolicy.ERROR ? EventKind.ERROR : EventKind.WARNING;
};
};

eventHandler.handle(Event.of(kind, location, message));
Expand All @@ -380,7 +375,7 @@ public BiConsumer<ConflictType, String> eventRunfilesConflictReceiver(
* @return Map<PathFragment, Artifact> path fragment to artifact, of normal source tree entries
* and elements that live outside the source tree. Null values represent empty input files.
*/
public Map<PathFragment, Artifact> getRunfilesInputs(
public SortedMap<PathFragment, Artifact> getRunfilesInputs(
BiConsumer<ConflictType, String> receiver, @Nullable Artifact repoMappingManifest) {
EnumSet<ConflictType> conflictsToReport =
conflictPolicy == ConflictPolicy.IGNORE
Expand All @@ -392,7 +387,7 @@ public Map<PathFragment, Artifact> getRunfilesInputs(
return getRunfilesInputs(conflictsToReport, receiver, repoMappingManifest);
}

private Map<PathFragment, Artifact> getRunfilesInputs(
private SortedMap<PathFragment, Artifact> getRunfilesInputs(
EnumSet<ConflictType> conflictSet,
BiConsumer<ConflictType, String> receiver,
@Nullable Artifact repoMappingManifest) {
Expand Down Expand Up @@ -437,7 +432,7 @@ public boolean isLegacyExternalRunfiles() {
@VisibleForTesting
static final class ManifestBuilder {
// Manifest of paths to artifacts. Path fragments are relative to the .runfiles directory.
private final Map<PathFragment, Artifact> manifest;
private final SortedMap<PathFragment, Artifact> manifest;
private final PathFragment workspaceName;
private final boolean legacyExternalRunfiles;
// Whether we saw the local workspace name in the runfiles. If legacyExternalRunfiles is true,
Expand Down Expand Up @@ -477,10 +472,8 @@ public void addRootSymlinks(
}
}

/**
* Returns the manifest, adding the workspaceName directory if it is not already present.
*/
public Map<PathFragment, Artifact> build() {
/** Returns the manifest, adding the workspaceName directory if it is not already present. */
public SortedMap<PathFragment, Artifact> build() {
if (!sawWorkspaceName) {
// If we haven't seen it and we have seen other files, add the workspace name directory.
// It might not be there if all of the runfiles are from other repos (and then running from
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -87,7 +88,7 @@ public final class RunfilesSupport {
@VisibleForTesting
public static class RunfilesTreeImpl implements RunfilesTree {

private static final WeakReference<Map<PathFragment, Artifact>> NOT_YET_COMPUTED =
private static final WeakReference<SortedMap<PathFragment, Artifact>> NOT_YET_COMPUTED =
new WeakReference<>(null);

private final PathFragment execPath;
Expand All @@ -108,7 +109,7 @@ public static class RunfilesTreeImpl implements RunfilesTree {
* com.google.devtools.build.lib.runtime.GcThrashingDetector} may throw a manual OOM before all
* soft references are collected. See b/322474776.
*/
@Nullable private volatile WeakReference<Map<PathFragment, Artifact>> cachedMapping;
@Nullable private volatile WeakReference<SortedMap<PathFragment, Artifact>> cachedMapping;

private final boolean buildRunfileLinks;
private final RunfileSymlinksMode runfileSymlinksMode;
Expand Down Expand Up @@ -145,12 +146,12 @@ public PathFragment getExecPath() {
}

@Override
public Map<PathFragment, Artifact> getMapping() {
public SortedMap<PathFragment, Artifact> getMapping() {
if (cachedMapping == null) {
return runfiles.getRunfilesInputs(repoMappingManifest);
}

Map<PathFragment, Artifact> result = cachedMapping.get();
SortedMap<PathFragment, Artifact> result = cachedMapping.get();
if (result != null) {
return result;
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/analysis/util/FakeRunfilesTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Map;
import java.util.SortedMap;
import javax.annotation.Nullable;

/** {@link RunfilesTree} implementation wrapping a single {@link Runfiles} directory mapping. */
Expand Down Expand Up @@ -74,7 +74,7 @@ public PathFragment getExecPath() {
}

@Override
public Map<PathFragment, Artifact> getMapping() {
public SortedMap<PathFragment, Artifact> getMapping() {
return runfiles.getRunfilesInputs(repoMappingManifest);
}

Expand Down
9 changes: 5 additions & 4 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
Expand Down Expand Up @@ -134,8 +134,8 @@
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.io.IOException;
import java.util.Collection;
import java.util.Map;
import java.util.Random;
import java.util.SortedMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
Expand Down Expand Up @@ -2677,8 +2677,9 @@ public PathFragment getExecPath() {
}

@Override
public Map<PathFragment, Artifact> getMapping() {
return artifacts.stream().collect(toImmutableMap(Artifact::getExecPath, a -> a));
public SortedMap<PathFragment, Artifact> getMapping() {
return artifacts.stream()
.collect(toImmutableSortedMap(PathFragment::compareTo, Artifact::getExecPath, a -> a));
}

@Override
Expand Down