From aed506f6072813883b3d97926808f8ddb3a4c619 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 17 Apr 2024 14:48:35 -0700 Subject: [PATCH 01/15] Break dependency from skyframe:bzl_load_value to packages:packages. A later change will make packages.StarlarkProvider depend on skyframe.BzlLoadValue which will be cyclic otherwise. This requires splitting out packages:bzl_visibility, packages:package_specification and skyframe:bzl_compile_value into their own targets. PiperOrigin-RevId: 625813461 Change-Id: If7a95be9e48a34afc9e3a47fa9232d672a7bbac9 --- .../google/devtools/build/lib/analysis/BUILD | 10 ++++-- .../google/devtools/build/lib/packages/BUILD | 29 +++++++++++++++++ .../google/devtools/build/lib/query2/BUILD | 1 + .../com/google/devtools/build/lib/rules/BUILD | 2 ++ .../devtools/build/lib/rules/java/BUILD | 1 + .../google/devtools/build/lib/skyframe/BUILD | 31 +++++++++++++------ .../devtools/build/lib/analysis/util/BUILD | 1 + .../google/devtools/build/lib/packages/BUILD | 1 + .../google/devtools/build/lib/skyframe/BUILD | 1 + .../build/lib/skyframe/serialization/BUILD | 2 +- 10 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index a55865af30b77e..ab8aa8a0993a12 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -403,6 +403,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/profiler:google-auto-profiler-utils", @@ -662,6 +663,7 @@ java_library( ":visibility_provider", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//third_party:guava", @@ -1253,7 +1255,7 @@ java_library( deps = [ ":transitive_info_provider", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", - "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", ], ) @@ -1264,7 +1266,7 @@ java_library( ":visibility_provider", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", - "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", ], ) @@ -2426,9 +2428,9 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", - "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", "//src/main/java/com/google/devtools/build/lib/skyframe:rule_configured_target_value", @@ -2481,6 +2483,8 @@ java_library( ":starlark/starlark_late_bound_default", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:bzl_visibility", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//src/main/java/net/starlark/java/eval", diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index 24b258ad43e4d7..f98a21b903d533 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -47,18 +47,22 @@ java_library( ["*.java"], exclude = [ "BuilderFactoryForTesting.java", # see builder_factory_for_testing + "BzlVisibility.java", "Globber.java", "GlobberUtils.java", "ExecGroup.java", "ConfiguredAttributeMapper.java", "LabelPrinter.java", + "PackageSpecification.java", ], ), deps = [ + ":bzl_visibility", ":exec_group", ":globber", ":globber_utils", ":label_printer", + ":package_specification", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/actions:thread_state_receiver", @@ -112,6 +116,16 @@ java_library( ], ) +java_library( + name = "bzl_visibility", + srcs = ["BzlVisibility.java"], + deps = [ + ":package_specification", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//third_party:guava", + ], +) + java_library( name = "exec_group", srcs = ["ExecGroup.java"], @@ -125,6 +139,21 @@ java_library( ], ) +java_library( + name = "package_specification", + srcs = ["PackageSpecification.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/net/starlark/java/eval", + "//third_party:auto_value", + "//third_party:guava", + "//third_party:jsr305", + ], +) + # TODO(kkress, adonovan) Eliminate this target, it is mostly unnecessary. It # depends on lib:build-base for BlazeDirectories, which it uses for a type # parameter, but the param is unused. diff --git a/src/main/java/com/google/devtools/build/lib/query2/BUILD b/src/main/java/com/google/devtools/build/lib/query2/BUILD index 8c3327c9eae63b..691292a0f8a720 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/BUILD +++ b/src/main/java/com/google/devtools/build/lib/query2/BUILD @@ -69,6 +69,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:label_printer", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/profiler:google-auto-profiler-utils", diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 9e426ad5dfd20c..2dee6c6668ac9f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -154,6 +154,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/util:filetype", "//src/main/java/net/starlark/java/eval", "//third_party:guava", @@ -272,6 +273,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//third_party:guava", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD index fb2654b3484cf5..31c4e346135db4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD @@ -152,6 +152,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/main/java/com/google/devtools/build/lib/shell", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 35c48e95f9b22a..64cd89f34c9232 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -101,6 +101,7 @@ java_library( ":build_info_collection_value", ":build_result_listener", ":bzl_compile", + ":bzl_compile_value", ":bzl_load_cycle_reporter", ":bzl_load_value", ":bzlmod_repo_cycle_reporter", @@ -297,6 +298,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:bzl_visibility", "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/packages:globber_utils", "//src/main/java/com/google/devtools/build/lib/packages/semantics", @@ -761,25 +763,36 @@ java_library( java_library( name = "bzl_compile", - srcs = [ - "BzlCompileFunction.java", - "BzlCompileValue.java", - ], + srcs = ["BzlCompileFunction.java"], deps = [ + ":bzl_compile_value", ":precomputed_value", - ":sky_functions", ":starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/java/net/starlark/java/eval", + "//src/main/java/net/starlark/java/syntax", + "//third_party:guava", + "//third_party:jsr305", + ], +) + +java_library( + name = "bzl_compile_value", + srcs = ["BzlCompileValue.java"], + deps = [ + ":sky_functions", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/vfs", - "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", - "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", "//third_party:error_prone_annotations", "//third_party:guava", @@ -2424,12 +2437,12 @@ java_library( name = "bzl_load_value", srcs = ["BzlLoadValue.java"], deps = [ - ":bzl_compile", + ":bzl_compile_value", ":sky_functions", ":starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent", - "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:bzl_visibility", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index 3dd0bc93a93e74..baff0c1f0dda77 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD @@ -95,6 +95,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", diff --git a/src/test/java/com/google/devtools/build/lib/packages/BUILD b/src/test/java/com/google/devtools/build/lib/packages/BUILD index 50ca4e93258b7c..85294e719afcef 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/test/java/com/google/devtools/build/lib/packages/BUILD @@ -73,6 +73,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", "//src/main/java/com/google/devtools/build/lib/packages:globber", + "//src/main/java/com/google/devtools/build/lib/packages:package_specification", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/runtime/commands", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index 1850066f52c54b..270c86accbce19 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -203,6 +203,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/skyframe:broken_diff_awareness_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:builder", "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_compile", + "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_compile_value", "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", "//src/main/java/com/google/devtools/build/lib/skyframe:cached_bzl_load_value_and_deps", "//src/main/java/com/google/devtools/build/lib/skyframe:cached_bzl_load_value_and_deps_builder_factory", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/BUILD index 16b86a045c7efe..e5d6ae0feed283 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/BUILD @@ -337,7 +337,7 @@ java_test( srcs = ["BzlLoadValueCodecTest.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:bzl_visibility", "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils", "//src/main/java/net/starlark/java/eval", From e4425d811a6da1da6e5d721bb3e32af2dfcf9d43 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 3 May 2024 06:19:25 -0700 Subject: [PATCH 02/15] Move SkyFunction into skyframe-objects. PiperOrigin-RevId: 630371031 Change-Id: Ie598933f6a827a7fef596a6bc3ab126334547271 --- .../google/devtools/build/lib/repository/BUILD | 4 ++-- .../devtools/build/lib/rules/platform/BUILD | 6 +----- .../build/lib/rules/starlarkdocextract/BUILD | 2 +- .../com/google/devtools/build/lib/skyframe/BUILD | 16 ++++++++++++++-- .../devtools/build/lib/skyframe/toolchains/BUILD | 7 +++---- .../java/com/google/devtools/build/lib/vfs/BUILD | 2 +- .../com/google/devtools/build/skyframe/BUILD | 4 ++++ .../build/lib/bazel/repository/starlark/BUILD | 2 +- .../devtools/build/lib/rules/android/BUILD | 2 +- 9 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/repository/BUILD b/src/main/java/com/google/devtools/build/lib/repository/BUILD index 6f0ed370937807..a5c2b20d73a409 100644 --- a/src/main/java/com/google/devtools/build/lib/repository/BUILD +++ b/src/main/java/com/google/devtools/build/lib/repository/BUILD @@ -16,7 +16,7 @@ java_library( srcs = ["ExternalPackageException.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval", ], ) @@ -48,7 +48,7 @@ java_library( ":external_package_exception", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/BUILD b/src/main/java/com/google/devtools/build/lib/rules/platform/BUILD index 6b9d249c91f33e..758bf58727bdf7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/BUILD @@ -21,13 +21,11 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", - "//src/main/java/com/google/devtools/build/lib/analysis:config/auto_cpu_converter", "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_config_transition", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:file_provider", "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration", - "//src/main/java/com/google/devtools/build/lib/analysis:platform_options", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", "//src/main/java/com/google/devtools/build/lib/analysis:template_variable_info", "//src/main/java/com/google/devtools/build/lib/analysis/platform", @@ -37,10 +35,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules/core", "//src/main/java/com/google/devtools/build/lib/skyframe:package_value", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform", - "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:filetype", - "//src/main/java/com/google/devtools/build/lib/util:os", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD index 9723704e69f5cd..68241b58a07597 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD @@ -34,7 +34,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/util:filetype", "//src/main/java/com/google/devtools/build/skydoc/rendering:rendering_util", "//src/main/java/com/google/devtools/build/skydoc/rendering/proto:stardoc_output_java_proto", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval", "//third_party:error_prone_annotations", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 64cd89f34c9232..ccc9c2f33a6715 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -443,6 +443,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:shared_action_event", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:error_prone_annotations", "//third_party:guava", "//third_party:jsr305", @@ -580,7 +581,7 @@ java_library( ":artifact_function", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:jsr305", ], ) @@ -2451,6 +2452,17 @@ java_library( ], ) +java_library( + name = "bzl_load_failed_exception", + srcs = ["BzlLoadFailedException.java"], + deps = [ + ":sane_analysis_exception", + "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/protobuf:failure_details_java_proto", + ], +) + java_library( name = "state_informing_sky_function_environment", srcs = ["StateInformingSkyFunctionEnvironment.java"], @@ -2515,7 +2527,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/pkgcache", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD index ae29a94a24983e..81d73533816576 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD @@ -25,9 +25,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/protobuf:failure_details_java_proto", - "//third_party:guava", "//third_party:jsr305", ], ) @@ -70,7 +69,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:package_value", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/protobuf:failure_details_java_proto", "//third_party:guava", "//third_party:jsr305", @@ -323,7 +322,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/protobuf:failure_details_java_proto", "//third_party:guava", "//third_party:jsr305", diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD index 9fd10921354be1..4d026c203907fe 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD +++ b/src/main/java/com/google/devtools/build/lib/vfs/BUILD @@ -93,7 +93,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/skyframe/BUILD b/src/main/java/com/google/devtools/build/skyframe/BUILD index 7b07ff1b43edae..3e57217899d290 100644 --- a/src/main/java/com/google/devtools/build/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/skyframe/BUILD @@ -14,6 +14,9 @@ SKYFRAME_OBJECT_SRCS = [ "GroupedDeps.java", "NodeVersion.java", "NotComparableSkyValue.java", + "SkyframeLookupResult.java", + "SkyFunction.java", + "SkyFunctionException.java", "SkyFunctionName.java", "SkyKey.java", "SkyValue.java", @@ -28,6 +31,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/collect/compacthashset", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/util:TestType", diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index 93ba5cb79c9ee3..cc5f3a3fd0d10d 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -39,7 +39,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", "//src/test/java/com/google/devtools/build/lib/analysis/util", diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/BUILD b/src/test/java/com/google/devtools/build/lib/rules/android/BUILD index 25e199f9474c05..8ff1ee12597bf8 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/android/BUILD @@ -219,7 +219,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", - "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/test/java/com/google/devtools/build/lib/skyframe:testutil", "//third_party:guava", "//third_party:junit4", From 8aa2c048d20f66b1e7b8c402531e01743f96a7b7 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Fri, 23 Aug 2024 02:37:47 -0700 Subject: [PATCH 03/15] Implement incompatible_autoload_externally Issue: https://github.com/bazelbuild/bazel/issues/22928 Incompatible flag issue: https://github.com/bazelbuild/bazel/issues/23043 The flag supports Bazel users in migrating the rules from being embedded in Bazel to external repositories. Listing a symbol or a rule in the flag automatically adds a load to the respective repository. Listing it with a '+' keeps the symbol available in the rules_repository. Listing a symbol with "-" forcefully removes it from Bazel. Cycles are prevented with a list of repositories where autoloads should not be used. The new environments are injected into BzlCompile, BzlLoad and Package function. StarlarkBuiltinsFunction with autoloads true key is used to load the external symbols. Closes #23016. Downstream test: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/4003 PiperOrigin-RevId: 666710259 Change-Id: Idaa9b6b13c9a474f700e69e22b6162ed59b7fab0 --- .../build/lib/packages/AutoloadSymbols.java | 701 ++++++++++++++++++ .../google/devtools/build/lib/packages/BUILD | 3 + .../semantics/BuildLanguageOptions.java | 59 ++ .../google/devtools/build/lib/skyframe/BUILD | 12 +- .../lib/skyframe/BzlCompileFunction.java | 10 +- .../build/lib/skyframe/BzlLoadFunction.java | 28 +- .../lib/skyframe/BzlmodRepoCycleReporter.java | 25 +- .../build/lib/skyframe/PackageFunction.java | 12 +- .../build/lib/skyframe/PrecomputedValue.java | 5 + .../build/lib/skyframe/SkyframeExecutor.java | 6 + .../skyframe/StarlarkBuiltinsFunction.java | 131 +++- .../lib/skyframe/StarlarkBuiltinsValue.java | 30 +- .../packages/AbstractPackageLoader.java | 6 + .../bzlmod/ModuleExtensionResolutionTest.java | 9 +- .../repository/RepositoryDelegatorTest.java | 9 +- src/test/shell/integration/BUILD | 8 + .../integration/load_removed_symbols_test.sh | 411 ++++++++++ 17 files changed, 1423 insertions(+), 42 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java create mode 100755 src/test/shell/integration/load_removed_symbols_test.sh diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java new file mode 100644 index 00000000000000..f15af962f86ec4 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -0,0 +1,701 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package com.google.devtools.build.lib.packages; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableMap.toImmutableMap; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.Label.RepoContext; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; +import com.google.devtools.build.lib.skyframe.BzlLoadValue; +import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; +import com.google.devtools.build.skyframe.SkyFunction; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.function.Predicate; +import javax.annotation.Nullable; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkSemantics; + +/** + * Implementation of --incompatible_autoload_externally. + * + *

The flag adds loads to external repository for rules and top-level symbols, or removes them. + * This class prepares new environment for BzlCompileFunction, BzlLoadFunction and PackageFuntions. + * + *

Environment for BzlCompileFunction is prepared immediately during construction. Only names of + * the symbols are needed and provided. Values of the symbols are set to None. + * + *

Environments for BzlLoadFunction and PackageFunctions are prepared in StarlarkBuilinsFunction. + * + *

