From 63bc14b095f1ea4043024e7fe1f9c476968897c5 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Thu, 21 Jul 2022 19:53:32 +0200 Subject: [PATCH] Implement native analysis_test call. (#15940) The call defines a rule and a target on the BUILD thread. Since target names need to be unique so is the rule class name. This removes boilerplate in analysis testing. This mechanism replaces rule.name mechanism, which will be removed. If this becomes the default for analysis tests, rule call may be further simplified. The implementation is safeguarded by experimental flag. PiperOrigin-RevId: 462309501 Change-Id: I918ddcc8efd3b27f822998bcaa454e467a98b7ea --- .../starlark/StarlarkRuleClassFunctions.java | 80 +++++++- .../semantics/BuildLanguageOptions.java | 14 ++ .../StarlarkRuleFunctionsApi.java | 95 +++++++-- .../FakeStarlarkRuleFunctionsApi.java | 11 ++ .../StarlarkRuleClassFunctionsTest.java | 186 ++++++++++++++++++ 5 files changed, 373 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 96b7ef0ced38e7..1c90805a6ac4c9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -100,6 +100,7 @@ import com.google.errorprone.annotations.FormatMethod; import java.util.Collection; import java.util.Map; +import java.util.regex.Pattern; import javax.annotation.Nullable; import net.starlark.java.eval.Debug; import net.starlark.java.eval.Dict; @@ -111,6 +112,7 @@ import net.starlark.java.eval.StarlarkCallable; import net.starlark.java.eval.StarlarkFunction; import net.starlark.java.eval.StarlarkInt; +import net.starlark.java.eval.StarlarkList; import net.starlark.java.eval.StarlarkThread; import net.starlark.java.eval.Tuple; import net.starlark.java.syntax.Identifier; @@ -138,6 +140,7 @@ public Label load(String from) throws Exception { } } }); + private static final Pattern RULE_NAME_PATTERN = Pattern.compile("[A-Za-z_][A-Za-z0-9_]*"); // TODO(bazel-team): Remove the code duplication (BaseRuleClasses and this class). /** Parent rule class for non-executable non-test Starlark rules. */ @@ -282,7 +285,7 @@ public Provider provider(String doc, Object fields, StarlarkThread thread) throw // TODO(bazel-team): implement attribute copy and other rule properties @Override - public StarlarkCallable rule( + public StarlarkRuleFunction rule( StarlarkFunction implementation, Boolean test, Object attrs, @@ -507,6 +510,81 @@ public StarlarkCallable rule( return starlarkRuleFunction; } + @Override + public void analysisTest( + String name, + StarlarkFunction implementation, + Object attrs, + Sequence fragments, + Sequence toolchains, + Object attrValuesApi, + StarlarkThread thread) + throws EvalException, InterruptedException { + if (!RULE_NAME_PATTERN.matcher(name).matches()) { + throw Starlark.errorf("'name' is limited to Starlark identifiers, got %s", name); + } + Dict attrValues = + Dict.cast(attrValuesApi, String.class, Object.class, "attr_values"); + if (attrValues.containsKey("name")) { + throw Starlark.errorf("'name' cannot be set or overridden in 'attr_values'"); + } + + StarlarkRuleFunction starlarkRuleFunction = + rule( + implementation, + /*test=*/ true, + attrs, + /*implicitOutputs=*/ Starlark.NONE, + /*executable=*/ false, + /*outputToGenfiles=*/ false, + /*fragments=*/ fragments, + /*hostFragments=*/ StarlarkList.empty(), + /*starlarkTestable=*/ false, + /*toolchains=*/ toolchains, + /*useToolchainTransition=*/ false, + /*doc=*/ "", + /*providesArg=*/ StarlarkList.empty(), + /*execCompatibleWith=*/ StarlarkList.empty(), + /*analysisTest=*/ Boolean.TRUE, + /*buildSetting=*/ Starlark.NONE, + /*cfg=*/ Starlark.NONE, + /*execGroups=*/ Starlark.NONE, + /*compileOneFiletype=*/ Starlark.NONE, + /*name=*/ Starlark.NONE, + thread); + + // Export the rule + // Because exporting can raise multiple errors, we need to accumulate them here into a single + // EvalException. This is a code smell because any non-ERROR events will be lost, and any + // location information in the events will be overwritten by the location of this rule's + // definition. + // However, this is currently fine because StarlarkRuleFunction#export only creates events that + // are ERRORs and that have the rule definition as their location. + // TODO(brandjon): Instead of accumulating events here, consider registering the rule in the + // BazelStarlarkContext, and exporting such rules after module evaluation in + // BzlLoadFunction#execAndExport. + PackageContext pkgContext = thread.getThreadLocal(PackageContext.class); + StoredEventHandler handler = new StoredEventHandler(); + starlarkRuleFunction.export( + handler, pkgContext.getLabel(), name + "_test"); // export in BUILD thread + if (handler.hasErrors()) { + StringBuilder errors = + handler.getEvents().stream() + .filter(e -> e.getKind() == EventKind.ERROR) + .reduce( + new StringBuilder(), + (sb, ev) -> sb.append("\n").append(ev.getMessage()), + StringBuilder::append); + throw Starlark.errorf("Errors in exporting %s: %s", name, errors.toString()); + } + + // Instantiate the target + Dict.Builder args = Dict.builder(); + args.put("name", name); + args.putAll(attrValues); + starlarkRuleFunction.call(thread, Tuple.of(), args.buildImmutable()); + } + /** * Returns the module (file) of the outermost enclosing Starlark function on the call stack or * null if none of the active calls are functions defined in Starlark. diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index dba3e3827b4fdc..a0f111d679d6e7 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -297,6 +297,18 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable { + " and 1 cpu.") public boolean experimentalActionResourceSet; + + @Option( + name = "experimental_analysis_test_call", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.EXPERIMENTAL, + }, + help = "If set to true, analysis_test native call is available.") + public boolean experimentalAnalysisTestCall; + @Option( name = "incompatible_struct_has_no_methods", defaultValue = "false", @@ -586,6 +598,7 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool( INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW, incompatibleExistingRulesImmutableView) .setBool(EXPERIMENTAL_ACTION_RESOURCE_SET, experimentalActionResourceSet) + .setBool(EXPERIMENTAL_ANALYSIS_TEST_CALL, experimentalAnalysisTestCall) .setBool(EXPERIMENTAL_GOOGLE_LEGACY_API, experimentalGoogleLegacyApi) .setBool(EXPERIMENTAL_NINJA_ACTIONS, experimentalNinjaActions) .setBool(EXPERIMENTAL_PLATFORMS_API, experimentalPlatformsApi) @@ -662,6 +675,7 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT = "-experimental_sibling_repository_layout"; public static final String EXPERIMENTAL_ACTION_RESOURCE_SET = "+experimental_action_resource_set"; + public static final String EXPERIMENTAL_ANALYSIS_TEST_CALL = "+experimental_analysis_test_call"; public static final String INCOMPATIBLE_ALLOW_TAGS_PROPAGATION = "-incompatible_allow_tags_propagation"; public static final String INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS = diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java index 7365caa4ac62a8..facd72ce53865d 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java @@ -359,18 +359,18 @@ public interface StarlarkRuleFunctionsApi { positional = false, allowedTypes = {@ParamType(type = String.class), @ParamType(type = NoneType.class)}, doc = - "The name of this rule, as understood by Bazel and reported in contexts such as" - + " logging, native.existing_rule(...)[kind], and bazel" - + " query. Usually this is the same as the Starlark identifier that gets" - + " bound to this rule; for instance a rule called foo_library" - + " would typically be declared as foo_library = rule(...) and" - + " instantiated in a BUILD file as foo_library(...).

If this" - + " parameter is omitted, the rule's name is set to the name of the first" - + " Starlark global variable to be bound to this rule within its declaring .bzl" - + " module. Thus, foo_library = rule(...) need not specify this" - + " parameter if the name is foo_library.

Specifying an explicit" - + " name for a rule does not change where you are allowed to instantiate the" - + " rule."), + "Deprecated: do not use.

The name of this rule, as understood by Bazel and" + + " reported in contexts such as logging," + + " native.existing_rule(...)[kind], and bazel query." + + " Usually this is the same as the Starlark identifier that gets bound to this" + + " rule; for instance a rule called foo_library would typically" + + " be declared as foo_library = rule(...) and instantiated in a" + + " BUILD file as foo_library(...).

If this parameter is" + + " omitted, the rule's name is set to the name of the first Starlark global" + + " variable to be bound to this rule within its declaring .bzl module. Thus," + + " foo_library = rule(...) need not specify this parameter if the" + + " name is foo_library.

Specifying an explicit name for a rule" + + " does not change where you are allowed to instantiate the rule."), }, useStarlarkThread = true) StarlarkCallable rule( @@ -397,6 +397,77 @@ StarlarkCallable rule( StarlarkThread thread) throws EvalException; + @StarlarkMethod( + name = "analysis_test", + doc = + "Creates a new analysis test target.

The number of transitive dependencies of the test" + + " are limited. The limit is controlled by" + + " --analysis_testing_deps_limit flag.", + parameters = { + @Param( + name = "name", + named = true, + doc = + "Name of the target. It should be a Starlark identifier, matching pattern" + + " '[A-Za-z_][A-Za-z0-9_]*'."), + @Param( + name = "implementation", + named = true, + doc = + "The Starlark function implementing this analysis test. It must have exactly one" + + " parameter: ctx. The function is called during the" + + " analysis phase. It can access the attributes declared by attrs" + + " and populated via attr_values. The implementation function may" + + " not register actions. Instead, it must register a pass/fail result" + + " via providing AnalysisTestResultInfo."), + @Param( + name = "attrs", + allowedTypes = { + @ParamType(type = Dict.class), + @ParamType(type = NoneType.class), + }, + named = true, + defaultValue = "None", + doc = + "Dictionary declaring the attributes. See the rule call." + + "Attributes are allowed to use configuration transitions defined using analysis_test_transition."), + @Param( + name = "fragments", + allowedTypes = {@ParamType(type = Sequence.class, generic1 = String.class)}, + named = true, + defaultValue = "[]", + doc = + "List of configuration fragments that are available to the implementation of the" + + " analysis test."), + @Param( + name = TOOLCHAINS_PARAM, + allowedTypes = {@ParamType(type = Sequence.class, generic1 = Object.class)}, + named = true, + defaultValue = "[]", + doc = + "The set of toolchains the test requires. See the rule" + + " call."), + @Param( + name = "attr_values", + allowedTypes = {@ParamType(type = Dict.class, generic1 = String.class)}, + named = true, + defaultValue = "{}", + doc = "Dictionary of attribute values to pass to the implementation."), + }, + useStarlarkThread = true, + enableOnlyWithFlag = BuildLanguageOptions.EXPERIMENTAL_ANALYSIS_TEST_CALL) + void analysisTest( + String name, + StarlarkFunction implementation, + Object attrs, + Sequence fragments, + Sequence toolchains, + Object argsValue, + StarlarkThread thread) + throws EvalException, InterruptedException; + @StarlarkMethod( name = "aspect", doc = diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java index ac4413a667f9eb..8c7d4f71e547d0 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java @@ -166,6 +166,17 @@ public StarlarkCallable rule( return functionIdentifier; } + @Override + public void analysisTest( + String name, + StarlarkFunction implementation, + Object attrs, + Sequence fragments, + Sequence toolchains, + Object argsValue, + StarlarkThread thread) + throws EvalException, InterruptedException {} + @Override public Label label(String labelString, StarlarkThread thread) throws EvalException { try { diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index 482ebb9039b971..636ce2b230a251 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -2391,4 +2391,190 @@ public void testAttrWithAspectRequiringAspects_requiredNativeAspect_getsParamsFr .getDefaultValueUnchecked()) .isEqualTo("v1"); } + + @Test + public void testAnalysisTest() throws Exception { + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro(name):", + " analysis_test(name = name, implementation = impl)"); + scratch.file( + "p/BUILD", // + "load(':b.bzl','my_test_macro')", + "my_test_macro(name = 'my_test_target')"); + + getConfiguredTarget("//p:my_test_target"); + + assertNoEvents(); + } + + @Test + public void testAnalysisTestAttrs() throws Exception { + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " ctx.attr.target_under_test", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro(name):", + " native.filegroup(name = 'my_subject', srcs = [])", + " analysis_test(name = name,", + " implementation = impl,", + " attrs = {'target_under_test': attr.label_list()},", + " attr_values = {'target_under_test': [':my_subject']},", + " )"); + scratch.file( + "p/BUILD", // + "load(':b.bzl','my_test_macro')", + "my_test_macro(name = 'my_test_target')"); + + getConfiguredTarget("//p:my_test_target"); + + assertNoEvents(); + } + + /** Tests two analysis_test calls with same name. */ + @Test + public void testAnalysisTestDuplicateName() throws Exception { + scratch.file( + "p/a.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro1(name):", + " analysis_test(name = name, implementation = impl)"); + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro2(name):", + " analysis_test(name = name, implementation = impl)"); + scratch.file( + "p/BUILD", // + "load(':a.bzl','my_test_macro1')", + "load(':b.bzl','my_test_macro2')", + "my_test_macro1(name = 'my_test_target')", + "my_test_macro2(name = 'my_test_target')"); + + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + getConfiguredTarget("//p:my_test_target"); + + ev.assertContainsError( + "Error in analysis_test: my_test_target_test rule 'my_test_target' in package 'p' conflicts" + + " with existing my_test_target_test rule"); + } + + /** + * Tests analysis_test call with a name that is not Starlark identifier (but still a good target + * name). + */ + @Test + public void testAnalysisTestBadName() throws Exception { + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro(name):", + " analysis_test(name = name, implementation = impl)"); + scratch.file( + "p/BUILD", // + "load(':b.bzl','my_test_macro')", + "my_test_macro(name = 'my+test+target')"); + + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + getConfiguredTarget("//p:my+test+target"); + + ev.assertContainsError( + "Error in analysis_test: 'name' is limited to Starlark identifiers, got my+test+target"); + } + + @Test + public void testAnalysisTestBadArgs() throws Exception { + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro(name):", + " analysis_test(name = name, implementation = impl, attr_values = {'notthere': []})"); + scratch.file( + "p/BUILD", // + "load(':b.bzl','my_test_macro')", + "my_test_macro(name = 'my_test_target')"); + + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + getConfiguredTarget("//p:my_test_target"); + + ev.assertContainsError("no such attribute 'notthere' in 'my_test_target_test' rule"); + } + + @Test + public void testAnalysisTestErrorOnExport() throws Exception { + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro(name):", + " analysis_test(name = name, implementation = impl, attrs = {'name': attr.string()})"); + scratch.file( + "p/BUILD", // + "load(':b.bzl','my_test_macro')", + "my_test_macro(name = 'my_test_target')"); + + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + getConfiguredTarget("//p:my_test_target"); + + ev.assertContainsError( + "Error in analysis_test: Errors in exporting my_test_target: \n" + + "cannot add attribute: There is already a built-in attribute 'name' which cannot be" + + " overridden."); + } + + @Test + public void testAnalysisTestErrorOverridingName() throws Exception { + scratch.file( + "p/b.bzl", + "def impl(ctx): ", + " return [AnalysisTestResultInfo(", + " success = True,", + " message = ''", + " )]", + "def my_test_macro(name):", + " analysis_test(name = name, implementation = impl, attr_values = {'name': 'override'})"); + scratch.file( + "p/BUILD", // + "load(':b.bzl','my_test_macro')", + "my_test_macro(name = 'my_test_target')"); + + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + getConfiguredTarget("//p:override"); + + ev.assertContainsError( + "Error in analysis_test: 'name' cannot be set or overridden in 'attr_values'"); + } }