Skip to content

Commit 38321ed

Browse files
ahumeskylaszlocsomor
ahumesky
authored and
laszlocsomor
committed
Rollback of commit a3f5f57.
*** Reason for rollback *** Breaks targets in the depot: [] *** Original change description *** output_group is not a real Skylark provider for aspects, as well as for rules. -- MOS_MIGRATED_REVID=139354682
1 parent 261b3c0 commit 38321ed

File tree

3 files changed

+40
-123
lines changed

3 files changed

+40
-123
lines changed

‎src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java

Lines changed: 30 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -186,47 +186,41 @@ private static void addOutputGroups(Object value, Location loc,
186186

187187
for (String outputGroup : outputGroups.keySet()) {
188188
SkylarkValue objects = outputGroups.get(outputGroup);
189-
NestedSet<Artifact> artifacts = convertToOutputGroupValue(loc, outputGroup, objects);
190-
builder.addOutputGroup(outputGroup, artifacts);
191-
}
192-
}
193-
194-
public static NestedSet<Artifact> convertToOutputGroupValue(Location loc, String outputGroup,
195-
SkylarkValue objects) throws EvalException {
196-
NestedSet<Artifact> artifacts;
189+
NestedSet<Artifact> artifacts;
197190

198-
String typeErrorMessage =
199-
"Output group '%s' is of unexpected type. "
200-
+ "Should be list or set of Files, but got '%s' instead.";
191+
String typeErrorMessage =
192+
"Output group '%s' is of unexpected type. "
193+
+ "Should be list or set of Files, but got '%s' instead.";
201194

202-
if (objects instanceof SkylarkList) {
203-
NestedSetBuilder<Artifact> nestedSetBuilder = NestedSetBuilder.stableOrder();
204-
for (Object o : (SkylarkList) objects) {
205-
if (o instanceof Artifact) {
206-
nestedSetBuilder.add((Artifact) o);
207-
} else {
208-
throw new EvalException(
209-
loc,
210-
String.format(
211-
typeErrorMessage,
212-
outputGroup,
213-
"list with an element of " + EvalUtils.getDataTypeNameFromClass(o.getClass())));
195+
if (objects instanceof SkylarkList) {
196+
NestedSetBuilder<Artifact> nestedSetBuilder = NestedSetBuilder.stableOrder();
197+
for (Object o : (SkylarkList) objects) {
198+
if (o instanceof Artifact) {
199+
nestedSetBuilder.add((Artifact) o);
200+
} else {
201+
throw new EvalException(
202+
loc,
203+
String.format(
204+
typeErrorMessage,
205+
outputGroup,
206+
"list with an element of " + EvalUtils.getDataTypeNameFromClass(o.getClass())));
207+
}
214208
}
209+
artifacts = nestedSetBuilder.build();
210+
} else {
211+
artifacts =
212+
SkylarkType.cast(
213+
objects,
214+
SkylarkNestedSet.class,
215+
Artifact.class,
216+
loc,
217+
typeErrorMessage,
218+
outputGroup,
219+
EvalUtils.getDataTypeName(objects, true))
220+
.getSet(Artifact.class);
215221
}
216-
artifacts = nestedSetBuilder.build();
217-
} else {
218-
artifacts =
219-
SkylarkType.cast(
220-
objects,
221-
SkylarkNestedSet.class,
222-
Artifact.class,
223-
loc,
224-
typeErrorMessage,
225-
outputGroup,
226-
EvalUtils.getDataTypeName(objects, true))
227-
.getSet(Artifact.class);
222+
builder.addOutputGroup(outputGroup, artifacts);
228223
}
229-
return artifacts;
230224
}
231225

232226
private static ConfiguredTarget addStructFieldsAndBuild(

‎src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import com.google.common.collect.ImmutableList;
1717
import com.google.common.collect.ImmutableMap;
18+
import com.google.devtools.build.lib.actions.Artifact;
1819
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
1920
import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
2021
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -24,13 +25,12 @@
2425
import com.google.devtools.build.lib.packages.AspectParameters;
2526
import com.google.devtools.build.lib.packages.SkylarkAspect;
2627
import com.google.devtools.build.lib.packages.SkylarkClassObject;
27-
import com.google.devtools.build.lib.rules.SkylarkRuleConfiguredTargetBuilder;
2828
import com.google.devtools.build.lib.rules.SkylarkRuleContext;
29-
import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
3029
import com.google.devtools.build.lib.syntax.Environment;
3130
import com.google.devtools.build.lib.syntax.EvalException;
3231
import com.google.devtools.build.lib.syntax.EvalExceptionWithStackTrace;
3332
import com.google.devtools.build.lib.syntax.Mutability;
33+
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
3434
import com.google.devtools.build.lib.syntax.SkylarkType;
3535
import java.util.Map;
3636

@@ -90,9 +90,8 @@ public ConfiguredAspect create(
9090
for (String key : struct.getKeys()) {
9191
if (key.equals("output_groups")) {
9292
addOutputGroups(struct.getValue(key), loc, builder);
93-
} else {
94-
builder.addSkylarkTransitiveInfo(key, struct.getValue(key), loc);
9593
}
94+
builder.addSkylarkTransitiveInfo(key, struct.getValue(key), loc);
9695
}
9796
ConfiguredAspect configuredAspect = builder.build();
9897
SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext);
@@ -102,20 +101,21 @@ public ConfiguredAspect create(
102101
ruleContext.ruleError("\n" + e.print());
103102
return null;
104103
}
104+
105105
}
106106
}
107107

108108
private static void addOutputGroups(Object value, Location loc,
109109
ConfiguredAspect.Builder builder)
110110
throws EvalException {
111-
Map<String, SkylarkValue> outputGroups =
112-
SkylarkType.castMap(value, String.class, SkylarkValue.class, "output_groups");
111+
Map<String, SkylarkNestedSet> outputGroups = SkylarkType
112+
.castMap(value, String.class, SkylarkNestedSet.class, "output_groups");
113113

114114
for (String outputGroup : outputGroups.keySet()) {
115-
SkylarkValue objects = outputGroups.get(outputGroup);
116-
115+
SkylarkNestedSet objects = outputGroups.get(outputGroup);
117116
builder.addOutputGroup(outputGroup,
118-
SkylarkRuleConfiguredTargetBuilder.convertToOutputGroupValue(loc, outputGroup, objects));
117+
SkylarkType.cast(objects, SkylarkNestedSet.class, Artifact.class, loc,
118+
"Output group '%s'", outputGroup).getSet(Artifact.class));
119119
}
120120
}
121121

