From b9999f47b30cfd677e53e1f659f58afa7fbc5e33 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 2 Oct 2024 07:49:19 -0700 Subject: [PATCH] Flip incompatible_disallow_struct_provider_syntax Downstream tests: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/4163 rules_jvm_external, rules_android_ndk, rules_python are failing because of Bazel 7 builtins/common/python/py_executable_bazel.bzl:103 All these failures are fixed by consistently using rules_python. Needs: https://github.com/bazelbuild/bazel/pull/23829 PiperOrigin-RevId: 681450758 Change-Id: I7580968b2d3b1460cc6a610a93110eaea2fb4f6c --- .../semantics/BuildLanguageOptions.java | 4 +-- .../analysis/StubbableFSBuildViewTest.java | 2 +- .../buildtool/GenQueryIntegrationTest.java | 4 +-- .../buildtool/OutputArtifactConflictTest.java | 8 ++--- .../build/lib/query2/testutil/QueryTest.java | 2 +- .../TransitiveTraversalFunctionTest.java | 4 +-- .../lib/starlark/StarlarkIntegrationTest.java | 14 ++++---- .../StarlarkRuleClassFunctionsTest.java | 4 +-- .../lib/starlark/StarlarkRuleContextTest.java | 4 +-- .../StarlarkStringRepresentationsTest.java | 33 +++++++++++-------- src/test/shell/bazel/bazel_java_test.sh | 32 +++++++++--------- .../bazel/external_starlark_load_test.sh | 4 +-- .../remote/build_without_the_bytes_test.sh | 2 +- .../integration/build_event_stream_test.sh | 18 +++++----- .../integration/validation_actions_test.sh | 5 ++- 15 files changed, 71 insertions(+), 69 deletions(-) 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 342542a945ddba..171a2cbea80aa4 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 @@ -481,7 +481,7 @@ public final class BuildLanguageOptions extends OptionsBase { @Option( name = "incompatible_disallow_struct_provider_syntax", - defaultValue = "false", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, @@ -960,7 +960,7 @@ public StarlarkSemantics toStarlarkSemantics() { "-incompatible_disable_target_provider_fields"; public static final String INCOMPATIBLE_DISALLOW_EMPTY_GLOB = "-incompatible_disallow_empty_glob"; public static final String INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX = - "-incompatible_disallow_struct_provider_syntax"; + "+incompatible_disallow_struct_provider_syntax"; public static final String INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX = FlagConstants.INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX; public static final String INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX = diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java index 9688617f0f6bb5..4faf1b270a2386 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java @@ -58,7 +58,7 @@ public void testCatastrophicAnalysisErrorAspect_keepGoing_noCrashCatastrophicErr """ def _impl(target, ctx): print("This aspect does nothing") - return struct() + return [] MyAspect = aspect(implementation = _impl) """); diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java index efbcce1753ab04..20974657c8efb0 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java @@ -774,9 +774,9 @@ private void writeAspectDefinition(String aspectPackage, String extraDep) throws write( aspectPackage + "/aspect.bzl", "def _aspect_impl(target, ctx):", - " return struct()", + " return []", "def _rule_impl(ctx):", - " return struct()", + " return []", "MyAspect = aspect(", " implementation=_aspect_impl,", " attr_aspects=['deps'],", diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java index f0a5a623cc1fdf..52278e0578ec6e 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java @@ -183,7 +183,7 @@ public void testAspectArtifactSharesPrefixWithTargetArtifact( """ def _aspect_impl(target, ctx): if not getattr(ctx.rule.attr, "outs", None): - return struct(output_groups = {}) + return [OutputGroupInfo()] conflict_outputs = list() for out in ctx.rule.attr.outs: if out.name[1:] == ".bad": @@ -271,7 +271,7 @@ public void testAspectArtifactPrefix( """ def _aspect_impl(target, ctx): if not getattr(ctx.rule.attr, "outs", None): - return struct(output_groups = {}) + return [OutputGroupInfo()] conflict_outputs = list() for out in ctx.rule.attr.outs: if out.name[1:] == ".bad": @@ -501,7 +501,7 @@ public void testConflictErrorAndUnfinishedAspectAnalysis_mergedAnalysisExecution """ def _aspect_impl(target, ctx): if not getattr(ctx.rule.attr, "outs", None): - return struct(output_groups = {}) + return [OutputGroupInfo()] conflict_outputs = list() for out in ctx.rule.attr.outs: if out.name[1:] == ".bad": @@ -713,7 +713,7 @@ public void testConflictAfterNullBuild(@TestParameter boolean keepGoing) throws """ def _aspect_impl(target, ctx): if not getattr(ctx.rule.attr, "outs", None): - return struct(output_groups = {}) + return [OutputGroupInfo()] conflict_outputs = list() for out in ctx.rule.attr.outs: if out.name[1:] == ".bad": diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/QueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/QueryTest.java index 537c9d19b41fae..6ee61157ad9c21 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/QueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/QueryTest.java @@ -398,7 +398,7 @@ private void writeStarlarkDefinedRuleClassBzlFile() throws java.io.IOException { """ def custom_rule_impl(ctx): ftb = depset(ctx.attr._secret_labels) - return struct(runfiles = ctx.runfiles(), files = ftb) + return DefaultInfo(runfiles = ctx.runfiles(), files = ftb) def secret_labels_func(prefix, suffix): return [ diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java index d48449cf29e78a..bbba3f434e169e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java @@ -174,10 +174,10 @@ public void getStrictLabelAspectKeys() throws Exception { "test/aspect.bzl", """ def _aspect_impl(target, ctx): - return struct() + return [] def _rule_impl(ctx): - return struct() + return [] MyAspect = aspect( implementation = _aspect_impl, diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java index ba2db9594bb35d..e42e0fdcec4068 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java @@ -710,7 +710,7 @@ public void testCannotSpecifyRunfilesWithDataOrDefaultRunfiles_struct() throws E """ def custom_rule_impl(ctx): rf = ctx.runfiles() - return struct(runfiles = rf, default_runfiles = rf) + return DefaultInfo(runfiles = rf, default_runfiles = rf) custom_rule = rule(implementation = custom_rule_impl) """); @@ -3563,7 +3563,7 @@ public void testDisallowStructProviderSyntax() throws Exception { "test/starlark/extension.bzl", """ def custom_rule_impl(ctx): - return struct() + return struct() # intentional custom_rule = rule(implementation = custom_rule_impl) """); @@ -3597,7 +3597,7 @@ def _my_rule_impl(ctx): print(ctx.attr.dep.my_info) def _dep_rule_impl(ctx): my_info = MyProvider(foo = 'bar') - return struct(my_info = my_info, providers = [my_info]) + return [my_info] my_rule = rule( implementation = _my_rule_impl, attrs = { @@ -3639,7 +3639,7 @@ def _my_rule_impl(ctx): print(ctx.attr.dep.actions) def _dep_rule_impl(ctx): my_info = MyProvider(foo = 'bar') - return struct(my_info = my_info, providers = [my_info]) + return [my_info] my_rule = rule( implementation = _my_rule_impl, attrs = { @@ -3673,7 +3673,7 @@ def _my_rule_impl(ctx): print(ctx.attr.dep.my_info) def _dep_rule_impl(ctx): my_info = MyProvider(foo = 'bar') - return struct(my_info = my_info, providers = [my_info]) + return struct(my_info = my_info, providers = [my_info]) # intentional my_rule = rule( implementation = _my_rule_impl, attrs = { @@ -3733,7 +3733,7 @@ def _my_rule_impl(ctx): ctx.actions.run_shell(outputs=[exe], command='touch exe') runfile = ctx.actions.declare_file('rrr') ctx.actions.run_shell(outputs=[runfile], command='touch rrr') - return struct(executable = exe, default_runfiles = ctx.runfiles(files = [runfile])) + return DefaultInfo(executable = exe, default_runfiles = ctx.runfiles(files = [runfile])) my_rule = rule(implementation = _my_rule_impl, executable = True) """); scratch.file( @@ -4404,7 +4404,7 @@ public void topLevelAspectOnTargetWithNonIdempotentRuleTransition() throws Excep """ def _impl(target, ctx): print('This aspect does nothing') - return struct() + return [] MyAspect = aspect(implementation=_impl) """); 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 eeee50fa5d638e..0eb9d840d54798 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 @@ -3329,9 +3329,9 @@ def f(ctx): "r/def.bzl", """ load(":create.bzl", "create") - + Info = provider() def f(ctx): - return struct(value = "NEW") + return Info(value = "NEW") r = create(f) """); diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java index 4a502f7cda6269..68f7d4180958e1 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java @@ -1906,7 +1906,7 @@ def symlink_impl(ctx): 'symlink_' + f.short_path: f for f in ctx.files.symlink } - return struct( + return DefaultInfo( runfiles = ctx.runfiles( symlinks=symlinks, ) @@ -2007,7 +2007,7 @@ def root_symlink_impl(ctx): 'root_symlink_' + f.short_path: f for f in ctx.files.root_symlink } - return struct( + return DefaultInfo( runfiles = ctx.runfiles( root_symlinks=root_symlinks, ) diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkStringRepresentationsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkStringRepresentationsTest.java index b1d52c1430eab7..cc11f1231f72d0 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkStringRepresentationsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkStringRepresentationsTest.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.packages.StarlarkInfo; import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -195,7 +196,7 @@ def _genfile_impl(ctx): implementation = _genfile_impl, outputs = {"my_output": "%{name}.txt"}, ) - + CheckInfo = provider() def _check_impl(ctx): source_file = ctx.attr.srcs[0].files.to_list()[0] generated_file = ctx.attr.srcs[1].files.to_list()[0] @@ -213,7 +214,7 @@ def _check_impl(ctx): "source_root": source_file.root, "generated_root": generated_file.root, } - return struct(**prepare_params(objects)) + return CheckInfo(**prepare_params(objects)) check = rule( implementation = _check_impl, @@ -329,13 +330,14 @@ public void testStringRepresentations_select() throws Exception { public void testStringRepresentations_ruleContext() throws Exception { generateFilesToTestStrings(); ConfiguredTarget target = getConfiguredTarget("//test/starlark:check"); + StarlarkInfo checkInfo = getStarlarkProvider(target, "CheckInfo"); for (String suffix : SUFFIXES) { - assertThat(target.get("rule_ctx" + suffix)) + assertThat(checkInfo.getValue("rule_ctx" + suffix)) .isEqualTo(""); - assertThat(target.get("aspect_ctx" + suffix)) + assertThat(checkInfo.getValue("aspect_ctx" + suffix)) .isEqualTo(""); - assertThat(target.get("aspect_ctx.rule" + suffix)) + assertThat(checkInfo.getValue("aspect_ctx.rule" + suffix)) .isEqualTo(""); } } @@ -344,11 +346,12 @@ public void testStringRepresentations_ruleContext() throws Exception { public void testStringRepresentations_files() throws Exception { generateFilesToTestStrings(); ConfiguredTarget target = getConfiguredTarget("//test/starlark:check"); + StarlarkInfo checkInfo = getStarlarkProvider(target, "CheckInfo"); for (String suffix : SUFFIXES) { - assertThat(target.get("source_file" + suffix)) + assertThat(checkInfo.getValue("source_file" + suffix)) .isEqualTo(""); - assertThat(target.get("generated_file" + suffix)) + assertThat(checkInfo.getValue("generated_file" + suffix)) .isEqualTo(""); } } @@ -357,10 +360,11 @@ public void testStringRepresentations_files() throws Exception { public void testStringRepresentations_root() throws Exception { generateFilesToTestStrings(); ConfiguredTarget target = getConfiguredTarget("//test/starlark:check"); + StarlarkInfo checkInfo = getStarlarkProvider(target, "CheckInfo"); for (String suffix : SUFFIXES) { - assertThat(target.get("source_root" + suffix)).isEqualTo(""); - assertThat(target.get("generated_root" + suffix)).isEqualTo(""); + assertThat(checkInfo.getValue("source_root" + suffix)).isEqualTo(""); + assertThat(checkInfo.getValue("generated_root" + suffix)).isEqualTo(""); } } @@ -396,16 +400,17 @@ public void testStringRepresentations_attr() throws Exception { public void testStringRepresentations_targets() throws Exception { generateFilesToTestStrings(); ConfiguredTarget target = getConfiguredTarget("//test/starlark:check"); + StarlarkInfo checkInfo = getStarlarkProvider(target, "CheckInfo"); for (String suffix : SUFFIXES) { - assertThat(target.get("target" + suffix)).isEqualTo(""); - assertThat(target.get("input_target" + suffix)) + assertThat(checkInfo.getValue("target" + suffix)).isEqualTo(""); + assertThat(checkInfo.getValue("input_target" + suffix)) .isEqualTo(""); - assertThat(target.get("output_target" + suffix)) + assertThat(checkInfo.getValue("output_target" + suffix)) .isEqualTo(""); - assertThat(target.get("alias_target" + suffix)) + assertThat(checkInfo.getValue("alias_target" + suffix)) .isEqualTo(""); - assertThat(target.get("aspect_target" + suffix)) + assertThat(checkInfo.getValue("aspect_target" + suffix)) .isEqualTo(""); } } diff --git a/src/test/shell/bazel/bazel_java_test.sh b/src/test/shell/bazel/bazel_java_test.sh index aa21da863548b1..05cd4de2ae647e 100755 --- a/src/test/shell/bazel/bazel_java_test.sh +++ b/src/test/shell/bazel/bazel_java_test.sh @@ -245,10 +245,10 @@ def _impl(ctx): strict_deps = "ERROR", java_toolchain = ctx.attr._java_toolchain[java_common.JavaToolchainInfo], ) - return struct( - files = depset([output_jar]), - providers = [compilation_provider] - ) + return [ + DefaultInfo(files = depset([output_jar])), + compilation_provider, + ] java_custom_library = rule( implementation = _impl, @@ -426,10 +426,10 @@ def _impl(ctx): strict_deps = "ERROR", java_toolchain = ctx.attr._java_toolchain[java_common.JavaToolchainInfo], ) - return struct( - files = depset([output_jar]), - providers = [compilation_provider] - ) + return [ + DefaultInfo(files = depset([output_jar])), + compilation_provider + ] java_custom_library = rule( implementation = _impl, @@ -497,10 +497,10 @@ def _impl(ctx): strict_deps = "ERROR", java_toolchain = ctx.attr._java_toolchain[java_common.JavaToolchainInfo], ) - return struct( - files = depset([output_jar]), - providers = [compilation_provider] - ) + return [ + DefaultInfo(files = depset([output_jar])), + compilation_provider, + ] java_custom_library = rule( implementation = _impl, @@ -1405,10 +1405,10 @@ def _impl(ctx): print(final_provider.outputs.jars[0].class_jar) print(final_provider.outputs.jars[1].class_jar) - return struct( - files = depset([compiled_jar, imported_jar]), - providers = [final_provider] - ) + return [ + DefaultInfo(files = depset([compiled_jar, imported_jar])), + final_provider, + ] java_custom_library = rule( implementation = _impl, diff --git a/src/test/shell/bazel/external_starlark_load_test.sh b/src/test/shell/bazel/external_starlark_load_test.sh index e552671d5f847e..d22fb663ca2fe4 100755 --- a/src/test/shell/bazel/external_starlark_load_test.sh +++ b/src/test/shell/bazel/external_starlark_load_test.sh @@ -230,7 +230,7 @@ EOF cat > rule.bzl < semifailingaspect.bzl <<'EOF' def _semifailing_aspect_impl(target, ctx): if not ctx.rule.attr.outs: - return struct(output_groups = {}) + return [OutputGroupInfo()] bad_outputs = list() good_outputs = list() for out in ctx.rule.attr.outs: diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index 3b6b1c294bf56d..ec2bf65798d447 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh @@ -116,8 +116,7 @@ def _simple_aspect_impl(target, ctx): ctx.actions.write( output=aspect_out, content = "Hello from aspect") - return struct(output_groups={ - "aspect-out" : depset([aspect_out]) }) + return [OutputGroupInfo(aspect_out=depset([aspect_out]))] simple_aspect = aspect(implementation=_simple_aspect_impl) EOF @@ -130,15 +129,14 @@ def _failing_aspect_impl(target, ctx): outputs = [aspect_out], command = "false", ) - return struct(output_groups={ - "aspect-out" : depset([aspect_out]) }) + return [OutputGroupInfo(aspect_out=depset([aspect_out]))] failing_aspect = aspect(implementation=_failing_aspect_impl) EOF cat > semifailingaspect.bzl <<'EOF' def _semifailing_aspect_impl(target, ctx): if not ctx.rule.attr.outs: - return struct(output_groups = {}) + return [OutputGroupInfo()] bad_outputs = list() good_outputs = list() mixed_outputs = list() @@ -791,10 +789,10 @@ EOF function test_aspect_artifacts() { bazel build --build_event_text_file=$TEST_log \ --aspects=simpleaspect.bzl%simple_aspect \ - --output_groups=aspect-out \ + --output_groups=aspect_out \ pkg:output_files_and_tags || fail "bazel build failed" expect_log 'aspect.*simple_aspect' - expect_log 'name.*aspect-out' + expect_log 'name.*aspect_out' expect_log 'name.*out1.txt.aspect' expect_not_log 'aborted' expect_log_n '^configured' 2 @@ -806,7 +804,7 @@ function test_aspect_target_summary() { bazel build --build_event_text_file=$TEST_log \ --experimental_bep_target_summary \ --aspects=simpleaspect.bzl%simple_aspect \ - --output_groups=aspect-out \ + --output_groups=aspect_out \ pkg:output_files_and_tags || fail "bazel build failed" expect_not_log 'aborted' expect_log_n '^configured' 2 @@ -820,7 +818,7 @@ function test_aspect_target_summary() { function test_failing_aspect() { bazel build --build_event_text_file=$TEST_log \ --aspects=failingaspect.bzl%failing_aspect \ - --output_groups=aspect-out \ + --output_groups=aspect_out \ pkg:output_files_and_tags && fail "expected failure" || true expect_log 'aspect.*failing_aspect' expect_log '^finished' @@ -832,7 +830,7 @@ function test_aspect_analysis_failure_no_target_summary() { bazel build -k --build_event_text_file=$TEST_log \ --experimental_bep_target_summary \ --aspects=failingaspect.bzl%failing_aspect \ - --output_groups=aspect-out \ + --output_groups=aspect_out \ pkg:output_files_and_tags && fail "expected failure" || true expect_log 'aspect.*failing_aspect' expect_log '^finished' diff --git a/src/test/shell/integration/validation_actions_test.sh b/src/test/shell/integration/validation_actions_test.sh index f76e7a2bedc7d8..6bc109bce3820b 100755 --- a/src/test/shell/integration/validation_actions_test.sh +++ b/src/test/shell/integration/validation_actions_test.sh @@ -254,8 +254,7 @@ def _simple_aspect_impl(target, ctx): ctx.actions.write( output=aspect_out, content = "Hello from aspect") - return struct(output_groups={ - "aspect-out" : depset([aspect_out]) }) + return [OutputGroupInfo(aspect_out=depset([aspect_out]))] simple_aspect = aspect(implementation=_simple_aspect_impl) EOF @@ -264,7 +263,7 @@ EOF --show_result=2 \ --experimental_use_validation_aspect \ --aspects=validation_actions/simpleaspect.bzl%simple_aspect \ - --output_groups=+aspect-out \ + --output_groups=+aspect_out \ //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed" expect_log "Target //validation_actions:foo0 up-to-date:"