From 3641e24d720577e687ecf49270f30a02c545868b Mon Sep 17 00:00:00 2001 From: wyv Date: Tue, 27 Jul 2021 04:23:06 -0700 Subject: [PATCH] bzlmod: Guarantee BFS iteration order for SelectionValue#getDepGraph (https://github.com/bazelbuild/bazel/issues/13316) This makes it easier for later usage (for example, when collecting toolchains across the dep graph, BFS order makes the most sense). Also added some missing test assertions for the reverse lookups (which actually fixed a small bug). PiperOrigin-RevId: 387076182 --- .../lib/bazel/bzlmod/SelectionFunction.java | 78 ++++--- .../lib/bazel/bzlmod/SelectionValue.java | 2 +- .../bazel/bzlmod/SelectionFunctionTest.java | 219 +++++++++++++++++- 3 files changed, 262 insertions(+), 37 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunction.java index afcbf2fdcf93dc..f0077a4392eec7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunction.java @@ -31,8 +31,12 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.util.ArrayDeque; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Queue; +import java.util.Set; import javax.annotation.Nullable; /** @@ -234,29 +238,23 @@ public SkyValue compute(SkyKey skyKey, Environment env) // We can also take this opportunity to check that none of the remaining modules conflict with // each other (e.g. same module name but different compatibility levels, or not satisfying // multiple_version_override). - DepGraphWalker walker = new DepGraphWalker(newDepGraph, overrides, selectionGroups); + DepGraphWalker walker = + new DepGraphWalker(newDepGraph, discovery.getRootModuleName(), overrides, selectionGroups); try { - walker.walk(ModuleKey.create(discovery.getRootModuleName(), Version.EMPTY), null); + newDepGraph = walker.walk(); } catch (ExternalDepsException e) { throw new SelectionFunctionException(e); } - newDepGraph = walker.getNewDepGraph(); ImmutableMap canonicalRepoNameLookup = - depGraph.keySet().stream() + newDepGraph.keySet().stream() .collect(toImmutableMap(ModuleKey::getCanonicalRepoName, key -> key)); ImmutableMap moduleNameLookup = - Maps.filterKeys( - newDepGraph, - key -> !(overrides.get(key.getName()) instanceof MultipleVersionOverride)) - .keySet() - .stream() + newDepGraph.keySet().stream() + .filter(key -> !(overrides.get(key.getName()) instanceof MultipleVersionOverride)) .collect(toImmutableMap(ModuleKey::getName, key -> key)); return SelectionValue.create( - discovery.getRootModuleName(), - walker.getNewDepGraph(), - canonicalRepoNameLookup, - moduleNameLookup); + discovery.getRootModuleName(), newDepGraph, canonicalRepoNameLookup, moduleNameLookup); } /** @@ -266,33 +264,51 @@ public SkyValue compute(SkyKey skyKey, Environment env) static class DepGraphWalker { private static final Joiner JOINER = Joiner.on(", "); private final ImmutableMap oldDepGraph; + private final ModuleKey rootModuleKey; private final ImmutableMap overrides; private final ImmutableMap selectionGroups; - private final HashMap newDepGraph; private final HashMap moduleByName; DepGraphWalker( ImmutableMap oldDepGraph, + String rootModuleName, ImmutableMap overrides, ImmutableMap selectionGroups) { this.oldDepGraph = oldDepGraph; + this.rootModuleKey = ModuleKey.create(rootModuleName, Version.EMPTY); this.overrides = overrides; this.selectionGroups = selectionGroups; - this.newDepGraph = new HashMap<>(); this.moduleByName = new HashMap<>(); } - ImmutableMap getNewDepGraph() { - return ImmutableMap.copyOf(newDepGraph); - } - - void walk(ModuleKey key, @Nullable ModuleKey from) throws ExternalDepsException { - if (newDepGraph.containsKey(key)) { - return; + /** + * Walks the old dep graph and builds a new dep graph containing only deps reachable from the + * root module. The returned map has a guaranteed breadth-first iteration order. + */ + ImmutableMap walk() throws ExternalDepsException { + ImmutableMap.Builder newDepGraph = ImmutableMap.builder(); + Set known = new HashSet<>(); + Queue toVisit = new ArrayDeque<>(); + toVisit.add(ModuleKeyAndDependent.create(rootModuleKey, null)); + known.add(rootModuleKey); + while (!toVisit.isEmpty()) { + ModuleKeyAndDependent moduleKeyAndDependent = toVisit.remove(); + ModuleKey key = moduleKeyAndDependent.getModuleKey(); + Module module = oldDepGraph.get(key); + visit(key, module, moduleKeyAndDependent.getDependent()); + + for (ModuleKey depKey : module.getDeps().values()) { + if (known.add(depKey)) { + toVisit.add(ModuleKeyAndDependent.create(depKey, key)); + } + } + newDepGraph.put(key, module); } - Module module = oldDepGraph.get(key); - newDepGraph.put(key, module); + return newDepGraph.build(); + } + void visit(ModuleKey key, Module module, @Nullable ModuleKey from) + throws ExternalDepsException { ModuleOverride override = overrides.get(key.getName()); if (override instanceof MultipleVersionOverride) { if (selectionGroups.get(key).getTargetAllowedVersion().isEmpty()) { @@ -351,10 +367,18 @@ void walk(ModuleKey key, @Nullable ModuleKey from) throws ExternalDepsException key.getName()); } } + } + + @AutoValue + abstract static class ModuleKeyAndDependent { + abstract ModuleKey getModuleKey(); + + @Nullable + abstract ModuleKey getDependent(); - // Now visit our dependencies. - for (ModuleKey depKey : module.getDeps().values()) { - walk(depKey, key); + static ModuleKeyAndDependent create(ModuleKey moduleKey, @Nullable ModuleKey dependent) { + return new AutoValue_SelectionFunction_DepGraphWalker_ModuleKeyAndDependent( + moduleKey, dependent); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionValue.java index 2f9b6eb29c6691..91aafcbee49540 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionValue.java @@ -39,7 +39,7 @@ public static SelectionValue create( public abstract String getRootModuleName(); - /** The post-selection dep graph. */ + /** The post-selection dep graph. Must have BFS iteration order, starting from the root module. */ public abstract ImmutableMap getDepGraph(); /** A mapping from a canonical repo name to the key of the module backing it. */ diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunctionTest.java index 155f34eca7a37f..72da54b526b303 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunctionTest.java @@ -182,7 +182,28 @@ public void testSimpleDiamond() throws Exception { .setName("D") .setVersion(Version.parse("2.0")) .setCompatibilityLevel(1) - .build()); + .build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.0", + createModuleKey("B", "1.0"), + "C.2.0", + createModuleKey("C", "2.0"), + "D.2.0", + createModuleKey("D", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B", + createModuleKey("B", "1.0"), + "C", + createModuleKey("C", "2.0"), + "D", + createModuleKey("D", "2.0")); } @Test @@ -259,7 +280,28 @@ public void testDiamondWithFurtherRemoval() throws Exception { .addDep("D", createModuleKey("D", "2.0")) .build(), createModuleKey("D", "2.0"), - Module.builder().setName("D").setVersion(Version.parse("2.0")).build()); + Module.builder().setName("D").setVersion(Version.parse("2.0")).build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.0", + createModuleKey("B", "1.0"), + "C.2.0", + createModuleKey("C", "2.0"), + "D.2.0", + createModuleKey("D", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B", + createModuleKey("B", "1.0"), + "C", + createModuleKey("C", "2.0"), + "D", + createModuleKey("D", "2.0")); } @Test @@ -327,8 +369,25 @@ public void testCircularDependencyDueToSelection() throws Exception { .setName("C") .setVersion(Version.parse("2.0")) .addDep("B", createModuleKey("B", "1.0")) - .build()); + .build()) + .inOrder(); // D is completely gone. + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.0", + createModuleKey("B", "1.0"), + "C.2.0", + createModuleKey("C", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B", + createModuleKey("B", "1.0"), + "C", + createModuleKey("C", "2.0")); } @Test @@ -493,7 +552,32 @@ public void differentCompatibilityLevelIsOkIfUnreferenced() throws Exception { .setName("E") .setVersion(Version.parse("1.0")) .addDep("C", createModuleKey("C", "1.1")) - .build()); + .build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.1", + createModuleKey("B", "1.1"), + "C.1.1", + createModuleKey("C", "1.1"), + "D.1.0", + createModuleKey("D", "1.0"), + "E.1.0", + createModuleKey("E", "1.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B", + createModuleKey("B", "1.1"), + "C", + createModuleKey("C", "1.1"), + "D", + createModuleKey("D", "1.0"), + "E", + createModuleKey("E", "1.0")); } @Test @@ -577,7 +661,18 @@ public void multipleVersionOverride_fork_goodCase() throws Exception { createModuleKey("B", "1.0"), Module.builder().setName("B").setVersion(Version.parse("1.0")).build(), createModuleKey("B", "2.0"), - Module.builder().setName("B").setVersion(Version.parse("2.0")).build()); + Module.builder().setName("B").setVersion(Version.parse("2.0")).build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.0", + createModuleKey("B", "1.0"), + "B.2.0", + createModuleKey("B", "2.0")); + // No B in the module name lookup because there's a multiple-version override. + assertThat(selectionValue.getModuleNameLookup()).containsExactly("A", createModuleKey("A", "")); } @Test @@ -704,7 +799,28 @@ public void multipleVersionOverride_diamond_differentCompatibilityLevels() throw .setName("D") .setVersion(Version.parse("2.0")) .setCompatibilityLevel(2) - .build()); + .build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.0", + createModuleKey("B", "1.0"), + "C.2.0", + createModuleKey("C", "2.0"), + "D.1.0", + createModuleKey("D", "1.0"), + "D.2.0", + createModuleKey("D", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B", + createModuleKey("B", "1.0"), + "C", + createModuleKey("C", "2.0")); } @Test @@ -777,7 +893,28 @@ public void multipleVersionOverride_diamond_sameCompatibilityLevel() throws Exce createModuleKey("D", "1.0"), Module.builder().setName("D").setVersion(Version.parse("1.0")).build(), createModuleKey("D", "2.0"), - Module.builder().setName("D").setVersion(Version.parse("2.0")).build()); + Module.builder().setName("D").setVersion(Version.parse("2.0")).build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B.1.0", + createModuleKey("B", "1.0"), + "C.2.0", + createModuleKey("C", "2.0"), + "D.1.0", + createModuleKey("D", "1.0"), + "D.2.0", + createModuleKey("D", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B", + createModuleKey("B", "1.0"), + "C", + createModuleKey("C", "2.0")); } @Test @@ -949,7 +1086,42 @@ public void multipleVersionOverride_diamond_snappingToNextHighestVersion() throw .setName("C") .setVersion(Version.parse("2.0")) .setCompatibilityLevel(2) - .build()); + .build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B1.1.0", + createModuleKey("B1", "1.0"), + "B2.1.0", + createModuleKey("B2", "1.0"), + "B3.1.0", + createModuleKey("B3", "1.0"), + "B4.1.0", + createModuleKey("B4", "1.0"), + "B5.1.0", + createModuleKey("B5", "1.0"), + "C.1.3", + createModuleKey("C", "1.3"), + "C.1.7", + createModuleKey("C", "1.7"), + "C.2.0", + createModuleKey("C", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B1", + createModuleKey("B1", "1.0"), + "B2", + createModuleKey("B2", "1.0"), + "B3", + createModuleKey("B3", "1.0"), + "B4", + createModuleKey("B4", "1.0"), + "B5", + createModuleKey("B5", "1.0")); } @Test @@ -1247,6 +1419,35 @@ public void multipleVersionOverride_diamond_badVersionsAreOkayIfUnreferenced() t .setName("C") .setVersion(Version.parse("2.0")) .setCompatibilityLevel(2) - .build()); + .build()) + .inOrder(); + assertThat(selectionValue.getCanonicalRepoNameLookup()) + .containsExactly( + "A.", + createModuleKey("A", ""), + "B1.1.0", + createModuleKey("B1", "1.0"), + "B2.1.1", + createModuleKey("B2", "1.1"), + "B3.1.0", + createModuleKey("B3", "1.0"), + "B4.1.1", + createModuleKey("B4", "1.1"), + "C.1.0", + createModuleKey("C", "1.0"), + "C.2.0", + createModuleKey("C", "2.0")); + assertThat(selectionValue.getModuleNameLookup()) + .containsExactly( + "A", + createModuleKey("A", ""), + "B1", + createModuleKey("B1", "1.0"), + "B2", + createModuleKey("B2", "1.1"), + "B3", + createModuleKey("B3", "1.0"), + "B4", + createModuleKey("B4", "1.1")); } }