‎src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java

Lines changed: 1 addition & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ public void testAspectWithOutputGroups() throws Exception {
258258
"",
259259
"MyAspect = aspect(",
260260
" implementation=_impl,",
261+
" attr_aspects=['deps'],",
261262
")");
262263
scratch.file(
263264
"test/BUILD",
@@ -291,50 +292,6 @@ public String apply(ConfiguredTarget configuredTarget) {
291292
assertThat(names).containsExactlyElementsIn(expectedSet);
292293
}
293294

294-
@Test
295-
public void testAspectWithOutputGroupsAsList() throws Exception {
296-
scratch.file(
297-
"test/aspect.bzl",
298-
"def _impl(target, ctx):",
299-
" g = target.output_group('_hidden_top_level" + INTERNAL_SUFFIX + "')",
300-
" return struct(output_groups = { 'my_result' : [ f for f in g] })",
301-
"",
302-
"MyAspect = aspect(",
303-
" implementation=_impl,",
304-
")");
305-
scratch.file(
306-
"test/BUILD",
307-
"java_library(",
308-
" name = 'xxx',",
309-
" srcs = ['A.java'],",
310-
")");
311-
312-
AnalysisResult analysisResult =
313-
update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
314-
assertThat(
315-
transform(
316-
analysisResult.getTargetsToBuild(),
317-
new Function<ConfiguredTarget, String>() {
318-
@Nullable
319-
@Override
320-
public String apply(ConfiguredTarget configuredTarget) {
321-
return configuredTarget.getLabel().toString();
322-
}
323-
}))
324-
.containsExactly("//test:xxx");
325-
AspectValue aspectValue = analysisResult.getAspects().iterator().next();
326-
OutputGroupProvider outputGroupProvider =
327-
aspectValue.getConfiguredAspect().getProvider(OutputGroupProvider.class);
328-
assertThat(outputGroupProvider).isNotNull();
329-
NestedSet<Artifact> names = outputGroupProvider.getOutputGroup("my_result");
330-
assertThat(names).isNotEmpty();
331-
NestedSet<Artifact> expectedSet = getConfiguredTarget("//test:xxx")
332-
.getProvider(OutputGroupProvider.class)
333-
.getOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL);
334-
assertThat(names).containsExactlyElementsIn(expectedSet);
335-
}
336-
337-
338295
@Test
339296
public void testAspectsFromSkylarkRules() throws Exception {
340297
scratch.file(
@@ -641,40 +598,6 @@ public void duplicateOutputGroups() throws Exception {
641598
assertContainsEvent("ERROR /workspace/test/BUILD:3:1: Output group duplicate provided twice");
642599
}
643600

644-
@Test
645-
public void duplicateGroupsFormAspects() throws Exception {
646-
scratch.file(
647-
"test/aspect.bzl",
648-
"def _a1_impl(target, ctx):",
649-
" f = ctx.new_file('f.txt')",
650-
" ctx.file_action(f, 'f')",
651-
" return struct(output_groups = { 'a1_group' : set([f]) })",
652-
"",
653-
"a1 = aspect(implementation=_a1_impl, attr_aspects = ['dep'])",
654-
"def _rule_impl(ctx):",
655-
" pass",
656-
"my_rule1 = rule(_rule_impl, attrs = { 'dep' : attr.label(aspects = [a1]) })",
657-
"def _a2_impl(target, ctx):",
658-
" f = ctx.new_file('f.txt')",
659-
" ctx.file_action(f, 'f')",
660-
" return struct(output_groups = { 'a2_group' : set([f]) })",
661-
"",
662-
"a2 = aspect(implementation=_a2_impl, attr_aspects = ['dep'])",
663-
"my_rule2 = rule(_rule_impl, attrs = { 'dep' : attr.label(aspects = [a2]) })"
664-
);
665-
scratch.file(
666-
"test/BUILD",
667-
"load(':aspect.bzl', 'my_rule1', 'my_rule2')",
668-
"my_rule1(name = 'base')",
669-
"my_rule1(name = 'xxx', dep = ':base')",
670-
"my_rule2(name = 'yyy', dep = ':xxx')"
671-
);
672-
673-
// no error.
674-
update("//test:yyy");
675-
}
676-
677-
678601
@Test
679602
public void duplicateSkylarkProviders() throws Exception {
680603
scratch.file(

0 commit comments

Comments
 (0)