Skip to content

Commit 3c07a34

Browse files
Wyveraldcopybara-github
Wyverald
authored and
copybara-github
committed
Change RepositoryMapping's key type to String
An instance of RepositoryName should always represent a post-repo-mapping, canonical repo name (as it already does except when used with the RepositoryMapping class). Well, also except when ownerRepo() is non-null :) PiperOrigin-RevId: 454141003 Change-Id: Ie79a9c0d6ead773f9b3f7633e1a7d260a0c49970
1 parent 3dd2b93 commit 3c07a34

21 files changed

Lines changed: 170 additions & 224 deletions

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionValue.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,17 +100,15 @@ public static BazelModuleResolutionValue create(
100100
* module deps and module extensions.
101101
*/
102102
public final RepositoryMapping getFullRepoMapping(ModuleKey key) {
103-
ImmutableMap.Builder<RepositoryName, RepositoryName> mapping = ImmutableMap.builder();
103+
ImmutableMap.Builder<String, RepositoryName> mapping = ImmutableMap.builder();
104104
for (Map.Entry<ModuleExtensionId, ModuleExtensionUsage> e :
105105
getExtensionUsagesTable().column(key).entrySet()) {
106106
ModuleExtensionId extensionId = e.getKey();
107107
ModuleExtensionUsage usage = e.getValue();
108108
for (Map.Entry<String, String> entry : usage.getImports().entrySet()) {
109109
String canonicalRepoName =
110110
getExtensionUniqueNames().get(extensionId) + "." + entry.getValue();
111-
mapping.put(
112-
RepositoryName.createUnvalidated(entry.getKey()),
113-
RepositoryName.createUnvalidated(canonicalRepoName));
111+
mapping.put(entry.getKey(), RepositoryName.createUnvalidated(canonicalRepoName));
114112
}
115113
}
116114
return getDepGraph()

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -106,24 +106,21 @@ public final String getCanonicalRepoName() {
106106
* extensions. For the full mapping, see {@link BazelModuleResolutionValue#getFullRepoMapping}.
107107
*/
108108
public final RepositoryMapping getRepoMappingWithBazelDepsOnly() {
109-
ImmutableMap.Builder<RepositoryName, RepositoryName> mapping = ImmutableMap.builder();
109+
ImmutableMap.Builder<String, RepositoryName> mapping = ImmutableMap.builder();
110110
// If this is the root module, then the main repository should be visible as `@`.
111111
if (getKey().equals(ModuleKey.ROOT)) {
112-
mapping.put(RepositoryName.MAIN, RepositoryName.MAIN);
112+
mapping.put("", RepositoryName.MAIN);
113113
}
114114
// Every module should be able to reference itself as @<module name>.
115115
// If this is the root module, this perfectly falls into @<module name> => @
116116
if (!getName().isEmpty()) {
117-
mapping.put(
118-
RepositoryName.createUnvalidated(getName()),
119-
RepositoryName.createUnvalidated(getCanonicalRepoName()));
117+
mapping.put(getName(), RepositoryName.createUnvalidated(getCanonicalRepoName()));
120118
}
121119
for (Map.Entry<String, ModuleKey> dep : getDeps().entrySet()) {
122120
// Special note: if `dep` is actually the root module, its ModuleKey would be ROOT whose
123121
// canonicalRepoName is the empty string. This perfectly maps to the main repo ("@").
124122
mapping.put(
125-
RepositoryName.createUnvalidated(dep.getKey()),
126-
RepositoryName.createUnvalidated(dep.getValue().getCanonicalRepoName()));
123+
dep.getKey(), RepositoryName.createUnvalidated(dep.getValue().getCanonicalRepoName()));
127124
}
128125
return RepositoryMapping.create(mapping.buildOrThrow(), getCanonicalRepoName());
129126
}

‎src/main/java/com/google/devtools/build/lib/cmdline/Label.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,7 @@ private static RepositoryName computeRepoNameWithRepoContext(
102102
// disregarding the current repo and repo mappings.
103103
return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg) ? RepositoryName.MAIN : currentRepo;
104104
}
105-
// TODO(b/200024947): Make repo mapping take a string and return a RepositoryName.
106-
return repoMapping.get(RepositoryName.createUnvalidated(parts.repo));
105+
return repoMapping.get(parts.repo);
107106
}
108107

109108
/**
@@ -174,7 +173,7 @@ public static Label parseAbsolute(String absName, RepositoryMapping repositoryMa
174173

175174
// TODO(b/200024947): Remove this.
176175
public static Label parseAbsolute(
177-
String absName, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
176+
String absName, ImmutableMap<String, RepositoryName> repositoryMapping)
178177
throws LabelSyntaxException {
179178
return parseAbsolute(absName, RepositoryMapping.createAllowingFallback(repositoryMapping));
180179
}
@@ -486,7 +485,7 @@ public Label getRelativeWithRemapping(String relName, RepositoryMapping reposito
486485

487486
// TODO(b/200024947): Remove this.
488487
public Label getRelativeWithRemapping(
489-
String relName, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
488+
String relName, ImmutableMap<String, RepositoryName> repositoryMapping)
490489
throws LabelSyntaxException {
491490
return getRelativeWithRemapping(
492491
relName, RepositoryMapping.createAllowingFallback(repositoryMapping));

‎src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public abstract class RepositoryMapping {
3535
// Always fallback to the requested name
3636
public static final RepositoryMapping ALWAYS_FALLBACK = createAllowingFallback(ImmutableMap.of());
3737

38-
abstract ImmutableMap<RepositoryName, RepositoryName> repositoryMapping();
38+
abstract ImmutableMap<String, RepositoryName> repositoryMapping();
3939

4040
/**
4141
* The owner repo of this repository mapping. It is for providing useful debug information when
@@ -46,14 +46,14 @@ public abstract class RepositoryMapping {
4646
abstract String ownerRepo();
4747

4848
public static RepositoryMapping create(
49-
Map<RepositoryName, RepositoryName> repositoryMapping, String ownerRepo) {
49+
Map<String, RepositoryName> repositoryMapping, String ownerRepo) {
5050
return new AutoValue_RepositoryMapping(
5151
ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)),
5252
Preconditions.checkNotNull(ownerRepo));
5353
}
5454

5555
public static RepositoryMapping createAllowingFallback(
56-
Map<RepositoryName, RepositoryName> repositoryMapping) {
56+
Map<String, RepositoryName> repositoryMapping) {
5757
return new AutoValue_RepositoryMapping(
5858
ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), null);
5959
}
@@ -62,9 +62,8 @@ public static RepositoryMapping createAllowingFallback(
6262
* Create a new {@link RepositoryMapping} instance based on existing repo mappings and given
6363
* additional mappings. If there are conflicts, existing mappings will take precedence.
6464
*/
65-
public RepositoryMapping withAdditionalMappings(
66-
Map<RepositoryName, RepositoryName> additionalMappings) {
67-
HashMap<RepositoryName, RepositoryName> allMappings = new HashMap<>(additionalMappings);
65+
public RepositoryMapping withAdditionalMappings(Map<String, RepositoryName> additionalMappings) {
66+
HashMap<String, RepositoryName> allMappings = new HashMap<>(additionalMappings);
6867
allMappings.putAll(repositoryMapping());
6968
return new AutoValue_RepositoryMapping(ImmutableMap.copyOf(allMappings), ownerRepo());
7069
}
@@ -78,13 +77,20 @@ public RepositoryMapping withAdditionalMappings(RepositoryMapping additionalMapp
7877
return withAdditionalMappings(additionalMappings.repositoryMapping());
7978
}
8079

81-
public RepositoryName get(RepositoryName repositoryName) {
82-
// If the owner repo is not present, that means we should fallback to the requested repo name.
80+
/**
81+
* Returns the canonical repository name associated with the given apparent repo name. The
82+
* provided apparent repo name is assumed to be valid.
83+
*/
84+
public RepositoryName get(String apparentRepoName) {
85+
RepositoryName canonicalRepoName = repositoryMapping().get(apparentRepoName);
86+
if (canonicalRepoName != null) {
87+
return canonicalRepoName;
88+
}
89+
// If the owner repo is not present, that means we should fall back to the requested repo name.
8390
if (ownerRepo() == null) {
84-
return repositoryMapping().getOrDefault(repositoryName, repositoryName);
91+
return RepositoryName.createUnvalidated(apparentRepoName);
8592
} else {
86-
return repositoryMapping()
87-
.getOrDefault(repositoryName, repositoryName.toNonVisible(ownerRepo()));
93+
return RepositoryName.createUnvalidated(apparentRepoName).toNonVisible(ownerRepo());
8894
}
8995
}
9096
}

‎src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
import java.util.regex.Pattern;
3030
import javax.annotation.Nullable;
3131

32-
/** The name of an external repository. */
32+
/** The canonical name of an external repository. */
3333
public final class RepositoryName {
3434

3535
@SerializationConstant

‎src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -909,11 +909,13 @@ public TargetPattern parse(String pattern) throws TargetParsingException {
909909
throw new TargetParsingException(
910910
"Couldn't find package in target " + pattern, TargetPatterns.Code.PACKAGE_NOT_FOUND);
911911
}
912+
String repoPart = pattern.substring(1, pkgStart);
912913
try {
913-
repository = repoMapping.get(RepositoryName.create(pattern.substring(1, pkgStart)));
914+
RepositoryName.validate(repoPart);
914915
} catch (LabelSyntaxException e) {
915916
throw new TargetParsingException(e.getMessage(), TargetPatterns.Code.LABEL_SYNTAX_ERROR);
916917
}
918+
repository = repoMapping.get(repoPart);
917919
if (!repository.isVisible()) {
918920
throw new TargetParsingException(
919921
String.format(

‎src/main/java/com/google/devtools/build/lib/packages/Package.java‎

Lines changed: 24 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -221,12 +221,11 @@ public enum ConfigSettingVisibilityPolicy {
221221
private Set<License.DistributionType> defaultDistributionSet;
222222

223223
/**
224-
* The map from each repository to that repository's remappings map.
225-
* This is only used in the //external package, it is an empty map for all other packages.
226-
* For example, an entry of {"@foo" : {"@x", "@y"}} indicates that, within repository foo,
227-
* "@x" should be remapped to "@y".
224+
* The map from each repository to that repository's remappings map. This is only used in the
225+
* //external package, it is an empty map for all other packages. For example, an entry of {"@foo"
226+
* : {"@x", "@y"}} indicates that, within repository foo, "@x" should be remapped to "@y".
228227
*/
229-
private ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
228+
private ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>>
230229
externalPackageRepositoryMappings;
231230

232231
/**
@@ -287,25 +286,15 @@ public PackageIdentifier getPackageIdentifier() {
287286
/**
288287
* Returns the repository mapping for the requested external repository.
289288
*
290-
* @throws UnsupportedOperationException if called from a package other than
291-
* the //external package
289+
* @throws UnsupportedOperationException if called from a package other than the //external
290+
* package
292291
*/
293-
public ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping(
294-
RepositoryName repository) {
292+
public ImmutableMap<String, RepositoryName> getRepositoryMapping(RepositoryName repository) {
295293
if (!packageIdentifier.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) {
296294
throw new UnsupportedOperationException("Can only access the external package repository"
297295
+ "mappings from the //external package");
298296
}
299-
300-
// We are passed a repository name as seen from the main repository, not necessarily
301-
// a canonical repository name. So, we first have to find the canonical name for the
302-
// repository in question before we can look up the mapping for it.
303-
RepositoryName actualRepositoryName =
304-
externalPackageRepositoryMappings
305-
.getOrDefault(RepositoryName.MAIN, ImmutableMap.of())
306-
.getOrDefault(repository, repository);
307-
308-
return externalPackageRepositoryMappings.getOrDefault(actualRepositoryName, ImmutableMap.of());
297+
return externalPackageRepositoryMappings.getOrDefault(repository, ImmutableMap.of());
309298
}
310299

311300
/** Get the repository mapping for this package. */
@@ -319,7 +308,7 @@ public RepositoryMapping getRepositoryMapping() {
319308
* @throws UnsupportedOperationException if called from a package other than the //external
320309
* package
321310
*/
322-
ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
311+
ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>>
323312
getExternalPackageRepositoryMappings() {
324313
if (!packageIdentifier.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) {
325314
throw new UnsupportedOperationException(
@@ -467,7 +456,7 @@ private void finishInit(Builder builder) {
467456
this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
468457
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
469458
this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
470-
ImmutableMap.Builder<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
459+
ImmutableMap.Builder<RepositoryName, ImmutableMap<String, RepositoryName>>
471460
repositoryMappingsBuilder = ImmutableMap.builder();
472461
if (!builder.externalPackageRepositoryMappings.isEmpty() && !builder.isWorkspace()) {
473462
// 'repo_mapping' should only be used in the //external package, i.e. should only appear
@@ -935,7 +924,7 @@ public boolean recordLoadedModules() {
935924

936925
// The map from each repository to that repository's remappings map.
937926
// This is only used in the //external package, it is an empty map for all other packages.
938-
private final HashMap<RepositoryName, HashMap<RepositoryName, RepositoryName>>
927+
private final HashMap<RepositoryName, HashMap<String, RepositoryName>>
939928
externalPackageRepositoryMappings = new HashMap<>();
940929
/**
941930
* The map of repository reassignments for BUILD packages loaded within external repositories.
@@ -1092,30 +1081,30 @@ String getPackageWorkspaceName() {
10921081
}
10931082

10941083
/**
1095-
* Updates the externalPackageRepositoryMappings entry for {@code repoWithin}. Adds new
1096-
* entry from {@code localName} to {@code mappedName} in {@code repoWithin}'s map.
1084+
* Updates the externalPackageRepositoryMappings entry for {@code repoWithin}. Adds new entry
1085+
* from {@code localName} to {@code mappedName} in {@code repoWithin}'s map.
10971086
*
10981087
* @param repoWithin the RepositoryName within which the mapping should apply
1099-
* @param localName the RepositoryName that actually appears in the WORKSPACE and BUILD files
1100-
* in the {@code repoWithin} repository
1088+
* @param localName the name that actually appears in the WORKSPACE and BUILD files in the
1089+
* {@code repoWithin} repository
11011090
* @param mappedName the RepositoryName by which localName should be referenced
11021091
*/
11031092
Builder addRepositoryMappingEntry(
1104-
RepositoryName repoWithin, RepositoryName localName, RepositoryName mappedName) {
1105-
HashMap<RepositoryName, RepositoryName> mapping =
1106-
externalPackageRepositoryMappings
1107-
.computeIfAbsent(repoWithin, (RepositoryName k) -> new HashMap<>());
1093+
RepositoryName repoWithin, String localName, RepositoryName mappedName) {
1094+
HashMap<String, RepositoryName> mapping =
1095+
externalPackageRepositoryMappings.computeIfAbsent(
1096+
repoWithin, (RepositoryName k) -> new HashMap<>());
11081097
mapping.put(localName, mappedName);
11091098
return this;
11101099
}
11111100

11121101
/** Adds all the mappings from a given {@link Package}. */
11131102
Builder addRepositoryMappings(Package aPackage) {
1114-
ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
1115-
repositoryMappings = aPackage.externalPackageRepositoryMappings;
1116-
for (Map.Entry<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>> repositoryName :
1103+
ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>> repositoryMappings =
1104+
aPackage.externalPackageRepositoryMappings;
1105+
for (Map.Entry<RepositoryName, ImmutableMap<String, RepositoryName>> repositoryName :
11171106
repositoryMappings.entrySet()) {
1118-
for (Map.Entry<RepositoryName, RepositoryName> repositoryNameRepositoryNameEntry :
1107+
for (Map.Entry<String, RepositoryName> repositoryNameRepositoryNameEntry :
11191108
repositoryName.getValue().entrySet()) {
11201109
addRepositoryMappingEntry(
11211110
repositoryName.getKey(),
@@ -1163,7 +1152,7 @@ Label getBuildFileLabel() {
11631152
* the main workspace to the canonical main name '@').
11641153
*/
11651154
RepositoryMapping getRepositoryMappingFor(RepositoryName name) {
1166-
Map<RepositoryName, RepositoryName> mapping = externalPackageRepositoryMappings.get(name);
1155+
Map<String, RepositoryName> mapping = externalPackageRepositoryMappings.get(name);
11671156
if (mapping == null) {
11681157
return RepositoryMapping.ALWAYS_FALLBACK;
11691158
} else {

‎src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ public static void addMainRepoEntry(
105105
// is not valid.
106106
builder.addRepositoryMappingEntry(
107107
RepositoryName.create(externalRepoName),
108-
RepositoryName.create(builder.getPackageWorkspaceName()),
108+
builder.getPackageWorkspaceName(),
109109
RepositoryName.MAIN);
110110
}
111111
}
@@ -128,15 +128,26 @@ public static void addRepoMappings(
128128
// prefixed with an @.
129129
if (!e.getKey().startsWith("@")) {
130130
throw new LabelSyntaxException(
131-
"invalid repository name '" + e.getKey() + "': repo names must start with '@'");
131+
"invalid repository name '"
132+
+ e.getKey()
133+
+ "': repo names used in the repo_mapping attribute must start with '@'");
132134
}
133135
if (!e.getValue().startsWith("@")) {
134136
throw new LabelSyntaxException(
135-
"invalid repository name '" + e.getValue() + "': repo names must start with '@'");
137+
"invalid repository name '"
138+
+ e.getValue()
139+
+ "': repo names used in the repo_mapping attribute must start with '@'");
140+
}
141+
if (!WorkspaceGlobals.isLegalWorkspaceName(e.getKey().substring(1))) {
142+
throw new LabelSyntaxException(
143+
"invalid repository name '"
144+
+ e.getKey().substring(1)
145+
+ "': must start with a letter and contain only letters, digits, '.', '-', or"
146+
+ " '_'");
136147
}
137148
builder.addRepositoryMappingEntry(
138149
RepositoryName.create(externalRepoName),
139-
RepositoryName.create(e.getKey().substring(1)),
150+
e.getKey().substring(1),
140151
RepositoryName.create(e.getValue().substring(1)));
141152
}
142153
}

‎src/main/java/com/google/devtools/build/lib/packages/WorkspaceFileValue.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public String toString() {
102102
private final ImmutableMap<String, Object> bindings;
103103
private final ImmutableMap<String, Module> loadedModules;
104104
private final ImmutableMap<String, Integer> loadToChunkMap;
105-
private final ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
105+
private final ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>>
106106
repositoryMapping;
107107
// Mapping of the relative paths of the incrementally updated managed directories
108108
// to the managing external repositories
@@ -221,8 +221,7 @@ public ImmutableMap<String, Integer> getLoadToChunkMap() {
221221
return loadToChunkMap;
222222
}
223223

224-
public ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
225-
getRepositoryMapping() {
224+
public ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>> getRepositoryMapping() {
226225
return repositoryMapping;
227226
}
228227

‎src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,7 @@ public void workspace(
100100
}
101101
// Add entry in repository map from "@name" --> "@" to avoid issue where bazel
102102
// treats references to @name as a separate external repo
103-
builder.addRepositoryMappingEntry(
104-
RepositoryName.MAIN, RepositoryName.createUnvalidated(name), RepositoryName.MAIN);
103+
builder.addRepositoryMappingEntry(RepositoryName.MAIN, name, RepositoryName.MAIN);
105104
parseManagedDirectories(
106105
thread.getSemantics().getBool(BuildLanguageOptions.INCOMPATIBLE_DISABLE_MANAGE_DIRECTORIES),
107106
Dict.cast(managedDirectories, String.class, Object.class, "managed_directories"));

0 commit comments

Comments
 (0)