Skip to content

Commit 39a902a

Browse files
fmeumfmeum
committed
Modify message and also check tags
1 parent 98195a8 commit 39a902a

File tree

8 files changed

+103
-101
lines changed

8 files changed

+103
-101
lines changed

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,18 @@
1515

1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

18+
import static java.util.Collections.singletonList;
19+
1820
import com.google.auto.value.AutoValue;
1921
import com.google.common.collect.Maps;
22+
import com.google.devtools.build.lib.cmdline.Label;
2023
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
24+
import java.util.Collection;
2125
import java.util.List;
2226
import java.util.Map;
27+
import java.util.Optional;
2328
import net.starlark.java.eval.Dict;
29+
import net.starlark.java.eval.EvalException;
2430
import net.starlark.java.eval.Starlark;
2531

2632
/** Wraps a dictionary of attribute names and values. Always uses a dict to represent them */
@@ -39,6 +45,49 @@ public static AttributeValues create(Map<String, Object> attribs) {
3945

4046
public abstract Dict<String, Object> attributes();
4147

48+
public static void validateAttrs(AttributeValues attributes, String what) throws EvalException {
49+
for (var entry : attributes.attributes().entrySet()) {
50+
validateSingleAttr(entry.getKey(), entry.getValue(), what);
51+
}
52+
}
53+
54+
public static void validateSingleAttr(String attrName, Object attrValue, String what)
55+
throws EvalException {
56+
var maybeNonVisibleLabel = getFirstNonVisibleLabel(attrValue);
57+
if (maybeNonVisibleLabel.isEmpty()) {
58+
return;
59+
}
60+
Label label = maybeNonVisibleLabel.get();
61+
String repoName = label.getRepository().getName();
62+
throw Starlark.errorf(
63+
"no repository visible as '@%s' to the %s, but referenced by label '@%s//%s:%s' in"
64+
+ " attribute '%s' of %s. Is the %s missing a bazel_dep or use_repo(..., \"%s\")?",
65+
repoName,
66+
label.getRepository().getOwnerRepoDisplayString(),
67+
repoName,
68+
label.getPackageName(),
69+
label.getName(),
70+
attrName,
71+
what,
72+
label.getRepository().getOwnerModuleDisplayString(),
73+
repoName);
74+
}
75+
76+
private static Optional<Label> getFirstNonVisibleLabel(Object nativeAttrValue) {
77+
Collection<?> toValidate =
78+
switch (nativeAttrValue) {
79+
case List<?> list -> list;
80+
case Map<?, ?> map -> map.keySet();
81+
case null, default -> singletonList(nativeAttrValue);
82+
};
83+
for (var item : toValidate) {
84+
if (item instanceof Label label && !label.getRepository().isVisible()) {
85+
return Optional.of(label);
86+
}
87+
}
88+
return Optional.empty();
89+
}
90+
4291
// TODO(salmasamy) this is a copy of Attribute::valueToStarlark, Maybe think of a better place?
4392
private static Object valueToStarlark(Object x) {
4493
// Is x a non-empty string_list_dict?

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,8 +279,6 @@ java_library(
279279
name = "repo_rule_creator",
280280
srcs = ["BzlmodRepoRuleCreator.java"],
281281
deps = [
282-
":common",
283-
":module_extension",
284282
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
285283
"//src/main/java/com/google/devtools/build/lib/cmdline",
286284
"//src/main/java/com/google/devtools/build/lib/events",

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

Lines changed: 0 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,11 @@
1414

1515
package com.google.devtools.build.lib.bazel.bzlmod;
1616

17-
import static java.util.Collections.singletonList;
18-
1917
import com.google.common.collect.ImmutableList;
2018
import com.google.devtools.build.lib.analysis.BlazeDirectories;
21-
import com.google.devtools.build.lib.cmdline.Label;
2219
import com.google.devtools.build.lib.cmdline.LabelConstants;
2320
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
2421
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
25-
import com.google.devtools.build.lib.cmdline.RepositoryName;
2622
import com.google.devtools.build.lib.events.ExtendedEventHandler;
2723
import com.google.devtools.build.lib.packages.NoSuchPackageException;
2824
import com.google.devtools.build.lib.packages.Package;
@@ -35,10 +31,7 @@
3531
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
3632
import com.google.devtools.build.lib.vfs.Root;
3733
import com.google.devtools.build.lib.vfs.RootedPath;
38-
import java.util.Collection;
39-
import java.util.List;
4034
import java.util.Map;
41-
import java.util.Optional;
4235
import net.starlark.java.eval.EvalException;
4336
import net.starlark.java.eval.Starlark;
4437
import net.starlark.java.eval.StarlarkSemantics;
@@ -100,62 +93,4 @@ public static Rule createRule(
10093
}
10194
return rule;
10295
}
103-
104-
public static void validateLabelAttrs(
105-
AttributeValues attributes, ModuleExtensionId owningExtension, String what)
106-
throws EvalException {
107-
for (var entry : attributes.attributes().entrySet()) {
108-
validateSingleAttr(entry.getKey(), entry.getValue(), owningExtension, what);
109-
}
110-
}
111-
112-
public static Optional<Label> getFirstNonVisibleLabel(Object nativeAttrValue) {
113-
Collection<?> toValidate =
114-
switch (nativeAttrValue) {
115-
case List<?> list -> list;
116-
case Map<?, ?> map -> map.keySet();
117-
case null, default -> singletonList(nativeAttrValue);
118-
};
119-
for (var item : toValidate) {
120-
if (item instanceof Label label && !label.getRepository().isVisible()) {
121-
return Optional.of(label);
122-
}
123-
}
124-
return Optional.empty();
125-
}
126-
127-
public static void validateSingleAttr(
128-
String attrName, Object attrValue, ModuleExtensionId owningExtension, String what)
129-
throws EvalException {
130-
var maybeNonVisibleLabel = getFirstNonVisibleLabel(attrValue);
131-
if (maybeNonVisibleLabel.isEmpty()) {
132-
return;
133-
}
134-
Label label = maybeNonVisibleLabel.get();
135-
RepositoryName owningModuleRepoName = owningExtension.getBzlFileLabel().getRepository();
136-
String owningModule;
137-
if (owningModuleRepoName.isMain()) {
138-
owningModule = "root module";
139-
} else {
140-
owningModule =
141-
String.format(
142-
"module '%s'",
143-
owningModuleRepoName
144-
.getName()
145-
.substring(0, owningModuleRepoName.getName().indexOf('~')));
146-
}
147-
String repoName = label.getRepository().getName();
148-
throw Starlark.errorf(
149-
"no repository visible as '@%s', but referenced by label '@%s//%s:%s' in attribute %s"
150-
+ " of %s. Only repositories visible to the %s can be referenced"
151-
+ " here, are you missing a bazel_dep or use_repo(..., \"%s\")?.",
152-
repoName,
153-
repoName,
154-
label.getPackageName(),
155-
label.getName(),
156-
attrName,
157-
what,
158-
owningModule,
159-
repoName);
160-
}
16196
}

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,22 +68,19 @@ static RepoSpecAndLocation create(RepoSpec repoSpec, Location location) {
6868
private final RepositoryMapping repoMapping;
6969
private final BlazeDirectories directories;
7070
private final ExtendedEventHandler eventHandler;
71-
private final ModuleExtensionId owningExtension;
7271
private final Map<String, RepoSpecAndLocation> generatedRepos = new HashMap<>();
7372

7473
public ModuleExtensionEvalStarlarkThreadContext(
7574
String repoPrefix,
7675
PackageIdentifier basePackageId,
7776
RepositoryMapping repoMapping,
7877
BlazeDirectories directories,
79-
ExtendedEventHandler eventHandler,
80-
ModuleExtensionId owningExtension) {
78+
ExtendedEventHandler eventHandler) {
8179
this.repoPrefix = repoPrefix;
8280
this.basePackageId = basePackageId;
8381
this.repoMapping = repoMapping;
8482
this.directories = directories;
8583
this.eventHandler = eventHandler;
86-
this.owningExtension = owningExtension;
8784
}
8885

8986
public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleClass ruleClass)
@@ -118,8 +115,8 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
118115
Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k)), k -> !k.equals("name"));
119116
String bzlFile = ruleClass.getRuleDefinitionEnvironmentLabel().getUnambiguousCanonicalForm();
120117
var attributesValue = AttributeValues.create(attributes);
121-
BzlmodRepoRuleCreator.validateLabelAttrs(
122-
attributesValue, owningExtension, String.format("%s '%s'", rule.getRuleClass(), name));
118+
AttributeValues.validateAttrs(
119+
attributesValue, String.format("%s '%s'", rule.getRuleClass(), name));
123120
RepoSpec repoSpec =
124121
RepoSpec.builder()
125122
.setBzlFile(bzlFile)

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -748,10 +748,8 @@ public RunModuleExtensionResult run(
748748
Maps.filterKeys(
749749
Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k)),
750750
k -> !k.equals("name")));
751-
BzlmodRepoRuleCreator.validateLabelAttrs(
752-
attributesValue,
753-
extensionId,
754-
String.format("%s '%s'", ruleInstance.getRuleClass(), name));
751+
AttributeValues.validateAttrs(
752+
attributesValue, String.format("%s '%s'", ruleInstance.getRuleClass(), name));
755753
} catch (InvalidRuleException | NoSuchPackageException | EvalException e) {
756754
throw new SingleExtensionEvalFunctionException(
757755
ExternalDepsException.withCauseAndMessage(
@@ -875,8 +873,7 @@ public RunModuleExtensionResult run(
875873
extensionId.getBzlFileLabel().getPackageIdentifier(),
876874
BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(),
877875
directories,
878-
env.getListener(),
879-
extensionId);
876+
env.getListener());
880877
ModuleExtensionContext moduleContext;
881878
Optional<ModuleExtensionMetadata> moduleExtensionMetadata;
882879
var repoMappingRecorder = new Label.RepoMappingRecorder();

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l
9898
}
9999

100100
// Check that all mandatory attributes have been specified, and fill in default values.
101+
// Along the way, verify that labels in the attribute values refer to visible repos only.
101102
for (int i = 0; i < attrValues.length; i++) {
102103
Attribute attr = tagClass.getAttributes().get(i);
103104
if (attr.isMandatory() && attrValues[i] == null) {
@@ -110,6 +111,13 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l
110111
if (attrValues[i] == null) {
111112
attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked());
112113
}
114+
try {
115+
AttributeValues.validateSingleAttr(
116+
attr.getPublicName(), attrValues[i], String.format("tag '%s'", tag.getTagName()));
117+
} catch (EvalException e) {
118+
throw ExternalDepsException.withMessage(
119+
Code.BAD_MODULE, "in tag at %s: %s", tag.getLocation(), e.getMessage());
120+
}
113121
}
114122
return new TypeCheckedTag(
115123
tagClass, attrValues, tag.isDevDependency(), tag.getLocation(), tag.getTagName());

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,20 @@ public String getOwnerRepoDisplayString() {
209209
}
210210
}
211211

