8000 Have `RunfilesTree#getMapping` return a `SortedMap` · coderabbit-test/bazel@336b428 · GitHub
[go: up one dir, main page]
Skip to content

Commit 336b428

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Have RunfilesTree#getMapping return a SortedMap
It already always returns a `SortedMap` in production code, but signaling this on the type level makes it safer to consume the map in contexts where sorting is required, e.g. when computing Merkle trees. Also drop an impossible switch case in adjacent code that is flagged by an IDE lint. Closes bazelbuild#25625. PiperOrigin-RevId: 747396796 Change-Id: I3be6e1320c4325fc0c52119cdda56b0073bb7934
1 parent c03e65c commit 336b428

File tree

6 files changed

+25
-27
lines changed

6 files changed

+25
-27
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import java.io.Closeable;
4444
import java.io.IOException;
4545
import java.util.Map;
46+
import java.util.SortedMap;
4647
import javax.annotation.Nullable;
4748

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

6970
@Override
70-
public Map<PathFragment, Artifact> getMapping() {
71+
public SortedMap<PathFragment, Artifact> getMapping() {
7172
return wrapped.getMapping();
7273
}
7374

‎src/main/java/com/google/devtools/build/lib/actions/RunfilesTree.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2020
import com.google.devtools.build.lib.util.Fingerprint;
2121
import com.google.devtools.build.lib.vfs.PathFragment;
22-
import java.util.Map;
22+
import java.util.SortedMap;
2323
import javax.annotation.Nullable;
2424

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

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

3535
/**
3636
* Returns artifacts the runfiles tree contain symlinks to.

‎src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import java.util.LinkedHashMap;
5252
import java.util.Map;
5353
import java.util.Set;
54+
import java.util.SortedMap;
5455
import java.util.TreeMap;
5556
import java.util.UUID;
5657
import java.util.function.BiConsumer;
@@ -345,7 +346,7 @@ static Map<PathFragment, Artifact> filterListForObscuringSymlinks(
345346
* entries and elements that live outside the source tree. Null values represent empty input
346347
* files.
347348
*/
348-
public Map<PathFragment, Artifact> getRunfilesInputs(Artifact repoMappingManifest) {
349+
public SortedMap<PathFragment, Artifact> getRunfilesInputs(Artifact repoMappingManifest) {
349350
return getRunfilesInputs(EnumSet.noneOf(ConflictType.class), null, repoMappingManifest);
350351
}
351352

@@ -358,12 +359,6 @@ public BiConsumer<ConflictType, String> eventRunfilesConflictReceiver(
358359
case NESTED_RUNFILES_TREE -> EventKind.ERROR;
359360
case PREFIX_CONFLICT ->
360361
conflictPolicy == ConflictPolicy.ERROR ? EventKind.ERROR : EventKind.WARNING;
361-
default ->
362-
switch (conflictPolicy) {
363-
case IGNORE -> throw new IllegalStateException();
364-
default ->
365-
conflictPolicy == ConflictPolicy.ERROR ? EventKind.ERROR : EventKind.WARNING;
366-
};
367362
};
368363

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

395-
private Map<PathFragment, Artifact> getRunfilesInputs(
390+
private SortedMap<PathFragment, Artifact> getRunfilesInputs(
396391
EnumSet<ConflictType> conflictSet,
397392
BiConsumer<ConflictType, String> receiver,
398393
@Nullable Artifact repoMappingManifest) {
@@ -437,7 +432,7 @@ public boolean isLegacyExternalRunfiles() {
437432
@VisibleForTesting
438433
static final class ManifestBuilder {
439434
// Manifest of paths to artifacts. Path fragments are relative to the .runfiles directory.
440-
private final Map<PathFragment, Artifact> manifest;
435+
private final SortedMap<PathFragment, Artifact> manifest;
441436
private final PathFragment workspaceName;
442437
private final boolean legacyExternalRunfiles;
443438
// Whether we saw the local workspace name in the runfiles. If legacyExternalRunfiles is true,
@@ -477,10 +472,8 @@ public void addRootSymlinks(
477472
}
478473
}
479474

480-
/**
481-
* Returns the manifest, adding the workspaceName directory if it is not already present.
482-
*/
483-
public Map<PathFragment, Artifact> build() {
475+
/** Returns the manifest, adding the workspaceName directory if it is not already present. */
476+
public SortedMap<PathFragment, Artifact> build() {
484477
if (!sawWorkspaceName) {
485478
// If we haven't seen it and we have seen other files, add the workspace name directory.
486479
// It might not be there if all of the runfiles are from other repos (and then running from

‎src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.util.LinkedHashSet;
5050
import java.util.Map;
5151
import java.util.Set;
52+
import java.util.SortedMap;
5253
import java.util.TreeMap;
5354
import javax.annotation.Nullable;
5455

@@ -87,7 +88,7 @@ public final class RunfilesSupport {
8788
@VisibleForTesting
8889
public static class RunfilesTreeImpl implements RunfilesTree {
8990

90-
private static final WeakReference<Map<PathFragment, Artifact>> NOT_YET_COMPUTED =
91+
private static final WeakReference<SortedMap<PathFragment, Artifact>> NOT_YET_COMPUTED =
9192
new WeakReference<>(null);
9293

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

113114
private final boolean buildRunfileLinks;
114115
private final RunfileSymlinksMode runfileSymlinksMode;
@@ -145,12 +146,12 @@ public PathFragment getExecPath() {
145146
}
146147

147148
@Override
148-
public Map<PathFragment, Artifact> getMapping() {
149+
public SortedMap<PathFragment, Artifact> getMapping() {
149150
if (cachedMapping == null) {
150151
return runfiles.getRunfilesInputs(repoMappingManifest);
151152
}
152153

153-
Map<PathFragment, Artifact> result = cachedMapping.get();
154+
SortedMap<PathFragment, Artifact> result = cachedMapping.get();
154155
if (result != null) {
155156
return result;
156157
}

‎src/test/java/com/google/devtools/build/lib/analysis/util/FakeRunfilesTree.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
2828
import com.google.devtools.build.lib.util.Fingerprint;
2929
import com.google.devtools.build.lib.vfs.PathFragment;
30-
import java.util.Map;
30+
import java.util.SortedMap;
3131
import javax.annotation.Nullable;
3232

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

7676
@Override
77-
public Map<PathFragment, Artifact> getMapping() {
77+
public SortedMap<PathFragment, Artifact> getMapping() {
7878
return runfiles.getRunfilesInputs(repoMappingManifest);
7979
}
8080

‎src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
import static com.google.common.base.Preconditions.checkNotNull;
1616
import static com.google.common.collect.ImmutableList.toImmutableList;
17-
import static com.google.common.collect.ImmutableMap.toImmutableMap;
17+
import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
1818
import static com.google.common.truth.Truth.assertThat;
1919
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
2020
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
@@ -24,6 +24,8 @@
2424
import static com.google.devtools.build.lib.util.StringEncoding.unicodeToInternal;
2525
import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContent;
2626
import static java.nio.charset.StandardCharsets.UTF_8;
27+
import static java.util.Comparator.naturalOrder;
28+
import static java.util.function.Function.identity;
2729
import static org.junit.Assert.assertThrows;
2830
import static org.mockito.ArgumentMatchers.any;
2931
import static org.mockito.Mockito.doAnswer;
@@ -136,8 +138,8 @@
136138
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
137139
import java.io.IOException;
138140
import java.util.Collection;
139-
import java.util.Map;
140141
import java.util.Random;
142+
import java.util.SortedMap;
141143
import java.util.concurrent.CountDownLatch;
142144
import java.util.concurrent.ExecutorService;
143145
import java.util.concurrent.Executors;
@@ -2681,8 +2683,9 @@ public PathFragment getExecPath() {
26812683
}
26822684

26832685
@Override
2684-
public Map<PathFragment, Artifact> getMapping() {
2685-
return artifacts.stream().collect(toImmutableMap(Artifact::getExecPath, a -> a));
2686+
public SortedMap<PathFragment, Artifact> getMapping() {
2687+
return artifacts.stream()
2688+
.collect(toImmutableSortedMap(naturalOrder(), Artifact::getExecPath, identity()));
26862689
}
26872690

26882691
@Override

0 commit comments

Comments
 (0)
0