From f1f5b5de19976afb0bfa28127b0f68e811634e68 Mon Sep 17 00:00:00 2001 From: wyv Date: Mon, 12 Jul 2021 01:39:09 -0700 Subject: [PATCH] bzlmod: compatibility_level support (https://github.com/bazelbuild/bazel/issues/13316) The compatibility_level is essentially the "major version" in semver, except that it's not embedded in the version string, but exists as a separate field. Modules with different compatibility levels participate in version resolution as if they're modules with different names, but the final dependency graph cannot contain multiple modules with the same name but different compatibility levels. PiperOrigin-RevId: 384185854 --- .../build/lib/bazel/bzlmod/Module.java | 11 +- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 9 +- .../lib/bazel/bzlmod/SelectionFunction.java | 138 ++++++++++++++--- .../repository/ModuleFileGlobalsApi.java | 15 +- .../bazel/bzlmod/ModuleFileFunctionTest.java | 10 +- .../bazel/bzlmod/SelectionFunctionTest.java | 145 +++++++++++++++++- 6 files changed, 292 insertions(+), 36 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java index 843ad377f244a2..eb1f0d3019c91e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java @@ -37,6 +37,12 @@ public abstract class Module { /** The version of the module. Must be empty iff the module has a {@link NonRegistryOverride}. */ public abstract String getVersion(); + /** + * The compatibility level of the module, which essentially signifies the "major version" of the + * module in terms of SemVer. + */ + public abstract int getCompatibilityLevel(); + /** * The direct dependencies of this module. The key type is the repo name of the dep, and the value * type is the ModuleKey (name+version) of the dep. @@ -55,7 +61,7 @@ public abstract class Module { /** Returns a new, empty {@link Builder}. */ public static Builder builder() { - return new AutoValue_Module.Builder(); + return new AutoValue_Module.Builder().setCompatibilityLevel(0); } /** @@ -77,6 +83,9 @@ public abstract static class Builder { public abstract Builder setVersion(String value); + /** Optional; defaults to {@code 0}. */ + public abstract Builder setCompatibilityLevel(int value); + public abstract Builder setDeps(ImmutableMap value); public abstract Builder setRegistry(Registry value); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 1bf9128af9c307..336292049c806a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -37,15 +37,18 @@ public class ModuleFileGlobals implements ModuleFileGlobalsApi depGraph = discovery.getDepGraph(); - Map selectedVersionForEachModule = new HashMap<>(); - for (ModuleKey key : depGraph.keySet()) { + Map selectedVersions = new HashMap<>(); + for (Map.Entry entry : depGraph.entrySet()) { + ModuleKey key = entry.getKey(); + Module module = entry.getValue(); + + ParsedVersion parsedVersion; try { - ParsedVersion parsedVersion = ParsedVersion.parse(key.getVersion()); - selectedVersionForEachModule.merge(key.getName(), parsedVersion, ParsedVersion::max); + parsedVersion = ParsedVersion.parse(key.getVersion()); } catch (ParsedVersion.ParseException e) { throw new SelectionFunctionException(e); } + selectedVersions.merge(SelectionGroup.of(module), parsedVersion, ParsedVersion::max); } // Now build a new dep graph where deps with unselected versions are removed. @@ -57,8 +77,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) for (Map.Entry entry : depGraph.entrySet()) { ModuleKey moduleKey = entry.getKey(); Module module = entry.getValue(); + // Remove any dep whose version isn't selected. - String selectedVersion = selectedVersionForEachModule.get(moduleKey.getName()).getOriginal(); + String selectedVersion = selectedVersions.get(SelectionGroup.of(module)).getOriginal(); if (!moduleKey.getVersion().equals(selectedVersion)) { continue; } @@ -70,31 +91,94 @@ public SkyValue compute(SkyKey skyKey, Environment env) depKey -> ModuleKey.create( depKey.getName(), - selectedVersionForEachModule.get(depKey.getName()).getOriginal()))); + selectedVersions + .get(SelectionGroup.of(depGraph.get(depKey))) + .getOriginal()))); } ImmutableMap newDepGraph = newDepGraphBuilder.build(); // Further remove unreferenced modules from the graph. We can find out which modules are // referenced by collecting deps transitively from the root. - HashMap finalDepGraph = new HashMap<>(); - collectDeps(ModuleKey.create(discovery.getRootModuleName(), ""), newDepGraph, finalDepGraph); + // 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); + try { + walker.walk(ModuleKey.create(discovery.getRootModuleName(), ""), null); + } catch (SelectionException e) { + throw new SelectionFunctionException(e); + } + return SelectionValue.create( - discovery.getRootModuleName(), - ImmutableMap.copyOf(finalDepGraph), - discovery.getOverrides()); + discovery.getRootModuleName(), walker.getNewDepGraph(), discovery.getOverrides()); } - private void collectDeps( - ModuleKey key, - ImmutableMap oldDepGraph, - HashMap newDepGraph) { - if (newDepGraph.containsKey(key)) { - return; + /** + * Walks the dependency graph from the root node, collecting any reachable nodes through deps into + * a new dep graph and checking that nothing conflicts. + */ + static class DepGraphWalker { + private final ImmutableMap oldDepGraph; + private final HashMap newDepGraph; + private final HashMap moduleByName; + + DepGraphWalker(ImmutableMap oldDepGraph) { + this.oldDepGraph = oldDepGraph; + this.newDepGraph = new HashMap<>(); + this.moduleByName = new HashMap<>(); } - Module module = oldDepGraph.get(key); - newDepGraph.put(key, module); - for (ModuleKey depKey : module.getDeps().values()) { - collectDeps(depKey, oldDepGraph, newDepGraph); + + ImmutableMap getNewDepGraph() { + return ImmutableMap.copyOf(newDepGraph); + } + + void walk(ModuleKey key, @Nullable ModuleKey from) throws SelectionException { + if (newDepGraph.containsKey(key)) { + return; + } + Module module = oldDepGraph.get(key); + newDepGraph.put(key, module); + + ExistingModule existingModuleWithSameName = + moduleByName.put( + module.getName(), ExistingModule.create(key, module.getCompatibilityLevel(), from)); + if (existingModuleWithSameName != null) { + // This has to mean that a module with the same name but a different compatibility level was + // also selected. + Preconditions.checkState( + from != null && existingModuleWithSameName.getDependent() != null, + "the root module cannot possibly exist more than once in the dep graph"); + throw new SelectionException( + String.format( + "%s depends on %s with compatibility level %d, but %s depends on %s with" + + " compatibility level %d which is different", + from, + key, + module.getCompatibilityLevel(), + existingModuleWithSameName.getDependent(), + existingModuleWithSameName.getModuleKey(), + existingModuleWithSameName.getCompatibilityLevel())); + } + + for (ModuleKey depKey : module.getDeps().values()) { + walk(depKey, key); + } + } + + @AutoValue + abstract static class ExistingModule { + abstract ModuleKey getModuleKey(); + + abstract int getCompatibilityLevel(); + + @Nullable + abstract ModuleKey getDependent(); + + static ExistingModule create( + ModuleKey moduleKey, int compatibilityLevel, ModuleKey dependent) { + return new AutoValue_SelectionFunction_DepGraphWalker_ExistingModule( + moduleKey, compatibilityLevel, dependent); + } } } @@ -108,4 +192,12 @@ static final class SelectionFunctionException extends SkyFunctionException { super(cause, Transience.PERSISTENT); } } + + // TODO(wyv): Replace this with a DetailedException (possibly named ModuleException or + // ExternalDepsException) and use it consistently across the space. + static final class SelectionException extends Exception { + SelectionException(String message) { + super(message); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java index d227d1f639c2df..65b3c23811a92a 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java @@ -37,6 +37,7 @@ public interface ModuleFileGlobalsApi result = @@ -141,7 +141,7 @@ public void testSimpleDiamond() throws Exception { .addDep("DfromC", ModuleKey.create("D", "2.0")) .build(), ModuleKey.create("D", "2.0"), - Module.builder().setName("D").setVersion("2.0").build()); + Module.builder().setName("D").setVersion("2.0").setCompatibilityLevel(1).build()); } @Test @@ -287,4 +287,143 @@ public void testCircularDependencyDueToSelection() throws Exception { .build()); // D is completely gone. } + + @Test + public void differentCompatibilityLevelIsRejected() throws Exception { + setUpDiscoveryResult( + "A", + ImmutableMap.builder() + .put( + ModuleKey.create("A", ""), + Module.builder() + .setName("A") + .setVersion("") + .addDep("BfromA", ModuleKey.create("B", "1.0")) + .addDep("CfromA", ModuleKey.create("C", "2.0")) + .build()) + .put( + ModuleKey.create("B", "1.0"), + Module.builder() + .setName("B") + .setVersion("1.0") + .addDep("DfromB", ModuleKey.create("D", "1.0")) + .build()) + .put( + ModuleKey.create("C", "2.0"), + Module.builder() + .setName("C") + .setVersion("2.0") + .addDep("DfromC", ModuleKey.create("D", "2.0")) + .build()) + .put( + ModuleKey.create("D", "1.0"), + Module.builder().setName("D").setVersion("1.0").setCompatibilityLevel(1).build()) + .put( + ModuleKey.create("D", "2.0"), + Module.builder().setName("D").setVersion("2.0").setCompatibilityLevel(2).build()) + .build()); + + EvaluationResult result = + driver.evaluate(ImmutableList.of(SelectionValue.KEY), evaluationContext); + assertThat(result.hasError()).isTrue(); + String error = result.getError().toString(); + assertThat(error).contains("B@1.0 depends on D@1.0 with compatibility level 1"); + assertThat(error).contains("C@2.0 depends on D@2.0 with compatibility level 2"); + assertThat(error).contains("which is different"); + } + + @Test + public void differentCompatibilityLevelIsOkIfUnreferenced() throws Exception { + // A 1.0 -> B 1.0 -> C 2.0 + // \-> C 1.0 + // \-> D 1.0 -> B 1.1 + // \-> E 1.0 -> C 1.1 + setUpDiscoveryResult( + "A", + ImmutableMap.builder() + .put( + ModuleKey.create("A", ""), + Module.builder() + .setName("A") + .setVersion("1.0") + .addDep("B", ModuleKey.create("B", "1.0")) + .addDep("C", ModuleKey.create("C", "1.0")) + .addDep("D", ModuleKey.create("D", "1.0")) + .addDep("E", ModuleKey.create("E", "1.0")) + .build()) + .put( + ModuleKey.create("B", "1.0"), + Module.builder() + .setName("B") + .setVersion("1.0") + .addDep("C", ModuleKey.create("C", "2.0")) + .build()) + .put( + ModuleKey.create("C", "2.0"), + Module.builder().setName("C").setVersion("2.0").setCompatibilityLevel(2).build()) + .put( + ModuleKey.create("C", "1.0"), + Module.builder().setName("C").setVersion("1.0").setCompatibilityLevel(1).build()) + .put( + ModuleKey.create("D", "1.0"), + Module.builder() + .setName("D") + .setVersion("1.0") + .addDep("B", ModuleKey.create("B", "1.1")) + .build()) + .put( + ModuleKey.create("B", "1.1"), + Module.builder().setName("B").setVersion("1.1").build()) + .put( + ModuleKey.create("E", "1.0"), + Module.builder() + .setName("E") + .setVersion("1.0") + .addDep("C", ModuleKey.create("C", "1.1")) + .build()) + .put( + ModuleKey.create("C", "1.1"), + Module.builder().setName("C").setVersion("1.1").setCompatibilityLevel(1).build()) + .build()); + + EvaluationResult result = + driver.evaluate(ImmutableList.of(SelectionValue.KEY), evaluationContext); + if (result.hasError()) { + fail(result.getError().toString()); + } + // After selection, C 2.0 is gone, so we're okay. + // A 1.0 -> B 1.1 + // \-> C 1.1 + // \-> D 1.0 -> B 1.1 + // \-> E 1.0 -> C 1.1 + SelectionValue selectionValue = result.get(SelectionValue.KEY); + assertThat(selectionValue.getRootModuleName()).isEqualTo("A"); + assertThat(selectionValue.getDepGraph()) + .containsExactly( + ModuleKey.create("A", ""), + Module.builder() + .setName("A") + .setVersion("1.0") + .addDep("B", ModuleKey.create("B", "1.1")) + .addDep("C", ModuleKey.create("C", "1.1")) + .addDep("D", ModuleKey.create("D", "1.0")) + .addDep("E", ModuleKey.create("E", "1.0")) + .build(), + ModuleKey.create("B", "1.1"), + Module.builder().setName("B").setVersion("1.1").build(), + ModuleKey.create("C", "1.1"), + Module.builder().setName("C").setVersion("1.1").setCompatibilityLevel(1).build(), + ModuleKey.create("D", "1.0"), + Module.builder() + .setName("D") + .setVersion("1.0") + .addDep("B", ModuleKey.create("B", "1.1")) + .build(), + ModuleKey.create("E", "1.0"), + Module.builder() + .setName("E") + .setVersion("1.0") + .addDep("C", ModuleKey.create("C", "1.1")) + .build()); + } }