Cycles are prevented by disallowing autoloads in repos that we autoload from and the repos + * they depend on. + */ +public class AutoloadSymbols { + // Following fields autoloadedSymbols, removedSymbols, partiallyRemovedSymbols, + // reposDisallowingAutoloads are empty if autoloads aren't used + // Symbols that aren't prefixed with '-', that get loaded from external repositories (in allowed + // repositories). + private final ImmutableList autoloadedSymbols; + // Symbols that are prefixed with '-', that are removed everywhere (from each repository) + private final ImmutableList removedSymbols; + // Symbols that aren't prefixed with '+', that are removed from everywhere, + // except in repositories where autoloaded symbols are defined + private final ImmutableList partiallyRemovedSymbols; + + // Repositories where autoloads shouldn't be used + private final ImmutableSet reposDisallowingAutoloads; + + // The environment formed by taking BazelStarlarkEnvironment's bzl environment and adding/removing + // autoloaded symbols. The values of any added symbols are set to None (i.e. not actually loaded). + // This is intended for BzlCompileFunction only. + private final ImmutableMap uninjectedBuildBzlEnvWithAutoloads; + + // bzl environment where autoloads aren't used, uninjected (not loaded yet) + private final ImmutableMap uninjectedBuildBzlEnvWithoutAutoloads; + + // Used for nicer error messages + private final boolean bzlmodEnabled; + private final boolean autoloadsEnabled; + + public AutoloadSymbols(RuleClassProvider ruleClassProvider, StarlarkSemantics semantics) { + ImmutableList symbolConfiguration = + ImmutableList.copyOf(semantics.get(BuildLanguageOptions.INCOMPATIBLE_AUTOLOAD_EXTERNALLY)); + this.bzlmodEnabled = semantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD); + this.autoloadsEnabled = !symbolConfiguration.isEmpty(); + + if (!autoloadsEnabled) { + ImmutableMap originalBuildBzlEnv = + ruleClassProvider.getBazelStarlarkEnvironment().getUninjectedBuildBzlEnv(); + this.uninjectedBuildBzlEnvWithAutoloads = originalBuildBzlEnv; + this.uninjectedBuildBzlEnvWithoutAutoloads = originalBuildBzlEnv; + this.reposDisallowingAutoloads = ImmutableSet.of(); + this.autoloadedSymbols = ImmutableList.of(); + this.removedSymbols = ImmutableList.of(); + this.partiallyRemovedSymbols = ImmutableList.of(); + return; + } + + // Validates the inputs + Set uniqueSymbols = new HashSet<>(); + for (String symbol : symbolConfiguration) { + String symbolWithoutPrefix = + symbol.startsWith("+") || symbol.startsWith("-") ? symbol.substring(1) : symbol; + if (!uniqueSymbols.add(symbolWithoutPrefix)) { + throw new IllegalStateException( + String.format( + "Duplicated symbol '%s' in --incompatible_autoload_externally", + symbolWithoutPrefix)); + } + if (!AUTOLOAD_CONFIG.containsKey(symbolWithoutPrefix)) { + throw new IllegalStateException("Undefined symbol in --incompatible_autoload_externally"); + } + } + + this.autoloadedSymbols = filterSymbols(symbolConfiguration, symbol -> !symbol.startsWith("-")); + this.removedSymbols = filterSymbols(symbolConfiguration, symbol -> symbol.startsWith("-")); + this.partiallyRemovedSymbols = + filterSymbols(symbolConfiguration, symbol -> !symbol.startsWith("+")); + + this.reposDisallowingAutoloads = + ImmutableSet.builder() + .addAll(PREDECLARED_REPOS_DISALLOWING_AUTOLOADS) + .addAll(semantics.get(BuildLanguageOptions.REPOSITORIES_WITHOUT_AUTOLOAD)) + .build(); + + ImmutableMap originalBuildBzlEnv = + ruleClassProvider.getBazelStarlarkEnvironment().getUninjectedBuildBzlEnv(); + + // Sets up environments for BzlCompile function + this.uninjectedBuildBzlEnvWithAutoloads = + modifyBuildBzlEnv( + /* isWithAutoloads= */ true, + originalBuildBzlEnv, + /* newSymbols= */ autoloadedSymbols.stream() + .collect(toImmutableMap(key -> key, key -> Starlark.NONE))); + this.uninjectedBuildBzlEnvWithoutAutoloads = + modifyBuildBzlEnv( + /* isWithAutoloads= */ false, originalBuildBzlEnv, /* newSymbols= */ ImmutableMap.of()); + + // Validate rdeps - this ensures that all the rules using a provider are also removed + // Check what's still available in Bazel (some symbols might already be deleted) + ImmutableSet allAvailableSymbols = + ImmutableSet.builder() + .addAll(uninjectedBuildBzlEnvWithoutAutoloads.keySet()) + .addAll( + convertNativeStructToMap( + (StarlarkInfo) uninjectedBuildBzlEnvWithoutAutoloads.get("native")) + .keySet()) + .build(); + for (String symbol : partiallyRemovedSymbols) { + ImmutableList unsatisfiedRdeps = + AUTOLOAD_CONFIG.get(symbol).getRdeps().stream() + .filter(allAvailableSymbols::contains) + .collect(toImmutableList()); + if (!unsatisfiedRdeps.isEmpty()) { + throw new IllegalStateException( + String.format( + "Symbol in '%s' can't be removed, because it's still used by: %s", + symbol, String.join(", ", unsatisfiedRdeps))); + } + } + } + + /** An optimisation, checking is autoloads are used at all. */ + public boolean isEnabled() { + return autoloadsEnabled; + } + + /** Returns the environment for BzlCompile function */ + public ImmutableMap getUninjectedBuildBzlEnv(@Nullable Label key) { + return autoloadsDisabledForRepo(key) + ? uninjectedBuildBzlEnvWithoutAutoloads + : uninjectedBuildBzlEnvWithAutoloads; + } + + /** Check if autoloads shouldn't be used. */ + public boolean autoloadsDisabledForRepo(@Nullable Label key) { + if (!autoloadsEnabled) { + return true; + } + return key == null || autoloadsDisabledForRepo(key.getRepository().getName()); + } + + /** + * Check if autoloads shouldn't be used in the given repository. + * + *

Autoloads aren't used for repos in {@link #PREDECLARED_REPOS_DISALLOWING_AUTOLOADS}, + * specified by the --repositories_without_autoloads flag or any of their immediate descendants + * (parsing the cannonical repository name to check this). + */ + public boolean autoloadsDisabledForRepo(String repo) { + if (!autoloadsEnabled) { + return true; + } + int separatorIndex = repo.contains("~") ? repo.indexOf("~") : repo.indexOf("+"); + return reposDisallowingAutoloads.contains( + separatorIndex >= 0 ? repo.substring(0, separatorIndex) : repo); + } + + /** + * Modifies the environment for BzlLoad function (returned from StarlarkBuiltinsFunction). + * + *

{@code originalEnv} contains original environment and {@code newSymbols} is a map from new + * symbol name to symbol's value. {@code isWithAutoloads} chooses the semantics, described in + * details on --incompatible_autoload_externally flag. + */ + public ImmutableMap modifyBuildBzlEnv( + boolean isWithAutoloads, + ImmutableMap originalEnv, + ImmutableMap newSymbols) { + if (isWithAutoloads) { + return modifyBuildBzlEnv(originalEnv, /* add= */ newSymbols, /* remove= */ removedSymbols); + } else { + return modifyBuildBzlEnv( + originalEnv, /* add= */ ImmutableMap.of(), /* remove= */ partiallyRemovedSymbols); + } + } + + /** + * Creates modified environment that's used in BzlCompileFunction and StarlarkBuiltinsFunction. + * + *

It starts with the original environment. Adds the symbols to it or removes them. + */ + private ImmutableMap modifyBuildBzlEnv( + ImmutableMap originalEnv, + ImmutableMap add, + ImmutableList remove) { + Map envBuilder = new LinkedHashMap<>(originalEnv); + Map nativeBindings = + convertNativeStructToMap((StarlarkInfo) envBuilder.remove("native")); + + for (Map.Entry symbol : add.entrySet()) { + if (AUTOLOAD_CONFIG.get(symbol.getKey()).isRule()) { + nativeBindings.put(symbol.getKey(), symbol.getValue()); + } else { + envBuilder.put(symbol.getKey(), symbol.getValue()); + } + } + for (String symbol : remove) { + if (AUTOLOAD_CONFIG.get(symbol).isRule()) { + nativeBindings.remove(symbol); + } else { + envBuilder.remove(symbol); + } + } + envBuilder.put( + "native", StructProvider.STRUCT.create(nativeBindings, "no native function or rule '%s'")); + return ImmutableMap.copyOf(envBuilder); + } + + /** Modifies the environment for Package function (returned from StarlarkBuiltinsFunction). */ + public ImmutableMap modifyBuildEnv( + boolean isWithAutoloads, + ImmutableMap originalEnv, + ImmutableMap newSymbols) { + final ImmutableMap add; + if (isWithAutoloads) { + add = newSymbols; + } else { + add = ImmutableMap.of(); + } + Map envBuilder = new LinkedHashMap<>(originalEnv); + for (Map.Entry symbol : add.entrySet()) { + if (AUTOLOAD_CONFIG.get(symbol.getKey()).isRule()) { + envBuilder.put(symbol.getKey(), symbol.getValue()); + } + } + for (String symbol : removedSymbols) { + if (AUTOLOAD_CONFIG.get(symbol).isRule()) { + envBuilder.remove(symbol); + } + } + return ImmutableMap.copyOf(envBuilder); + } + + private static ImmutableList filterSymbols( + ImmutableList symbols, Predicate when) { + return symbols.stream() + .filter(when) + .map( + symbol -> + symbol.startsWith("+") || symbol.startsWith("-") ? symbol.substring(1) : symbol) + .collect(toImmutableList()); + } + + private static Map convertNativeStructToMap(StarlarkInfo struct) { + LinkedHashMap destr = new LinkedHashMap<>(); + for (String field : struct.getFieldNames()) { + destr.put(field, struct.getValue(field)); + } + return destr; + } + + /** + * Returns a list of all the extra .bzl files that need to be loaded + * + *

Keys are coming from {@link AUTOLOAD_CONFIG} table. + * + *

Actual loading is done in {@link StarlarkBuiltinsValue} and then passed to {@link + * #processLoads} for final processing. The parameter {@code autoloadValues} must correspond to + * the map returned by * {@link #getLoadKeys}. + */ + @Nullable + public ImmutableMap getLoadKeys(SkyFunction.Environment env) + throws InterruptedException { + RepositoryMappingValue repositoryMappingValue = + (RepositoryMappingValue) + env.getValue(RepositoryMappingValue.key(RepositoryName.BAZEL_TOOLS)); + + if (repositoryMappingValue == null) { + return null; + } + + RepoContext repoContext = + Label.RepoContext.of( + RepositoryName.BAZEL_TOOLS, repositoryMappingValue.getRepositoryMapping()); + + // Inject loads for rules and symbols removed from Bazel + ImmutableMap.Builder loadKeysBuilder = + ImmutableMap.builderWithExpectedSize(autoloadedSymbols.size()); + for (String symbol : autoloadedSymbols) { + loadKeysBuilder.put(symbol, AUTOLOAD_CONFIG.get(symbol).getKey(repoContext)); + } + return loadKeysBuilder.buildOrThrow(); + } + + /** + * Processes LoadedValues into a map of symbols + * + *

The parameter {@code autoloadValues} must correspond to the map returned by {@link + * #getLoadKeys}. Actual loading is done in {@link StarlarkBuiltinsValue}. + * + *

Keys are coming from {@link AUTOLOAD_CONFIG} table. + */ + public ImmutableMap processLoads( + ImmutableMap autoloadValues) throws AutoloadException { + if (autoloadValues.isEmpty()) { + return ImmutableMap.of(); + } + + ImmutableMap.Builder newSymbols = + ImmutableMap.builderWithExpectedSize(autoloadValues.size()); + String workspaceWarning = + bzlmodEnabled + ? "" + : " Most likely you need to upgrade the version of rules repository in the" + + " WORKSPACE file."; + for (Map.Entry autoload : autoloadValues.entrySet()) { + String symbol = autoload.getKey(); + // Check if the symbol is named differently in the bzl file than natively. Renames are rare: + // Example is renaming native.ProguardSpecProvider to ProguardSpecInfo. + String newName = AUTOLOAD_CONFIG.get(symbol).getNewName(); + if (newName == null) { + newName = symbol; + } + BzlLoadValue v = autoload.getValue(); + Object symbolValue = v.getModule().getGlobal(newName); + if (symbolValue == null) { + throw new AutoloadException( + String.format( + "The toplevel symbol '%s' set by --incompatible_load_symbols_externally couldn't" + + " be loaded. '%s' not found in auto loaded '%s'.%s", + symbol, newName, AUTOLOAD_CONFIG.get(symbol).getLoadLabel(), workspaceWarning)); + } + newSymbols.put(symbol, symbolValue); // Exposed as old name + } + return newSymbols.buildOrThrow(); + } + + @Override + public final int hashCode() { + // These fields are used to generate all other private fields. + // Thus, other fields don't need to be included in hash code. + return Objects.hash( + autoloadedSymbols, removedSymbols, partiallyRemovedSymbols, reposDisallowingAutoloads); + } + + @Override + public final boolean equals(Object that) { + if (this == that) { + return true; + } + if (that instanceof AutoloadSymbols) { + AutoloadSymbols other = (AutoloadSymbols) that; + // These fields are used to generate all other private fields. + // Thus, other fields don't need to be included in comparison. + return this.autoloadedSymbols.equals(other.autoloadedSymbols) + && this.removedSymbols.equals(other.removedSymbols) + && this.partiallyRemovedSymbols.equals(other.partiallyRemovedSymbols) + && this.reposDisallowingAutoloads.equals(other.reposDisallowingAutoloads); + } + return false; + } + + /** Configuration of a symbol */ + @AutoValue + public abstract static class SymbolRedirect { + + public abstract String getLoadLabel(); + + public abstract boolean isRule(); + + @Nullable + public abstract String getNewName(); + + public abstract ImmutableSet getRdeps(); + + public BzlLoadValue.Key getKey(RepoContext bazelToolsRepoContext) throws InterruptedException { + try { + return BzlLoadValue.keyForBuild( + Label.parseWithRepoContext(getLoadLabel(), bazelToolsRepoContext)); + } catch (LabelSyntaxException e) { + throw new IllegalStateException(e); + } + } + } + + /** Indicates a problem performing automatic loads. */ + public static final class AutoloadException extends Exception { + + AutoloadException(String message) { + super(message); + } + } + + private static SymbolRedirect ruleRedirect(String label) { + return new AutoValue_AutoloadSymbols_SymbolRedirect(label, true, null, ImmutableSet.of()); + } + + private static SymbolRedirect symbolRedirect(String label, String... rdeps) { + return new AutoValue_AutoloadSymbols_SymbolRedirect( + label, false, null, ImmutableSet.copyOf(rdeps)); + } + + private static SymbolRedirect renamedSymbolRedirect( + String label, String newName, String... rdeps) { + return new AutoValue_AutoloadSymbols_SymbolRedirect( + label, false, newName, ImmutableSet.copyOf(rdeps)); + } + + private static final String[] androidRules = { + "aar_import", "android_binary", "android_library", "android_local_test", "android_sdk" + }; + + private static final ImmutableSet PREDECLARED_REPOS_DISALLOWING_AUTOLOADS = + ImmutableSet.of( + "protobuf", + "com_google_protobuf", + "rules_android", + "rules_cc", + "rules_java", + "rules_java_builtin", + "rules_python", + "rules_python_internal", + "rules_sh", + "apple_common", + "bazel_skylib", + "bazel_tools"); + + private static final ImmutableMap AUTOLOAD_CONFIG = + ImmutableMap.builder() + .put( + "CcSharedLibraryInfo", + symbolRedirect( + "@rules_cc//cc/common:cc_shared_library_info.bzl", "cc_shared_library")) + .put( + "CcSharedLibraryHintInfo", + symbolRedirect("@rules_cc//cc/common:cc_shared_library_hint_info.bzl", "cc_common")) + .put( + "cc_proto_aspect", + symbolRedirect("@protobuf//bazel/common:cc_proto_library.bzl", "cc_proto_library")) + .put( + "ProtoInfo", + symbolRedirect( + "@protobuf//bazel/common:proto_info.bzl", + "proto_library", + "cc_proto_library", + "cc_shared_library", + "java_lite_proto_library", + "java_proto_library", + "proto_lang_toolchain", + "java_binary", + "py_extension", + "proto_common_do_not_use")) + .put( + "proto_common_do_not_use", symbolRedirect("@protobuf//bazel/common:proto_common.bzl")) + .put("cc_common", symbolRedirect("@rules_cc//cc/common:cc_common.bzl")) + .put( + "CcInfo", + symbolRedirect( + "@rules_cc//cc/common:cc_info.bzl", + "cc_binary", + "cc_library", + "cc_test", + "cc_shared_library", + "cc_common", + "java_library", + "cc_proto_library", + "java_import", + "java_runtime", + "java_binary", + "objc_library", + "java_common", + "JavaInfo", + "py_extension", + "cc_import", + "objc_import", + "objc_library", + "cc_toolchain", + "PyCcLinkParamsProvider", + "py_library")) + .put( + "DebugPackageInfo", + symbolRedirect("@rules_cc//cc/common:debug_package_info.bzl", "cc_binary", "cc_test")) + .put( + "CcToolchainConfigInfo", + symbolRedirect( + "@rules_cc//cc/toolchains:cc_toolchain_config_info.bzl", "cc_toolchain")) + .put("java_common", symbolRedirect("@rules_java//java/common:java_common.bzl")) + .put( + "JavaInfo", + symbolRedirect( + "@rules_java//java/common:java_info.bzl", + "java_binary", + "java_library", + "java_test", + "java_proto_library", + "java_lite_proto_library", + "java_plugin", + "java_import", + "java_common")) + .put( + "JavaPluginInfo", + symbolRedirect( + "@rules_java//java/common:java_plugin_info.bzl", + "java_plugin", + "java_library", + "java_binary", + "java_test")) + .put( + "ProguardSpecProvider", + renamedSymbolRedirect( + "@rules_java//java/common:proguard_spec_info.bzl", + "ProguardSpecInfo", + "java_lite_proto_library", + "java_import", + "android_binary", + "android_library")) + .put("android_common", symbolRedirect("@rules_android//rules:common.bzl")) + .put( + "AndroidIdeInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put("ApkInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidInstrumentationInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidResourcesInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidNativeLibsInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidApplicationResourceInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidBinaryNativeLibsInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidSdkInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidManifestInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidAssetsInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidLibraryAarInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidProguardInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidIdlInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidPreDexJarInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidCcLinkParamsInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "DataBindingV2Info", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidLibraryResourceClassJarProvider", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidFeatureFlagSet", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "ProguardMappingInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidBinaryData", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "BaselineProfileProvider", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidNeverLinkLibrariesProvider", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidOptimizedJarInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidDexInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "AndroidOptimizationInfo", + symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) + .put( + "PyInfo", + symbolRedirect( + "@rules_python//python:py_info.bzl", "py_binary", "py_test", "py_library")) + .put( + "PyRuntimeInfo", + symbolRedirect( + "@rules_python//python:py_runtime_info.bzl", + "py_binary", + "py_test", + "py_library")) + .put( + "PyCcLinkParamsProvider", + renamedSymbolRedirect( + "@rules_python//python:py_cc_link_params_info.bzl", + "PyCcLinkParamsInfo", + "py_binary", + "py_test", + "py_library")) + .put("aar_import", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_binary", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_device_script_fixture", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_host_service_fixture", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_library", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_local_test", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_sdk", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("android_tools_defaults_jar", ruleRedirect("@rules_android//rules:rules.bzl")) + .put("cc_binary", ruleRedirect("@rules_cc//cc:cc_binary.bzl")) + .put("cc_import", ruleRedirect("@rules_cc//cc:cc_import.bzl")) + .put("cc_library", ruleRedirect("@rules_cc//cc:cc_library.bzl")) + .put("cc_proto_library", ruleRedirect("@protobuf//bazel:cc_proto_library.bzl")) + .put("cc_shared_library", ruleRedirect("@rules_cc//cc:cc_shared_library.bzl")) + .put("cc_test", ruleRedirect("@rules_cc//cc:cc_test.bzl")) + .put("cc_toolchain", ruleRedirect("@rules_cc//cc/toolchains:cc_toolchain.bzl")) + .put( + "cc_toolchain_suite", ruleRedirect("@rules_cc//cc/toolchains:cc_toolchain_suite.bzl")) + .put( + "fdo_prefetch_hints", ruleRedirect("@rules_cc//cc/toolchains:fdo_prefetch_hints.bzl")) + .put("fdo_profile", ruleRedirect("@rules_cc//cc/toolchains:fdo_profile.bzl")) + .put("java_binary", ruleRedirect("@rules_java//java:java_binary.bzl")) + .put("java_import", ruleRedirect("@rules_java//java:java_import.bzl")) + .put("java_library", ruleRedirect("@rules_java//java:java_library.bzl")) + .put( + "java_lite_proto_library", + ruleRedirect("@protobuf//bazel:java_lite_proto_library.bzl")) + .put( + "java_package_configuration", + ruleRedirect("@rules_java//java/toolchains:java_package_configuration.bzl")) + .put("java_plugin", ruleRedirect("@rules_java//java:java_plugin.bzl")) + .put("java_proto_library", ruleRedirect("@protobuf//bazel:java_proto_library.bzl")) + .put("java_runtime", ruleRedirect("@rules_java//java/toolchains:java_runtime.bzl")) + .put("java_test", ruleRedirect("@rules_java//java:java_test.bzl")) + .put("java_toolchain", ruleRedirect("@rules_java//java/toolchains:java_toolchain.bzl")) + .put("memprof_profile", ruleRedirect("@rules_cc//cc/toolchains:memprof_profile.bzl")) + .put("objc_import", ruleRedirect("@rules_cc//cc:objc_import.bzl")) + .put("objc_library", ruleRedirect("@rules_cc//cc:objc_library.bzl")) + .put( + "propeller_optimize", ruleRedirect("@rules_cc//cc/toolchains:propeller_optimize.bzl")) + .put( + "proto_lang_toolchain", + ruleRedirect("@protobuf//bazel/toolchain:proto_lang_toolchain.bzl")) + .put("proto_library", ruleRedirect("@protobuf//bazel:proto_library.bzl")) + .put("py_binary", ruleRedirect("@rules_python//python:py_binary.bzl")) + .put("py_library", ruleRedirect("@rules_python//python:py_library.bzl")) + .put("py_runtime", ruleRedirect("@rules_python//python:py_runtime.bzl")) + .put("py_test", ruleRedirect("@rules_python//python:py_test.bzl")) + .put("sh_binary", ruleRedirect("@rules_sh//sh:sh_binary.bzl")) + .put("sh_library", ruleRedirect("@rules_sh//sh:sh_library.bzl")) + .put("sh_test", ruleRedirect("@rules_sh//sh:sh_test.bzl")) + .put("available_xcodes", ruleRedirect("@apple_support//xcode:available_xcodes.bzl")) + .put("xcode_config", ruleRedirect("@apple_support//xcode:xcode_config.bzl")) + .put("xcode_config_alias", ruleRedirect("@apple_support//xcode:xcode_config_alias.bzl")) + .put("xcode_version", ruleRedirect("@apple_support//xcode:xcode_version.bzl")) + // this redirect doesn't exists and probably never will, we still need a configuration for + // it, so that it can be removed from Bazels <= 7 if needed + .put( + "apple_common", + symbolRedirect("@apple_support//lib:apple_common.bzl", "objc_import", "objc_library")) + .buildOrThrow(); +} diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index f98a21b903d533..198511309cea06 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -85,7 +85,10 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:file_symlink_exception", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions", + "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", 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 fb48c0724341a5..79614d6f46696d 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 @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.common.options.Converters.CommaSeparatedNonEmptyOptionListConverter; import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter; +import com.google.devtools.common.options.Converters.CommaSeparatedOptionSetConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -116,6 +117,58 @@ public final class BuildLanguageOptions extends OptionsBase { + "the builtins injection mechanism entirely.") public String experimentalBuiltinsBzlPath; + @Option( + name = "incompatible_autoload_externally", + converter = CommaSeparatedOptionSetConverter.class, + defaultValue = "", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "A comma-separated list of rules (or other symbols) that were previously part of Bazel" + + " and which are now to be retrieved from their respective external repositories." + + " This flag is intended to be used to facilitate migration of rules out of Bazel." + + " See also https://github.com/bazelbuild/bazel/issues/23043.\n" + + "A symbol that is autoloaded within a file behaves as if its built-into-Bazel" + + " definition were replaced by its canonical new definition in an external" + + " repository. For a BUILD file, this essentially means implicitly adding a load()" + + " statement. For a .bzl file, it's either a load() statement or a change to a field" + + " of the `native` object, depending on whether the autoloaded symbol is a rule.\n" + + "Bazel maintains a hardcoded list of all symbols that may be autoloaded; only those" + + " symbols may appear in this flag. For each symbol, Bazel knows the new definition" + + " location in an external repository, as well as a set of special-cased" + + " repositories that must not autoload it to avoid creating cycles.\n" + + "A list item of \"+foo\" in this flag causes symbol foo to be autoloaded, except in" + + " foo's exempt repositories, within which the Bazel-defined version of foo is still" + + " available.\n" + + "A list item of \"foo\" triggers autoloading as above, but the Bazel-defined" + + " version of foo is not made available to the excluded repositories. This ensures" + + " that foo's external repository does not depend on the old Bazel implementation of" + + " foo\n" + + "A list item of \"-foo\" does not trigger any autoloading, but makes the" + + " Bazel-defined version of foo inaccessible throughout the workspace. This is used" + + " to validate that the workspace is ready for foo's definition to be deleted from" + + " Bazel.\n" + + "If a symbol is not named in this flag then it continues to work as normal -- no" + + " autoloading is done, nor is the Bazel-defined version suppressed. For" + + " configuration see" + + " https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java") + public List incompatibleAutoloadExternally; + + @Option( + name = "repositories_without_autoloads", + converter = CommaSeparatedOptionSetConverter.class, + defaultValue = "", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "A list of additional repositories (beyond the hardcoded ones Bazel knows about) where " + + "autoloads are not to be added. This should typically contain repositories that are" + + " transitively depended on by a repository that may be loaded automatically " + + "(and which can therefore potentially create a cycle).") + public List repositoriesWithoutAutoloads; + @Option( name = "experimental_builtins_dummy", defaultValue = "false", @@ -790,6 +843,8 @@ public StarlarkSemantics toStarlarkSemantics() { incompatibleStopExportingLanguageModules) .setBool(INCOMPATIBLE_ALLOW_TAGS_PROPAGATION, experimentalAllowTagsPropagation) .set(EXPERIMENTAL_BUILTINS_BZL_PATH, experimentalBuiltinsBzlPath) + .set(INCOMPATIBLE_AUTOLOAD_EXTERNALLY, incompatibleAutoloadExternally) + .set(REPOSITORIES_WITHOUT_AUTOLOAD, repositoriesWithoutAutoloads) .setBool(EXPERIMENTAL_BUILTINS_DUMMY, experimentalBuiltinsDummy) .set(EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE, experimentalBuiltinsInjectionOverride) .setBool(EXPERIMENTAL_BZL_VISIBILITY, experimentalBzlVisibility) @@ -989,6 +1044,10 @@ public StarlarkSemantics toStarlarkSemantics() { // non-booleans public static final StarlarkSemantics.Key EXPERIMENTAL_BUILTINS_BZL_PATH = new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%"); + public static final StarlarkSemantics.Key> INCOMPATIBLE_AUTOLOAD_EXTERNALLY = + new StarlarkSemantics.Key<>("incompatible_autoload_externally", ImmutableList.of()); + public static final StarlarkSemantics.Key> REPOSITORIES_WITHOUT_AUTOLOAD = + new StarlarkSemantics.Key<>("repositories_without_autoloads", ImmutableList.of()); public static final StarlarkSemantics.Key> EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE = new StarlarkSemantics.Key<>("experimental_builtins_injection_override", ImmutableList.of()); public static final StarlarkSemantics.Key MAX_COMPUTATION_STEPS = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index ccc9c2f33a6715..15c420517c11ef 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2407,6 +2407,7 @@ java_library( ":bzl_load_value", ":repository_mapping_value", ":sky_functions", + ":starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution", @@ -2452,17 +2453,6 @@ java_library( ], ) -java_library( - name = "bzl_load_failed_exception", - srcs = ["BzlLoadFailedException.java"], - deps = [ - ":sane_analysis_exception", - "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", - "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", - "//src/main/protobuf:failure_details_java_proto", - ], -) - java_library( name = "state_informing_sky_function_environment", srcs = ["StateInformingSkyFunctionEnvironment.java"], diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java index d18bfe2ca3e699..38c691fce1a27e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.cmdline.BazelCompileContext; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -157,7 +158,14 @@ static BzlCompileValue computeInline( // For WORKSPACE-loaded bzl files, the env isn't quite right not because of injection but // because the "native" object is different. But A) that will be fixed with #11954, and B) we // don't care for the same reason as above. - predeclared = bazelStarlarkEnvironment.getUninjectedBuildBzlEnv(); + + // Takes into account --incompatible_autoload_externally, similarly to the comment above, this + // only defines the correct set of symbols, but does not load them yet. + AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + if (autoloadSymbols == null) { + return null; + } + predeclared = autoloadSymbols.getUninjectedBuildBzlEnv(key.getLabel()); } // We have all deps. Parse, resolve, and return. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index fdd2e3f2097e08..587f57e4b25f13 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.io.InconsistentFilesystemException; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment; import com.google.devtools.build.lib.packages.BuildFileNotFoundException; import com.google.devtools.build.lib.packages.BzlInitThreadContext; @@ -597,13 +598,19 @@ private StarlarkBuiltinsValue getBuiltins( } return StarlarkBuiltinsValue.createEmpty(starlarkSemantics); } + AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + if (autoloadSymbols == null) { + return null; + } try { + boolean withAutoloads = requiresAutoloads(key, autoloadSymbols); if (inliningState == null) { return (StarlarkBuiltinsValue) - env.getValueOrThrow(StarlarkBuiltinsValue.key(), BuiltinsFailedException.class); + env.getValueOrThrow( + StarlarkBuiltinsValue.key(withAutoloads), BuiltinsFailedException.class); } else { return StarlarkBuiltinsFunction.computeInline( - StarlarkBuiltinsValue.key(), + StarlarkBuiltinsValue.key(withAutoloads), inliningState, ruleClassProvider.getBazelStarlarkEnvironment(), /* bzlLoadFunction= */ this); @@ -623,6 +630,23 @@ private static boolean requiresBuiltinsInjection(BzlLoadValue.Key key) { && !(key instanceof BzlLoadValue.KeyForBzlmodBootstrap)); } + private static boolean requiresAutoloads(BzlLoadValue.Key key, AutoloadSymbols autoloadSymbols) { + // We do autoloads for all BUILD files and BUILD-loaded .bzl files, except for files in + // certain rule repos (see AutoloadSymbols#reposDisallowingAutoloads). + // + // We don't do autoloads for the WORKSPACE file, Bzlmod files, or .bzls loaded by them, + // because in general the rules repositories that we would load are not yet available. + // + // We never do autoloads for builtins bzls. + // + // We don't do autoloads for the prelude file, but that's a single file so users can migrate it + // easily. (We do autoloads in .bzl files that are loaded by the prelude file.) + return autoloadSymbols.isEnabled() + && key instanceof BzlLoadValue.KeyForBuild + && !key.isBuildPrelude() + && !autoloadSymbols.autoloadsDisabledForRepo(key.getLabel()); + } + /** * Given a bzl key, validates that the corresponding package exists (if required) and returns the * associated compile key based on the package's root. Returns null for a missing Skyframe dep or diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java index 8fc44dd312b46d..16b9296f37e13d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java @@ -188,12 +188,25 @@ public boolean maybeReportCycle( } else if (Iterables.any(cycle, IS_BZL_LOAD)) { Label fileLabel = ((BzlLoadValue.Key) Iterables.getLast(Iterables.filter(cycle, IS_BZL_LOAD))).getLabel(); - eventHandler.handle( - Event.error( - null, - String.format( - "Failed to load .bzl file '%s': possible dependency cycle detected.\n", - fileLabel))); + final String errorMessage; + if (cycle.get(0).equals(StarlarkBuiltinsValue.key(true))) { + // We know `fileLabel` is the last .bzl visited in the cycle. We also know that + // BzlLoadFunction triggered the cycle by requesting StarlarkBuiltinsValue w/autoloads. + // We know that we're not in builtins .bzls, because they don't request w/autoloads. + // Thus, `fileLabel` is a .bzl transitively depended on by an autoload. + errorMessage = + String.format( + "Cycle caused by autoloads, failed to load .bzl file '%s'.\n" + + "Add '%s' to --repositories_without_autoloads or disable autoloads by setting" + + " '--incompatible_autoload_externally='\n" + + "More information on https://github.com/bazelbuild/bazel/issues/23043.\n", + fileLabel, fileLabel.getRepository().getName()); + } else { + errorMessage = + String.format( + "Failed to load .bzl file '%s': possible dependency cycle detected.\n", fileLabel); + } + eventHandler.handle(Event.error(null, errorMessage)); return true; } else if (Iterables.any(cycle, IS_PACKAGE_LOOKUP)) { PackageIdentifier pkg = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 0ae628042aab90..5a011582d06cd5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; import com.google.devtools.build.lib.io.FileSymlinkException; import com.google.devtools.build.lib.io.InconsistentFilesystemException; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.BuildFileNotFoundException; import com.google.devtools.build.lib.packages.CachingPackageLocator; @@ -473,14 +474,21 @@ public SkyValue compute(SkyKey key, Environment env) StarlarkBuiltinsValue starlarkBuiltinsValue; try { + // Bazel: we do autoloads for all BUILD files if enabled + AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + if (autoloadSymbols == null) { + return null; + } if (bzlLoadFunctionForInlining == null) { starlarkBuiltinsValue = (StarlarkBuiltinsValue) - env.getValueOrThrow(StarlarkBuiltinsValue.key(), BuiltinsFailedException.class); + env.getValueOrThrow( + StarlarkBuiltinsValue.key(/* withAutoloads= */ autoloadSymbols.isEnabled()), + BuiltinsFailedException.class); } else { starlarkBuiltinsValue = StarlarkBuiltinsFunction.computeInline( - StarlarkBuiltinsValue.key(), + StarlarkBuiltinsValue.key(/* withAutoloads= */ autoloadSymbols.isEnabled()), BzlLoadFunction.InliningState.create(env), packageFactory.getRuleClassProvider().getBazelStarlarkEnvironment(), bzlLoadFunctionForInlining); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java index 9ebeaa31e08c96..e46e934b1f2141 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java @@ -19,6 +19,7 @@ import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -84,6 +85,10 @@ public static Injected injected(Precomputed precomputed, T value) { public static final Precomputed STARLARK_SEMANTICS = new Precomputed<>("starlark_semantics"); + // Configuration of --incompatible_load_externally + public static final Precomputed AUTOLOAD_SYMBOLS = + new Precomputed<>("autoload_symbols"); + static final Precomputed BUILD_ID = new Precomputed<>("build_id", /*shareable=*/ false); public static final Precomputed> ACTION_ENV = new Precomputed<>("action_env"); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 92978152888957..3f3ada15d3eab8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -122,6 +122,7 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.io.FileSymlinkCycleUniquenessFunction; import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.BuildFileName; import com.google.devtools.build.lib.packages.NoSuchPackageException; @@ -1261,6 +1262,10 @@ private void setStarlarkSemantics(StarlarkSemantics starlarkSemantics) { PrecomputedValue.STARLARK_SEMANTICS.set(injectable(), starlarkSemantics); } + private void setAutoloadsConfiguration(AutoloadSymbols autoloadSymbols) { + PrecomputedValue.AUTOLOAD_SYMBOLS.set(injectable(), autoloadSymbols); + } + public void setBaselineConfiguration(BuildOptions buildOptions) { PrecomputedValue.BASELINE_CONFIGURATION.set(injectable(), buildOptions); } @@ -1395,6 +1400,7 @@ public void preparePackageLoading( StarlarkSemantics starlarkSemantics = getEffectiveStarlarkSemantics(buildLanguageOptions); setStarlarkSemantics(starlarkSemantics); + setAutoloadsConfiguration(new AutoloadSymbols(ruleClassProvider, starlarkSemantics)); setSiblingDirectoryLayout( starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT)); setPackageLocator(pkgLocator); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java index 2ffbc6cca615c9..75550aff5c8292 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java @@ -15,17 +15,22 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.packages.AutoloadSymbols; +import com.google.devtools.build.lib.packages.AutoloadSymbols.AutoloadException; import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment; import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment.InjectionException; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException; +import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.skyframe.RecordingSkyFunctionEnvironment; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyframeLookupResult; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; @@ -59,8 +64,21 @@ * href="https://docs.google.com/document/d/1GW7UVo1s9X0cti9OMgT3ga5ozKYUWLPk9k8c4-34rC4">design * doc: * - *

This function has a trivial key, so there can only be one value in the build at a time. It has - * a single dependency, on the result of evaluating the exports.bzl file to a {@link BzlLoadValue}. + *

This function has two trivial keys, so there can only be two values in the build at a time. + * First key/value (without autoloads) has a single dependency, on the result of evaluating the + * exports.bzl file to a {@link BzlLoadValue}. Second key/value has dependencies on the results of + * extenally loaded symbols and rules. For more information on the second value see {@link + * AutoloadSymbols}. + * + *

This function supports a special "inlining" mode, similar to {@link BzlLoadFunction} (see that + * class's javadoc and code comments). Whenever we inline {@link BzlLoadFunction} we also inline + * {@link StarlarkBuiltinsFunction} (and {@link StarlarkBuiltinsFunction}'s calls to {@link + * BzlLoadFunction} are then themselves inlined!). Similar to {@link BzlLoadFunction}'s inlining, we + * cache the result of this computation, and this caching is managed by {@link + * BzlLoadFunction.InlineCacheManager}. But since there's only a single {@link + * StarlarkBuiltinsValue} node and we don't need to worry about that node's value changing at future + * invocations or subsequent versions (see {@link InlineCacheManager#reset} for why), our caching + * strategy is much simpler and we don't need to bother inlining deps of the Skyframe subgraph. */ public class StarlarkBuiltinsFunction implements SkyFunction { @@ -93,10 +111,13 @@ public StarlarkBuiltinsFunction(BazelStarlarkEnvironment bazelStarlarkEnvironmen @Nullable public SkyValue compute(SkyKey skyKey, Environment env) throws StarlarkBuiltinsFunctionException, InterruptedException { - // skyKey is a singleton, unused. try { return computeInternal( - env, bazelStarlarkEnvironment, /* inliningState= */ null, /* bzlLoadFunction= */ null); + env, + bazelStarlarkEnvironment, + ((StarlarkBuiltinsValue.Key) skyKey.argument()).isWithAutoloads(), + /* inliningState= */ null, + /* bzlLoadFunction= */ null); } catch (BuiltinsFailedException e) { throw new StarlarkBuiltinsFunctionException(e); } @@ -113,7 +134,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) */ @Nullable public static StarlarkBuiltinsValue computeInline( - StarlarkBuiltinsValue.Key key, // singleton value, unused + StarlarkBuiltinsValue.Key key, BzlLoadFunction.InliningState inliningState, BazelStarlarkEnvironment bazelStarlarkEnvironment, BzlLoadFunction bzlLoadFunction) @@ -122,7 +143,8 @@ public static StarlarkBuiltinsValue computeInline( // inlining mechanism and its invariants. For our purposes, the Skyframe environment to use // comes from inliningState. return computeInternal( - inliningState.getEnvironment(), bazelStarlarkEnvironment, inliningState, bzlLoadFunction); + inliningState.getEnvironment(), bazelStarlarkEnvironment, key.isWithAutoloads(), + inliningState, bzlLoadFunction); } // bzlLoadFunction and inliningState are non-null iff using inlining code path. @@ -130,6 +152,7 @@ public static StarlarkBuiltinsValue computeInline( private static StarlarkBuiltinsValue computeInternal( Environment env, BazelStarlarkEnvironment bazelStarlarkEnvironment, + boolean isWithAutoloads, @Nullable BzlLoadFunction.InliningState inliningState, @Nullable BzlLoadFunction bzlLoadFunction) throws BuiltinsFailedException, InterruptedException { @@ -141,9 +164,27 @@ private static StarlarkBuiltinsValue computeInternal( if (starlarkSemantics.get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH).isEmpty()) { return StarlarkBuiltinsValue.createEmpty(starlarkSemantics); } + AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + if (autoloadSymbols == null) { + return null; + } + + if (autoloadSymbols.isEnabled()) { + // We can't do autoloads where the rules are implemented (disabling them when running in + // main repository named rules_python) + ModuleFileValue mainModule = + (ModuleFileValue) env.getValue(ModuleFileValue.KEY_FOR_ROOT_MODULE); + if (mainModule == null) { + return null; + } + if (autoloadSymbols.autoloadsDisabledForRepo(mainModule.getModule().getName())) { + isWithAutoloads = false; + } + } // Load exports.bzl. If we were requested using inlining, make sure to inline the call back into // BzlLoadFunction. + // If requested also loads "autoloads": rules and providers from external repositories. BzlLoadValue exportsValue; try { if (inliningState == null) { @@ -156,12 +197,59 @@ private static StarlarkBuiltinsValue computeInternal( } catch (BzlLoadFailedException ex) { throw BuiltinsFailedException.errorEvaluatingBuiltinsBzls(ex); } - if (exportsValue == null) { + + ImmutableMap autoBzlLoadValues; + try { + ImmutableMap autoBzlLoadKeys = + isWithAutoloads ? autoloadSymbols.getLoadKeys(env) : ImmutableMap.of(); + if (autoBzlLoadKeys == null) { + return null; + } + ImmutableMap.Builder autoBzlLoadValuesBuilder = + ImmutableMap.builderWithExpectedSize(autoBzlLoadKeys.size()); + if (inliningState == null) { + SkyframeLookupResult values = env.getValuesAndExceptions(autoBzlLoadKeys.values()); + for (var symbolKeyEntry : autoBzlLoadKeys.entrySet()) { + String symbol = symbolKeyEntry.getKey(); + BzlLoadValue value = + (BzlLoadValue) + values.getOrThrow(symbolKeyEntry.getValue(), BzlLoadFailedException.class); + if (value != null) { + autoBzlLoadValuesBuilder.put(symbol, value); + } + } + } else { + for (var symbolKeyEntry : autoBzlLoadKeys.entrySet()) { + String symbol = symbolKeyEntry.getKey(); + BzlLoadValue value = + bzlLoadFunction.computeInline(symbolKeyEntry.getValue(), inliningState); + if (value != null) { + autoBzlLoadValuesBuilder.put(symbol, value); + } + } + } + autoBzlLoadValues = autoBzlLoadValuesBuilder.buildOrThrow(); + } catch (BzlLoadFailedException ex) { + + throw BuiltinsFailedException.errorEvaluatingAutoloadedBzls( + ex, starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)); + } + if (env.valuesMissing()) { return null; } - // Apply declarations of exports.bzl to the native predeclared symbols. + // Compute digest of exports.bzl and possibly externally loaded symbols (Bazel) byte[] transitiveDigest = exportsValue.getTransitiveDigest(); + if (!autoBzlLoadValues.isEmpty()) { + Fingerprint fp = new Fingerprint(); + fp.addBytes(transitiveDigest); + for (BzlLoadValue value : autoBzlLoadValues.values()) { + fp.addBytes(value.getTransitiveDigest()); + } + transitiveDigest = fp.digestAndReset(); + } + + // Apply declarations of exports.bzl to the native predeclared symbols. Module module = exportsValue.getModule(); try { ImmutableMap exportedToplevels = getDict(module, "exported_toplevels"); @@ -181,6 +269,14 @@ private static StarlarkBuiltinsValue computeInternal( bazelStarlarkEnvironment.createBuildEnvUsingInjection( exportedRules, starlarkSemantics.get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE)); + + // Apply declarations of externally loaded symbols to the native predeclared symbols. + ImmutableMap newSymbols = autoloadSymbols.processLoads(autoBzlLoadValues); + predeclaredForBuild = + autoloadSymbols.modifyBuildEnv(isWithAutoloads, predeclaredForBuild, newSymbols); + predeclaredForBuildBzl = + autoloadSymbols.modifyBuildBzlEnv(isWithAutoloads, predeclaredForBuildBzl, newSymbols); + return StarlarkBuiltinsValue.create( predeclaredForBuildBzl, predeclaredForWorkspaceBzl, @@ -190,6 +286,11 @@ private static StarlarkBuiltinsValue computeInternal( starlarkSemantics); } catch (EvalException | InjectionException ex) { throw BuiltinsFailedException.errorApplyingExports(ex); + } catch (AutoloadException ex) { + throw new BuiltinsFailedException( + String.format("Failed to apply symbols loaded externally: %s", ex.getMessage()), + ex, + Transience.PERSISTENT); } } @@ -241,6 +342,20 @@ static BuiltinsFailedException errorEvaluatingBuiltinsBzls( transience); } + static BuiltinsFailedException errorEvaluatingAutoloadedBzls( + BzlLoadFailedException cause, boolean bzlmodEnabled) { + String additionalMessage = + bzlmodEnabled + ? "" + : ". Most likely you need to upgrade the version of rules repository in the" + + " WORKSPACE file."; + return new BuiltinsFailedException( + String.format( + "Failed to autoload external symbols: %s%s", cause.getMessage(), additionalMessage), + cause, + cause.getTransience()); + } + static BuiltinsFailedException errorApplyingExports(Exception cause) { return new BuiltinsFailedException( String.format("Failed to apply declared builtins: %s", cause.getMessage()), diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsValue.java index 7f1dc1a4be2f0e..7a28b0c2ce8c9c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsValue.java @@ -23,7 +23,8 @@ import net.starlark.java.eval.StarlarkSemantics; /** - * A Skyframe value representing the result of evaluating the {@code @_builtins} pseudo-repository. + * A Skyframe value representing the result of evaluating the {@code @_builtins} pseudo-repository, + * and in Bazel where applicable, applying autoloads. * *

To avoid unnecessary Skyframe edges, the {@code StarlarkSemantics} are included in this value, * so that a caller who obtains a StarlarkBuiltinsValue can also access the StarlarkSemantics @@ -135,11 +136,19 @@ public static StarlarkBuiltinsValue createEmpty(StarlarkSemantics starlarkSemant starlarkSemantics); } - /** Returns the singleton SkyKey for this type of value. */ + /** Returns the SkyKey for BuiltinsValue containing only additional builtin symbols and rules. */ public static Key key() { return Key.INSTANCE; } + /** + * Returns the SkyKey for BuiltinsValue optionally amended with externally loaded symbols and + * rules. + */ + public static Key key(boolean withAutoloads) { + return withAutoloads ? Key.INSTANCE_WITH_AUTOLOADS : Key.INSTANCE; + } + /** * Skyframe key for retrieving the {@code @_builtins} definitions. * @@ -147,9 +156,18 @@ public static Key key() { */ static final class Key implements SkyKey { - private static final Key INSTANCE = new Key(); + private final boolean withAutoloads; - private Key() {} + private static final Key INSTANCE = new Key(false); + private static final Key INSTANCE_WITH_AUTOLOADS = new Key(true); + + private Key(boolean withAutoloads) { + this.withAutoloads = withAutoloads; + } + + public boolean isWithAutoloads() { + return withAutoloads; + } @Override public SkyFunctionName functionName() { @@ -163,12 +181,12 @@ public String toString() { @Override public boolean equals(Object other) { - return other instanceof Key; + return other instanceof Key key && this.withAutoloads == key.withAutoloads; } @Override public int hashCode() { - return 7727; // more or less xkcd/221 + return withAutoloads ? 7727 : 7277; // more or less xkcd/221 } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index 54a9fbe4939a47..a5541a88e9201e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.io.FileSymlinkCycleUniquenessFunction; import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.BuildFileName; import com.google.devtools.build.lib.packages.CachingPackageLocator; @@ -47,6 +48,7 @@ import com.google.devtools.build.lib.packages.PackageLoadingListener; import com.google.devtools.build.lib.packages.PackageOverheadEstimator; import com.google.devtools.build.lib.packages.PackageValidator; +import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -304,6 +306,7 @@ public final PackageLoader build() { makePreinjectedDiff( starlarkSemantics, builder.pkgLocator, + ruleClassProvider, ImmutableList.copyOf(builder.extraPrecomputedValues)); pkgFactory = new PackageFactory( @@ -318,6 +321,7 @@ public final PackageLoader build() { private static ImmutableDiff makePreinjectedDiff( StarlarkSemantics starlarkSemantics, PathPackageLocator pkgLocator, + RuleClassProvider ruleClassProvider, ImmutableList extraPrecomputedValues) { final Map valuesToInject = new HashMap<>(); Injectable injectable = @@ -340,6 +344,8 @@ public void inject(SkyKey key, Delta delta) { PrecomputedValue.CONFIG_SETTING_VISIBILITY_POLICY .set(injectable, ConfigSettingVisibilityPolicy.LEGACY_OFF); PrecomputedValue.STARLARK_SEMANTICS.set(injectable, starlarkSemantics); + PrecomputedValue.AUTOLOAD_SYMBOLS.set( + injectable, new AutoloadSymbols(ruleClassProvider, starlarkSemantics)); return new ImmutableDiff(ImmutableList.of(), valuesToInject); } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index c87924cf4d35af..68dc2cf7f600c4 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.EventKind; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -278,12 +279,14 @@ public void setup() throws Exception { .build(), differencer); - PrecomputedValue.STARLARK_SEMANTICS.set( - differencer, + StarlarkSemantics semantics = StarlarkSemantics.builder() .setBool(BuildLanguageOptions.ENABLE_BZLMOD, true) .setBool(BuildLanguageOptions.EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, true) - .build()); + .build(); + PrecomputedValue.STARLARK_SEMANTICS.set(differencer, semantics); + PrecomputedValue.AUTOLOAD_SYMBOLS.set( + differencer, new AutoloadSymbols(ruleClassProvider, semantics)); RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(differencer, ImmutableMap.of()); RepositoryDelegatorFunction.FORCE_FETCH.set( differencer, RepositoryDelegatorFunction.FORCE_FETCH_DISABLED); diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index 7dd7d6159d4108..a068335e439244 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.StoredEventHandler; +import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -282,9 +283,11 @@ public void setupDelegator() throws Exception { RepositoryDelegatorFunction.FORCE_FETCH.set( differencer, RepositoryDelegatorFunction.FORCE_FETCH_DISABLED); PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); - PrecomputedValue.STARLARK_SEMANTICS.set( - differencer, - StarlarkSemantics.builder().setBool(BuildLanguageOptions.ENABLE_BZLMOD, true).build()); + StarlarkSemantics semantics = + StarlarkSemantics.builder().setBool(BuildLanguageOptions.ENABLE_BZLMOD, true).build(); + PrecomputedValue.STARLARK_SEMANTICS.set(differencer, semantics); + PrecomputedValue.AUTOLOAD_SYMBOLS.set( + differencer, new AutoloadSymbols(ruleClassProvider, semantics)); RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set( differencer, Optional.empty()); PrecomputedValue.REPO_ENV.set(differencer, ImmutableMap.of()); diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD index 3d1261327618e5..a20d2666eee849 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -718,6 +718,14 @@ sh_test( tags = ["no_windows"], ) +sh_test( + name = "load_removed_symbols_test", + size = "medium", + srcs = ["load_removed_symbols_test.sh"], + data = [":test-deps"], + tags = ["no_windows"], +) + sh_test( name = "bazel_java_test", size = "medium", diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh new file mode 100755 index 00000000000000..6c649ef85900aa --- /dev/null +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -0,0 +1,411 @@ +#!/bin/bash +# +# Copyright 2024 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Tests the behaviour of --incompatible_autoload_externally flag. + +# Load the test setup defined in the parent directory +CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${CURRENT_DIR}/../integration_test_setup.sh" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +#### SETUP ############################################################# + +set -e + +#### TESTS ############################################################# + +function setup_module_dot_bazel() { + cat > "MODULE.bazel" < "${rules_android_workspace}/MODULE.bazel" << EOF +module(name = "rules_android") +EOF + cat > "${rules_android_workspace}/rules/rules.bzl" << EOF +def _impl(ctx): + pass + +aar_import = rule( + implementation = _impl, + attrs = { + "aar": attr.label(allow_files = True), + "deps": attr.label_list(), + } +) +EOF + + cat >> MODULE.bazel << EOF +bazel_dep( + name = "rules_android", +) +local_path_override( + module_name = "rules_android", + path = "${rules_android_workspace}", +) +EOF +} + +function disabled_test_removed_rule_loaded() { + setup_module_dot_bazel + mock_rules_android + + cat > BUILD << EOF +aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], +) +EOF + bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 || fail "build failed" + # TODO(b/355260271): add test with workspace enabled +} + +function disabled_test_removed_rule_loaded_from_bzl() { + setup_module_dot_bazel + mock_rules_android + + cat > macro.bzl << EOF +def macro(): + native.aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], + ) +EOF + + cat > BUILD << EOF +load(":macro.bzl", "macro") +macro() +EOF + bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 || fail "build failed" + # TODO(b/355260271): add test with workspace enabled +} + +# TODO(b/355260271): enable this once we have a removed symbol +function disabled_test_removed_symbol_loaded() { + setup_module_dot_bazel + + cat > symbol.bzl << EOF +def symbol(): + a = ProtoInfo +EOF + + cat > BUILD << EOF +load(":symbol.bzl", "symbol") +symbol() +EOF + bazel build --incompatible_autoload_externally=ProtoInfo :all >&$TEST_log 2>&1 || fail "build failed" +} + +function test_existing_rule_is_redirected() { + setup_module_dot_bazel + + cat > BUILD << EOF +py_library( + name = 'py_library', +) +EOF + bazel query --incompatible_autoload_externally=+py_library ':py_library' --output=build >&$TEST_log 2>&1 || fail "build failed" + expect_log "rules_python./python/py_library.bzl" +} + +function test_existing_rule_is_redirected_in_bzl() { + setup_module_dot_bazel + + cat > macro.bzl << EOF +def macro(): + native.py_library( + name = 'py_library', + ) +EOF + + cat > BUILD << EOF +load(":macro.bzl", "macro") +macro() +EOF + bazel query --incompatible_autoload_externally=+py_library ':py_library' --output=build >&$TEST_log 2>&1 || fail "build failed" + expect_log "rules_python./python/py_library.bzl" +} + + +function test_removed_rule_not_loaded() { + setup_module_dot_bazel + + cat > BUILD << EOF +aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], + visibility = ['//visibility:public'], +) +EOF + + bazel build --incompatible_autoload_externally= :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "name 'aar_import' is not defined" +} + +function test_removed_rule_not_loaded_in_bzl() { + setup_module_dot_bazel + + cat > macro.bzl << EOF +def macro(): + native.aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], + visibility = ['//visibility:public'], + ) +EOF + + cat > BUILD << EOF +load(":macro.bzl", "macro") +macro() +EOF + + bazel build --incompatible_autoload_externally= :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "no native function or rule 'aar_import'" +} + +# TODO: enable once we have a removed symbol +function disabled_test_removed_symbol_not_loaded_in_bzl() { + setup_module_dot_bazel + + cat > symbol.bzl << EOF +def symbol(): + a = ProtoInfo +EOF + + cat > BUILD << EOF +load(":symbol.bzl", "symbol") +symbol() +EOF + + bazel build --incompatible_autoload_externally= :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "name 'ProtoInfo' is not defined" +} + + +function test_removing_existing_rule() { + setup_module_dot_bazel + + cat > BUILD << EOF +android_binary( + name = "bin", + srcs = [ + "MainActivity.java", + "Jni.java", + ], + manifest = "AndroidManifest.xml", + deps = [ + ":lib", + ":jni" + ], +) +EOF + + bazel build --incompatible_autoload_externally=-android_binary :bin >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "name 'android_binary' is not defined" +} + +function test_removing_existing_rule_in_bzl() { + setup_module_dot_bazel + + cat > macro.bzl << EOF +def macro(): + native.android_binary( + name = "bin", + srcs = [ + "MainActivity.java", + "Jni.java", + ], + manifest = "AndroidManifest.xml", + deps = [ + ":lib", + ":jni" + ], + ) +EOF + + cat > BUILD << EOF +load(":macro.bzl", "macro") +macro() +EOF + + bazel build --incompatible_autoload_externally=-android_binary :bin >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "no native function or rule 'android_binary'" +} + +function test_removing_symbol_incompletely() { + setup_module_dot_bazel + + cat > symbol.bzl << EOF +def symbol(): + a = ProtoInfo +EOF + + cat > BUILD << EOF +load(":symbol.bzl", "symbol") +symbol() +EOF + + bazel build --incompatible_autoload_externally=-ProtoInfo :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Symbol in 'ProtoInfo' can't be removed, because it's still used by: proto_library, cc_proto_library, cc_shared_library, java_lite_proto_library, java_proto_library, proto_lang_toolchain, java_binary, proto_common_do_not_use" +} + +function test_removing_existing_symbol() { + setup_module_dot_bazel + + cat > symbol.bzl << EOF +def symbol(): + a = DebugPackageInfo +EOF + + cat > BUILD << EOF +load(":symbol.bzl", "symbol") +symbol() +EOF + + bazel build --incompatible_autoload_externally=-DebugPackageInfo,-cc_binary,-cc_test :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "name 'DebugPackageInfo' is not defined" +} + +function test_removing_symbol_typo() { + setup_module_dot_bazel + + cat > bzl_file.bzl << EOF +def bzl_file(): + pass +EOF + + cat > BUILD << EOF +load(":bzl_file.bzl", "bzl_file") +EOF + + bazel build --incompatible_autoload_externally=-ProtozzzInfo :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Undefined symbol in --incompatible_autoload_externally" +} + +function test_removing_rule_typo() { + setup_module_dot_bazel + + touch BUILD + + bazel build --incompatible_autoload_externally=-androidzzz_binary :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Undefined symbol in --incompatible_autoload_externally" +} + +function test_redirecting_rule_with_bzl_typo() { + setup_module_dot_bazel + + # Bzl file is evaluated first, so this should cover bzl file support + cat > bzl_file.bzl << EOF +def bzl_file(): + pass +EOF + + cat > BUILD << EOF +load(":bzl_file.bzl", "bzl_file") +EOF + + bazel build --incompatible_autoload_externally=pyzzz_library :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Undefined symbol in --incompatible_autoload_externally" +} + +function test_redirecting_rule_typo() { + setup_module_dot_bazel + + cat > BUILD << EOF +EOF + + + bazel build --incompatible_autoload_externally=pyzzz_library :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Undefined symbol in --incompatible_autoload_externally" +} + +function test_redirecting_symbols_typo() { + setup_module_dot_bazel + + # Bzl file is evaluated first, so this should cover bzl file support + cat > bzl_file.bzl << EOF +def bzl_file(): + pass +EOF + + cat > BUILD << EOF +load(":bzl_file.bzl", "bzl_file") +EOF + + bazel build --incompatible_autoload_externally=ProotoInfo :all >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Undefined symbol in --incompatible_autoload_externally" +} + +function test_bad_flag_value() { + setup_module_dot_bazel + + cat > BUILD << EOF +py_library( + name = 'py_library', +) +EOF + bazel query --incompatible_autoload_externally=py_library,-py_library ':py_library' --output=build >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Duplicated symbol 'py_library' in --incompatible_autoload_externally" +} + +function disabled_test_missing_symbol_error() { + setup_module_dot_bazel + mock_rules_android + rules_android_workspace="${TEST_TMPDIR}/rules_android_workspace" + # emptying the file simulates a missing symbol + cat > "${rules_android_workspace}/rules/rules.bzl" << EOF +EOF + + cat > BUILD << EOF +aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], +) +EOF + bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Failed to apply symbols loaded externally: The toplevel symbol 'aar_import' set by --incompatible_load_symbols_externally couldn't be loaded. 'aar_import' not found in auto loaded '@rules_android//rules:rules.bzl'." +} + +function disabled_test_missing_bzlfile_error() { + setup_module_dot_bazel + mock_rules_android + rules_android_workspace="${TEST_TMPDIR}/rules_android_workspace" + rm "${rules_android_workspace}/rules/rules.bzl" + + cat > BUILD << EOF +aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], +) +EOF + bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "Failed to autoload external symbols: cannot load '@@rules_android+//rules:rules.bzl': no such file" +} + + +run_suite "load_removed_symbols" From 0ce42bbaa96e5e1b628dc750af23861614aa1012 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 23 Aug 2024 06:48:33 -0700 Subject: [PATCH 04/15] Implement shortcut semantics incompatible_load_externally Listing 30 different symbols on the command line is not really users friendly. Once we finish a rules set, we can set it to the whole rules repository. PiperOrigin-RevId: 666776777 Change-Id: I6ddd8aec4daec26a427f962f6f76e496163d1108 --- .../build/lib/packages/AutoloadSymbols.java | 27 +++++++++++++++++++ .../semantics/BuildLanguageOptions.java | 4 ++- .../integration/load_removed_symbols_test.sh | 12 +++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index f15af962f86ec4..2ed71795212dda 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -36,6 +36,7 @@ import java.util.Objects; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Stream; import javax.annotation.Nullable; import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkSemantics; @@ -99,6 +100,25 @@ public AutoloadSymbols(RuleClassProvider ruleClassProvider, StarlarkSemantics se return; } + // Expand symbols given with @rules_foo + symbolConfiguration = + symbolConfiguration.stream() + .flatMap( + flag -> { + String prefix = ""; + String flagWithoutPrefix = flag; + if (flag.startsWith("+") || flag.startsWith("-")) { + prefix = flag.substring(0, 1); + flagWithoutPrefix = flag.substring(1); + } + if (flagWithoutPrefix.startsWith("@")) { + return getAllSymbols(flagWithoutPrefix, prefix).stream(); + } else { + return Stream.of(flag); + } + }) + .collect(toImmutableList()); + // Validates the inputs Set uniqueSymbols = new HashSet<>(); for (String symbol : symbolConfiguration) { @@ -286,6 +306,13 @@ private static ImmutableList filterSymbols( .collect(toImmutableList()); } + private ImmutableList getAllSymbols(String repository, String prefix) { + return AUTOLOAD_CONFIG.entrySet().stream() + .filter(entry -> entry.getValue().getLoadLabel().startsWith(repository + "//")) + .map(entry -> prefix + entry.getKey()) + .collect(toImmutableList()); + } + private static Map convertNativeStructToMap(StarlarkInfo struct) { LinkedHashMap destr = new LinkedHashMap<>(); for (String field : struct.getFieldNames()) { 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 79614d6f46696d..a4495a48f646cd 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 @@ -152,7 +152,9 @@ public final class BuildLanguageOptions extends OptionsBase { + "If a symbol is not named in this flag then it continues to work as normal -- no" + " autoloading is done, nor is the Bazel-defined version suppressed. For" + " configuration see" - + " https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java") + + " https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java" + + " As a shortcut also whole repository may be used, for example +@rules_python will" + + " autoload all Python rules.") public List incompatibleAutoloadExternally; @Option( diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index 6c649ef85900aa..575c3c503fe300 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -408,4 +408,16 @@ EOF } +function test_whole_repo_flag() { + setup_module_dot_bazel + + cat > BUILD << EOF +py_library( + name = 'py_library', +) +EOF + bazel query --incompatible_autoload_externally=+@rules_python ':py_library' --output=build >&$TEST_log 2>&1 || fail "build failed" +} + + run_suite "load_removed_symbols" From eb392818e63f3e969b404523c3ff853faa19f940 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 9 Sep 2024 11:14:00 -0700 Subject: [PATCH 05/15] Remove android_device_script_fixture and android_host_service_fixture They're not provided by rules_android. PiperOrigin-RevId: 672607197 Change-Id: Ieb81a72c0d26b165f98d8b1f11c9bf2149f550d8 --- .../com/google/devtools/build/lib/packages/AutoloadSymbols.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 2ed71795212dda..e9f08c407ac801 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -667,8 +667,6 @@ private static SymbolRedirect renamedSymbolRedirect( "py_library")) .put("aar_import", ruleRedirect("@rules_android//rules:rules.bzl")) .put("android_binary", ruleRedirect("@rules_android//rules:rules.bzl")) - .put("android_device_script_fixture", ruleRedirect("@rules_android//rules:rules.bzl")) - .put("android_host_service_fixture", ruleRedirect("@rules_android//rules:rules.bzl")) .put("android_library", ruleRedirect("@rules_android//rules:rules.bzl")) .put("android_local_test", ruleRedirect("@rules_android//rules:rules.bzl")) .put("android_sdk", ruleRedirect("@rules_android//rules:rules.bzl")) From d7a3c3911fc2eda28ed35cc92c4755581f6d0791 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 12 Sep 2024 01:58:37 -0700 Subject: [PATCH 06/15] Implement legacy_symbols In the repositories that don't have autoloads we also expose native.legacy_symbols. Those can be used to fallback to the native symbol, whenever it's still available in Bazel. Fallback using a top-level symbol doesn't work, because BzlCompileFunction would throw an error when it's mentioned. legacy_symbols aren't available when autoloads are not enabled. The feature is intended to be used with bazel_features repository, which can correctly report native symbols on all versions of Bazel. PiperOrigin-RevId: 673741927 Change-Id: I2334030d70cbb944b92784e32a184841ea238d51 --- .../build/lib/packages/AutoloadSymbols.java | 39 +++++++++++-- .../integration/load_removed_symbols_test.sh | 57 +++++++++++++++++++ 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index e9f08c407ac801..03a96471f4ac4d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -38,6 +38,7 @@ import java.util.function.Predicate; import java.util.stream.Stream; import javax.annotation.Nullable; +import net.starlark.java.eval.GuardedValue; import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkSemantics; @@ -232,10 +233,14 @@ public ImmutableMap modifyBuildBzlEnv( ImmutableMap originalEnv, ImmutableMap newSymbols) { if (isWithAutoloads) { - return modifyBuildBzlEnv(originalEnv, /* add= */ newSymbols, /* remove= */ removedSymbols); + return modifyBuildBzlEnv( + originalEnv, /* add= */ newSymbols, /* remove= */ removedSymbols, isWithAutoloads); } else { return modifyBuildBzlEnv( - originalEnv, /* add= */ ImmutableMap.of(), /* remove= */ partiallyRemovedSymbols); + originalEnv, + /* add= */ ImmutableMap.of(), + /* remove= */ partiallyRemovedSymbols, + isWithAutoloads); } } @@ -247,7 +252,8 @@ public ImmutableMap modifyBuildBzlEnv( private ImmutableMap modifyBuildBzlEnv( ImmutableMap originalEnv, ImmutableMap add, - ImmutableList remove) { + ImmutableList remove, + boolean isWithAutoloads) { Map envBuilder = new LinkedHashMap<>(originalEnv); Map nativeBindings = convertNativeStructToMap((StarlarkInfo) envBuilder.remove("native")); @@ -266,6 +272,30 @@ private ImmutableMap modifyBuildBzlEnv( envBuilder.remove(symbol); } } + + if (!isWithAutoloads) { + // In the repositories that don't have autoloads we also expose native.legacy_symbols. + // Those can be used to fallback to the native symbol, whenever it's still available in Bazel. + // Fallback using a top-level symbol doesn't work, because BzlCompileFunction would throw an + // error when it's mentioned. + // legacy_symbols aren't available when autoloads are not enabled. The feature is intended to + // be use with bazel_features repository, which can correctly report native symbols on all + // versions of Bazel. + ImmutableMap legacySymbols = + envBuilder.entrySet().stream() + .filter(entry -> AUTOLOAD_CONFIG.containsKey(entry.getKey())) + .collect( + toImmutableMap( + e -> e.getKey(), + // Drop GuardedValue - it doesn't work on non-toplevel symbols + e -> + e.getValue() instanceof GuardedValue + ? ((GuardedValue) e.getValue()).getObject() + : e.getValue())); + nativeBindings.put( + "legacy_symbols", StructProvider.STRUCT.create(legacySymbols, "no native symbol '%s'")); + } + envBuilder.put( "native", StructProvider.STRUCT.create(nativeBindings, "no native function or rule '%s'")); return ImmutableMap.copyOf(envBuilder); @@ -485,7 +515,8 @@ private static SymbolRedirect renamedSymbolRedirect( "rules_sh", "apple_common", "bazel_skylib", - "bazel_tools"); + "bazel_tools", + "bazel_features"); private static final ImmutableMap AUTOLOAD_CONFIG = ImmutableMap.builder() diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index 575c3c503fe300..9265cfa4f6c3d8 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -65,6 +65,26 @@ local_path_override( EOF } + +function mock_rules_java() { + rules_java_workspace="${TEST_TMPDIR}/rules_java_workspace" + mkdir -p "${rules_java_workspace}/java" + touch "${rules_java_workspace}/java/BUILD" + touch "${rules_java_workspace}/WORKSPACE" + cat > "${rules_java_workspace}/MODULE.bazel" << EOF +module(name = "rules_java") +EOF + cat >> MODULE.bazel << EOF +bazel_dep( + name = "rules_java", +) +local_path_override( + module_name = "rules_java", + path = "${rules_java_workspace}", +) +EOF +} + function disabled_test_removed_rule_loaded() { setup_module_dot_bazel mock_rules_android @@ -419,5 +439,42 @@ EOF bazel query --incompatible_autoload_externally=+@rules_python ':py_library' --output=build >&$TEST_log 2>&1 || fail "build failed" } +function test_legacy_symbols() { + setup_module_dot_bazel + mock_rules_java + + rules_java_workspace="${TEST_TMPDIR}/rules_java_workspace" + + mkdir -p "${rules_java_workspace}/java/common" + touch "${rules_java_workspace}/java/common/BUILD" + cat > "${rules_java_workspace}/java/common/proguard_spec_info.bzl" << EOF +def _init(specs): + return {"specs": specs} + +def _proguard_spec_info(): + if hasattr(native, "legacy_symbols"): + if hasattr(native.legacy_symbols, "ProguardSpecProvider"): + print("Native provider") + return native.legacy_symbols.ProguardSpecProvider + print("Starlark provider") + return provider(fields = ["specs"], init = _init)[0] + +ProguardSpecInfo = _proguard_spec_info() +EOF + + cat > BUILD << EOF +load("@rules_java//java/common:proguard_spec_info.bzl", "ProguardSpecInfo") +EOF + + bazel build --incompatible_autoload_externally=+ProguardSpecProvider :all >&$TEST_log 2>&1 || fail "build unexpectedly failed" + expect_log "Native provider" + + + bazel build --incompatible_autoload_externally=ProguardSpecProvider,-java_lite_proto_library,-java_import :all >&$TEST_log 2>&1 || fail "build unexpectedly failed" + expect_log "Starlark provider" +} + + + run_suite "load_removed_symbols" From 70f10b5eb63f05e0048ec275764c029dd0276bfb Mon Sep 17 00:00:00 2001 From: Tim Peut Date: Fri, 27 Sep 2024 11:59:55 -0700 Subject: [PATCH 07/15] Remove autoloads for rules_android internals. We generally only expect people to be loading our top-level rules. At this stage all other providers, helpers etc are considered internal implementation details and we don't want to suggest otherwise. In the future this may change but for now just remove the autoloads. PiperOrigin-RevId: 679677200 Change-Id: I50fd2dbddb0a59c12d4e5444cf1832b995bf370e --- .../build/lib/packages/AutoloadSymbols.java | 86 +++---------------- 1 file changed, 12 insertions(+), 74 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 03a96471f4ac4d..7387e5f4ce1ae8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -498,10 +498,6 @@ private static SymbolRedirect renamedSymbolRedirect( label, false, newName, ImmutableSet.copyOf(rdeps)); } - private static final String[] androidRules = { - "aar_import", "android_binary", "android_library", "android_local_test", "android_sdk" - }; - private static final ImmutableSet PREDECLARED_REPOS_DISALLOWING_AUTOLOADS = ImmutableSet.of( "protobuf", @@ -607,76 +603,6 @@ private static SymbolRedirect renamedSymbolRedirect( "java_import", "android_binary", "android_library")) - .put("android_common", symbolRedirect("@rules_android//rules:common.bzl")) - .put( - "AndroidIdeInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put("ApkInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidInstrumentationInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidResourcesInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidNativeLibsInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidApplicationResourceInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidBinaryNativeLibsInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidSdkInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidManifestInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidAssetsInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidLibraryAarInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidProguardInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidIdlInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidPreDexJarInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidCcLinkParamsInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "DataBindingV2Info", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidLibraryResourceClassJarProvider", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidFeatureFlagSet", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "ProguardMappingInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidBinaryData", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "BaselineProfileProvider", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidNeverLinkLibrariesProvider", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidOptimizedJarInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidDexInfo", symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) - .put( - "AndroidOptimizationInfo", - symbolRedirect("@rules_android//rules:providers.bzl", androidRules)) .put( "PyInfo", symbolRedirect( @@ -696,6 +622,18 @@ private static SymbolRedirect renamedSymbolRedirect( "py_binary", "py_test", "py_library")) + // Note: AndroidIdeInfo is intended to be autoloaded for ASwBazel/IntelliJ migration + // purposes. It is not intended to be used by other teams and projects, and is effectively + // an internal implementation detail. + .put( + "AndroidIdeInfo", + symbolRedirect( + "@rules_android//providers:providers.bzl", + "aar_import", + "android_binary", + "android_library", + "android_local_test", + "android_sdk")) .put("aar_import", ruleRedirect("@rules_android//rules:rules.bzl")) .put("android_binary", ruleRedirect("@rules_android//rules:rules.bzl")) .put("android_library", ruleRedirect("@rules_android//rules:rules.bzl")) From ef87395ff56da27b4d30822368781100be586868 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 27 Sep 2024 14:46:48 -0700 Subject: [PATCH 08/15] Fix proto autoloads and legacy_globals With legacy_symbols I made a typo in 2 location: protobuf repo and in bazel_feature. At this point it's easier to rename it in Bazel. Tested manually on protobuf repository and this works with and without `--incompatible_autoload_externally=+@protobuf` PiperOrigin-RevId: 679735770 Change-Id: I5879060404deab9656acc045da3a6fb37c1d1e5f --- .../build/lib/packages/AutoloadSymbols.java | 29 ++++++++++++++----- .../integration/load_removed_symbols_test.sh | 8 ++--- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 7387e5f4ce1ae8..6b27cfb7d08bbd 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -269,16 +269,31 @@ private ImmutableMap modifyBuildBzlEnv( if (AUTOLOAD_CONFIG.get(symbol).isRule()) { nativeBindings.remove(symbol); } else { - envBuilder.remove(symbol); + if (symbol.equals("proto_common_do_not_use") + && envBuilder.get("proto_common_do_not_use") instanceof StarlarkInfo) { + // proto_common_do_not_use can't be completely removed, because the implementation of + // proto rules in protobuf still relies on INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION, + // that reads the build language flag. + envBuilder.put( + "proto_common_do_not_use", + StructProvider.STRUCT.create( + ImmutableMap.of( + "INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION", + ((StarlarkInfo) envBuilder.get("proto_common_do_not_use")) + .getValue("INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION")), + "no native symbol '%s'")); + } else { + envBuilder.remove(symbol); + } } } if (!isWithAutoloads) { - // In the repositories that don't have autoloads we also expose native.legacy_symbols. + // In the repositories that don't have autoloads we also expose native.legacy_globals. // Those can be used to fallback to the native symbol, whenever it's still available in Bazel. // Fallback using a top-level symbol doesn't work, because BzlCompileFunction would throw an // error when it's mentioned. - // legacy_symbols aren't available when autoloads are not enabled. The feature is intended to + // legacy_globals aren't available when autoloads are not enabled. The feature is intended to // be use with bazel_features repository, which can correctly report native symbols on all // versions of Bazel. ImmutableMap legacySymbols = @@ -293,7 +308,7 @@ private ImmutableMap modifyBuildBzlEnv( ? ((GuardedValue) e.getValue()).getObject() : e.getValue())); nativeBindings.put( - "legacy_symbols", StructProvider.STRUCT.create(legacySymbols, "no native symbol '%s'")); + "legacy_globals", StructProvider.STRUCT.create(legacySymbols, "no native symbol '%s'")); } envBuilder.put( @@ -525,7 +540,8 @@ private static SymbolRedirect renamedSymbolRedirect( symbolRedirect("@rules_cc//cc/common:cc_shared_library_hint_info.bzl", "cc_common")) .put( "cc_proto_aspect", - symbolRedirect("@protobuf//bazel/common:cc_proto_library.bzl", "cc_proto_library")) + symbolRedirect( + "@protobuf//bazel/private:bazel_cc_proto_library.bzl", "cc_proto_aspect")) .put( "ProtoInfo", symbolRedirect( @@ -537,7 +553,6 @@ private static SymbolRedirect renamedSymbolRedirect( "java_proto_library", "proto_lang_toolchain", "java_binary", - "py_extension", "proto_common_do_not_use")) .put( "proto_common_do_not_use", symbolRedirect("@protobuf//bazel/common:proto_common.bzl")) @@ -673,7 +688,7 @@ private static SymbolRedirect renamedSymbolRedirect( "propeller_optimize", ruleRedirect("@rules_cc//cc/toolchains:propeller_optimize.bzl")) .put( "proto_lang_toolchain", - ruleRedirect("@protobuf//bazel/toolchain:proto_lang_toolchain.bzl")) + ruleRedirect("@protobuf//bazel/toolchains:proto_lang_toolchain.bzl")) .put("proto_library", ruleRedirect("@protobuf//bazel:proto_library.bzl")) .put("py_binary", ruleRedirect("@rules_python//python:py_binary.bzl")) .put("py_library", ruleRedirect("@rules_python//python:py_library.bzl")) diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index 9265cfa4f6c3d8..f06acf9c4d9a35 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -439,7 +439,7 @@ EOF bazel query --incompatible_autoload_externally=+@rules_python ':py_library' --output=build >&$TEST_log 2>&1 || fail "build failed" } -function test_legacy_symbols() { +function test_legacy_globals() { setup_module_dot_bazel mock_rules_java @@ -452,10 +452,10 @@ def _init(specs): return {"specs": specs} def _proguard_spec_info(): - if hasattr(native, "legacy_symbols"): - if hasattr(native.legacy_symbols, "ProguardSpecProvider"): + if hasattr(native, "legacy_globals"): + if hasattr(native.legacy_globals, "ProguardSpecProvider"): print("Native provider") - return native.legacy_symbols.ProguardSpecProvider + return native.legacy_globals.ProguardSpecProvider print("Starlark provider") return provider(fields = ["specs"], init = _init)[0] From b0ce2c42d4cfa1327d048df174a0c03aaa648be3 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 2 Oct 2024 01:26:15 -0700 Subject: [PATCH 09/15] Use any dependency while autoloading symbols externally This changes from loading symbols as a dependency of bazel_tools to loading them as any dependency of any of the resolved modules. In case the dependency is missing a warning is reported and Bazel only fails when the actual symbol is needed but not available. PiperOrigin-RevId: 681347025 Change-Id: I4847a6a0420d2ae9e22d347dcd898ae9cdbdb0d6 --- .../build/lib/packages/AutoloadSymbols.java | 82 ++++++++++++++++--- .../google/devtools/build/lib/packages/BUILD | 23 +++++- .../google/devtools/build/lib/skyframe/BUILD | 2 + .../lib/skyframe/BzlCompileFunction.java | 2 +- .../build/lib/skyframe/BzlLoadFunction.java | 2 +- .../build/lib/skyframe/PackageFunction.java | 2 +- .../build/lib/skyframe/PrecomputedValue.java | 5 -- .../build/lib/skyframe/SkyframeExecutor.java | 2 +- .../skyframe/StarlarkBuiltinsFunction.java | 5 +- .../packages/AbstractPackageLoader.java | 2 +- .../build/lib/skyframe/packages/BUILD | 1 + .../devtools/build/lib/bazel/bzlmod/BUILD | 1 + .../bzlmod/ModuleExtensionResolutionTest.java | 2 +- .../devtools/build/lib/rules/repository/BUILD | 1 + .../repository/RepositoryDelegatorTest.java | 2 +- .../integration/load_removed_symbols_test.sh | 42 ++++++++-- 16 files changed, 140 insertions(+), 36 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 6b27cfb7d08bbd..4ac518232fb963 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -22,12 +22,17 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue; +import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.Label.RepoContext; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.skyframe.BzlLoadValue; +import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; import com.google.devtools.build.skyframe.SkyFunction; import java.util.HashSet; @@ -83,6 +88,10 @@ public class AutoloadSymbols { private final boolean bzlmodEnabled; private final boolean autoloadsEnabled; + // Configuration of --incompatible_load_externally + public static final Precomputed AUTOLOAD_SYMBOLS = + new Precomputed<>("autoload_symbols"); + public AutoloadSymbols(RuleClassProvider ruleClassProvider, StarlarkSemantics semantics) { ImmutableList symbolConfiguration = ImmutableList.copyOf(semantics.get(BuildLanguageOptions.INCOMPATIBLE_AUTOLOAD_EXTERNALLY)); @@ -378,23 +387,71 @@ private static Map convertNativeStructToMap(StarlarkInfo struct) @Nullable public ImmutableMap getLoadKeys(SkyFunction.Environment env) throws InterruptedException { - RepositoryMappingValue repositoryMappingValue = - (RepositoryMappingValue) - env.getValue(RepositoryMappingValue.key(RepositoryName.BAZEL_TOOLS)); - if (repositoryMappingValue == null) { - return null; - } + final RepoContext repoContext; + if (bzlmodEnabled) { + BazelDepGraphValue bazelDepGraphValue = + (BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY); + if (bazelDepGraphValue == null) { + return null; + } - RepoContext repoContext = - Label.RepoContext.of( - RepositoryName.BAZEL_TOOLS, repositoryMappingValue.getRepositoryMapping()); + ImmutableMap highestVersions = + bazelDepGraphValue.getCanonicalRepoNameLookup().values().stream() + .collect( + toImmutableMap( + ModuleKey::getName, + moduleKey -> moduleKey, + (m1, m2) -> m1.getVersion().compareTo(m2.getVersion()) >= 0 ? m1 : m1)); + RepositoryMapping repositoryMapping = + RepositoryMapping.create( + highestVersions.entrySet().stream() + .collect( + toImmutableMap( + Map.Entry::getKey, + entry -> + bazelDepGraphValue + .getCanonicalRepoNameLookup() + .inverse() + .get(entry.getValue()))), + RepositoryName.MAIN); + repoContext = Label.RepoContext.of(RepositoryName.MAIN, repositoryMapping); + } else { + RepositoryMappingValue repositoryMappingValue = + (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN)); + if (repositoryMappingValue == null) { + return null; + } + // Create with owner, so that we can report missing references (isVisible is false if missing) + repoContext = + Label.RepoContext.of( + RepositoryName.MAIN, + RepositoryMapping.create( + repositoryMappingValue.getRepositoryMapping().entries(), RepositoryName.MAIN)); + } // Inject loads for rules and symbols removed from Bazel ImmutableMap.Builder loadKeysBuilder = ImmutableMap.builderWithExpectedSize(autoloadedSymbols.size()); + ImmutableSet.Builder missingRepositories = ImmutableSet.builder(); for (String symbol : autoloadedSymbols) { - loadKeysBuilder.put(symbol, AUTOLOAD_CONFIG.get(symbol).getKey(repoContext)); + Label label = AUTOLOAD_CONFIG.get(symbol).getLabel(repoContext); + // Only load if the dependency is present + if (label.getRepository().isVisible()) { + loadKeysBuilder.put(symbol, BzlLoadValue.keyForBuild(label)); + } else { + missingRepositories.add(label.getRepository().getName()); + } + } + for (String missingRepository : missingRepositories.build()) { + env.getListener() + .handle( + Event.warn( + String.format( + "Couldn't auto load rules or symbols, because no dependency on" + + " module/repository '%s' found. This will result in a failure if" + + " there's a reference to those rules or symbols.", + missingRepository))); } return loadKeysBuilder.buildOrThrow(); } @@ -480,10 +537,9 @@ public abstract static class SymbolRedirect { public abstract ImmutableSet getRdeps(); - public BzlLoadValue.Key getKey(RepoContext bazelToolsRepoContext) throws InterruptedException { + Label getLabel(RepoContext repoContext) throws InterruptedException { try { - return BzlLoadValue.keyForBuild( - Label.parseWithRepoContext(getLoadLabel(), bazelToolsRepoContext)); + return Label.parseWithRepoContext(getLoadLabel(), repoContext); } catch (LabelSyntaxException e) { throw new IllegalStateException(e); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index 198511309cea06..808ea62ae5c4ac 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -54,6 +54,7 @@ java_library( "ConfiguredAttributeMapper.java", "LabelPrinter.java", "PackageSpecification.java", + "AutoloadSymbols.java", ], ), deps = [ @@ -87,7 +88,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions", - "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", @@ -119,6 +119,27 @@ java_library( ], ) +java_library( + name = "autoload_symbols", + srcs = ["AutoloadSymbols.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common", + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", + "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/java/net/starlark/java/eval", + "//third_party:auto_value", + "//third_party:guava", + "//third_party:jsr305", + ], +) + java_library( name = "bzl_visibility", srcs = ["BzlVisibility.java"], diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 15c420517c11ef..cd0b3f443efd5a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -298,6 +298,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols", "//src/main/java/com/google/devtools/build/lib/packages:bzl_visibility", "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/packages:globber_utils", @@ -773,6 +774,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java index 38c691fce1a27e..f5bc53498b3c17 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java @@ -161,7 +161,7 @@ static BzlCompileValue computeInline( // Takes into account --incompatible_autoload_externally, similarly to the comment above, this // only defines the correct set of symbols, but does not load them yet. - AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env); if (autoloadSymbols == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 587f57e4b25f13..bd175e2f8a57f4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -598,7 +598,7 @@ private StarlarkBuiltinsValue getBuiltins( } return StarlarkBuiltinsValue.createEmpty(starlarkSemantics); } - AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env); if (autoloadSymbols == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 5a011582d06cd5..d667eb2a7ba833 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -475,7 +475,7 @@ public SkyValue compute(SkyKey key, Environment env) StarlarkBuiltinsValue starlarkBuiltinsValue; try { // Bazel: we do autoloads for all BUILD files if enabled - AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env); if (autoloadSymbols == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java index e46e934b1f2141..9ebeaa31e08c96 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java @@ -19,7 +19,6 @@ import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.packages.AutoloadSymbols; import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -85,10 +84,6 @@ public static Injected injected(Precomputed precomputed, T value) { public static final Precomputed STARLARK_SEMANTICS = new Precomputed<>("starlark_semantics"); - // Configuration of --incompatible_load_externally - public static final Precomputed AUTOLOAD_SYMBOLS = - new Precomputed<>("autoload_symbols"); - static final Precomputed BUILD_ID = new Precomputed<>("build_id", /*shareable=*/ false); public static final Precomputed> ACTION_ENV = new Precomputed<>("action_env"); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 3f3ada15d3eab8..e6f8c5580dfe92 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -1263,7 +1263,7 @@ private void setStarlarkSemantics(StarlarkSemantics starlarkSemantics) { } private void setAutoloadsConfiguration(AutoloadSymbols autoloadSymbols) { - PrecomputedValue.AUTOLOAD_SYMBOLS.set(injectable(), autoloadSymbols); + AutoloadSymbols.AUTOLOAD_SYMBOLS.set(injectable(), autoloadSymbols); } public void setBaselineConfiguration(BuildOptions buildOptions) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java index 75550aff5c8292..c66afeaacdf966 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java @@ -164,7 +164,7 @@ private static StarlarkBuiltinsValue computeInternal( if (starlarkSemantics.get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH).isEmpty()) { return StarlarkBuiltinsValue.createEmpty(starlarkSemantics); } - AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env); + AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env); if (autoloadSymbols == null) { return null; } @@ -230,7 +230,6 @@ private static StarlarkBuiltinsValue computeInternal( } autoBzlLoadValues = autoBzlLoadValuesBuilder.buildOrThrow(); } catch (BzlLoadFailedException ex) { - throw BuiltinsFailedException.errorEvaluatingAutoloadedBzls( ex, starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)); } @@ -347,7 +346,7 @@ static BuiltinsFailedException errorEvaluatingAutoloadedBzls( String additionalMessage = bzlmodEnabled ? "" - : ". Most likely you need to upgrade the version of rules repository in the" + : " Most likely you need to upgrade the version of rules repository in the" + " WORKSPACE file."; return new BuiltinsFailedException( String.format( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index a5541a88e9201e..99980699d68508 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -344,7 +344,7 @@ public void inject(SkyKey key, Delta delta) { PrecomputedValue.CONFIG_SETTING_VISIBILITY_POLICY .set(injectable, ConfigSettingVisibilityPolicy.LEGACY_OFF); PrecomputedValue.STARLARK_SEMANTICS.set(injectable, starlarkSemantics); - PrecomputedValue.AUTOLOAD_SYMBOLS.set( + AutoloadSymbols.AUTOLOAD_SYMBOLS.set( injectable, new AutoloadSymbols(ruleClassProvider, starlarkSemantics)); return new ImmutableDiff(ImmutableList.of(), valuesToInject); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD index eb62abe0c3e701..7cc624fe16272d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD @@ -44,6 +44,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:file_symlink_cycle_uniqueness_function", "//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/repository:external_package_helper", "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_compile", diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index 45003206dea8b5..e4675de299c4db 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -52,6 +52,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 68dc2cf7f600c4..3c36c8e1e4ba51 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -285,7 +285,7 @@ public void setup() throws Exception { .setBool(BuildLanguageOptions.EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, true) .build(); PrecomputedValue.STARLARK_SEMANTICS.set(differencer, semantics); - PrecomputedValue.AUTOLOAD_SYMBOLS.set( + AutoloadSymbols.AUTOLOAD_SYMBOLS.set( differencer, new AutoloadSymbols(ruleClassProvider, semantics)); RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(differencer, ImmutableMap.of()); RepositoryDelegatorFunction.FORCE_FETCH.set( diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD b/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD index 261f0d2a8de2aa..3e77f30e248df1 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD @@ -31,6 +31,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index a068335e439244..3c5e67d6c7be1e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java @@ -286,7 +286,7 @@ public void setupDelegator() throws Exception { StarlarkSemantics semantics = StarlarkSemantics.builder().setBool(BuildLanguageOptions.ENABLE_BZLMOD, true).build(); PrecomputedValue.STARLARK_SEMANTICS.set(differencer, semantics); - PrecomputedValue.AUTOLOAD_SYMBOLS.set( + AutoloadSymbols.AUTOLOAD_SYMBOLS.set( differencer, new AutoloadSymbols(ruleClassProvider, semantics)); RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set( differencer, Optional.empty()); diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index f06acf9c4d9a35..ddcf3aae532bd8 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -27,6 +27,8 @@ set -e #### TESTS ############################################################# +add_to_bazelrc "common --allow_yanked_versions=zlib@1.2.12" + function setup_module_dot_bazel() { cat > "MODULE.bazel" < BUILD << EOF +aar_import( + name = 'aar', + aar = 'aar.file', + deps = [], +) +EOF + bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "name 'aar_import' is not defined" + expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_android' found. This will result in a failure if there's a reference to those rules or symbols." +} + +function test_missing_unnecessary_bzmod_dep() { + # Intentionally not adding rules_android to MODULE.bazel + cat > BUILD << EOF +filegroup( + name = 'filegroup', + srcs = [], +) +EOF + bazel build --incompatible_autoload_externally=aar_import :filegroup >&$TEST_log 2>&1 || fail "build failed" + expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_android' found. This will result in a failure if there's a reference to those rules or symbols." +} + +function test_removed_rule_loaded() { setup_module_dot_bazel mock_rules_android @@ -100,7 +128,7 @@ EOF # TODO(b/355260271): add test with workspace enabled } -function disabled_test_removed_rule_loaded_from_bzl() { +function test_removed_rule_loaded_from_bzl() { setup_module_dot_bazel mock_rules_android @@ -391,7 +419,7 @@ EOF expect_log "Duplicated symbol 'py_library' in --incompatible_autoload_externally" } -function disabled_test_missing_symbol_error() { +function test_missing_symbol_error() { setup_module_dot_bazel mock_rules_android rules_android_workspace="${TEST_TMPDIR}/rules_android_workspace" @@ -410,7 +438,7 @@ EOF expect_log "Failed to apply symbols loaded externally: The toplevel symbol 'aar_import' set by --incompatible_load_symbols_externally couldn't be loaded. 'aar_import' not found in auto loaded '@rules_android//rules:rules.bzl'." } -function disabled_test_missing_bzlfile_error() { +function test_missing_bzlfile_error() { setup_module_dot_bazel mock_rules_android rules_android_workspace="${TEST_TMPDIR}/rules_android_workspace" @@ -424,7 +452,7 @@ aar_import( ) EOF bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" - expect_log "Failed to autoload external symbols: cannot load '@@rules_android+//rules:rules.bzl': no such file" + expect_log "Failed to autoload external symbols: cannot load '@@rules_android.//rules:rules.bzl': no such file" } @@ -466,11 +494,11 @@ EOF load("@rules_java//java/common:proguard_spec_info.bzl", "ProguardSpecInfo") EOF - bazel build --incompatible_autoload_externally=+ProguardSpecProvider :all >&$TEST_log 2>&1 || fail "build unexpectedly failed" + bazel build --incompatible_autoload_externally=+ProguardSpecProvider,-android_binary,-android_library :all >&$TEST_log 2>&1 || fail "build unexpectedly failed" expect_log "Native provider" - bazel build --incompatible_autoload_externally=ProguardSpecProvider,-java_lite_proto_library,-java_import :all >&$TEST_log 2>&1 || fail "build unexpectedly failed" + bazel build --incompatible_autoload_externally=ProguardSpecProvider,-android_binary,-android_library,-java_lite_proto_library,-java_import :all >&$TEST_log 2>&1 || fail "build unexpectedly failed" expect_log "Starlark provider" } From 054f5833e56d76d966af4f32b5cde1be331930cb Mon Sep 17 00:00:00 2001 From: Googler Date: Sun, 6 Oct 2024 18:37:30 -0700 Subject: [PATCH 10/15] Don't autoload proto_common_do_not_use PiperOrigin-RevId: 682997914 Change-Id: I30f5f9e752d1c91e16c838639790e90f3559b0f5 --- .../devtools/build/lib/packages/AutoloadSymbols.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 4ac518232fb963..604d20a8be2f5a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -435,6 +435,11 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen ImmutableMap.builderWithExpectedSize(autoloadedSymbols.size()); ImmutableSet.Builder missingRepositories = ImmutableSet.builder(); for (String symbol : autoloadedSymbols) { + if (symbol.equals("proto_common_do_not_use")) { + // Special case that is not autoloaded, just removed + continue; + } + Label label = AUTOLOAD_CONFIG.get(symbol).getLabel(repoContext); // Only load if the dependency is present if (label.getRepository().isVisible()) { @@ -610,8 +615,7 @@ private static SymbolRedirect renamedSymbolRedirect( "proto_lang_toolchain", "java_binary", "proto_common_do_not_use")) - .put( - "proto_common_do_not_use", symbolRedirect("@protobuf//bazel/common:proto_common.bzl")) + .put("proto_common_do_not_use", symbolRedirect("")) .put("cc_common", symbolRedirect("@rules_cc//cc/common:cc_common.bzl")) .put( "CcInfo", From da1c25cbf7cf18ea833a1945331477ec2da8cea9 Mon Sep 17 00:00:00 2001 From: Googler Date: Sun, 6 Oct 2024 18:40:48 -0700 Subject: [PATCH 11/15] Autoload only when version has rules PiperOrigin-RevId: 682998564 Change-Id: I20316c27050c97a0040953b0fd5b163adf30851f --- .../build/lib/packages/AutoloadSymbols.java | 33 ++++++++++++++++++- .../integration/load_removed_symbols_test.sh | 11 +++---- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 604d20a8be2f5a..07920a085d5831 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -24,6 +24,8 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue; import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey; +import com.google.devtools.build.lib.bazel.bzlmod.Version; +import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.Label.RepoContext; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; @@ -389,6 +391,7 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen throws InterruptedException { final RepoContext repoContext; + ImmutableMap highestVersions = ImmutableMap.of(); if (bzlmodEnabled) { BazelDepGraphValue bazelDepGraphValue = (BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY); @@ -396,7 +399,7 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen return null; } - ImmutableMap highestVersions = + highestVersions = bazelDepGraphValue.getCanonicalRepoNameLookup().values().stream() .collect( toImmutableMap( @@ -440,6 +443,20 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen continue; } + String requiredModule = AUTOLOAD_CONFIG.get(symbol).getModuleName(); + // Skip if version doesn't have the rules + if (highestVersions.containsKey(requiredModule) + && requiredVersions.containsKey(requiredModule)) { + if (highestVersions + .get(requiredModule) + .getVersion() + .compareTo(requiredVersions.get(requiredModule)) + <= 0) { + missingRepositories.add(requiredModule); + continue; + } + } + Label label = AUTOLOAD_CONFIG.get(symbol).getLabel(repoContext); // Only load if the dependency is present if (label.getRepository().isVisible()) { @@ -542,6 +559,10 @@ public abstract static class SymbolRedirect { public abstract ImmutableSet getRdeps(); + String getModuleName() throws InterruptedException { + return Label.parseCanonicalUnchecked(getLoadLabel()).getRepository().getName(); + } + Label getLabel(RepoContext repoContext) throws InterruptedException { try { return Label.parseWithRepoContext(getLoadLabel(), repoContext); @@ -590,6 +611,16 @@ private static SymbolRedirect renamedSymbolRedirect( "bazel_tools", "bazel_features"); + private static final ImmutableMap requiredVersions; + + static { + try { + requiredVersions = ImmutableMap.of("protobuf", Version.parse("29.0-rc1")); + } catch (ParseException e) { + throw new IllegalStateException(e); + } + } + private static final ImmutableMap AUTOLOAD_CONFIG = ImmutableMap.builder() .put( diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index ddcf3aae532bd8..036d3df84f2cda 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -90,15 +90,14 @@ EOF function test_missing_necessary_bzlmod_dep() { # Intentionally not adding rules_android to MODULE.bazel cat > BUILD << EOF -aar_import( +sh_library( name = 'aar', aar = 'aar.file', deps = [], ) EOF - bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" - expect_log "name 'aar_import' is not defined" - expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_android' found. This will result in a failure if there's a reference to those rules or symbols." + bazel build --incompatible_autoload_externally=sh_library :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_sh' found. This will result in a failure if there's a reference to those rules or symbols." } function test_missing_unnecessary_bzmod_dep() { @@ -109,8 +108,8 @@ filegroup( srcs = [], ) EOF - bazel build --incompatible_autoload_externally=aar_import :filegroup >&$TEST_log 2>&1 || fail "build failed" - expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_android' found. This will result in a failure if there's a reference to those rules or symbols." + bazel build --incompatible_autoload_externally=sh_library :filegroup >&$TEST_log 2>&1 || fail "build failed" + expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_sh' found. This will result in a failure if there's a reference to those rules or symbols." } function test_removed_rule_loaded() { From 80c369522060e75e75f4798c7ed9de9c56c3c3cd Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 7 Oct 2024 07:38:29 -0700 Subject: [PATCH 12/15] Fix rules_sh autoloads to rules_shell PiperOrigin-RevId: 683179562 Change-Id: I7e0885e33b65dc5e24be705a676108c2aa3c8e9a --- .../devtools/build/lib/packages/AutoloadSymbols.java | 8 ++++---- src/test/shell/integration/load_removed_symbols_test.sh | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 07920a085d5831..df198ae350d459 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -605,7 +605,7 @@ private static SymbolRedirect renamedSymbolRedirect( "rules_java_builtin", "rules_python", "rules_python_internal", - "rules_sh", + "rules_shell", "apple_common", "bazel_skylib", "bazel_tools", @@ -785,9 +785,9 @@ private static SymbolRedirect renamedSymbolRedirect( .put("py_library", ruleRedirect("@rules_python//python:py_library.bzl")) .put("py_runtime", ruleRedirect("@rules_python//python:py_runtime.bzl")) .put("py_test", ruleRedirect("@rules_python//python:py_test.bzl")) - .put("sh_binary", ruleRedirect("@rules_sh//sh:sh_binary.bzl")) - .put("sh_library", ruleRedirect("@rules_sh//sh:sh_library.bzl")) - .put("sh_test", ruleRedirect("@rules_sh//sh:sh_test.bzl")) + .put("sh_binary", ruleRedirect("@rules_shell//shell:sh_binary.bzl")) + .put("sh_library", ruleRedirect("@rules_shell//shell:sh_library.bzl")) + .put("sh_test", ruleRedirect("@rules_shell//shell:sh_test.bzl")) .put("available_xcodes", ruleRedirect("@apple_support//xcode:available_xcodes.bzl")) .put("xcode_config", ruleRedirect("@apple_support//xcode:xcode_config.bzl")) .put("xcode_config_alias", ruleRedirect("@apple_support//xcode:xcode_config_alias.bzl")) diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index 036d3df84f2cda..1436e11b57be47 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -87,7 +87,8 @@ local_path_override( EOF } -function test_missing_necessary_bzlmod_dep() { +# TODO - ilist@: reeenable with a fake repository (we now have autoload all of them) +function disabled_test_missing_necessary_bzlmod_dep() { # Intentionally not adding rules_android to MODULE.bazel cat > BUILD << EOF sh_library( @@ -100,7 +101,8 @@ EOF expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_sh' found. This will result in a failure if there's a reference to those rules or symbols." } -function test_missing_unnecessary_bzmod_dep() { +# TODO - ilist@: reeenable with a fake repository (we now have autoload all of them) +function disabled_test_missing_unnecessary_bzmod_dep() { # Intentionally not adding rules_android to MODULE.bazel cat > BUILD << EOF filegroup( From ad69c6dbe41f99dd5e637591adc53ad441964007 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 7 Oct 2024 11:58:40 -0700 Subject: [PATCH 13/15] Enable autoloads for rules_android Downstream build: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/4164 Downstream build shows that rules_android isn't quite yet ready, but I think it's better to enable the flag for Bazel 8 now, than to do an incompatible change later. PiperOrigin-RevId: 683271234 Change-Id: I4b1a8c5fe087f8dbf0742440d6af3c436bbf26d2 --- .../google/devtools/build/lib/packages/AutoloadSymbols.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index df198ae350d459..1de7d085e0e0ef 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -615,7 +615,10 @@ private static SymbolRedirect renamedSymbolRedirect( static { try { - requiredVersions = ImmutableMap.of("protobuf", Version.parse("29.0-rc1")); + requiredVersions = + ImmutableMap.of( + "protobuf", Version.parse("29.0-rc1"), // + "rules_android", Version.parse("0.6.0-rc1")); } catch (ParseException e) { throw new IllegalStateException(e); } From 3f7682190a43a93c034a7da211d6a0b7485632a6 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 9 Oct 2024 09:39:03 +0200 Subject: [PATCH 14/15] Disable tests that need addtional Android setup in Bazel 7 --- src/test/shell/integration/load_removed_symbols_test.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index 1436e11b57be47..6a9b072d83fbdd 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -114,7 +114,7 @@ EOF expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_sh' found. This will result in a failure if there's a reference to those rules or symbols." } -function test_removed_rule_loaded() { +function disabled_test_removed_rule_loaded() { setup_module_dot_bazel mock_rules_android @@ -129,7 +129,7 @@ EOF # TODO(b/355260271): add test with workspace enabled } -function test_removed_rule_loaded_from_bzl() { +function disabled_test_removed_rule_loaded_from_bzl() { setup_module_dot_bazel mock_rules_android From 532d486bb8fef59c5448cc77d511f92717dec599 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 9 Oct 2024 09:50:59 +0200 Subject: [PATCH 15/15] Remove aar_import in the tests, because it's still present in Bazel7 --- src/test/shell/integration/load_removed_symbols_test.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/shell/integration/load_removed_symbols_test.sh b/src/test/shell/integration/load_removed_symbols_test.sh index 6a9b072d83fbdd..3dcaf55bbbfb9c 100755 --- a/src/test/shell/integration/load_removed_symbols_test.sh +++ b/src/test/shell/integration/load_removed_symbols_test.sh @@ -209,7 +209,7 @@ aar_import( ) EOF - bazel build --incompatible_autoload_externally= :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + bazel build --incompatible_autoload_externally=-aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" expect_log "name 'aar_import' is not defined" } @@ -231,7 +231,7 @@ load(":macro.bzl", "macro") macro() EOF - bazel build --incompatible_autoload_externally= :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" + bazel build --incompatible_autoload_externally=-aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded" expect_log "no native function or rule 'aar_import'" }