Skip to content

Commit

Permalink
Add flag --incompatible_use_plus_in_repo_names
Browse files Browse the repository at this point in the history
Defaults to false. When set to true, we use `+` instead of `~` as the separator in canonical repo names.

Some more subtle changes include:
- We now officially say that the "build" part of version strings (the part that begins with a plus) is ignored and stripped.
- When the flag is set to true, we effectively increase the lockfile version by 1 (see code comment in BazelLockFileModule).
- When true, we no longer insert a `_main` in front of names of repos generated by module extensions hosted in the main repo. (`~abc` as a name was problematic, but `+abc` is not.)
- When true, we no longer insert a `v` in front of numerical versions in canonical repo names. (`my_mod~1.1` could be a Windows short path, but `my_mod+1.1` cannot.)

Work towards #22865.
  • Loading branch information
Wyverald committed Jul 24, 2024
1 parent 3f07c4e commit 4c1a50b
Show file tree
Hide file tree
Showing 17 changed files with 162 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ java_library(
],
deps = [
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/net/starlark/java/eval",
"//third_party:auto_value",
"//third_party:gson",
Expand Down Expand Up @@ -251,6 +252,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.LabelConverter;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
Expand All @@ -42,6 +44,7 @@
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Map.Entry;
import javax.annotation.Nullable;
import net.starlark.java.eval.StarlarkSemantics;

/**
* This function runs Bazel module resolution, extracts the dependency graph from it and creates a
Expand All @@ -58,13 +61,14 @@ public SkyValue compute(SkyKey skyKey, Environment env)
throws BazelDepGraphFunctionException, InterruptedException {
BazelModuleResolutionValue selectionResult =
(BazelModuleResolutionValue) env.getValue(BazelModuleResolutionValue.KEY);
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (env.valuesMissing()) {
return null;
}
var depGraph = selectionResult.getResolvedDepGraph();

ImmutableBiMap<RepositoryName, ModuleKey> canonicalRepoNameLookup =
computeCanonicalRepoNameLookup(depGraph);
computeCanonicalRepoNameLookup(depGraph, starlarkSemantics);
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById;
try {
extensionUsagesById = getExtensionUsagesById(depGraph, canonicalRepoNameLookup.inverse());
Expand All @@ -73,14 +77,17 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

ImmutableBiMap<String, ModuleExtensionId> extensionUniqueNames =
calculateUniqueNameForUsedExtensionId(extensionUsagesById);
calculateUniqueNameForUsedExtensionId(extensionUsagesById, starlarkSemantics);

return BazelDepGraphValue.create(
depGraph,
canonicalRepoNameLookup,
depGraph.values().stream().map(AbridgedModule::from).collect(toImmutableList()),
extensionUsagesById,
extensionUniqueNames.inverse());
extensionUniqueNames.inverse(),
starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES)
? '+'
: '~');
}

private static ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
Expand Down Expand Up @@ -126,7 +133,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

private static ImmutableBiMap<RepositoryName, ModuleKey> computeCanonicalRepoNameLookup(
ImmutableMap<ModuleKey, Module> depGraph) {
ImmutableMap<ModuleKey, Module> depGraph, StarlarkSemantics semantics) {
// Find modules with multiple versions in the dep graph. Currently, the only source of such
// modules is multiple_version_override.
ImmutableSet<String> multipleVersionsModules =
Expand All @@ -151,13 +158,14 @@ private static ImmutableBiMap<RepositoryName, ModuleKey> computeCanonicalRepoNam
toImmutableBiMap(
key ->
multipleVersionsModules.contains(key.getName())
? key.getCanonicalRepoNameWithVersion()
: key.getCanonicalRepoNameWithoutVersion(),
? key.getCanonicalRepoNameWithVersion(semantics)
: key.getCanonicalRepoNameWithoutVersion(semantics),
key -> key));
}

private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExtensionId(
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById,
StarlarkSemantics starlarkSemantics) {
// Calculate a unique name for each used extension id with the following property that is
// required for BzlmodRepoRuleFunction to unambiguously identify the extension that generates a
// given repo:
Expand All @@ -166,18 +174,23 @@ private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExte
BiMap<String, ModuleExtensionId> extensionUniqueNames = HashBiMap.create();
for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
int attempt = 1;
while (extensionUniqueNames.putIfAbsent(makeUniqueNameCandidate(id, attempt), id) != null) {
while (extensionUniqueNames.putIfAbsent(
makeUniqueNameCandidate(id, attempt, starlarkSemantics), id)
!= null) {
attempt++;
}
}
return ImmutableBiMap.copyOf(extensionUniqueNames);
}

private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) {
private static String makeUniqueNameCandidate(
ModuleExtensionId id, int attempt, StarlarkSemantics starlarkSemantics) {
boolean usePlus =
starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES);
// Ensure that the resulting extension name (and thus the repository names derived from it) do
// not start with a tilde.
RepositoryName repository = id.getBzlFileLabel().getRepository();
String nonEmptyRepoPart = repository.isMain() ? "_main" : repository.getName();
String nonEmptyRepoPart = repository.isMain() && !usePlus ? "_main" : repository.getName();
// When using a namespace, prefix the extension name with "_" to distinguish the prefix from
// those generated by non-namespaced extension usages. Extension names are identified by their
// Starlark identifier, which in the case of an exported symbol cannot start with "_".
Expand All @@ -191,14 +204,18 @@ private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt)
.map(
namespace ->
String.format(
"%s~_%s%s~%s~%s~%s",
usePlus ? "%s+_%s%s+%s+%s+%s" : "%s~_%s%s~%s~%s~%s",
nonEmptyRepoPart,
id.getExtensionName(),
extensionNameDisambiguator,
namespace.getModule().getName(),
namespace.getModule().getVersion(),
namespace.getUsageExportedName()))
.orElse(nonEmptyRepoPart + "~" + id.getExtensionName() + extensionNameDisambiguator);
.orElse(
nonEmptyRepoPart
+ (usePlus ? "+" : "~")
+ id.getExtensionName()
+ extensionNameDisambiguator);
}

static class BazelDepGraphFunctionException extends SkyFunctionException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ public static BazelDepGraphValue create(
ImmutableMap<RepositoryName, ModuleKey> canonicalRepoNameLookup,
ImmutableList<AbridgedModule> abridgedModules,
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesTable,
ImmutableMap<ModuleExtensionId, String> extensionUniqueNames) {
ImmutableMap<ModuleExtensionId, String> extensionUniqueNames,
char repoNameSeparator) {
return new AutoValue_BazelDepGraphValue(
depGraph,
ImmutableBiMap.copyOf(canonicalRepoNameLookup),
abridgedModules,
extensionUsagesTable,
extensionUniqueNames);
extensionUniqueNames,
repoNameSeparator);
}

public static BazelDepGraphValue createEmptyDepGraph() {
Expand Down Expand Up @@ -80,7 +82,8 @@ public static BazelDepGraphValue createEmptyDepGraph() {
canonicalRepoNameLookup,
ImmutableList.of(),
ImmutableTable.of(),
ImmutableMap.of());
ImmutableMap.of(),
'~');
}

/**
Expand Down Expand Up @@ -112,6 +115,9 @@ public static BazelDepGraphValue createEmptyDepGraph() {
*/
public abstract ImmutableMap<ModuleExtensionId, String> getExtensionUniqueNames();

/** The character to use to separate the different segments of a canonical repo name. */
public abstract char getRepoNameSeparator();

/**
* Returns the full {@link RepositoryMapping} for the given module, including repos from Bazel
* module deps and module extensions.
Expand All @@ -122,7 +128,7 @@ public final RepositoryMapping getFullRepoMapping(ModuleKey key) {
getExtensionUsagesTable().column(key).entrySet()) {
ModuleExtensionId extensionId = extIdAndUsage.getKey();
ModuleExtensionUsage usage = extIdAndUsage.getValue();
String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + "~";
String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + getRepoNameSeparator();
for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) {
for (Map.Entry<String, String> entry : proxy.getImports().entrySet()) {
String canonicalRepoName = repoNamePrefix + entry.getValue();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,17 @@ public void afterCommand() {
// lockfile that are still up-to-date and adding the newly resolved extension results.
BazelLockFileValue newLockfile =
BazelLockFileValue.builder()
// HACK: Flipping the flag `--incompatible_use_plus_in_repo_names` causes the canonical
// repo name format to change, which warrants a lockfile invalidation. We could do this
// properly by introducing another field in the lockfile, but that's very annoying since
// we'll need to 1) keep it around for a while; 2) be careful not to parse the rest of
// the lockfile to avoid triggering parsing errors ('~' will be an invalid character in
// repo names eventually). So we just increment the lockfile version if '+' is being
// used.
.setLockFileVersion(
depGraphValue.getRepoNameSeparator() == '+'
? BazelLockFileValue.LOCK_FILE_VERSION + 1
: BazelLockFileValue.LOCK_FILE_VERSION)
.setRegistryFileHashes(
ImmutableSortedMap.copyOf(moduleResolutionValue.getRegistryFileHashes()))
.setSelectedYankedVersions(moduleResolutionValue.getSelectedYankedVersions())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public static IsolationKey create(ModuleKey module, String usageExportedName) {

@Override
public final String toString() {
// NOTE: Can't be bothered to switch this based on the flag.
return getModule() + "~" + getUsageExportedName();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
GetModuleFileResult getModuleFileResult;
try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.BZLMOD, () -> "fetch module file: " + moduleKey)) {
getModuleFileResult = getModuleFile(moduleKey, moduleFileKey.getOverride(), env);
getModuleFileResult =
getModuleFile(moduleKey, moduleFileKey.getOverride(), starlarkSemantics, env);
}
if (getModuleFileResult == null) {
return null;
Expand Down Expand Up @@ -481,7 +482,8 @@ public static RootModuleFileValue evaluateRootModuleFile(
// A module with a non-registry override always has a unique version across the
// entire dep graph.
name ->
ModuleKey.create(name, Version.EMPTY).getCanonicalRepoNameWithoutVersion(),
ModuleKey.create(name, Version.EMPTY)
.getCanonicalRepoNameWithoutVersion(starlarkSemantics),
name -> name));
ImmutableSet<PathFragment> moduleFilePaths =
Stream.concat(
Expand Down Expand Up @@ -552,14 +554,17 @@ private record GetModuleFileResult(

@Nullable
private GetModuleFileResult getModuleFile(
ModuleKey key, @Nullable ModuleOverride override, Environment env)
ModuleKey key,
@Nullable ModuleOverride override,
StarlarkSemantics starlarkSemantics,
Environment env)
throws ModuleFileFunctionException, InterruptedException {
// If there is a non-registry override for this module, we need to fetch the corresponding repo
// first and read the module file from there.
if (override instanceof NonRegistryOverride) {
// A module with a non-registry override always has a unique version across the entire dep
// graph.
RepositoryName canonicalRepoName = key.getCanonicalRepoNameWithoutVersion();
RepositoryName canonicalRepoName = key.getCanonicalRepoNameWithoutVersion(starlarkSemantics);
RepositoryDirectoryValue repoDir =
(RepositoryDirectoryValue) env.getValue(RepositoryDirectoryValue.key(canonicalRepoName));
if (repoDir == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import java.util.Comparator;
import java.util.List;
import net.starlark.java.eval.StarlarkSemantics;

/** A module name, version pair that identifies a module in the external dependency graph. */
@AutoValue
Expand Down Expand Up @@ -79,35 +81,44 @@ public final String toString() {
*
* <p>This method must not be called if the module has a {@link NonRegistryOverride}.
*/
public RepositoryName getCanonicalRepoNameWithVersion(StarlarkSemantics semantics) {
return getCanonicalRepoName(/* includeVersion= */ true, semantics);
}

public RepositoryName getCanonicalRepoNameWithVersion() {
return getCanonicalRepoName(/* includeVersion= */ true);
return getCanonicalRepoNameWithVersion(StarlarkSemantics.DEFAULT);
}

/**
* Returns the canonical name of the repo backing this module, excluding its version. This name is
* only guaranteed to be unique when there is a single version of the module in the entire dep
* graph.
*/
public RepositoryName getCanonicalRepoNameWithoutVersion(StarlarkSemantics semantics) {
return getCanonicalRepoName(/* includeVersion= */ false, semantics);
}

public RepositoryName getCanonicalRepoNameWithoutVersion() {
return getCanonicalRepoName(/* includeVersion= */ false);
return getCanonicalRepoNameWithoutVersion(StarlarkSemantics.DEFAULT);
}

private RepositoryName getCanonicalRepoName(boolean includeVersion) {
private RepositoryName getCanonicalRepoName(boolean includeVersion, StarlarkSemantics semantics) {
if (WELL_KNOWN_MODULES.containsKey(getName())) {
return WELL_KNOWN_MODULES.get(getName());
}
if (ROOT.equals(this)) {
return RepositoryName.MAIN;
}
boolean usePlus = semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES);
String suffix;
if (includeVersion) {
// getVersion().isEmpty() is true only for modules with non-registry overrides, which enforce
// that there is a single version of the module in the dep graph.
Preconditions.checkState(!getVersion().isEmpty());
// Prepend "v" to prevent canonical repo names, which form segments of file paths, from
// looking like a Windows short path. Such paths segments would incur additional file IO
// during analysis (see WindowsShortPath).
suffix = "v" + getVersion().toString();
// When using `~` as the separator, prepend "v" to prevent canonical repo names, which form
// segments of file paths, from looking like a Windows short path. Such paths segments would
// incur additional file IO during analysis (see WindowsShortPath).
suffix = usePlus ? getVersion().toString() : "v" + getVersion().toString();
} else {
// This results in canonical repository names such as `rules_foo~` for the module `rules_foo`.
// This particular format is chosen since:
Expand All @@ -125,7 +136,8 @@ private RepositoryName getCanonicalRepoName(boolean includeVersion) {
// rarely used.
suffix = "";
}
return RepositoryName.createUnvalidated(String.format("%s~%s", getName(), suffix));
return RepositoryName.createUnvalidated(
String.format("%s%s%s", getName(), usePlus ? "+" : "~", suffix));
}

public static ModuleKey fromString(String s) throws Version.ParseException {
Expand Down
Loading

0 comments on commit 4c1a50b

Please sign in to comment.