212+
// Must only be called if isVisible() returns true.
213+
public String getOwnerModuleDisplayString() {
214+
Preconditions.checkNotNull(ownerRepoIfNotVisible);
215+
if (ownerRepoIfNotVisible.isMain()) {
216+
return "root module";
217+
} else {
218+
return String.format(
219+
"module '%s'",
220+
ownerRepoIfNotVisible
221+
.getName()
222+
.substring(0, ownerRepoIfNotVisible.getName().indexOf('~')));
223+
}
224+
}
225+
212226
/** Returns if this is the main repository. */
213227
public boolean isMain() {
214228
return equals(MAIN);

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,8 @@ public void setup() throws Exception {
289289
RepositoryDelegatorFunction.IS_VENDOR_COMMAND.set(differencer, false);
290290
RepositoryDelegatorFunction.VENDOR_DIRECTORY.set(differencer, Optional.empty());
291291

292+
registry.addModule(
293+
createModuleKey("platforms", "0.0.9"), "module(name='platforms', version = '0.0.9')");
292294
// Set up a simple repo rule.
293295
registry.addModule(
294296
createModuleKey("data_repo", "1.0"), "module(name='data_repo',version='1.0')");
@@ -1399,10 +1401,9 @@ public void nonVisibleLabelInLabelAttr() throws Exception {
13991401
reporter.removeHandler(failFastHandler);
14001402
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
14011403
assertContainsEvent(
1402-
"Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
1403-
+ " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
1404-
+ " visible to the root module can be referenced here, are you missing a bazel_dep or"
1405-
+ " use_repo(..., \"other_repo\")?");
1404+
"Error in repository_rule: no repository visible as '@other_repo' to the main repository,"
1405+
+ " but referenced by label '@other_repo//:foo' in attribute 'data' of data_repo 'ext'."
1406+
+ " Is the root module missing a bazel_dep or use_repo(..., \"other_repo\")?");
14061407
}
14071408

14081409
@Test
@@ -1423,6 +1424,7 @@ public void nonVisibleLabelInLabelAttrNonRootModule() throws Exception {
14231424
" data_repo(name='other_repo')",
14241425
" data_repo(name='ext',data='@other_repo//:foo')",
14251426
"ext = module_extension(implementation=_ext_impl)");
1427+
14261428
scratch.file(
14271429
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
14281430
"bazel_dep(name = 'ext_module', version = '1.0')",
@@ -1438,10 +1440,10 @@ public void nonVisibleLabelInLabelAttrNonRootModule() throws Exception {
14381440
reporter.removeHandler(failFastHandler);
14391441
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
14401442
assertContainsEvent(
1441-
"Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
1442-
+ " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
1443-
+ " visible to the module 'ext_module' can be referenced here, are you missing a"
1444-
+ " bazel_dep or use_repo(..., \"other_repo\")?");
1443+
"Error in repository_rule: no repository visible as '@other_repo' to the repository"
1444+
+ " '@@ext_module~', but referenced by label '@other_repo//:foo' in attribute 'data' of"
1445+
+ " data_repo 'ext'. Is the module 'ext_module' missing a bazel_dep or use_repo(...,"
1446+
+ " \"other_repo\")?");
14451447
}
14461448

14471449
@Test
@@ -1475,12 +1477,16 @@ public void nonVisibleLabelInLabelAttrForwardedFromTag() throws Exception {
14751477

14761478
SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
14771479
reporter.removeHandler(failFastHandler);
1478-
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
1479-
assertContainsEvent(
1480-
"Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
1481-
+ " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
1482-
+ " visible to the root module can be referenced here, are you missing a bazel_dep or"
1483-
+ " use_repo(..., \"other_repo\")?");
1480+
var result = evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
1481+
1482+
assertThat(result.hasError()).isTrue();
1483+
assertThat(result.getError().getException())
1484+
.hasMessageThat()
1485+
.isEqualTo(
1486+
"in tag at /ws/MODULE.bazel:2:10: no repository visible as '@other_repo' to the main"
1487+
+ " repository, but referenced by label '@other_repo//:foo' in attribute 'label' of"
1488+
+ " tag 'label'. Is the root module missing a bazel_dep or use_repo(...,"
1489+
+ " \"other_repo\")?");
14841490
}
14851491

14861492
@Test
@@ -1511,10 +1517,9 @@ public void nonVisibleLabelInLabelListAttr() throws Exception {
15111517
reporter.removeHandler(failFastHandler);
15121518
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
15131519
assertContainsEvent(
1514-
"Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
1515-
+ " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
1516-
+ " visible to the root module can be referenced here, are you missing a bazel_dep or"
1517-
+ " use_repo(..., \"other_repo\")?");
1520+
"Error in repository_rule: no repository visible as '@other_repo' to the main repository,"
1521+
+ " but referenced by label '@other_repo//:foo' in attribute 'data' of data_repo 'ext'."
1522+
+ " Is the root module missing a bazel_dep or use_repo(..., \"other_repo\")?");
15181523
}
15191524

15201525
@Test
@@ -1545,10 +1550,9 @@ public void nonVisibleLabelInLabelKeyedStringDictAttr() throws Exception {
15451550
reporter.removeHandler(failFastHandler);
15461551
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
15471552
assertContainsEvent(
1548-
"Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
1549-
+ " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
1550-
+ " visible to the root module can be referenced here, are you missing a bazel_dep or"
1551-
+ " use_repo(..., \"other_repo\")?");
1553+
"Error in repository_rule: no repository visible as '@other_repo' to the main repository,"
1554+
+ " but referenced by label '@other_repo//:foo' in attribute 'data' of data_repo 'ext'."
1555+
+ " Is the root module missing a bazel_dep or use_repo(..., \"other_repo\")?");
15521556
}
15531557

15541558
@Test

0 commit comments

Comments
 (0)