From 62dd72fd3c5e38da4c0d48aa44230eb98b7ea906 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 01:36:27 -0700 Subject: [PATCH 01/19] Fix so that the cherry-pick bot can be triggered on the PR/issue close. PiperOrigin-RevId: 557744827 Change-Id: I1ba859f176d214f0136fae78e0cb2b10649eaf15 --- .github/workflows/cherry-picker-on-close.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cherry-picker-on-close.yml b/.github/workflows/cherry-picker-on-close.yml index ac6474865b2885..56777ff4862eb7 100644 --- a/.github/workflows/cherry-picker-on-close.yml +++ b/.github/workflows/cherry-picker-on-close.yml @@ -1,7 +1,7 @@ name: cherry-picker-on-close on: - pull_request: + pull_request_target: types: [closed] env: From e6e79fc8dbc978477adac1daed266a8d0635580b Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 02:24:29 -0700 Subject: [PATCH 02/19] Add launcher_flag_alias The rule and target will support follow-up cl, that will simplfy launcher attribute on java rules. PiperOrigin-RevId: 557756133 Change-Id: Ib074f12cfd0c841cfa90ddd59994ab4c1ff214ff --- tools/jdk/BUILD | 1 + tools/jdk/BUILD.tools | 14 ++++++++--- tools/jdk/launcher_flag_alias.bzl | 40 +++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 tools/jdk/launcher_flag_alias.bzl diff --git a/tools/jdk/BUILD b/tools/jdk/BUILD index f1269cfe386bd4..0d021c607ac8cc 100644 --- a/tools/jdk/BUILD +++ b/tools/jdk/BUILD @@ -14,6 +14,7 @@ filegroup( "BUILD-jdk", # Tools are build from the workspace for tests. "default_java_toolchain.bzl", "java_toolchain_alias.bzl", + "launcher_flag_alias.bzl", "local_java_repository.bzl", "nosystemjdk/README", "proguard_whitelister.py", diff --git a/tools/jdk/BUILD.tools b/tools/jdk/BUILD.tools index 1ab0cb8e600a0a..a8af76e90c23f0 100644 --- a/tools/jdk/BUILD.tools +++ b/tools/jdk/BUILD.tools @@ -1,6 +1,7 @@ package(default_visibility = ["//visibility:public"]) load("@rules_python//python:defs.bzl", "py_binary", "py_test") +load(":launcher_flag_alias.bzl", "launcher_flag_alias") exports_files([ "BUILD.java_tools", @@ -83,7 +84,6 @@ alias( actual = "@remote_java_tools//:VanillaJavaBuilder", ) - alias( name = "JacocoCoverageRunner", actual = "@remote_java_tools//:jacoco_coverage_runner", @@ -167,9 +167,11 @@ TARGET_NAMES = [ "singlejar", "toolchain", ] + [ - "toolchain_java%d" % version for version in (8, 9, 10, 11) + "toolchain_java%d" % version + for version in (8, 9, 10, 11) ] + [ - "toolchain_jdk_%d" % version for version in (14, 15, 16, 17, 20) + "toolchain_jdk_%d" % version + for version in (14, 15, 16, 17, 20) ] [ @@ -181,3 +183,9 @@ TARGET_NAMES = [ ] #### Aliases to rules_java to keep backward-compatibility (end) #### + +# TODO(b/295221112): replace this with Starlark label_flag (or an alias resolving to either java_launcher or host_java_launcher flag) +launcher_flag_alias( + name = "launcher_flag_alias", + visibility = ["//visibility:public"], +) diff --git a/tools/jdk/launcher_flag_alias.bzl b/tools/jdk/launcher_flag_alias.bzl new file mode 100644 index 00000000000000..73c443ae5e4c78 --- /dev/null +++ b/tools/jdk/launcher_flag_alias.bzl @@ -0,0 +1,40 @@ +# Copyright 2023 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. + +"""Temporary implementation of a rule that aliases the value of --java_launcher flag""" + +_providers = ( + [CcInfo, getattr(cc_common, "launcher_provider")] if hasattr(cc_common, "launcher_provider") else [CcInfo] +) + +def _impl(ctx): + if not ctx.attr._launcher: + return None + launcher = ctx.attr._launcher + providers = [ctx.attr._launcher[p] for p in _providers] + providers.append(DefaultInfo(files = launcher[DefaultInfo].files, runfiles = launcher[DefaultInfo].default_runfiles)) + return providers + +launcher_flag_alias = rule( + implementation = _impl, + attrs = { + "_launcher": attr.label( + default = configuration_field( + fragment = "java", + name = "launcher", + ), + providers = _providers, + ), + }, +) From e2c4d3c86b582e23af16a965ebf4bbbab60403b9 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 04:22:16 -0700 Subject: [PATCH 03/19] Remove py_transitions implementation RELNOTES[INC]: py_transitions top-level was removed. PiperOrigin-RevId: 557780220 Change-Id: I0b803a33c758a0bced048f7d17d83c31c9ce34e6 --- .../bazel/rules/BazelRuleClassProvider.java | 3 +- .../rules/python/PyStarlarkTransitions.java | 81 ------------- .../build/lib/starlarkbuildapi/python/BUILD | 1 - .../starlarkbuildapi/python/PyBootstrap.java | 11 +- .../python/PyStarlarkTransitionsApi.java | 46 ------- .../devtools/build/lib/rules/python/BUILD | 15 --- .../python/PyStarlarkTransitionsTest.java | 114 ------------------ 7 files changed, 2 insertions(+), 269 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java delete mode 100644 src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyStarlarkTransitionsApi.java delete mode 100644 src/test/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitionsTest.java diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index b3b2ca83c9fe91..355106295997d1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -115,7 +115,6 @@ import com.google.devtools.build.lib.rules.proto.ProtoLangToolchainRule; import com.google.devtools.build.lib.rules.python.PyRuleClasses.PySymlink; import com.google.devtools.build.lib.rules.python.PyRuntimeRule; -import com.google.devtools.build.lib.rules.python.PyStarlarkTransitions; import com.google.devtools.build.lib.rules.python.PythonConfiguration; import com.google.devtools.build.lib.rules.repository.CoreWorkspaceRules; import com.google.devtools.build.lib.rules.repository.NewLocalRepositoryRule; @@ -479,7 +478,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) { ContextGuardedValue.onlyInAllowedRepos( Starlark.NONE, PyBootstrap.allowedRepositories)); builder.addStarlarkBuiltinsInternal(BazelPyBuiltins.NAME, new BazelPyBuiltins()); - builder.addStarlarkBootstrap(new PyBootstrap(PyStarlarkTransitions.INSTANCE)); + builder.addStarlarkBootstrap(new PyBootstrap()); builder.addSymlinkDefinition(PySymlink.PY2); builder.addSymlinkDefinition(PySymlink.PY3); diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java deleted file mode 100644 index e8996b90f62bb0..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java +++ /dev/null @@ -1,81 +0,0 @@ -// Copyright 2019 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.rules.python; - -import static com.google.devtools.build.lib.packages.Type.STRING; - -import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; -import com.google.devtools.build.lib.analysis.config.transitions.NoTransition; -import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory; -import com.google.devtools.build.lib.packages.AttributeTransitionData; -import com.google.devtools.build.lib.starlarkbuildapi.python.PyStarlarkTransitionsApi; -import net.starlark.java.eval.Printer; -import net.starlark.java.eval.StarlarkValue; - -/** - * Exposes a native transition to Starlark for changing the Python version. - * - *

This is not intended to be a stable API. It is only available when {@code - * --experimental_google_legacy_api} is enabled. For production code, users should generally use a - * Starlark-defined transition (SBC) based on {@code //command_line_option:python_version}. - * - *

The only reason to expose the native transition here is that it performs better; - * Starlark-based transitions cannot yet collapse equivalent configurations together due to - * differences in the output root. Most builds should not need this level of optimization. For the - * ones that do, this transition can unblock PY2 -> PY3 migration. - * - *

See also internal bugs b/137574156 and b/140565289. - */ -public final class PyStarlarkTransitions implements PyStarlarkTransitionsApi { - - /** Singleton instance of {@link PyStarlarkTransitions}. */ - public static final PyStarlarkTransitions INSTANCE = new PyStarlarkTransitions(); - - @Override - public StarlarkValue getTransition() { - return StarlarkVersionTransition.INSTANCE; - } - - private static class StarlarkVersionTransition - implements TransitionFactory, StarlarkValue { - - private static final StarlarkVersionTransition INSTANCE = new StarlarkVersionTransition(); - - @Override - public ConfigurationTransition create(AttributeTransitionData data) { - if (!data.attributes().has("python_version")) { - return NoTransition.INSTANCE; - } - String version = data.attributes().get("python_version", STRING); - if ("DEFAULT".equals(version)) { - return PythonVersionTransition.toDefault(); - } else if (PythonVersion.TARGET_STRINGS.contains(version)) { - return PythonVersionTransition.toConstant(PythonVersion.parseTargetValue(version)); - } else { - return NoTransition.INSTANCE; - } - } - - @Override - public TransitionType transitionType() { - return TransitionType.ATTRIBUTE; - } - - @Override - public void repr(Printer printer) { - printer.append(""); - } - } -} diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD index 8974820e08e42b..8b00ceb1701cff 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD @@ -29,7 +29,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/core", - "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/stubs", "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyBootstrap.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyBootstrap.java index 4e65a29071f3d7..baf7a6b4ec4927 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyBootstrap.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyBootstrap.java @@ -21,7 +21,6 @@ import com.google.devtools.build.lib.starlarkbuildapi.core.Bootstrap; import com.google.devtools.build.lib.starlarkbuildapi.core.ContextAndFlagGuardedValue; import com.google.devtools.build.lib.starlarkbuildapi.stubs.ProviderStub; -import net.starlark.java.eval.FlagGuardedValue; /** {@link Bootstrap} for Starlark objects related to the Python rules. */ public class PyBootstrap implements Bootstrap { @@ -32,11 +31,7 @@ public class PyBootstrap implements Bootstrap { PackageIdentifier.createUnchecked("rules_python", ""), PackageIdentifier.createUnchecked("", "tools/build_defs/python")); - private final PyStarlarkTransitionsApi pyStarlarkTransitionsApi; - - public PyBootstrap(PyStarlarkTransitionsApi pyStarlarkTransitionsApi) { - this.pyStarlarkTransitionsApi = pyStarlarkTransitionsApi; - } + public PyBootstrap() {} @Override public void addBindingsToBuilder(ImmutableMap.Builder builder) { @@ -55,10 +50,6 @@ public void addBindingsToBuilder(ImmutableMap.Builder builder) { new ProviderStub(), allowedRepositories)); - builder.put( - "py_transitions", - FlagGuardedValue.onlyWhenExperimentalFlagIsTrue( - BuildLanguageOptions.EXPERIMENTAL_GOOGLE_LEGACY_API, pyStarlarkTransitionsApi)); builder.put( "PyWrapCcInfo", ContextAndFlagGuardedValue.onlyInAllowedReposOrWhenIncompatibleFlagIsFalse( diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyStarlarkTransitionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyStarlarkTransitionsApi.java deleted file mode 100644 index 3619b8924eb0ee..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyStarlarkTransitionsApi.java +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2019 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.starlarkbuildapi.python; - -import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.annot.StarlarkMethod; -import net.starlark.java.eval.StarlarkValue; - -/** - * DO NOT USE. Skarlark module exposing Python transitions for Python 2 to 3 migration purposes - * only. - */ -@StarlarkBuiltin( - name = "py_transitions", - doc = - "DO NOT USE. This is intended for Python 2 to 3 migration purposes only. If you depend" - + " on it, you will be broken when it is removed.", - documented = false) -public interface PyStarlarkTransitionsApi extends StarlarkValue { - - @StarlarkMethod( - name = "cfg", - doc = - "DO NOT USE. This is intended for Python 2 to 3 migration purposes only. If you depend on" - + " it, you will be broken when it is removed. A configuration that transitions to" - + " the Python version specified by the 'python_version' attribute of the rule." - + " Valid versions are: PY2, PY3, and DEFAULT. If 'python_version' attribute is not" - + " available, or has an invalid value, it succeeds silently without transitions.", - documented = false, - structField = true, - enableOnlyWithFlag = BuildLanguageOptions.EXPERIMENTAL_GOOGLE_LEGACY_API) - public StarlarkValue getTransition(); -} diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD index 53b20d0d2ee32c..c907bc1bdd2270 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD @@ -203,21 +203,6 @@ java_test( ], ) -java_test( - name = "PyStarlarkTransitionsTest", - srcs = ["PyStarlarkTransitionsTest.java"], - deps = [ - "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", - "//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/rules/python", - "//src/test/java/com/google/devtools/build/lib/analysis/util", - "//third_party:guava", - "//third_party:junit4", - "//third_party:truth", - ], -) - java_test( name = "PythonStarlarkApiTest", srcs = ["PythonStarlarkApiTest.java"], diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitionsTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitionsTest.java deleted file mode 100644 index b0691eb10d37fb..00000000000000 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitionsTest.java +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright 2019 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.rules.python; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.packages.Provider; -import com.google.devtools.build.lib.packages.StarlarkProvider; -import com.google.devtools.build.lib.packages.StructImpl; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link PyStarlarkTransitions}. */ -@RunWith(JUnit4.class) -public final class PyStarlarkTransitionsTest extends BuildViewTestCase { - - @Before - public void setUp() throws Exception { - scratch.file("myinfo/myinfo.bzl", "MyInfo = provider()"); - scratch.file("myinfo/BUILD"); - - scratch.file( - "my_package/my_rule.bzl", - "load('//myinfo:myinfo.bzl', 'MyInfo')", - "def impl(ctx):", - " return MyInfo(", - " wrapped = ctx.attr.wrapped,", - " )", - "my_rule = rule(", - " implementation = impl,", - " attrs = {", - " 'wrapped': attr.label(cfg = py_transitions.cfg),", - " 'python_version': attr.string(),", - " },", - ")", - "missing_attr_rule = rule(", - " implementation = impl,", - " attrs = {", - " 'wrapped': attr.label(cfg = py_transitions.cfg),", - " },", - ")"); - - scratch.file( - "my_package/BUILD", - "load('//my_package:my_rule.bzl', 'my_rule', 'missing_attr_rule')", - "cc_binary(name = 'wrapped', srcs = ['wrapped.cc'])", - "my_rule(name = 'py2', wrapped = ':wrapped', python_version = 'PY2')", - "my_rule(name = 'py3', wrapped = ':wrapped', python_version = 'PY3')", - "my_rule(name = 'default', wrapped = ':wrapped', python_version = 'DEFAULT')", - "my_rule(name = 'invalid', wrapped = ':wrapped', python_version = 'invalid')", - "missing_attr_rule(name = 'missing_attr', wrapped = ':wrapped')"); - setBuildLanguageOptions("--experimental_google_legacy_api"); - } - - @Test - public void testTransitionToPY2() throws Exception { - useConfiguration("--python_version=PY3"); - verifyVersion("//my_package:py2", PythonVersion.PY2); - } - - @Test - public void testTransitionToPY3() throws Exception { - useConfiguration("--python_version=PY2"); - verifyVersion("//my_package:py3", PythonVersion.PY3); - } - - @Test - public void testTransitionToDefault() throws Exception { - useConfiguration("--python_version=PY2", "--incompatible_py3_is_default=true"); - verifyVersion("//my_package:default", PythonVersion.PY3); - } - - @Test - public void testNoPythonVersionAttribute() throws Exception { - useConfiguration("--python_version=PY3", "--incompatible_py3_is_default=false"); - // Make sure no errors occur for invalid values, and no transition is applied. - verifyVersion("//my_package:missing_attr", PythonVersion.PY3); - } - - @Test - public void testInvalidPythonVersionAttribute() throws Exception { - useConfiguration("--python_version=PY2", "--incompatible_py3_is_default=true"); - // Make sure no errors occur for missing python_version attribute, and no transition is applied. - verifyVersion("//my_package:invalid", PythonVersion.PY2); - } - - private void verifyVersion(String target, PythonVersion version) throws Exception { - ConfiguredTarget configuredTarget = getConfiguredTarget(target); - Provider.Key key = - new StarlarkProvider.Key(Label.parseCanonical("//myinfo:myinfo.bzl"), "MyInfo"); - StructImpl myInfo = (StructImpl) configuredTarget.get(key); - ConfiguredTarget wrapped = (ConfiguredTarget) myInfo.getValue("wrapped"); - PythonOptions wrappedPythonOptions = - getConfiguration(wrapped).getOptions().get(PythonOptions.class); - assertThat(wrappedPythonOptions.pythonVersion).isEqualTo(version); - } -} From 6d510e6b8c6a5b64f77294da1922e4994eb9fda5 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 06:55:50 -0700 Subject: [PATCH 04/19] Remove PyStarlarkTransitionsTest from PythonTests test_suite https://buildkite.com/bazel/bazel-bazel/builds/24521#018a033e-04e5-44e9-bde7-53244540be15 ``` (13:31:48) ERROR: /Users/buildkite/builds/bk-imacpro-1/bazel/bazel-bazel/src/test/java/com/google/devtools/build/lib/rules/python/BUILD:16:11: in test_suite rule '//src/test/java/com/google/devtools/build/lib/rules/python:PythonTests': expecting a test or a test_suite rule but ``` PiperOrigin-RevId: 557809745 Change-Id: Ie2b1843e531ec41add6d0a4be8b8b54b030305b6 --- src/test/java/com/google/devtools/build/lib/rules/python/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD index c907bc1bdd2270..43a3f39e161df4 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD @@ -21,7 +21,6 @@ test_suite( ":PyLibraryConfiguredTargetTest", ":PyRuntimeConfiguredTargetTest", ":PyRuntimeInfoTest", - ":PyStarlarkTransitionsTest", ":PyTestConfiguredTargetTest", ":PythonConfigurationTest", ":PythonStarlarkApiTest", From 2c2b07bb23e3b4028961fe2fd6538010436b171e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 17 Aug 2023 11:08:44 -0700 Subject: [PATCH 05/19] Do not rerun module extensions when only imports or locations change When using the lockfile, extension evaluation results are now persisted even if the imports (`use_repo`s) fail validation against the actually generated repos and reused if only imports or Starlark locations change. This requires storing `ModuleExtensionMetadata` in the lockfile to ensure that the correct fixup warnings are shown if imports are updated but the extension isn't rerun. Closes #19253. PiperOrigin-RevId: 557878432 Change-Id: I25909294b118fb445f9c48f61e18463762f78208 --- .../devtools/build/lib/bazel/bzlmod/BUILD | 24 ++- .../lib/bazel/bzlmod/BazelLockFileValue.java | 2 +- .../bazel/bzlmod/LockFileModuleExtension.java | 8 +- .../bazel/bzlmod/ModuleExtensionMetadata.java | 55 ++++--- .../bazel/bzlmod/ModuleExtensionUsage.java | 2 + .../bzlmod/SingleExtensionEvalFunction.java | 149 ++++++++++++++---- .../py/bazel/bzlmod/bazel_lockfile_test.py | 125 +++++++++++++++ 7 files changed, 304 insertions(+), 61 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index d643bdee4bdbf0..96cbe7e86a2c2a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -137,6 +137,7 @@ java_library( ":common", ":inspection", ":module_extension", + ":module_extension_metadata", ":registry", ":repo_rule_creator", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", @@ -168,7 +169,6 @@ java_library( "Discovery.java", "GsonTypeAdapterUtil.java", "ModuleExtensionContext.java", - "ModuleExtensionMetadata.java", "ModuleFileFunction.java", "ModuleFileGlobals.java", "Selection.java", @@ -182,6 +182,7 @@ java_library( ":common", ":exception", ":module_extension", + ":module_extension_metadata", ":registry", ":resolution", "//src/main/java/com/google/devtools/build/docgen/annot", @@ -282,3 +283,24 @@ java_library( "//third_party:jsr305", ], ) + +java_library( + name = "module_extension_metadata", + srcs = [ + "ModuleExtensionMetadata.java", + ], + deps = [ + ":common", + ":module_extension", + "//src/main/java/com/google/devtools/build/docgen/annot", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/net/starlark/java/annot", + "//src/main/java/net/starlark/java/eval", + "//src/main/java/net/starlark/java/syntax", + "//third_party:auto_value", + "//third_party:gson", + "//third_party:guava", + "//third_party:jsr305", + ], +) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index fc49eaeafde2ea..0f44205600d549 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -137,7 +137,7 @@ public ImmutableList getModuleExtensionDiff( extDiff.add("One or more files the extension '" + extensionId + "' is using have changed"); } if (!extensionUsages.equals(lockedExtensionUsages)) { - extDiff.add("The usages of the extension '" + extensionId + "' has changed"); + extDiff.add("The usages of the extension '" + extensionId + "' have changed"); } if (!envVariables.equals(lockedExtension.getEnvVariables())) { extDiff.add( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 1b5eb71d60c35b..9b111974694c19 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java @@ -19,6 +19,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; +import java.util.Optional; /** * This object serves as a container for the transitive digest (obtained from transitive .bzl files) @@ -33,7 +34,8 @@ public static Builder builder() { return new AutoValue_LockFileModuleExtension.Builder() // TODO(salmasamy) can be removed when updating lockfile version .setEnvVariables(ImmutableMap.of()) - .setAccumulatedFileDigests(ImmutableMap.of()); + .setAccumulatedFileDigests(ImmutableMap.of()) + .setModuleExtensionMetadata(Optional.empty()); } @SuppressWarnings("mutable") @@ -45,6 +47,8 @@ public static Builder builder() { public abstract ImmutableMap getGeneratedRepoSpecs(); + public abstract Optional getModuleExtensionMetadata(); + public abstract Builder toBuilder(); /** Builder type for {@link LockFileModuleExtension}. */ @@ -59,6 +63,8 @@ public abstract static class Builder { public abstract Builder setGeneratedRepoSpecs(ImmutableMap value); + public abstract Builder setModuleExtensionMetadata(Optional value); + public abstract LockFileModuleExtension build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index a161cf0cff2e59..443a71c86b69db 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -18,6 +18,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.stream.Collectors.joining; +import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; @@ -27,6 +28,7 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.ryanharter.auto.value.gson.GenerateTypeAdapter; import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashSet; @@ -49,24 +51,29 @@ doc = "Return values of this type from a module extension's implementation function to " + "provide metadata about the repositories generated by the extension to Bazel.") -public class ModuleExtensionMetadata implements StarlarkValue { - @Nullable private final ImmutableSet explicitRootModuleDirectDeps; - @Nullable private final ImmutableSet explicitRootModuleDirectDevDeps; - private final UseAllRepos useAllRepos; +@AutoValue +@GenerateTypeAdapter +public abstract class ModuleExtensionMetadata implements StarlarkValue { + @Nullable + abstract ImmutableSet getExplicitRootModuleDirectDeps(); - private ModuleExtensionMetadata( + @Nullable + abstract ImmutableSet getExplicitRootModuleDirectDevDeps(); + + abstract UseAllRepos getUseAllRepos(); + + private static ModuleExtensionMetadata create( @Nullable Set explicitRootModuleDirectDeps, @Nullable Set explicitRootModuleDirectDevDeps, UseAllRepos useAllRepos) { - this.explicitRootModuleDirectDeps = + return new AutoValue_ModuleExtensionMetadata( explicitRootModuleDirectDeps != null ? ImmutableSet.copyOf(explicitRootModuleDirectDeps) - : null; - this.explicitRootModuleDirectDevDeps = + : null, explicitRootModuleDirectDevDeps != null ? ImmutableSet.copyOf(explicitRootModuleDirectDevDeps) - : null; - this.useAllRepos = useAllRepos; + : null, + useAllRepos); } static ModuleExtensionMetadata create( @@ -76,19 +83,19 @@ static ModuleExtensionMetadata create( throws EvalException { if (rootModuleDirectDepsUnchecked == Starlark.NONE && rootModuleDirectDevDepsUnchecked == Starlark.NONE) { - return new ModuleExtensionMetadata(null, null, UseAllRepos.NO); + return create(null, null, UseAllRepos.NO); } // When root_module_direct_deps = "all", accept both root_module_direct_dev_deps = None and // root_module_direct_dev_deps = [], but not root_module_direct_dev_deps = ["some_repo"]. if (rootModuleDirectDepsUnchecked.equals("all") && rootModuleDirectDevDepsUnchecked.equals(StarlarkList.immutableOf())) { - return new ModuleExtensionMetadata(null, null, UseAllRepos.REGULAR); + return create(null, null, UseAllRepos.REGULAR); } if (rootModuleDirectDevDepsUnchecked.equals("all") && rootModuleDirectDepsUnchecked.equals(StarlarkList.immutableOf())) { - return new ModuleExtensionMetadata(null, null, UseAllRepos.DEV); + return create(null, null, UseAllRepos.DEV); } if (rootModuleDirectDepsUnchecked.equals("all") @@ -146,8 +153,7 @@ static ModuleExtensionMetadata create( } } - return new ModuleExtensionMetadata( - explicitRootModuleDirectDeps, explicitRootModuleDirectDevDeps, UseAllRepos.NO); + return create(explicitRootModuleDirectDeps, explicitRootModuleDirectDevDeps, UseAllRepos.NO); } public void evaluate( @@ -358,10 +364,10 @@ private static Optional makeUseRepoCommand( private Optional> getRootModuleDirectDeps(Set allRepos) throws EvalException { - switch (useAllRepos) { + switch (getUseAllRepos()) { case NO: - if (explicitRootModuleDirectDeps != null) { - Set invalidRepos = Sets.difference(explicitRootModuleDirectDeps, allRepos); + if (getExplicitRootModuleDirectDeps() != null) { + Set invalidRepos = Sets.difference(getExplicitRootModuleDirectDeps(), allRepos); if (!invalidRepos.isEmpty()) { throw Starlark.errorf( "root_module_direct_deps contained the following repositories " @@ -369,7 +375,7 @@ private Optional> getRootModuleDirectDeps(Set allRe String.join(", ", invalidRepos)); } } - return Optional.ofNullable(explicitRootModuleDirectDeps); + return Optional.ofNullable(getExplicitRootModuleDirectDeps()); case REGULAR: return Optional.of(ImmutableSet.copyOf(allRepos)); case DEV: @@ -380,10 +386,11 @@ private Optional> getRootModuleDirectDeps(Set allRe private Optional> getRootModuleDirectDevDeps(Set allRepos) throws EvalException { - switch (useAllRepos) { + switch (getUseAllRepos()) { case NO: - if (explicitRootModuleDirectDevDeps != null) { - Set invalidRepos = Sets.difference(explicitRootModuleDirectDevDeps, allRepos); + if (getExplicitRootModuleDirectDevDeps() != null) { + Set invalidRepos = + Sets.difference(getExplicitRootModuleDirectDevDeps(), allRepos); if (!invalidRepos.isEmpty()) { throw Starlark.errorf( "root_module_direct_dev_deps contained the following " @@ -391,7 +398,7 @@ private Optional> getRootModuleDirectDevDeps(Set al String.join(", ", invalidRepos)); } } - return Optional.ofNullable(explicitRootModuleDirectDevDeps); + return Optional.ofNullable(getExplicitRootModuleDirectDevDeps()); case REGULAR: return Optional.of(ImmutableSet.of()); case DEV: @@ -400,7 +407,7 @@ private Optional> getRootModuleDirectDevDeps(Set al throw new IllegalStateException("not reached"); } - private enum UseAllRepos { + enum UseAllRepos { NO, REGULAR, DEV, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index c84f87d62529f2..9d186432afef10 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -80,6 +80,8 @@ public abstract class ModuleExtensionUsage { */ public abstract boolean getHasNonDevUseExtension(); + public abstract Builder toBuilder(); + public static Builder builder() { return new AutoValue_ModuleExtensionUsage.Builder(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 9f2f9bb5add348..cd22a4cc792d1e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -19,10 +19,12 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableTable; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; @@ -57,6 +59,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.Optional; import java.util.function.Function; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -190,9 +193,14 @@ public SkyValue compute(SkyKey skyKey, Environment env) } ImmutableMap generatedRepoSpecs = moduleExtensionResult.getGeneratedRepoSpecs(); - // Check that all imported repos have been actually generated - validateAllImportsAreGenerated(generatedRepoSpecs, usagesValue, extensionId); - + Optional moduleExtensionMetadata = + moduleExtensionResult.getModuleExtensionMetadata(); + + // At this point the extension has been evaluated successfully, but SingleExtensionEvalFunction + // may still fail if imported repositories were not generated. However, since imports do not + // influence the evaluation of the extension and the validation also runs when the extension + // result is taken from the lockfile, we can already post the update event. This is necessary to + // prevent the extension from rerunning when only the imports change. if (lockfileMode.equals(LockfileMode.UPDATE)) { env.getListener() .post( @@ -203,9 +211,11 @@ public SkyValue compute(SkyKey skyKey, Environment env) .setAccumulatedFileDigests(moduleExtensionResult.getAccumulatedFileDigests()) .setEnvVariables(extensionEnvVars) .setGeneratedRepoSpecs(generatedRepoSpecs) + .setModuleExtensionMetadata(moduleExtensionMetadata) .build())); } - return createSingleExtentionValue(generatedRepoSpecs, usagesValue); + return validateAndCreateSingleExtensionEvalValue( + generatedRepoSpecs, moduleExtensionMetadata, extensionId, usagesValue, env); } @Nullable @@ -247,12 +257,22 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( return null; } - // Check extension data in lockfile still valid + // Check extension data in lockfile is still valid, disregarding usage information that is not + // relevant for the evaluation of the extension. + ImmutableMap trimmedLockedUsages = + trimUsagesForEvaluation(lockedExtensionUsages); + ImmutableMap trimmedUsages = + trimUsagesForEvaluation(usagesValue.getExtensionUsages()); if (!filesChanged && Arrays.equals(bzlTransitiveDigest, lockedExtension.getBzlTransitiveDigest()) - && usagesValue.getExtensionUsages().equals(lockedExtensionUsages) + && trimmedUsages.equals(trimmedLockedUsages) && envVariables.equals(lockedExtension.getEnvVariables())) { - return createSingleExtentionValue(lockedExtension.getGeneratedRepoSpecs(), usagesValue); + return validateAndCreateSingleExtensionEvalValue( + lockedExtension.getGeneratedRepoSpecs(), + lockedExtension.getModuleExtensionMetadata(), + extensionId, + usagesValue, + env); } else if (lockfileMode.equals(LockfileMode.ERROR)) { ImmutableList extDiff = lockfile.getModuleExtensionDiff( @@ -260,8 +280,8 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( bzlTransitiveDigest, filesChanged, envVariables, - usagesValue.getExtensionUsages(), - lockedExtensionUsages); + trimmedUsages, + trimmedLockedUsages); throw new SingleExtensionEvalFunctionException( ExternalDepsException.withMessage( Code.BAD_MODULE, @@ -309,24 +329,46 @@ private Boolean didFilesChange( return false; } - private SingleExtensionEvalValue createSingleExtentionValue( - ImmutableMap generatedRepoSpecs, SingleExtensionUsagesValue usagesValue) { - return SingleExtensionEvalValue.create( - generatedRepoSpecs, - generatedRepoSpecs.keySet().stream() - .collect( - toImmutableBiMap( - e -> - RepositoryName.createUnvalidated( - usagesValue.getExtensionUniqueName() + "~" + e), - Function.identity()))); - } - - private void validateAllImportsAreGenerated( + /** + * Validates the result of the module extension evaluation against the declared imports, throwing + * an exception if validation fails, and returns a SingleExtensionEvalValue otherwise. + * + *

Since extension evaluation does not depend on the declared imports, the result of the + * evaluation of the extension implementation function can be reused and persisted in the lockfile + * even if validation fails. + */ + private SingleExtensionEvalValue validateAndCreateSingleExtensionEvalValue( ImmutableMap generatedRepoSpecs, + Optional moduleExtensionMetadata, + ModuleExtensionId extensionId, SingleExtensionUsagesValue usagesValue, - ModuleExtensionId extensionId) + Environment env) throws SingleExtensionEvalFunctionException { + // Evaluate the metadata before failing on invalid imports so that fixup warning are still + // emitted in case of an error. + if (moduleExtensionMetadata.isPresent()) { + try { + // TODO: ModuleExtensionMetadata#evaluate should throw ExternalDepsException instead of + // EvalException. + moduleExtensionMetadata + .get() + .evaluate( + usagesValue.getExtensionUsages().values(), + generatedRepoSpecs.keySet(), + env.getListener()); + } catch (EvalException e) { + env.getListener().handle(Event.error(e.getMessageWithStack())); + throw new SingleExtensionEvalFunctionException( + ExternalDepsException.withMessage( + Code.BAD_MODULE, + "error evaluating module extension %s in %s", + extensionId.getExtensionName(), + extensionId.getBzlFileLabel()), + Transience.TRANSIENT); + } + } + + // Check that all imported repos have actually been generated. for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) { for (Entry repoImport : usage.getImports().entrySet()) { if (!generatedRepoSpecs.containsKey(repoImport.getValue())) { @@ -345,6 +387,43 @@ private void validateAllImportsAreGenerated( } } } + + return SingleExtensionEvalValue.create( + generatedRepoSpecs, + generatedRepoSpecs.keySet().stream() + .collect( + toImmutableBiMap( + e -> + RepositoryName.createUnvalidated( + usagesValue.getExtensionUniqueName() + "~" + e), + Function.identity()))); + } + + /** + * Returns usages with all information removed that does not influence the evaluation of the + * extension. + */ + private static ImmutableMap trimUsagesForEvaluation( + Map usages) { + return ImmutableMap.copyOf( + Maps.transformValues( + usages, + usage -> + // We start with the full usage and selectively remove information that does not + // influence the evaluation of the extension. Compared to explicitly copying over + // the parts that do, this preserves correctness in case new fields are added to + // ModuleExtensionUsage without updating this code. + usage.toBuilder() + // Locations are only used for error reporting and thus don't influence whether + // the evaluation of the extension is successful and what its result is + // in case of success. + .setLocation(Location.BUILTIN) + // Extension implementation functions do not see the imports, they are only + // validated against the set of generated repos in a validation step that comes + // afterward. + .setImports(ImmutableBiMap.of()) + .setDevImports(ImmutableSet.of()) + .build())); } private BzlLoadValue loadBzlFile( @@ -401,6 +480,7 @@ private RunModuleExtensionResult runModuleExtension( directories, env.getListener()); ModuleExtensionContext moduleContext; + Optional moduleExtensionMetadata; try (Mutability mu = Mutability.create("module extension", usagesValue.getExtensionUniqueName())) { StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics); @@ -422,11 +502,9 @@ private RunModuleExtensionResult runModuleExtension( Transience.PERSISTENT); } if (returnValue instanceof ModuleExtensionMetadata) { - ModuleExtensionMetadata metadata = (ModuleExtensionMetadata) returnValue; - metadata.evaluate( - usagesValue.getExtensionUsages().values(), - threadContext.getGeneratedRepoSpecs().keySet(), - env.getListener()); + moduleExtensionMetadata = Optional.of((ModuleExtensionMetadata) returnValue); + } else { + moduleExtensionMetadata = Optional.empty(); } } catch (NeedsSkyframeRestartException e) { // Clean up and restart by returning null. @@ -456,7 +534,9 @@ private RunModuleExtensionResult runModuleExtension( } } return RunModuleExtensionResult.create( - moduleContext.getAccumulatedFileDigests(), threadContext.getGeneratedRepoSpecs()); + moduleContext.getAccumulatedFileDigests(), + threadContext.getGeneratedRepoSpecs(), + moduleExtensionMetadata); } private ModuleExtensionContext createContext( @@ -517,13 +597,14 @@ abstract static class RunModuleExtensionResult { abstract ImmutableMap getGeneratedRepoSpecs(); + abstract Optional getModuleExtensionMetadata(); + static RunModuleExtensionResult create( ImmutableMap accumulatedFileDigests, - ImmutableMap generatedRepoSpecs) { + ImmutableMap generatedRepoSpecs, + Optional moduleExtensionMetadata) { return new AutoValue_SingleExtensionEvalFunction_RunModuleExtensionResult( - accumulatedFileDigests, generatedRepoSpecs); + accumulatedFileDigests, generatedRepoSpecs, moduleExtensionMetadata); } } } - - diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index b88881661a0c8d..102ad544f397a0 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -839,6 +839,131 @@ def testOldVersion(self): data = json.load(json_file) self.assertEqual(data['lockFileVersion'], 1) + def testExtensionEvaluationDoesNotRerunOnChangedImports(self): + """ + + """ + self.ScratchFile( + 'MODULE.bazel', + [ + 'lockfile_ext = use_extension("extension.bzl", "lockfile_ext")', + 'use_repo(lockfile_ext, "dep", "indirect_dep", "invalid_dep")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("WORKSPACE")', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + '', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _module_ext_impl(ctx):', + ' print("I am being evaluated")', + ' repo_rule(name="dep")', + ' repo_rule(name="missing_dep")', + ' repo_rule(name="indirect_dep")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=["dep", "missing_dep"],', + ' root_module_direct_dev_deps=[],', + ' )', + '', + 'lockfile_ext = module_extension(', + ' implementation=_module_ext_impl', + ')', + ], + ) + + # The build fails due to the "invalid_dep" import, which is not + # generated by the extension. + # Warnings should still be shown. + _, _, stderr = self.RunBazel(['build', '@dep//:all'], allow_failure=True) + stderr = '\n'.join(stderr) + self.assertIn('I am being evaluated', stderr) + self.assertIn( + 'Imported, but not created by the extension (will cause the build to' + ' fail):\ninvalid_dep', + stderr, + ) + self.assertIn( + 'Not imported, but reported as direct dependencies by the extension' + ' (may cause the build to fail):\nmissing_dep', + stderr, + ) + self.assertIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + self.assertIn( + 'ERROR: module extension "lockfile_ext" from "//:extension.bzl" does' + ' not generate repository "invalid_dep"', + stderr, + ) + + # Shut down bazel to empty cache and run with no changes to verify + # that the warnings are still shown. + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel(['build', '@dep//:all'], allow_failure=True) + stderr = '\n'.join(stderr) + self.assertNotIn('I am being evaluated', stderr) + self.assertIn( + 'Imported, but not created by the extension (will cause the build to' + ' fail):\ninvalid_dep', + stderr, + ) + self.assertIn( + 'Not imported, but reported as direct dependencies by the extension' + ' (may cause the build to fail):\nmissing_dep', + stderr, + ) + self.assertIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + self.assertIn( + 'ERROR: module extension "lockfile_ext" from "//:extension.bzl" does' + ' not generate repository "invalid_dep"', + stderr, + ) + + # Fix the imports, which should not trigger a rerun of the extension + # even though imports and locations changed. + self.ScratchFile( + 'MODULE.bazel', + [ + '# This is a comment that changes the location of the usage below.', + 'lockfile_ext = use_extension("extension.bzl", "lockfile_ext")', + 'use_repo(lockfile_ext, "dep", "missing_dep")', + ], + ) + _, _, stderr = self.RunBazel(['build', '@dep//:all']) + stderr = '\n'.join(stderr) + self.assertNotIn('I am being evaluated', stderr) + self.assertNotIn( + 'Not imported, but reported as direct dependencies by the extension' + ' (may cause the build to fail):\nmissing_dep', + stderr, + ) + self.assertNotIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + self.assertNotIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + self.assertNotIn( + 'ERROR: module extension "lockfile_ext" from "//:extension.bzl" does' + ' not generate repository "invalid_dep"', + stderr, + ) + if __name__ == '__main__': unittest.main() From 0a74086f7b7df7c231633bd3afa9d63fb1b231f1 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Thu, 17 Aug 2023 11:39:02 -0700 Subject: [PATCH 06/19] Allow CppLinkAction to inspect input sizes in resource estimation This PR instrumets the CppLinkAction to print log lines for every link operation and dumps the number of inputs, the aggregate size of the inputs, the predicted memory usage, and the actual consumed memory. Calculating the aggregate size of the inputs is a costly operation because we need to expand a nested set and stat all of its files, which explains why this PR is complex. I've modified the local memory estimator to compute the inputs size exactly once per link as we need this information both to predict memory consumption _and_ to emit the log line after the spawn completes. This is meant to aid in assessing the local resources prediction for linking with the goal of adjusting the model, but that should be done in a separate change. Helps address #17368. Closes #19203. PiperOrigin-RevId: 557888150 Change-Id: Icbfc94b4761497ce78a79baccc56ea15c5ca0053 --- .../google/devtools/build/lib/rules/cpp/BUILD | 3 +- .../build/lib/rules/cpp/CppLinkAction.java | 158 +++++++++++++++--- .../google/devtools/build/lib/rules/cpp/BUILD | 1 + .../lib/rules/cpp/CppLinkActionTest.java | 52 +++++- 4 files changed, 185 insertions(+), 29 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD index c89a74acaea0e4..76f9bf6aedbdbd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -38,6 +38,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/actions:middleman_type", "//src/main/java/com/google/devtools/build/lib/actions:runfiles_supplier", @@ -74,7 +75,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:starlark/args", "//src/main/java/com/google/devtools/build/lib/analysis:template_variable_info", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection", - "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider", "//src/main/java/com/google/devtools/build/lib/analysis:workspace_status_action", "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations", @@ -123,6 +123,7 @@ java_library( "//src/main/protobuf:failure_details_java_proto", "//third_party:auto_value", "//third_party:caffeine", + "//third_party:flogger", "//third_party:guava", "//third_party:jsr305", "//third_party/protobuf:protobuf_java", diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 9c4a34372341d4..2217a16da6d339 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; +import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -38,8 +39,10 @@ import com.google.devtools.build.lib.actions.CommandLines.CommandLineAndParamFileInfo; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionRequirements; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.SimpleSpawn; +import com.google.devtools.build.lib.actions.SimpleSpawn.LocalResourcesSupplier; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.extra.CppLinkInfo; @@ -62,6 +65,7 @@ import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.ShellEscaper; import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -74,6 +78,8 @@ @ThreadCompatible public final class CppLinkAction extends AbstractAction implements CommandAction { + private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + /** * An abstraction for creating intermediate and output artifacts for C++ linking. * @@ -328,19 +334,34 @@ ImmutableCollection getLinkstampObjectFileInputs() { @Override public ActionResult execute(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { - Spawn spawn = createSpawn(actionExecutionContext); + LocalResourcesEstimator localResourcesEstimator = + new LocalResourcesEstimator( + actionExecutionContext, OS.getCurrent(), linkCommandLine.getLinkerInputArtifacts()); + + Spawn spawn = createSpawn(actionExecutionContext, localResourcesEstimator); try { - ImmutableList spawnResult = + ImmutableList spawnResults = actionExecutionContext .getContext(SpawnStrategyResolver.class) .exec(spawn, actionExecutionContext); - return ActionResult.create(spawnResult); + + // As per the documentation of SpawnStrategy.exec(), the first entry is always the result of + // the successful execution + SpawnResult result = spawnResults.get(0); + Long consumedMemoryInKb = result.getMemoryInKb(); + if (consumedMemoryInKb != null) { + localResourcesEstimator.logMetrics(consumedMemoryInKb); + } + + return ActionResult.create(spawnResults); } catch (ExecException e) { throw ActionExecutionException.fromExecException(e, CppLinkAction.this); } } - private Spawn createSpawn(ActionExecutionContext actionExecutionContext) + private Spawn createSpawn( + ActionExecutionContext actionExecutionContext, + LocalResourcesEstimator localResourcesEstimator) throws ActionExecutionException { try { ArtifactExpander actionContextExpander = actionExecutionContext.getArtifactExpander(); @@ -352,10 +373,7 @@ private Spawn createSpawn(ActionExecutionContext actionExecutionContext) getExecutionInfo(), getInputs(), getOutputs(), - () -> - estimateResourceConsumptionLocal( - OS.getCurrent(), - linkCommandLine.getLinkerInputArtifacts().memoizedFlattenAndGetSize())); + localResourcesEstimator); } catch (CommandLineExpansionException e) { String message = String.format( @@ -466,20 +484,118 @@ protected String getRawProgressMessage() { } /** - * Estimates resource consumption when this action is executed locally. During investigation we - * found linear dependency between used memory by action and number of inputs. For memory - * estimation we are using form C + K * inputs, where C and K selected in such way, that more than - * 95% of actions used less than C + K * inputs MB of memory during execution. + * Estimates resource consumption when this action is executed locally. + * + *

Because resource estimation for linking can be very costly (we need to inspect the size of + * the inputs, which means we have to expand a nested set and stat its files), we memoize those + * metrics. We do this to enable logging details about these computations after the linker has + * run, without having to re-do them. */ - static ResourceSet estimateResourceConsumptionLocal(OS os, int inputs) { - switch (os) { - case DARWIN: - return ResourceSet.createWithRamCpu(/* memoryMb= */ 15 + 0.05 * inputs, /* cpuUsage= */ 1); - case LINUX: - return ResourceSet.createWithRamCpu( - /* memoryMb= */ Math.max(50, -100 + 0.1 * inputs), /* cpuUsage= */ 1); - default: - return ResourceSet.createWithRamCpu(/* memoryMb= */ 1500 + inputs, /* cpuUsage= */ 1); + @VisibleForTesting + static class LocalResourcesEstimator implements LocalResourcesSupplier { + private final ActionExecutionContext actionExecutionContext; + private final OS os; + private final NestedSet inputs; + + /** Container for all lazily-initialized details. */ + private static class LazyData { + private final int inputsCount; + private final long inputsBytes; + private final ResourceSet resourceSet; + + LazyData(int inputsCount, long inputsBytes, ResourceSet resourceSet) { + this.inputsCount = inputsCount; + this.inputsBytes = inputsBytes; + this.resourceSet = resourceSet; + } + } + + private LazyData lazyData = null; + + public LocalResourcesEstimator( + ActionExecutionContext actionExecutionContext, OS os, NestedSet inputs) { + this.actionExecutionContext = actionExecutionContext; + this.os = os; + this.inputs = inputs; + } + + /** Performs costly computations required to predict linker resources consumption. */ + private LazyData doCostlyEstimation() { + int inputsCount = 0; + long inputsBytes = 0; + for (Artifact input : inputs.toList()) { + inputsCount += 1; + try { + FileArtifactValue value = + actionExecutionContext.getInputMetadataProvider().getInputMetadata(input); + if (value != null) { + inputsBytes += value.getSize(); + } else { + throw new IOException("no metadata"); + } + } catch (IOException e) { + // TODO(https://github.com/bazelbuild/bazel/issues/17368): Propagate as ExecException when + // input sizes are used in the model. + logger.atWarning().log( + "Linker metrics: failed to get size of %s: %s (ignored)", input.getExecPath(), e); + } + } + + // TODO(https://github.com/bazelbuild/bazel/issues/17368): Use inputBytes in the computations. + ResourceSet resourceSet; + switch (os) { + case DARWIN: + resourceSet = + ResourceSet.createWithRamCpu( + /* memoryMb= */ 15 + 0.05 * inputsCount, /* cpuUsage= */ 1); + break; + case LINUX: + resourceSet = + ResourceSet.createWithRamCpu( + /* memoryMb= */ Math.max(50, -100 + 0.1 * inputsCount), /* cpuUsage= */ 1); + break; + default: + resourceSet = + ResourceSet.createWithRamCpu(/* memoryMb= */ 1500 + inputsCount, /* cpuUsage= */ 1); + break; + } + + return new LazyData(inputsCount, inputsBytes, resourceSet); + } + + @Override + public ResourceSet get() throws ExecException { + if (lazyData == null) { + lazyData = doCostlyEstimation(); + } + return lazyData.resourceSet; + } + + /** + * Emits a log entry with the linker resource prediction statistics. + * + *

This should only be called for spawns where we have actual linker consumption metrics so + * that we can compare those to the prediction. In practice, this means that this can only be + * called for locally-executed linkers. + * + * @param actualMemoryKb memory consumed by the linker in KB + */ + public void logMetrics(long actualMemoryKb) { + if (lazyData == null) { + // We should never have to do this here because, today, the memory consumption numbers in + // actualMemoryKb are only available for locally-executed linkers, which means that get() + // has been called beforehand. But this does not necessarily have to be the case: we can + // imagine a remote spawn runner that does return consumed resources, at which point we + // would get here but get() would not have been called. + lazyData = doCostlyEstimation(); + } + + logger.atInfo().log( + "Linker metrics: inputs_count=%d,inputs_mb=%.2f,estimated_mb=%.2f,consumed_mb=%.2f", + lazyData.inputsCount, + ((double) lazyData.inputsBytes) / 1024 / 1024, + lazyData.resourceSet.getMemoryMb(), + ((double) actualMemoryKb) / 1024); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD index 24decb4fd165da..ad2deffdfa6701 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -451,6 +451,7 @@ java_test( "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:guava", "//third_party:junit4", + "//third_party:mockito", "//third_party:truth", ], ) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index f643cf60805784..968141da3efa36 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java @@ -16,6 +16,8 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.base.Joiner; import com.google.common.base.Preconditions; @@ -25,12 +27,14 @@ import com.google.common.primitives.Ints; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; +import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; +import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -41,6 +45,7 @@ import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; @@ -49,6 +54,7 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.VariableValue; import com.google.devtools.build.lib.rules.cpp.CppActionConfigs.CppPlatform; +import com.google.devtools.build.lib.rules.cpp.CppLinkAction.LocalResourcesEstimator; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; @@ -770,24 +776,56 @@ public void testCommandLineSplittingWithArchiveParamFileFeature_shouldBeOnForArc assertThat(builder.canSplitCommandLine()).isTrue(); } + private static NestedSet createInputs(RuleContext ruleContext, int count) { + NestedSetBuilder builder = new NestedSetBuilder<>(Order.LINK_ORDER); + for (int i = 0; i < count; i++) { + Artifact artifact = + ActionsTestUtil.createArtifact( + ruleContext.getBinDirectory(), String.format("input-%d", i)); + builder.add(artifact); + } + return builder.build(); + } + + private ResourceSet estimateResourceConsumptionLocal( + RuleContext ruleContext, OS os, int inputsCount) throws Exception { + InputMetadataProvider inputMetadataProvider = mock(InputMetadataProvider.class); + + ActionExecutionContext actionExecutionContext = mock(ActionExecutionContext.class); + when(actionExecutionContext.getInputMetadataProvider()).thenReturn(inputMetadataProvider); + + NestedSet inputs = createInputs(ruleContext, inputsCount); + try { + LocalResourcesEstimator estimator = + new LocalResourcesEstimator(actionExecutionContext, os, inputs); + return estimator.get(); + } finally { + for (Artifact input : inputs.toList()) { + input.getPath().delete(); + } + } + } + @Test - public void tesLocalLinkResourceEstimate() { - assertThat(CppLinkAction.estimateResourceConsumptionLocal(OS.DARWIN, 100)) + public void testLocalLinkResourceEstimate() throws Exception { + RuleContext ruleContext = createDummyRuleContext(); + + assertThat(estimateResourceConsumptionLocal(ruleContext, OS.DARWIN, 100)) .isEqualTo(ResourceSet.createWithRamCpu(20, 1)); - assertThat(CppLinkAction.estimateResourceConsumptionLocal(OS.DARWIN, 1000)) + assertThat(estimateResourceConsumptionLocal(ruleContext, OS.DARWIN, 1000)) .isEqualTo(ResourceSet.createWithRamCpu(65, 1)); - assertThat(CppLinkAction.estimateResourceConsumptionLocal(OS.LINUX, 100)) + assertThat(estimateResourceConsumptionLocal(ruleContext, OS.LINUX, 100)) .isEqualTo(ResourceSet.createWithRamCpu(50, 1)); - assertThat(CppLinkAction.estimateResourceConsumptionLocal(OS.LINUX, 10000)) + assertThat(estimateResourceConsumptionLocal(ruleContext, OS.LINUX, 10000)) .isEqualTo(ResourceSet.createWithRamCpu(900, 1)); - assertThat(CppLinkAction.estimateResourceConsumptionLocal(OS.WINDOWS, 0)) + assertThat(estimateResourceConsumptionLocal(ruleContext, OS.WINDOWS, 0)) .isEqualTo(ResourceSet.createWithRamCpu(1500, 1)); - assertThat(CppLinkAction.estimateResourceConsumptionLocal(OS.WINDOWS, 1000)) + assertThat(estimateResourceConsumptionLocal(ruleContext, OS.WINDOWS, 1000)) .isEqualTo(ResourceSet.createWithRamCpu(2500, 1)); } From 253e9862112e7ab6bdbe5d0b3fadabb8e544315d Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 11:56:13 -0700 Subject: [PATCH 07/19] Set the default value of `--android_fixed_resource_neverlinking` to true so that Android resources will properly not propagate through neverlinked libraries. RELNOTES: Android resources will no longer propagate through neverlinked libraries by default. PiperOrigin-RevId: 557893161 Change-Id: I48438c638e9cc5380c95d06a0b94738a14d471a5 --- .../devtools/build/lib/rules/android/AndroidConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java index eb077f896cc925..04664e95abd9e1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java @@ -746,7 +746,7 @@ public static class Options extends FragmentOptions { @Option( name = "android_fixed_resource_neverlinking", - defaultValue = "false", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, help = From 9092e29f1bd0fe8a94bcee18fe108a9ca2dab075 Mon Sep 17 00:00:00 2001 From: "arun.sampathkumar" Date: Thu, 17 Aug 2023 12:43:44 -0700 Subject: [PATCH 08/19] Implement worker support for `GenerateDataBindingBaseClasses` - Moved `GenerateDataBindingBaseClasses` to `ResourceProcessorBusyBox` since Android databinding classes were already a dependency there. - `ProcessDatabinding` is already part of `ResourceProcessorBusyBox` but it was not enabled in `persistent_android_resource_processor` expansion flag. Added that as well. This change is multiplex compatible and probably could use expansion flags as done here https://github.com/bazelbuild/bazel/pull/15950. Results: In a synthetic generated project with 30 modules here https://github.com/arunkumar9t2/bazel-playground, the worker can be turned off/on with `.bazel.sh build //android/databinding_workers:android_data_binding --strategy=GenerateDataBindingBaseClasses=[sandboxed|worker]` and worker is about 9 to 10% faster. Closes #16067. PiperOrigin-RevId: 557906961 Change-Id: I085d8a8e92d1882834ad90db31767ab9c7d2e5cf --- .../rules/android/AndroidConfiguration.java | 2 + .../rules/android/BusyBoxActionBuilder.java | 18 ++ .../databinding/DataBindingV2Context.java | 54 ++---- .../GenerateDatabindingBaseClassesAction.java | 154 ++++++++++++++++++ .../android/ResourceProcessorBusyBox.java | 6 + 5 files changed, 198 insertions(+), 36 deletions(-) create mode 100644 src/tools/android/java/com/google/devtools/build/android/GenerateDatabindingBaseClassesAction.java diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java index 04664e95abd9e1..896e59e5b6fd98 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java @@ -851,6 +851,8 @@ public static class Options extends FragmentOptions { "--strategy=AndroidManifestMerger=worker", "--strategy=Aapt2Optimize=worker", "--strategy=AARGenerator=worker", + "--strategy=ProcessDatabinding=worker", + "--strategy=GenerateDataBindingBaseClasses=worker" }) public Void persistentResourceProcessor; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java index 24fcfe93190265..d990a3267beafe 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java @@ -246,6 +246,24 @@ public BusyBoxActionBuilder addTransitiveFlagForEach( return this; } + /** + * Adds an efficient flag based on transitive artifact exec paths. + * + *

The exec paths of each transitive artifact will be proceeded by the flag, for example: + * --flag execpath1 --flag execpath2 + * + *

The values will only be collapsed and turned into a flag at execution time. + * + *

The values will also be added as inputs. + */ + @CanIgnoreReturnValue + public BusyBoxActionBuilder addTransitiveExecPathsFlagForEachAndInputs( + @CompileTimeConstant String arg, NestedSet transitiveValues) { + commandLine.addExecPaths(VectorArg.addBefore(arg).each(transitiveValues)); + inputs.addTransitive(transitiveValues); + return this; + } + /** * Adds an efficient flag and inputs based on transitive values. * diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/databinding/DataBindingV2Context.java b/src/main/java/com/google/devtools/build/lib/rules/android/databinding/DataBindingV2Context.java index fc04d721b6fe15..a2aecc09a78b95 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/databinding/DataBindingV2Context.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/databinding/DataBindingV2Context.java @@ -22,13 +22,9 @@ import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; -import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; -import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg; -import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.packages.BuildType; @@ -38,6 +34,7 @@ import com.google.devtools.build.lib.rules.android.AndroidDataBindingProcessorBuilder; import com.google.devtools.build.lib.rules.android.AndroidDataContext; import com.google.devtools.build.lib.rules.android.AndroidResources; +import com.google.devtools.build.lib.rules.android.BusyBoxActionBuilder; import com.google.devtools.build.lib.rules.java.JavaPluginInfo; import com.google.devtools.build.lib.starlarkbuildapi.android.DataBindingV2ProviderApi; import com.google.devtools.build.lib.starlarkbuildapi.android.DataBindingV2ProviderApi.LabelJavaPackagePair; @@ -290,44 +287,29 @@ public ImmutableList getAnnotationSourceFiles(RuleContext ruleContext) } private ImmutableList createBaseClasses(RuleContext ruleContext) { - if (!AndroidResources.definesAndroidResources(ruleContext.attributes())) { return ImmutableList.of(); // no resource, no base classes or class info } - Artifact layoutInfo = getLayoutInfoFile(); - Artifact classInfoFile = getClassInfoFile(ruleContext); - Artifact srcOutFile = + final Artifact layoutInfo = getLayoutInfoFile(); + final Artifact classInfoFile = getClassInfoFile(ruleContext); + final Artifact srcOutFile = DataBinding.getDataBindingArtifact( ruleContext, "baseClassSrc.srcjar", /* isDirectory= */ false); - - FilesToRunProvider exec = - ruleContext.getExecutablePrerequisite(DataBinding.DATABINDING_EXEC_PROCESSOR_ATTR); - - CustomCommandLine.Builder commandLineBuilder = - CustomCommandLine.builder() - .add("GEN_BASE_CLASSES") - .addExecPath("-layoutInfoFiles", layoutInfo) - .add("-package", AndroidCommon.getJavaPackage(ruleContext)) - .addExecPath("-classInfoOut", classInfoFile) - .addExecPath("-sourceOut", srcOutFile) - .add("-zipSourceOutput", "true") - .add("-useAndroidX", useAndroidX ? "true" : "false"); - - NestedSet dependencyClassInfo = getDirectClassInfo(ruleContext); - commandLineBuilder.addExecPaths( - VectorArg.addBefore("-dependencyClassInfoList").each(dependencyClassInfo)); - - ruleContext.registerAction( - new SpawnAction.Builder() - .setExecutable(exec) - .setMnemonic("GenerateDataBindingBaseClasses") - .addInput(layoutInfo) - .addTransitiveInputs(dependencyClassInfo) - .addOutput(classInfoFile) - .addOutput(srcOutFile) - .addCommandLine(commandLineBuilder.build()) - .build(ruleContext)); + final NestedSet dependencyClassInfo = getDirectClassInfo(ruleContext); + + final BusyBoxActionBuilder builder = + BusyBoxActionBuilder.create(AndroidDataContext.forNative(ruleContext), "GEN_BASE_CLASSES") + .addInput("--layoutInfoFiles", layoutInfo) + .addFlag("--package", AndroidCommon.getJavaPackage(ruleContext)) + .addOutput("--classInfoOut", classInfoFile) + .addOutput("--sourceOut", srcOutFile) + .addFlag("--useDataBindingAndroidX", useAndroidX ? "true" : "false") + .addTransitiveExecPathsFlagForEachAndInputs( + "--dependencyClassInfoList", dependencyClassInfo); + + builder.buildAndRegister( + "Generating databinding base classes", "GenerateDataBindingBaseClasses"); return ImmutableList.of(srcOutFile); } diff --git a/src/tools/android/java/com/google/devtools/build/android/GenerateDatabindingBaseClassesAction.java b/src/tools/android/java/com/google/devtools/build/android/GenerateDatabindingBaseClassesAction.java new file mode 100644 index 00000000000000..3955a9946b3eab --- /dev/null +++ b/src/tools/android/java/com/google/devtools/build/android/GenerateDatabindingBaseClassesAction.java @@ -0,0 +1,154 @@ +// Copyright 2018 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.android; + +import android.databinding.AndroidDataBinding; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Streams; +import com.google.devtools.build.android.AndroidResourceProcessor.AaptConfigOptions; +import com.google.devtools.build.android.Converters.PathConverter; +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionsBase; +import com.google.devtools.common.options.OptionsParser; +import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor; +import java.nio.file.FileSystems; +import java.nio.file.Path; +import java.util.List; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** Generates databinding base classes for an Android target. */ +public final class GenerateDatabindingBaseClassesAction { + + /** Options for GenerateDatabindingBaseClassesAction. */ + public static final class Options extends OptionsBase { + @Option( + name = "layoutInfoFiles", + defaultValue = "null", + converter = PathConverter.class, + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Path to layout-info.zip file produced by databinding processor") + public Path layoutInfoFile; + + @Option( + name = "package", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "Package name of the android target") + public String packageName; + + @Option( + name = "classInfoOut", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + defaultValue = "null", + converter = PathConverter.class, + category = "output", + help = "Path to write classInfo.zip file") + public Path classInfoOut; + + @Option( + name = "sourceOut", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + defaultValue = "null", + converter = PathConverter.class, + category = "output", + help = "Path to write databinding base classes to be used in Java compilation") + public Path sourceOut; + + @Option( + name = "dependencyClassInfoList", + defaultValue = "null", + converter = PathConverter.class, + allowMultiple = true, + category = "input", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "List of dependency class info zip files") + public List dependencyClassInfoList; + } + + static final Logger logger = + Logger.getLogger(GenerateDatabindingBaseClassesAction.class.getName()); + + public static void main(String[] args) throws Exception { + final OptionsParser optionsParser = + OptionsParser.builder() + .allowResidue(true) + .optionsClasses( + Options.class, AaptConfigOptions.class, ResourceProcessorCommonOptions.class) + .argsPreProcessor(new ShellQuotedParamsFilePreProcessor(FileSystems.getDefault())) + .build(); + optionsParser.parseAndExitUponError(args); + final Options options = optionsParser.getOptions(Options.class); + final AaptConfigOptions aaptConfigOptions = optionsParser.getOptions(AaptConfigOptions.class); + + if (options.layoutInfoFile == null) { + throw new IllegalArgumentException("--layoutInfoFiles is required"); + } + + if (options.packageName == null) { + throw new IllegalArgumentException("--packageName is required"); + } + + if (options.classInfoOut == null) { + throw new IllegalArgumentException("--classInfoOut is required"); + } + + if (options.sourceOut == null) { + throw new IllegalArgumentException("--sourceOut is required"); + } + + final List dependencyClassInfoList = + options.dependencyClassInfoList == null + ? ImmutableList.of() + : options.dependencyClassInfoList; + + final ImmutableList.Builder dbArgsBuilder = + ImmutableList.builder() + .add("GEN_BASE_CLASSES") + .add("-layoutInfoFiles") + .add(options.layoutInfoFile.toString()) + .add("-package", options.packageName) + .add("-classInfoOut") + .add(options.classInfoOut.toString()) + .add("-sourceOut") + .add(options.sourceOut.toString()) + .add("-zipSourceOutput") + .add("true") + .add("-useAndroidX") + .add(Boolean.toString(aaptConfigOptions.useDataBindingAndroidX)); + + dependencyClassInfoList.forEach( + classInfo -> dbArgsBuilder.add("-dependencyClassInfoList").add(classInfo.toString())); + + try { + AndroidDataBinding.main( + Streams.mapWithIndex( + dbArgsBuilder.build().stream(), (arg, index) -> index == 0 ? arg : arg + " ") + .toArray(String[]::new)); + } catch (RuntimeException e) { + logger.log(Level.SEVERE, "Unexpected", e); + throw e; + } + } + + private GenerateDatabindingBaseClassesAction() {} +} diff --git a/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java b/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java index 58e3b48b3df996..b3f5c42d114b19 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java +++ b/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java @@ -138,6 +138,12 @@ void call(String[] args) throws Exception { void call(String[] args) throws Exception { AndroidDataBindingProcessingAction.main(args); } + }, + GEN_BASE_CLASSES { + @Override + void call(String[] args) throws Exception { + GenerateDatabindingBaseClassesAction.main(args); + } }; abstract void call(String[] args) throws Exception; From 3abbf20f691877d328cbf1b455ba2002e6ac10cd Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 17 Aug 2023 13:16:32 -0700 Subject: [PATCH 09/19] Use `debugPrint` instead of `str` for `fail` arguments This allows non-hermetic information (such as Starlark `Location`s) to be included in `fail` messages without allowing access to it from Starlark. It also aligns the content of `fail` output with that of `print`. This change has very mild implications for backwards compatibility: it only affects the contents of `fail` messages, which aren't accessible from Starlark; `debugPrint`'s default implementation is `str` and no built-in Starlark type overrides it; if a type overrides `debugPrint`, it would usually contain at least as detailed information as `str`; `debugPrint` is a "Bazelism" that isn't governed by the Starlark spec. Work towards #17375 Closes #18818. Co-authored-by: Alexandre Rostovtsev PiperOrigin-RevId: 557916312 Change-Id: I8f202cd8530bcebb2d99f57745289b3992d03cac --- .../net/starlark/java/eval/MethodLibrary.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/main/java/net/starlark/java/eval/MethodLibrary.java b/src/main/java/net/starlark/java/eval/MethodLibrary.java index 38cecba0e081b7..36990a8586c53b 100644 --- a/src/main/java/net/starlark/java/eval/MethodLibrary.java +++ b/src/main/java/net/starlark/java/eval/MethodLibrary.java @@ -15,13 +15,10 @@ package net.starlark.java.eval; import com.google.common.base.Ascii; -import com.google.common.base.Joiner; import com.google.common.collect.Ordering; -import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; import java.util.Iterator; -import java.util.List; import java.util.NoSuchElementException; import net.starlark.java.annot.Param; import net.starlark.java.annot.ParamType; @@ -706,24 +703,33 @@ public StarlarkList dir(Object object, StarlarkThread thread) throws EvalExce @Param( name = "args", doc = - "A list of values, formatted with str and joined with spaces, that appear in the" - + " error message."), + "A list of values, formatted with debugPrint (which is equivalent to str by" + + " default) and joined with spaces, that appear in the error message."), useStarlarkThread = true) public void fail(Object msg, Object attr, Tuple args, StarlarkThread thread) throws EvalException { - List elems = new ArrayList<>(); + Printer printer = new Printer(); + boolean needSeparator = false; + if (attr != Starlark.NONE) { + printer.append("attribute ").append((String) attr).append(":"); + needSeparator = true; + } // msg acts like a leading element of args. if (msg != Starlark.NONE) { - elems.add(Starlark.str(msg, thread.getSemantics())); + if (needSeparator) { + printer.append(" "); + } + printer.debugPrint(msg, thread.getSemantics()); + needSeparator = true; } for (Object arg : args) { - elems.add(Starlark.str(arg, thread.getSemantics())); - } - String str = Joiner.on(" ").join(elems); - if (attr != Starlark.NONE) { - str = String.format("attribute %s: %s", attr, str); + if (needSeparator) { + printer.append(" "); + } + printer.debugPrint(arg, thread.getSemantics()); + needSeparator = true; } - throw Starlark.errorf("%s", str); + throw Starlark.errorf("%s", printer.toString()); } @StarlarkMethod( From 74bc04653cd8635f19a887bd1171d2e8566166c1 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 13:39:43 -0700 Subject: [PATCH 10/19] support persistent worker for aar extractors This PR supports persistent worker for Android aar extractors, it'll significantly improve the aar extractors performance especially in the first time compilation without cache. It's not enable by default, can use flag `build --experimental_persistent_aar_extractor=true` to enable it. using worker: ![image](https://github.com/bazelbuild/bazel/assets/10076965/804f8167-4a52-4321-ba4a-9d1f1dd57644) not using worker: ![image](https://github.com/bazelbuild/bazel/assets/10076965/26ef4047-4ff7-45f3-9767-ce48418aa089) Closes #18496. PiperOrigin-RevId: 557923249 Change-Id: Ica6b6e0a6e3f1dcb68bb9aba941d538888ca013d --- .../build/lib/rules/android/AarImport.java | 95 ++++++++++++++----- .../rules/android/AndroidConfiguration.java | 18 ++++ .../android/AndroidConfigurationApi.java | 7 ++ .../bazel/android/aar_integration_test.sh | 34 +++++++ tools/android/BUILD | 11 +++ tools/android/BUILD.tools | 11 +++ tools/android/aar_embedded_jars_extractor.py | 5 +- .../aar_embedded_proguard_extractor.py | 5 +- tools/android/aar_native_libs_zip_creator.py | 4 +- tools/android/aar_resources_extractor.py | 5 +- tools/android/json_worker_wrapper.py | 59 ++++++++++++ 11 files changed, 219 insertions(+), 35 deletions(-) create mode 100644 tools/android/json_worker_wrapper.py diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java index c160a981aa9fe7..4f302f60cfa23b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java @@ -14,10 +14,14 @@ package com.google.devtools.build.lib.rules.android; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; +import com.google.devtools.build.lib.actions.ParamFileInfo; +import com.google.devtools.build.lib.actions.ParameterFile; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; @@ -49,6 +53,7 @@ import com.google.devtools.build.lib.rules.java.ProguardSpecProvider; import com.google.devtools.build.lib.starlarkbuildapi.android.DataBindingV2ProviderApi; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.List; import javax.annotation.Nullable; /** @@ -65,6 +70,9 @@ public class AarImport implements RuleConfiguredTargetFactory { private static final String MERGED_JAR = "classes_and_libs_merged.jar"; private static final String PROGUARD_SPEC = "proguard.txt"; + private static final ParamFileInfo WORKERS_FORCED_PARAM_FILE_INFO = + ParamFileInfo.builder(ParameterFile.ParameterFileType.UNQUOTED).setUseAlways(true).build(); + private final JavaSemantics javaSemantics; private final AndroidSemantics androidSemantics; @@ -272,13 +280,41 @@ private NestedSet extractProguardSpecs(RuleContext ruleContext, Artifa return builder.addTransitive(proguardSpecs).add(proguardSpecArtifact).build(); } + private static boolean isPersistentAarExtractor(RuleContext ruleContext) { + AndroidConfiguration androidConfig = + ruleContext.getConfiguration().getFragment(AndroidConfiguration.class); + return androidConfig.persistentAarExtractor(); + } + + /** + * Returns either the worker param file or null depending on whether persistent worker mode is on. + */ + @Nullable + private static ParamFileInfo getParamFileInfo(RuleContext ruleContext) { + return isPersistentAarExtractor(ruleContext) ? WORKERS_FORCED_PARAM_FILE_INFO : null; + } + + /** Adds worker-related execution requirements if worker mode enabled. */ + private static void modifyExecutionInfo( + RuleContext ruleContext, SpawnAction.Builder actionBuilder) { + if (isPersistentAarExtractor(ruleContext)) { + ImmutableMap.Builder executionInfo = ImmutableMap.builder(); + executionInfo.putAll(ExecutionRequirements.WORKER_MODE_ENABLED); + executionInfo.put(ExecutionRequirements.REQUIRES_WORKER_PROTOCOL, "json"); + actionBuilder.setExecutionInfo(executionInfo.buildOrThrow()); + } + } + /** * Creates action to extract embedded Proguard.txt from an AAR. If the file is not found, an empty * file will be created */ private static SpawnAction createAarEmbeddedProguardExtractorActions( RuleContext ruleContext, Artifact aar, Artifact proguardSpecArtifact) { - return new SpawnAction.Builder() + SpawnAction.Builder actionBuilder = new SpawnAction.Builder(); + ParamFileInfo paramFileInfo = getParamFileInfo(ruleContext); + modifyExecutionInfo(ruleContext, actionBuilder); + return actionBuilder .useDefaultShellEnvironment() .setExecutable( ruleContext.getExecutablePrerequisite(AarImportBaseRule.AAR_EMBEDDED_PROGUARD_EXTACTOR)) @@ -290,7 +326,8 @@ private static SpawnAction createAarEmbeddedProguardExtractorActions( CustomCommandLine.builder() .addExecPath("--input_aar", aar) .addExecPath("--output_proguard_file", proguardSpecArtifact) - .build()) + .build(), + paramFileInfo) .build(ruleContext); } @@ -352,7 +389,10 @@ private static SpawnAction createAarResourcesExtractorActions( Artifact databindingBrFiles, Artifact databindingSetterStoreFiles) { - return new SpawnAction.Builder() + SpawnAction.Builder actionBuilder = new SpawnAction.Builder(); + ParamFileInfo paramFileInfo = getParamFileInfo(ruleContext); + modifyExecutionInfo(ruleContext, actionBuilder); + return actionBuilder .useDefaultShellEnvironment() .setExecutable( ruleContext.getExecutablePrerequisite(AarImportBaseRule.AAR_RESOURCES_EXTRACTOR)) @@ -370,7 +410,8 @@ private static SpawnAction createAarResourcesExtractorActions( .addExecPath("--output_assets_dir", assetsDir) .addExecPath("--output_databinding_br_dir", databindingBrFiles) .addExecPath("--output_databinding_setter_store_dir", databindingSetterStoreFiles) - .build()) + .build(), + paramFileInfo) .build(ruleContext); } @@ -379,7 +420,10 @@ private static SpawnAction createAarEmbeddedJarsExtractorActions( Artifact aar, Artifact jarsTreeArtifact, Artifact singleJarParamFile) { - return new SpawnAction.Builder() + SpawnAction.Builder actionBuilder = new SpawnAction.Builder(); + ParamFileInfo paramFileInfo = getParamFileInfo(ruleContext); + modifyExecutionInfo(ruleContext, actionBuilder); + return actionBuilder .useDefaultShellEnvironment() .setExecutable( ruleContext.getExecutablePrerequisite(AarImportBaseRule.AAR_EMBEDDED_JARS_EXTACTOR)) @@ -393,7 +437,8 @@ private static SpawnAction createAarEmbeddedJarsExtractorActions( .addExecPath("--input_aar", aar) .addExecPath("--output_dir", jarsTreeArtifact) .addExecPath("--output_singlejar_param_file", singleJarParamFile) - .build()) + .build(), + paramFileInfo) .build(ruleContext); } @@ -420,23 +465,25 @@ private static SpawnAction createAarJarsMergingActions( private static SpawnAction createAarNativeLibsFilterActions( RuleContext ruleContext, Artifact aar, Artifact outputZip) { - SpawnAction.Builder actionBuilder = - new SpawnAction.Builder() - .useDefaultShellEnvironment() - .setExecutable( - ruleContext.getExecutablePrerequisite( - AarImportBaseRule.AAR_NATIVE_LIBS_ZIP_CREATOR)) - .setMnemonic("AarNativeLibsFilter") - .setProgressMessage("Filtering AAR native libs by architecture for %{label}") - .addInput(aar) - .addOutput(outputZip) - .addCommandLine( - CustomCommandLine.builder() - .addExecPath("--input_aar", aar) - .add("--cpu", ruleContext.getConfiguration().getCpu()) - .addExecPath("--output_zip", outputZip) - .build()); - return actionBuilder.build(ruleContext); + SpawnAction.Builder actionBuilder = new SpawnAction.Builder(); + ParamFileInfo paramFileInfo = getParamFileInfo(ruleContext); + modifyExecutionInfo(ruleContext, actionBuilder); + return actionBuilder + .useDefaultShellEnvironment() + .setExecutable( + ruleContext.getExecutablePrerequisite(AarImportBaseRule.AAR_NATIVE_LIBS_ZIP_CREATOR)) + .setMnemonic("AarNativeLibsFilter") + .setProgressMessage("Filtering AAR native libs by architecture for %{label}") + .addInput(aar) + .addOutput(outputZip) + .addCommandLine( + CustomCommandLine.builder() + .addExecPath("--input_aar", aar) + .add("--cpu", ruleContext.getConfiguration().getCpu()) + .addExecPath("--output_zip", outputZip) + .build(), + paramFileInfo) + .build(ruleContext); } private static DataBindingV2Provider createDatabindingProvider( @@ -444,7 +491,7 @@ private static DataBindingV2Provider createDatabindingProvider( SpecialArtifact databindingBrFiles, SpecialArtifact databindingSetterStoreFiles) { - Iterable> databindingProvidersFromDeps = + List> databindingProvidersFromDeps = ruleContext.getPrerequisites("deps", DataBindingV2Provider.PROVIDER); Iterable> databindingProvidersFromExports = diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java index 896e59e5b6fd98..e8a1d3cbf8fdfd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java @@ -825,6 +825,16 @@ public static class Options extends FragmentOptions { + "transitive classpath, otherwise it uses the deploy jar") public boolean oneVersionEnforcementUseTransitiveJarsForBinaryUnderTest; + @Option( + name = "experimental_persistent_aar_extractor", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = { + OptionEffectTag.EXECUTION, + }, + help = "Enable persistent aar extractor by using workers.") + public boolean persistentAarExtractor; + @Option( name = "persistent_android_resource_processor", defaultValue = "null", @@ -1132,6 +1142,7 @@ public FragmentOptions getExec() { exec.manifestMergerOrder = manifestMergerOrder; exec.oneVersionEnforcementUseTransitiveJarsForBinaryUnderTest = oneVersionEnforcementUseTransitiveJarsForBinaryUnderTest; + exec.persistentAarExtractor = persistentAarExtractor; exec.persistentBusyboxTools = persistentBusyboxTools; exec.persistentMultiplexBusyboxTools = persistentMultiplexBusyboxTools; exec.disableNativeAndroidRules = disableNativeAndroidRules; @@ -1177,6 +1188,7 @@ public FragmentOptions getExec() { private final boolean dataBindingV2; private final boolean dataBindingUpdatedArgs; private final boolean dataBindingAndroidX; + private final boolean persistentAarExtractor; private final boolean persistentBusyboxTools; private final boolean persistentMultiplexBusyboxTools; private final boolean persistentDexDesugar; @@ -1239,6 +1251,7 @@ public AndroidConfiguration(BuildOptions buildOptions) throws InvalidConfigurati this.dataBindingV2 = options.dataBindingV2; this.dataBindingUpdatedArgs = options.dataBindingUpdatedArgs; this.dataBindingAndroidX = options.dataBindingAndroidX; + this.persistentAarExtractor = options.persistentAarExtractor; this.persistentBusyboxTools = options.persistentBusyboxTools; this.persistentMultiplexBusyboxTools = options.persistentMultiplexBusyboxTools; this.persistentDexDesugar = options.persistentDexDesugar; @@ -1481,6 +1494,11 @@ public boolean useDataBindingAndroidX() { return dataBindingAndroidX; } + @Override + public boolean persistentAarExtractor() { + return persistentAarExtractor; + } + @Override public boolean persistentBusyboxTools() { return persistentBusyboxTools; diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java index 159822b31f51bc..be9248a01ec421 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java @@ -231,6 +231,13 @@ public interface AndroidConfigurationApi extends StarlarkValue { documented = false) boolean useDataBindingAndroidX(); + @StarlarkMethod( + name = "persistent_aar_extractor", + structField = true, + doc = "", + documented = false) + boolean persistentAarExtractor(); + @StarlarkMethod( name = "persistent_busybox_tools", structField = true, diff --git a/src/test/shell/bazel/android/aar_integration_test.sh b/src/test/shell/bazel/android/aar_integration_test.sh index 283e1311e58740..31025bb9fd022e 100755 --- a/src/test/shell/bazel/android/aar_integration_test.sh +++ b/src/test/shell/bazel/android/aar_integration_test.sh @@ -147,4 +147,38 @@ EOF assert_one_of $apk_contents "lib/armeabi-v7a/libapp.so" } +function test_aar_extractor_worker() { + create_new_workspace + setup_android_sdk_support + cat > AndroidManifest.xml < +EOF + mkdir -p res/layout + cat > res/layout/mylayout.xml < + +EOF + mkdir assets + echo "some asset" > assets/a + zip example.aar AndroidManifest.xml res/layout/mylayout.xml assets/a + cat > BUILD < Date: Thu, 17 Aug 2023 14:09:15 -0700 Subject: [PATCH 11/19] Add support for more workspace boundary files to bash completion Bash completion now also recognized `WORKSPACE.bazel`, `MODULE.bazel` and `REPO.bazel`. Also fixes a shellcheck finding. Closes #19268. PiperOrigin-RevId: 557931944 Change-Id: Ia2b6e463f4643d9c0d829845fee4228103cf90a0 --- scripts/bash_completion_test.sh | 26 ++++++++++++++++++++++++++ scripts/bazel-complete-template.bash | 10 +++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/scripts/bash_completion_test.sh b/scripts/bash_completion_test.sh index 50a808ecbde76e..6a2f39622795fd 100755 --- a/scripts/bash_completion_test.sh +++ b/scripts/bash_completion_test.sh @@ -768,4 +768,30 @@ test_info() { 'info --show_make_env ' } +test_workspace_boundary() { + # "Test that workspace boundary files are recognized" + # this test only works for Bazel + if [[ ! " ${COMMAND_ALIASES[*]} " =~ " bazel " ]]; then return; fi + + mkdir -p sub_repo/some/pkg + touch sub_repo/some/pkg/BUILD + cd sub_repo 2>/dev/null + + touch WORKSPACE.bazel + assert_expansion 'build //s' \ + 'build //some/' + + mv WORKSPACE.bazel MODULE.bazel + assert_expansion 'build //s' \ + 'build //some/' + + mv MODULE.bazel REPO.bazel + assert_expansion 'build //s' \ + 'build //some/' + + rm REPO.bazel + assert_expansion 'build //s' \ + 'build //sub_repo/' +} + run_suite "Tests of bash completion of 'blaze' command." diff --git a/scripts/bazel-complete-template.bash b/scripts/bazel-complete-template.bash index 3b6812f387c52c..320d77670c7820 100644 --- a/scripts/bazel-complete-template.bash +++ b/scripts/bazel-complete-template.bash @@ -79,13 +79,17 @@ _bazel__get_rule_match_pattern() { } # Compute workspace directory. Search for the innermost -# enclosing directory with a WORKSPACE file. +# enclosing directory with a boundary file (see +# src/main/cpp/workspace_layout.cc). _bazel__get_workspace_path() { local workspace=$PWD while true; do - if [ -f "${workspace}/WORKSPACE" ]; then + if [ -f "${workspace}/WORKSPACE" ] || \ + [ -f "${workspace}/WORKSPACE.bazel" ] || \ + [ -f "${workspace}/MODULE.bazel" ] || \ + [ -f "${workspace}/REPO.bazel" ]; then break - elif [ -z "$workspace" -o "$workspace" = "/" ]; then + elif [ -z "$workspace" ] || [ "$workspace" = "/" ]; then workspace=$PWD break; fi From 48f081f6f18277d4bbbb5a1740f9c59cecdf88fc Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Aug 2023 15:00:43 -0700 Subject: [PATCH 12/19] Fix the list of default Error Prone errors for Bazel Instead of automatically enabling new default errors that are introduced to OSS Error Prone. PiperOrigin-RevId: 557946099 Change-Id: I232473d5a0158e18a23557859a571f5507c48d85 --- .../com/google/devtools/build/buildjar/BUILD | 1 + .../build/buildjar/BazelJavaBuilder.java | 3 +- .../build/buildjar/BazelScannerSuppliers.java | 393 ++++++++++++++++++ 3 files changed, 396 insertions(+), 1 deletion(-) create mode 100644 src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelScannerSuppliers.java diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD index 320f33d831ca91..ed8faa5a35d229 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD @@ -106,6 +106,7 @@ java_library( "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:processing", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/statistics", "//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers", + "//third_party:error_prone", "//third_party:error_prone_annotations", "//third_party:guava", "//third_party:jsr305", diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelJavaBuilder.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelJavaBuilder.java index 43ab1759764137..c8449a0073174f 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelJavaBuilder.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelJavaBuilder.java @@ -126,7 +126,8 @@ public JavaLibraryBuildRequest parse(List args) throws IOException, InvalidCommandLineException { OptionsParser optionsParser = new OptionsParser(args, JavacOptions.createWithWarningsAsErrorsDefault(ImmutableList.of())); - ImmutableList plugins = ImmutableList.of(new ErrorPronePlugin()); + ImmutableList plugins = + ImmutableList.of(new ErrorPronePlugin(BazelScannerSuppliers.bazelChecks())); return new JavaLibraryBuildRequest(optionsParser, plugins, new DependencyModule.Builder()); } } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelScannerSuppliers.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelScannerSuppliers.java new file mode 100644 index 00000000000000..58306f4696606d --- /dev/null +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelScannerSuppliers.java @@ -0,0 +1,393 @@ +// Copyright 2023 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.buildjar; + +import static com.google.errorprone.scanner.BuiltInCheckerSuppliers.getSuppliers; + +import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugCheckerInfo; +import com.google.errorprone.bugpatterns.AlwaysThrows; +import com.google.errorprone.bugpatterns.ArrayEquals; +import com.google.errorprone.bugpatterns.ArrayFillIncompatibleType; +import com.google.errorprone.bugpatterns.ArrayHashCode; +import com.google.errorprone.bugpatterns.ArrayToString; +import com.google.errorprone.bugpatterns.ArraysAsListPrimitiveArray; +import com.google.errorprone.bugpatterns.AsyncCallableReturnsNull; +import com.google.errorprone.bugpatterns.AsyncFunctionReturnsNull; +import com.google.errorprone.bugpatterns.AutoValueBuilderDefaultsInConstructor; +import com.google.errorprone.bugpatterns.BadAnnotationImplementation; +import com.google.errorprone.bugpatterns.BadShiftAmount; +import com.google.errorprone.bugpatterns.BanJNDI; +import com.google.errorprone.bugpatterns.BoxedPrimitiveEquality; +import com.google.errorprone.bugpatterns.ChainingConstructorIgnoresParameter; +import com.google.errorprone.bugpatterns.CheckNotNullMultipleTimes; +import com.google.errorprone.bugpatterns.CheckReturnValue; +import com.google.errorprone.bugpatterns.CollectionToArraySafeParameter; +import com.google.errorprone.bugpatterns.ComparableType; +import com.google.errorprone.bugpatterns.ComparingThisWithNull; +import com.google.errorprone.bugpatterns.ComparisonOutOfRange; +import com.google.errorprone.bugpatterns.CompileTimeConstantChecker; +import com.google.errorprone.bugpatterns.ComputeIfAbsentAmbiguousReference; +import com.google.errorprone.bugpatterns.ConditionalExpressionNumericPromotion; +import com.google.errorprone.bugpatterns.ConstantOverflow; +import com.google.errorprone.bugpatterns.DangerousLiteralNullChecker; +import com.google.errorprone.bugpatterns.DeadException; +import com.google.errorprone.bugpatterns.DeadThread; +import com.google.errorprone.bugpatterns.DiscardedPostfixExpression; +import com.google.errorprone.bugpatterns.DoNotCallChecker; +import com.google.errorprone.bugpatterns.DoNotMockChecker; +import com.google.errorprone.bugpatterns.DoubleBraceInitialization; +import com.google.errorprone.bugpatterns.DuplicateMapKeys; +import com.google.errorprone.bugpatterns.EqualsHashCode; +import com.google.errorprone.bugpatterns.EqualsNaN; +import com.google.errorprone.bugpatterns.EqualsNull; +import com.google.errorprone.bugpatterns.EqualsReference; +import com.google.errorprone.bugpatterns.EqualsWrongThing; +import com.google.errorprone.bugpatterns.ForOverrideChecker; +import com.google.errorprone.bugpatterns.FunctionalInterfaceMethodChanged; +import com.google.errorprone.bugpatterns.FuturesGetCheckedIllegalExceptionType; +import com.google.errorprone.bugpatterns.FuzzyEqualsShouldNotBeUsedInEqualsMethod; +import com.google.errorprone.bugpatterns.GetClassOnAnnotation; +import com.google.errorprone.bugpatterns.GetClassOnClass; +import com.google.errorprone.bugpatterns.HashtableContains; +import com.google.errorprone.bugpatterns.IdentityBinaryExpression; +import com.google.errorprone.bugpatterns.IdentityHashMapBoxing; +import com.google.errorprone.bugpatterns.ImpossibleNullComparison; +import com.google.errorprone.bugpatterns.Incomparable; +import com.google.errorprone.bugpatterns.IncompatibleModifiersChecker; +import com.google.errorprone.bugpatterns.IndexOfChar; +import com.google.errorprone.bugpatterns.InexactVarargsConditional; +import com.google.errorprone.bugpatterns.InfiniteRecursion; +import com.google.errorprone.bugpatterns.InvalidPatternSyntax; +import com.google.errorprone.bugpatterns.InvalidTimeZoneID; +import com.google.errorprone.bugpatterns.InvalidZoneId; +import com.google.errorprone.bugpatterns.IsInstanceIncompatibleType; +import com.google.errorprone.bugpatterns.IsInstanceOfClass; +import com.google.errorprone.bugpatterns.JUnit3TestNotRun; +import com.google.errorprone.bugpatterns.JUnit4ClassAnnotationNonStatic; +import com.google.errorprone.bugpatterns.JUnit4SetUpNotRun; +import com.google.errorprone.bugpatterns.JUnit4TearDownNotRun; +import com.google.errorprone.bugpatterns.JUnit4TestNotRun; +import com.google.errorprone.bugpatterns.JUnit4TestsNotRunWithinEnclosed; +import com.google.errorprone.bugpatterns.JUnitAssertSameCheck; +import com.google.errorprone.bugpatterns.JUnitParameterMethodNotFound; +import com.google.errorprone.bugpatterns.LenientFormatStringValidation; +import com.google.errorprone.bugpatterns.LiteByteStringUtf8; +import com.google.errorprone.bugpatterns.LockOnBoxedPrimitive; +import com.google.errorprone.bugpatterns.LoopConditionChecker; +import com.google.errorprone.bugpatterns.LossyPrimitiveCompare; +import com.google.errorprone.bugpatterns.MathRoundIntLong; +import com.google.errorprone.bugpatterns.MissingSuperCall; +import com.google.errorprone.bugpatterns.MissingTestCall; +import com.google.errorprone.bugpatterns.MisusedDayOfYear; +import com.google.errorprone.bugpatterns.MisusedWeekYear; +import com.google.errorprone.bugpatterns.MixedDescriptors; +import com.google.errorprone.bugpatterns.MockitoUsage; +import com.google.errorprone.bugpatterns.ModifyingCollectionWithItself; +import com.google.errorprone.bugpatterns.MustBeClosedChecker; +import com.google.errorprone.bugpatterns.NCopiesOfChar; +import com.google.errorprone.bugpatterns.NonCanonicalStaticImport; +import com.google.errorprone.bugpatterns.NonFinalCompileTimeConstant; +import com.google.errorprone.bugpatterns.NonRuntimeAnnotation; +import com.google.errorprone.bugpatterns.NullTernary; +import com.google.errorprone.bugpatterns.NullableOnContainingClass; +import com.google.errorprone.bugpatterns.OptionalEquality; +import com.google.errorprone.bugpatterns.OptionalMapUnusedValue; +import com.google.errorprone.bugpatterns.OptionalOfRedundantMethod; +import com.google.errorprone.bugpatterns.PackageInfo; +import com.google.errorprone.bugpatterns.ParametersButNotParameterized; +import com.google.errorprone.bugpatterns.PreconditionsInvalidPlaceholder; +import com.google.errorprone.bugpatterns.PrivateSecurityContractProtoAccess; +import com.google.errorprone.bugpatterns.ProtoBuilderReturnValueIgnored; +import com.google.errorprone.bugpatterns.ProtoStringFieldReferenceEquality; +import com.google.errorprone.bugpatterns.ProtoTruthMixedDescriptors; +import com.google.errorprone.bugpatterns.ProtocolBufferOrdinal; +import com.google.errorprone.bugpatterns.RandomCast; +import com.google.errorprone.bugpatterns.RandomModInteger; +import com.google.errorprone.bugpatterns.RequiredModifiersChecker; +import com.google.errorprone.bugpatterns.RestrictedApiChecker; +import com.google.errorprone.bugpatterns.ReturnValueIgnored; +import com.google.errorprone.bugpatterns.SelfAssignment; +import com.google.errorprone.bugpatterns.SelfComparison; +import com.google.errorprone.bugpatterns.SelfEquals; +import com.google.errorprone.bugpatterns.ShouldHaveEvenArgs; +import com.google.errorprone.bugpatterns.SizeGreaterThanOrEqualsZero; +import com.google.errorprone.bugpatterns.StreamToString; +import com.google.errorprone.bugpatterns.StringBuilderInitWithChar; +import com.google.errorprone.bugpatterns.SubstringOfZero; +import com.google.errorprone.bugpatterns.SuppressWarningsDeprecated; +import com.google.errorprone.bugpatterns.TestParametersNotInitialized; +import com.google.errorprone.bugpatterns.TheoryButNoTheories; +import com.google.errorprone.bugpatterns.ThrowIfUncheckedKnownChecked; +import com.google.errorprone.bugpatterns.ThrowNull; +import com.google.errorprone.bugpatterns.TreeToString; +import com.google.errorprone.bugpatterns.TruthSelfEquals; +import com.google.errorprone.bugpatterns.TryFailThrowable; +import com.google.errorprone.bugpatterns.TypeParameterQualifier; +import com.google.errorprone.bugpatterns.UnicodeDirectionalityCharacters; +import com.google.errorprone.bugpatterns.UnicodeInCode; +import com.google.errorprone.bugpatterns.UnnecessaryTypeArgument; +import com.google.errorprone.bugpatterns.UnusedAnonymousClass; +import com.google.errorprone.bugpatterns.UnusedCollectionModifiedInPlace; +import com.google.errorprone.bugpatterns.VarTypeName; +import com.google.errorprone.bugpatterns.WrongOneof; +import com.google.errorprone.bugpatterns.XorPower; +import com.google.errorprone.bugpatterns.android.BundleDeserializationCast; +import com.google.errorprone.bugpatterns.android.IsLoggableTagLength; +import com.google.errorprone.bugpatterns.android.MislabeledAndroidString; +import com.google.errorprone.bugpatterns.android.ParcelableCreator; +import com.google.errorprone.bugpatterns.android.RectIntersectReturnValueIgnored; +import com.google.errorprone.bugpatterns.argumentselectiondefects.AutoValueConstructorOrderChecker; +import com.google.errorprone.bugpatterns.checkreturnvalue.NoCanIgnoreReturnValueOnClasses; +import com.google.errorprone.bugpatterns.collectionincompatibletype.CollectionIncompatibleType; +import com.google.errorprone.bugpatterns.collectionincompatibletype.CompatibleWithMisuse; +import com.google.errorprone.bugpatterns.collectionincompatibletype.IncompatibleArgumentType; +import com.google.errorprone.bugpatterns.flogger.FloggerFormatString; +import com.google.errorprone.bugpatterns.flogger.FloggerLogString; +import com.google.errorprone.bugpatterns.flogger.FloggerLogVarargs; +import com.google.errorprone.bugpatterns.flogger.FloggerSplitLogStatement; +import com.google.errorprone.bugpatterns.formatstring.FormatString; +import com.google.errorprone.bugpatterns.formatstring.FormatStringAnnotationChecker; +import com.google.errorprone.bugpatterns.inject.InjectOnMemberAndConstructor; +import com.google.errorprone.bugpatterns.inject.JavaxInjectOnAbstractMethod; +import com.google.errorprone.bugpatterns.inject.MisplacedScopeAnnotations; +import com.google.errorprone.bugpatterns.inject.MoreThanOneInjectableConstructor; +import com.google.errorprone.bugpatterns.inject.MoreThanOneScopeAnnotationOnClass; +import com.google.errorprone.bugpatterns.inject.OverlappingQualifierAndScopeAnnotation; +import com.google.errorprone.bugpatterns.inject.dagger.AndroidInjectionBeforeSuper; +import com.google.errorprone.bugpatterns.inject.dagger.ProvidesNull; +import com.google.errorprone.bugpatterns.inject.guice.AssistedInjectScoping; +import com.google.errorprone.bugpatterns.inject.guice.AssistedParameters; +import com.google.errorprone.bugpatterns.inject.guice.InjectOnFinalField; +import com.google.errorprone.bugpatterns.inject.guice.OverridesJavaxInjectableMethod; +import com.google.errorprone.bugpatterns.inject.guice.ProvidesMethodOutsideOfModule; +import com.google.errorprone.bugpatterns.inlineme.Validator; +import com.google.errorprone.bugpatterns.nullness.DereferenceWithNullBranch; +import com.google.errorprone.bugpatterns.nullness.NullArgumentForNonNullParameter; +import com.google.errorprone.bugpatterns.nullness.UnnecessaryCheckNotNull; +import com.google.errorprone.bugpatterns.nullness.UnsafeWildcard; +import com.google.errorprone.bugpatterns.threadsafety.GuardedByChecker; +import com.google.errorprone.bugpatterns.threadsafety.ImmutableChecker; +import com.google.errorprone.bugpatterns.time.DurationFrom; +import com.google.errorprone.bugpatterns.time.DurationGetTemporalUnit; +import com.google.errorprone.bugpatterns.time.DurationTemporalUnit; +import com.google.errorprone.bugpatterns.time.DurationToLongTimeUnit; +import com.google.errorprone.bugpatterns.time.FromTemporalAccessor; +import com.google.errorprone.bugpatterns.time.InstantTemporalUnit; +import com.google.errorprone.bugpatterns.time.InvalidJavaTimeConstant; +import com.google.errorprone.bugpatterns.time.JodaToSelf; +import com.google.errorprone.bugpatterns.time.LocalDateTemporalAmount; +import com.google.errorprone.bugpatterns.time.PeriodFrom; +import com.google.errorprone.bugpatterns.time.PeriodGetTemporalUnit; +import com.google.errorprone.bugpatterns.time.PeriodTimeMath; +import com.google.errorprone.bugpatterns.time.TemporalAccessorGetChronoField; +import com.google.errorprone.bugpatterns.time.ZoneIdOfZ; +import com.google.errorprone.scanner.BuiltInCheckerSuppliers; +import com.google.errorprone.scanner.ScannerSupplier; + +/** A factory for the {@link ScannerSupplier} that supplies Error Prone checks for Bazel. */ +final class BazelScannerSuppliers { + static ScannerSupplier bazelChecks() { + return BuiltInCheckerSuppliers.allChecks().filter(Predicates.in(ENABLED_ERRORS)); + } + + // The list of default Error Prone errors as of 2023-8-17, generated from: + // https://github.com/google/error-prone/blob/1b1ef67c6dc59eb1060e37cf989f95312e84e76d/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java#L635 + // New errors should not be enabled in this list to avoid breaking changes in java_rules release + private static final ImmutableSet ENABLED_ERRORS = + getSuppliers( + // keep-sorted start + AlwaysThrows.class, + AndroidInjectionBeforeSuper.class, + ArrayEquals.class, + ArrayFillIncompatibleType.class, + ArrayHashCode.class, + ArrayToString.class, + ArraysAsListPrimitiveArray.class, + AssistedInjectScoping.class, + AssistedParameters.class, + AsyncCallableReturnsNull.class, + AsyncFunctionReturnsNull.class, + AutoValueBuilderDefaultsInConstructor.class, + AutoValueConstructorOrderChecker.class, + BadAnnotationImplementation.class, + BadShiftAmount.class, + BanJNDI.class, + BoxedPrimitiveEquality.class, + BundleDeserializationCast.class, + ChainingConstructorIgnoresParameter.class, + CheckNotNullMultipleTimes.class, + CheckReturnValue.class, + CollectionIncompatibleType.class, + CollectionToArraySafeParameter.class, + ComparableType.class, + ComparingThisWithNull.class, + ComparisonOutOfRange.class, + CompatibleWithMisuse.class, + CompileTimeConstantChecker.class, + ComputeIfAbsentAmbiguousReference.class, + ConditionalExpressionNumericPromotion.class, + ConstantOverflow.class, + DangerousLiteralNullChecker.class, + DeadException.class, + DeadThread.class, + DereferenceWithNullBranch.class, + DiscardedPostfixExpression.class, + DoNotCallChecker.class, + DoNotMockChecker.class, + DoubleBraceInitialization.class, + DuplicateMapKeys.class, + DurationFrom.class, + DurationGetTemporalUnit.class, + DurationTemporalUnit.class, + DurationToLongTimeUnit.class, + EqualsHashCode.class, + EqualsNaN.class, + EqualsNull.class, + EqualsReference.class, + EqualsWrongThing.class, + FloggerFormatString.class, + FloggerLogString.class, + FloggerLogVarargs.class, + FloggerSplitLogStatement.class, + ForOverrideChecker.class, + FormatString.class, + FormatStringAnnotationChecker.class, + FromTemporalAccessor.class, + FunctionalInterfaceMethodChanged.class, + FuturesGetCheckedIllegalExceptionType.class, + FuzzyEqualsShouldNotBeUsedInEqualsMethod.class, + GetClassOnAnnotation.class, + GetClassOnClass.class, + GuardedByChecker.class, + HashtableContains.class, + IdentityBinaryExpression.class, + IdentityHashMapBoxing.class, + ImmutableChecker.class, + ImpossibleNullComparison.class, + Incomparable.class, + IncompatibleArgumentType.class, + IncompatibleModifiersChecker.class, + IndexOfChar.class, + InexactVarargsConditional.class, + InfiniteRecursion.class, + InjectOnFinalField.class, + InjectOnMemberAndConstructor.class, + InstantTemporalUnit.class, + InvalidJavaTimeConstant.class, + InvalidPatternSyntax.class, + InvalidTimeZoneID.class, + InvalidZoneId.class, + IsInstanceIncompatibleType.class, + IsInstanceOfClass.class, + IsLoggableTagLength.class, + JUnit3TestNotRun.class, + JUnit4ClassAnnotationNonStatic.class, + JUnit4SetUpNotRun.class, + JUnit4TearDownNotRun.class, + JUnit4TestNotRun.class, + JUnit4TestsNotRunWithinEnclosed.class, + JUnitAssertSameCheck.class, + JUnitParameterMethodNotFound.class, + JavaxInjectOnAbstractMethod.class, + JodaToSelf.class, + LenientFormatStringValidation.class, + LiteByteStringUtf8.class, + LocalDateTemporalAmount.class, + LockOnBoxedPrimitive.class, + LoopConditionChecker.class, + LossyPrimitiveCompare.class, + MathRoundIntLong.class, + MislabeledAndroidString.class, + MisplacedScopeAnnotations.class, + MissingSuperCall.class, + MissingTestCall.class, + MisusedDayOfYear.class, + MisusedWeekYear.class, + MixedDescriptors.class, + MockitoUsage.class, + ModifyingCollectionWithItself.class, + MoreThanOneInjectableConstructor.class, + MoreThanOneScopeAnnotationOnClass.class, + MustBeClosedChecker.class, + NCopiesOfChar.class, + NoCanIgnoreReturnValueOnClasses.class, + NonCanonicalStaticImport.class, + NonFinalCompileTimeConstant.class, + NonRuntimeAnnotation.class, + NullArgumentForNonNullParameter.class, + NullTernary.class, + NullableOnContainingClass.class, + OptionalEquality.class, + OptionalMapUnusedValue.class, + OptionalOfRedundantMethod.class, + OverlappingQualifierAndScopeAnnotation.class, + OverridesJavaxInjectableMethod.class, + PackageInfo.class, + ParametersButNotParameterized.class, + ParcelableCreator.class, + PeriodFrom.class, + PeriodGetTemporalUnit.class, + PeriodTimeMath.class, + PreconditionsInvalidPlaceholder.class, + PrivateSecurityContractProtoAccess.class, + ProtoBuilderReturnValueIgnored.class, + ProtoStringFieldReferenceEquality.class, + ProtoTruthMixedDescriptors.class, + ProtocolBufferOrdinal.class, + ProvidesMethodOutsideOfModule.class, + ProvidesNull.class, + RandomCast.class, + RandomModInteger.class, + RectIntersectReturnValueIgnored.class, + RequiredModifiersChecker.class, + RestrictedApiChecker.class, + ReturnValueIgnored.class, + SelfAssignment.class, + SelfComparison.class, + SelfEquals.class, + ShouldHaveEvenArgs.class, + SizeGreaterThanOrEqualsZero.class, + StreamToString.class, + StringBuilderInitWithChar.class, + SubstringOfZero.class, + SuppressWarningsDeprecated.class, + TemporalAccessorGetChronoField.class, + TestParametersNotInitialized.class, + TheoryButNoTheories.class, + ThrowIfUncheckedKnownChecked.class, + ThrowNull.class, + TreeToString.class, + TruthSelfEquals.class, + TryFailThrowable.class, + TypeParameterQualifier.class, + UnicodeDirectionalityCharacters.class, + UnicodeInCode.class, + UnnecessaryCheckNotNull.class, + UnnecessaryTypeArgument.class, + UnsafeWildcard.class, + UnusedAnonymousClass.class, + UnusedCollectionModifiedInPlace.class, + Validator.class, + VarTypeName.class, + WrongOneof.class, + XorPower.class, + ZoneIdOfZ.class + // keep-sorted end + ); + + private BazelScannerSuppliers() {} +} From 93253eeae1b4ca4b1d24923fd4be49dd7b3b3a4c Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 18 Aug 2023 00:31:05 -0700 Subject: [PATCH 13/19] Replace rules_docker with rules_oci on recommended rules page I believe it is no longer maintained, so I'm following the process outlined in the Bazel documentation: https://bazel.build/community/recommended-rules#demotion Here's some data on rules_docker: - Maintenance signals have gone down significantly in the last year: https://oss-compass.org/analyze/snbnfub0?range=1Y - Hasn't been released in a year - Second-most-recent commit was mine from about five months ago, adding the "minimal maintenance mode" https://github.com/bazelbuild/rules_docker/pull/2236 - there's more context in that PR. - Never reached 1.0 rules_oci seems like the replacement. - It has strong adoption testimonials https://github.com/bazel-contrib/rules_oci/discussions/299 - Version is 1.2 with semver guarantees Closes #19123. PiperOrigin-RevId: 558059587 Change-Id: I4c175a79b3429947fa036a22165839dcf285b28d --- site/en/rules/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/en/rules/index.md b/site/en/rules/index.md index a9a676af1333b9..c12d4f57604def 100644 --- a/site/en/rules/index.md +++ b/site/en/rules/index.md @@ -18,7 +18,7 @@ Here is a selection of recommended rules: * [Android](/docs/bazel-and-android) * [C / C++](/docs/bazel-and-cpp) -* [Docker](https://github.com/bazelbuild/rules_docker){: .external} +* [Docker/OCI](https://github.com/bazel-contrib/rules_oci){: .external} * [Go](https://github.com/bazelbuild/rules_go){: .external} * [Haskell](https://github.com/tweag/rules_haskell){: .external} * [Java](/docs/bazel-and-java) From 7b87ae1b81b681d4fc7e42282b6966fbcaf6301c Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 18 Aug 2023 04:54:46 -0700 Subject: [PATCH 14/19] Obey --experimental_inprocess_symlink_creation in RunfilesTreeUpdater. The bulk of this CL is just plumbing the flag from the configuration into the RunfilesSupplier implementations. The functional changes are in RunfilesTreeUpdater; compare with the preexisting SymlinkTreeStrategy implementation. Additionally, I've fixed some variable and method names to exactly match the flags defined in CoreOptions; the code is less confusing and easier to refactor that way. The motivation behind this change is to investigate the feasibility of defaulting to the in-process implementation and deleting the out-of-process one, which has known issues (see https://github.com/bazelbuild/bazel/issues/4327). PiperOrigin-RevId: 558112830 Change-Id: I4c509be665776013ffc2d483694df515d5ab32f2 --- .../google/devtools/build/lib/actions/BUILD | 2 + .../actions/CompositeRunfilesSupplier.java | 16 ++-- .../lib/actions/EmptyRunfilesSupplier.java | 9 ++- .../build/lib/actions/RunfilesSupplier.java | 28 +++---- .../google/devtools/build/lib/analysis/BUILD | 1 + .../build/lib/analysis/RunfilesSupport.java | 58 ++++++------- .../lib/analysis/SingleRunfilesSupplier.java | 61 +++++++------- .../analysis/actions/SymlinkTreeAction.java | 32 +++----- .../config/BuildConfigurationValue.java | 56 ++++++++----- .../lib/analysis/config/CoreOptions.java | 12 +-- .../lib/analysis/test/TestActionBuilder.java | 4 +- .../com/google/devtools/build/lib/exec/BUILD | 3 +- .../build/lib/exec/RunfilesTreeUpdater.java | 31 +++++-- .../build/lib/exec/SymlinkTreeHelper.java | 19 ----- .../build/lib/exec/SymlinkTreeStrategy.java | 7 +- .../build/lib/rules/python/PyBuiltins.java | 4 +- .../lib/runtime/commands/RunCommand.java | 4 +- .../analysis/SingleRunfilesSupplierTest.java | 47 +++++------ .../lib/analysis/actions/SpawnActionTest.java | 9 ++- .../actions/SymlinkTreeActionTest.java | 81 ++++++++++--------- .../lib/analysis/util/AnalysisTestUtil.java | 9 ++- .../com/google/devtools/build/lib/exec/BUILD | 3 +- .../lib/exec/SymlinkTreeStrategyTest.java | 7 +- .../google/devtools/build/lib/rules/cpp/BUILD | 1 + .../lib/rules/cpp/LtoBackendActionTest.java | 9 ++- .../rewinding/RewindingTestsHelper.java | 2 +- 26 files changed, 278 insertions(+), 237 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index b154e2127a2548..79ab70b3480b9f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -488,10 +488,12 @@ java_library( srcs = ["RunfilesSupplier.java"], deps = [ ":artifacts", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/net/starlark/java/eval", "//third_party:guava", + "//third_party:jsr305", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java index 6bd1fe5cb8fb33..36776d62fb2626 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java @@ -18,11 +18,13 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collection; import java.util.Map; +import javax.annotation.Nullable; /** A {@link RunfilesSupplier} implementation for composing multiple instances. */ public final class CompositeRunfilesSupplier implements RunfilesSupplier { @@ -101,19 +103,21 @@ public ImmutableList getManifests() { } @Override - public boolean isBuildRunfileLinks(PathFragment runfilesDir) { + @Nullable + public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) { for (RunfilesSupplier supplier : suppliers) { - if (supplier.isBuildRunfileLinks(runfilesDir)) { - return true; + RunfileSymlinksMode mode = supplier.getRunfileSymlinksMode(runfilesDir); + if (mode != null) { + return mode; } } - return false; + return null; } @Override - public boolean isRunfileLinksEnabled(PathFragment runfilesDir) { + public boolean isBuildRunfileLinks(PathFragment runfilesDir) { for (RunfilesSupplier supplier : suppliers) { - if (supplier.isRunfileLinksEnabled(runfilesDir)) { + if (supplier.isBuildRunfileLinks(runfilesDir)) { return true; } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java index e13f85bfe74be4..c7847cb6fbdc0a 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java +++ b/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java @@ -17,12 +17,14 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Map; +import javax.annotation.Nullable; /** Empty implementation of RunfilesSupplier */ public final class EmptyRunfilesSupplier implements RunfilesSupplier { @@ -53,12 +55,13 @@ public ImmutableList getManifests() { } @Override - public boolean isBuildRunfileLinks(PathFragment runfilesDir) { - return false; + @Nullable + public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) { + return null; } @Override - public boolean isRunfileLinksEnabled(PathFragment runfilesDir) { + public boolean isBuildRunfileLinks(PathFragment runfilesDir) { return false; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java index 2ba623f86a4492..ded16ef000e6db 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java +++ b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java @@ -17,9 +17,11 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Map; +import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkValue; /** Convenience wrapper around runfiles allowing lazy expansion. */ @@ -30,37 +32,35 @@ // they are exposed through ctx.resolve_tools[2], for example. public interface RunfilesSupplier extends StarlarkValue { - /** @return the contained artifacts */ + /** Returns the contained artifacts. */ NestedSet getArtifacts(); - /** @return the runfiles' root directories. */ + /** Returns the runfiles' root directories. */ ImmutableSet getRunfilesDirs(); - /** - * Returns mappings from runfiles directories to artifact mappings in that directory. - * - * @return runfiles' mappings - */ + /** Returns mappings from runfiles directories to artifact mappings in that directory. */ ImmutableMap> getMappings(); - /** @return the runfiles manifest artifacts, if any. */ + /** Returns the runfiles manifest artifacts, if any. */ ImmutableList getManifests(); /** - * Returns whether for a given {@code runfilesDir} the runfile symlinks are materialized during - * build. Also returns {@code false} if the runfiles supplier doesn't know about the directory. + * Returns the {@link RunfileSymlinksMode} for the given {@code runfilesDir}, or {@code null} if + * the {@link RunfilesSupplier} doesn't know about the directory. * * @param runfilesDir runfiles directory relative to the exec root */ - boolean isBuildRunfileLinks(PathFragment runfilesDir); + @Nullable + RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir); /** - * Returns whether it's allowed to create runfile symlinks in the {@code runfilesDir}. Also - * returns {@code false} if the runfiles supplier doesn't know about the directory. + * Returns whether the runfile symlinks should be materialized during the build for the given + * {@code runfilesDir}, or {@code false} if the {@link RunfilesSupplier} doesn't know about the + * directory. * * @param runfilesDir runfiles directory relative to the exec root */ - boolean isRunfileLinksEnabled(PathFragment runfilesDir); + boolean isBuildRunfileLinks(PathFragment runfilesDir); /** * Returns a {@link RunfilesSupplier} identical to this one, but with the given runfiles 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 c78ea51bd046fa..8ea1bef5418f4b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1669,6 +1669,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/skyframe:skyframe-objects", + "//src/main/java/com/google/devtools/common/options", "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", "//src/main/protobuf:failure_details_java_proto", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index fd303942d35afa..30b8fd54867873 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.analysis.config.RunUnder; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -87,8 +88,8 @@ public final class RunfilesSupport implements RunfilesSupplier { private final Artifact repoMappingManifest; private final Artifact runfilesMiddleman; private final Artifact owningExecutable; + private final RunfileSymlinksMode runfileSymlinksMode; private final boolean buildRunfileLinks; - private final boolean runfilesEnabled; private final CommandLine args; private final ActionEnvironment actionEnvironment; @@ -106,7 +107,9 @@ private static RunfilesSupport create( CommandLine args, ActionEnvironment actionEnvironment) { Artifact owningExecutable = Preconditions.checkNotNull(executable); - boolean createManifest = ruleContext.getConfiguration().buildRunfilesManifests(); + RunfileSymlinksMode runfileSymlinksMode = + ruleContext.getConfiguration().getRunfileSymlinksMode(); + boolean buildRunfileManifests = ruleContext.getConfiguration().buildRunfileManifests(); boolean buildRunfileLinks = ruleContext.getConfiguration().buildRunfileLinks(); // Adding run_under target to the runfiles manifest so it would become part @@ -131,7 +134,7 @@ private static RunfilesSupport create( Artifact runfilesInputManifest; Artifact runfilesManifest; - if (createManifest) { + if (buildRunfileManifests) { runfilesInputManifest = createRunfilesInputManifestArtifact(ruleContext, owningExecutable); runfilesManifest = createRunfilesAction( @@ -144,8 +147,6 @@ private static RunfilesSupport create( createRunfilesMiddleman( ruleContext, owningExecutable, runfiles, runfilesManifest, repoMappingManifest); - boolean runfilesEnabled = ruleContext.getConfiguration().runfilesEnabled(); - return new RunfilesSupport( runfiles, runfilesInputManifest, @@ -153,8 +154,8 @@ private static RunfilesSupport create( repoMappingManifest, runfilesMiddleman, owningExecutable, + runfileSymlinksMode, buildRunfileLinks, - runfilesEnabled, args, actionEnvironment); } @@ -166,8 +167,8 @@ private RunfilesSupport( Artifact repoMappingManifest, Artifact runfilesMiddleman, Artifact owningExecutable, + RunfileSymlinksMode runfileSymlinksMode, boolean buildRunfileLinks, - boolean runfilesEnabled, CommandLine args, ActionEnvironment actionEnvironment) { this.runfiles = runfiles; @@ -176,8 +177,8 @@ private RunfilesSupport( this.repoMappingManifest = repoMappingManifest; this.runfilesMiddleman = runfilesMiddleman; this.owningExecutable = owningExecutable; + this.runfileSymlinksMode = runfileSymlinksMode; this.buildRunfileLinks = buildRunfileLinks; - this.runfilesEnabled = runfilesEnabled; this.args = args; this.actionEnvironment = actionEnvironment; } @@ -194,27 +195,33 @@ public PathFragment getRunfilesDirectoryExecPath() { } /** - * Returns {@code true} if runfile symlinks should be materialized when building an executable. - * - *

Also see {@link #isRunfilesEnabled()}. + * Same as {@link #getRunfileSymlinksMode(PathFragment)} with {@link + * #getRunfilesDirectoryExecPath} as the implied argument. */ - public boolean isBuildRunfileLinks() { - return buildRunfileLinks; + public RunfileSymlinksMode getRunfileSymlinksMode() { + return runfileSymlinksMode; } @Override - public boolean isBuildRunfileLinks(PathFragment runfilesDir) { - return buildRunfileLinks && runfilesDir.equals(getRunfilesDirectoryExecPath()); + @Nullable + public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) { + if (runfilesDir.equals(getRunfilesDirectoryExecPath())) { + return runfileSymlinksMode; + } + return null; } /** - * Returns {@code true} if runfile symlinks are enabled. - * - *

This option differs from {@link #isBuildRunfileLinks()} in that if {@code false} it also - * disables runfile symlinks creation during run/test. + * Same as {@link #isBuildRunfileLinks(PathFragment)} with {@link #getRunfilesDirectoryExecPath} + * as the implied argument. */ - public boolean isRunfilesEnabled() { - return runfilesEnabled; + public boolean isBuildRunfileLinks() { + return buildRunfileLinks; + } + + @Override + public boolean isBuildRunfileLinks(PathFragment runfilesDir) { + return buildRunfileLinks && runfilesDir.equals(getRunfilesDirectoryExecPath()); } public Runfiles getRunfiles() { @@ -552,11 +559,6 @@ public ImmutableList getManifests() { return ImmutableList.of(); } - @Override - public boolean isRunfileLinksEnabled(PathFragment runfilesDir) { - return runfilesEnabled && runfilesDir.equals(getRunfilesDirectoryExecPath()); - } - @Override public RunfilesSupplier withOverriddenRunfilesDir(PathFragment newRunfilesDir) { return newRunfilesDir.equals(getRunfilesDirectoryExecPath()) @@ -566,7 +568,7 @@ public RunfilesSupplier withOverriddenRunfilesDir(PathFragment newRunfilesDir) { runfiles, /* manifest= */ null, repoMappingManifest, - buildRunfileLinks, - runfilesEnabled); + runfileSymlinksMode, + buildRunfileLinks); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java index 458dabe38c8d07..4d9ac6ee2a2772 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.RunfilesSupplier; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.vfs.PathFragment; @@ -39,8 +40,8 @@ public final class SingleRunfilesSupplier implements RunfilesSupplier { private final Supplier> runfilesInputs; @Nullable private final Artifact manifest; @Nullable private final Artifact repoMappingManifest; + private final RunfileSymlinksMode runfileSymlinksMode; private final boolean buildRunfileLinks; - private final boolean runfileLinksEnabled; /** * Same as {@link SingleRunfilesSupplier#SingleRunfilesSupplier(PathFragment, Runfiles, Artifact, @@ -54,16 +55,16 @@ public static SingleRunfilesSupplier createCaching( PathFragment runfilesDir, Runfiles runfiles, @Nullable Artifact repoMappingManifest, - boolean buildRunfileLinks, - boolean runfileLinksEnabled) { + RunfileSymlinksMode runfileSymlinksMode, + boolean buildRunfileLinks) { return new SingleRunfilesSupplier( runfilesDir, runfiles, - /*runfilesCachingEnabled=*/ true, - /*manifest=*/ null, + /* runfilesCachingEnabled= */ true, + /* manifest= */ null, repoMappingManifest, - buildRunfileLinks, - runfileLinksEnabled); + runfileSymlinksMode, + buildRunfileLinks); } /** @@ -74,8 +75,8 @@ public static SingleRunfilesSupplier createCaching( * @param manifest runfiles' associated runfiles manifest artifact, if present. Important: this * parameter will be used to filter the resulting spawn's inputs to not poison downstream * caches. - * @param buildRunfileLinks whether runfile symlinks are created during build - * @param runfileLinksEnabled whether it's allowed to create runfile symlinks + * @param runfileSymlinksMode how to create runfile symlinks + * @param buildRunfileLinks whether runfile symlinks should be created during the build */ @AutoCodec.Instantiator public SingleRunfilesSupplier( @@ -83,16 +84,16 @@ public SingleRunfilesSupplier( Runfiles runfiles, @Nullable Artifact manifest, @Nullable Artifact repoMappingManifest, - boolean buildRunfileLinks, - boolean runfileLinksEnabled) { + RunfileSymlinksMode runfileSymlinksMode, + boolean buildRunfileLinks) { this( runfilesDir, runfiles, - /*runfilesCachingEnabled=*/ false, + /* runfilesCachingEnabled= */ false, manifest, repoMappingManifest, - buildRunfileLinks, - runfileLinksEnabled); + runfileSymlinksMode, + buildRunfileLinks); } private SingleRunfilesSupplier( @@ -101,8 +102,8 @@ private SingleRunfilesSupplier( boolean runfilesCachingEnabled, @Nullable Artifact manifest, @Nullable Artifact repoMappingManifest, - boolean buildRunfileLinks, - boolean runfileLinksEnabled) { + RunfileSymlinksMode runfileSymlinksMode, + boolean buildRunfileLinks) { this( runfilesDir, runfiles, @@ -110,11 +111,11 @@ private SingleRunfilesSupplier( ? new RunfilesCacher(runfiles, repoMappingManifest) : () -> runfiles.getRunfilesInputs( - /*eventHandler=*/ null, /*location=*/ null, repoMappingManifest), + /* eventHandler= */ null, /* location= */ null, repoMappingManifest), manifest, repoMappingManifest, - buildRunfileLinks, - runfileLinksEnabled); + runfileSymlinksMode, + buildRunfileLinks); } private SingleRunfilesSupplier( @@ -123,16 +124,16 @@ private SingleRunfilesSupplier( Supplier> runfilesInputs, @Nullable Artifact manifest, @Nullable Artifact repoMappingManifest, - boolean buildRunfileLinks, - boolean runfileLinksEnabled) { + RunfileSymlinksMode runfileSymlinksMode, + boolean buildRunfileLinks) { checkArgument(!runfilesDir.isAbsolute()); this.runfilesDir = checkNotNull(runfilesDir); this.runfiles = checkNotNull(runfiles); this.runfilesInputs = checkNotNull(runfilesInputs); this.manifest = manifest; this.repoMappingManifest = repoMappingManifest; + this.runfileSymlinksMode = runfileSymlinksMode; this.buildRunfileLinks = buildRunfileLinks; - this.runfileLinksEnabled = runfileLinksEnabled; } @Override @@ -156,13 +157,17 @@ public ImmutableList getManifests() { } @Override - public boolean isBuildRunfileLinks(PathFragment runfilesDir) { - return buildRunfileLinks && this.runfilesDir.equals(runfilesDir); + @Nullable + public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) { + if (this.runfilesDir.equals(runfilesDir)) { + return runfileSymlinksMode; + } + return null; } @Override - public boolean isRunfileLinksEnabled(PathFragment runfilesDir) { - return runfileLinksEnabled && this.runfilesDir.equals(runfilesDir); + public boolean isBuildRunfileLinks(PathFragment runfilesDir) { + return buildRunfileLinks && this.runfilesDir.equals(runfilesDir); } @Override @@ -175,8 +180,8 @@ public SingleRunfilesSupplier withOverriddenRunfilesDir(PathFragment newRunfiles runfilesInputs, manifest, repoMappingManifest, - buildRunfileLinks, - runfileLinksEnabled); + runfileSymlinksMode, + buildRunfileLinks); } /** Softly caches the result of {@link Runfiles#getRunfilesInputs}. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java index 569cbbdad931c1..65a66241f1b40b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; @@ -48,8 +49,7 @@ public final class SymlinkTreeAction extends AbstractAction { private final Artifact outputManifest; @Nullable private final String filesetRoot; private final ActionEnvironment env; - private final boolean enableRunfiles; - private final boolean inprocessSymlinkCreation; + private final RunfileSymlinksMode runfileSymlinksMode; private final Artifact repoMappingManifest; /** @@ -80,8 +80,7 @@ public SymlinkTreeAction( repoMappingManifest, filesetRoot, config.getActionEnvironment(), - config.runfilesEnabled(), - config.inprocessSymlinkCreation()); + config.getRunfileSymlinksMode()); } /** @@ -106,11 +105,10 @@ public SymlinkTreeAction( @Nullable Artifact repoMappingManifest, @Nullable String filesetRoot, ActionEnvironment env, - boolean enableRunfiles, - boolean inprocessSymlinkCreation) { + RunfileSymlinksMode runfileSymlinksMode) { super( owner, - computeInputs(enableRunfiles, runfiles, inputManifest, repoMappingManifest), + computeInputs(runfileSymlinksMode, runfiles, inputManifest, repoMappingManifest), ImmutableSet.of(outputManifest)); Preconditions.checkArgument(outputManifest.getPath().getBaseName().equals("MANIFEST")); Preconditions.checkArgument( @@ -120,14 +118,13 @@ public SymlinkTreeAction( this.outputManifest = outputManifest; this.filesetRoot = filesetRoot; this.env = env; - this.enableRunfiles = enableRunfiles; - this.inprocessSymlinkCreation = inprocessSymlinkCreation; + this.runfileSymlinksMode = runfileSymlinksMode; this.inputManifest = inputManifest; this.repoMappingManifest = repoMappingManifest; } private static NestedSet computeInputs( - boolean enableRunfiles, + RunfileSymlinksMode runfileSymlinksMode, Runfiles runfiles, Artifact inputManifest, @Nullable Artifact repoMappingManifest) { @@ -136,7 +133,9 @@ private static NestedSet computeInputs( // All current strategies (in-process and build-runfiles-windows) for // making symlink trees on Windows depend on the target files // existing, so directory or file links can be made as appropriate. - if (enableRunfiles && runfiles != null && OS.getCurrent() == OS.WINDOWS) { + if (runfileSymlinksMode != RunfileSymlinksMode.SKIP + && runfiles != null + && OS.getCurrent() == OS.WINDOWS) { inputs.addTransitive(runfiles.getAllArtifacts()); if (repoMappingManifest != null) { inputs.add(repoMappingManifest); @@ -176,12 +175,8 @@ public PathFragment getFilesetRoot() { return PathFragment.create(filesetRoot); } - public boolean isRunfilesEnabled() { - return enableRunfiles; - } - - public boolean inprocessSymlinkCreation() { - return inprocessSymlinkCreation; + public RunfileSymlinksMode getRunfileSymlinksMode() { + return runfileSymlinksMode; } @Override @@ -202,8 +197,7 @@ protected void computeKey( Fingerprint fp) { fp.addString(GUID); fp.addNullableString(filesetRoot); - fp.addBoolean(enableRunfiles); - fp.addBoolean(inprocessSymlinkCreation); + fp.addInt(runfileSymlinksMode.ordinal()); env.addTo(fp); // We need to ensure that the fingerprints for two different instances of this action are // different. Consider the hypothetical scenario where we add a second runfiles object to this diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 6f9854d13980c8..4055f149704521 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.util.RegexFilter; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.common.options.TriState; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -638,13 +639,13 @@ public boolean extendedSanityChecks() { } /** Returns true if we are building runfiles manifests for this configuration. */ - public boolean buildRunfilesManifests() { - return options.buildRunfilesManifests; + public boolean buildRunfileManifests() { + return options.buildRunfileManifests; } /** Returns true if we are building runfile links for this configuration. */ public boolean buildRunfileLinks() { - return options.buildRunfilesManifests && options.buildRunfiles; + return options.buildRunfileManifests && options.buildRunfileLinks; } /** Returns if we are building external runfiles symlinks using the old-style structure. */ @@ -807,30 +808,49 @@ public String getHostCpu() { return options.hostCpu; } - // TODO(buchgr): Revisit naming and functionality of this flag. See #9248 for details. - public static boolean runfilesEnabled(CoreOptions options) { - switch (options.enableRunfiles) { - case YES: - return true; - case NO: - return false; - default: - return OS.getCurrent() != OS.WINDOWS; + /** + * Describes how to create runfile symlink trees. + * + *

May be overridden if an {@link OutputService} capable of creating symlink trees is + * available. + */ + public enum RunfileSymlinksMode { + /** Do not create. */ + SKIP, + /** Use the out-of-process implementation. */ + EXTERNAL, + /** Use the in-process implementation. */ + INTERNAL + } + + @VisibleForTesting + public static RunfileSymlinksMode getRunfileSymlinksMode(CoreOptions options) { + // TODO(buchgr): Revisit naming and functionality of this flag. See #9248 for details. + if (options.enableRunfiles == TriState.YES + || (options.enableRunfiles == TriState.AUTO && OS.getCurrent() != OS.WINDOWS)) { + return options.inProcessSymlinkCreation + ? RunfileSymlinksMode.INTERNAL + : RunfileSymlinksMode.EXTERNAL; } + return RunfileSymlinksMode.SKIP; + } + + public RunfileSymlinksMode getRunfileSymlinksMode() { + return getRunfileSymlinksMode(options); + } + + public static boolean runfilesEnabled(CoreOptions options) { + return getRunfileSymlinksMode(options) != RunfileSymlinksMode.SKIP; } public boolean runfilesEnabled() { - return runfilesEnabled(this.options); + return runfilesEnabled(options); } @Override public boolean runfilesEnabledForStarlark(StarlarkThread thread) throws EvalException { BuiltinRestriction.failIfCalledOutsideBuiltins(thread); - return runfilesEnabled(this.options); - } - - public boolean inprocessSymlinkCreation() { - return options.inprocessSymlinkCreation; + return runfilesEnabled(); } public boolean remotableSourceManifestActions() { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 62b6b7a87002b4..3837c7d6c58fad 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -450,7 +450,7 @@ public ExecConfigurationDistinguisherSchemeConverter() { help = "If true, write runfiles manifests for all targets. If false, omit them. Local tests will" + " fail to run when false.") - public boolean buildRunfilesManifests; + public boolean buildRunfileManifests; @Option( name = "build_runfile_links", @@ -459,8 +459,8 @@ public ExecConfigurationDistinguisherSchemeConverter() { effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, help = "If true, build runfiles symlink forests for all targets. " - + "If false, write only manifests when possible.") - public boolean buildRunfiles; + + "If false, write them only when required by a local action, test or run command.") + public boolean buildRunfileLinks; @Option( name = "legacy_external_runfiles", @@ -881,7 +881,7 @@ public OutputPathsConverter() { metadataTags = OptionMetadataTag.EXPERIMENTAL, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.EXECUTION}, help = "Whether to make direct file system calls to create symlink trees") - public boolean inprocessSymlinkCreation; + public boolean inProcessSymlinkCreation; @Option( name = "experimental_remotable_source_manifests", @@ -974,8 +974,8 @@ public FragmentOptions getExec() { exec.disallowUnsoundDirectoryOutputs = disallowUnsoundDirectoryOutputs; // === Runfiles === - exec.buildRunfilesManifests = buildRunfilesManifests; - exec.buildRunfiles = buildRunfiles; + exec.buildRunfileManifests = buildRunfileManifests; + exec.buildRunfileLinks = buildRunfileLinks; exec.legacyExternalRunfiles = legacyExternalRunfiles; exec.remotableSourceManifestActions = remotableSourceManifestActions; exec.alwaysIncludeFilesToBuildInData = alwaysIncludeFilesToBuildInData; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index fae45ba9e49833..f8b5652fa8aa58 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -387,8 +387,8 @@ private TestParams createTestAction(int shards) runfilesSupport.getRunfilesDirectoryExecPath(), runfilesSupport.getRunfiles(), runfilesSupport.getRepoMappingManifest(), - runfilesSupport.isBuildRunfileLinks(), - runfilesSupport.isRunfilesEnabled()); + runfilesSupport.getRunfileSymlinksMode(), + runfilesSupport.isBuildRunfileLinks()); } else { testRunfilesSupplier = runfilesSupport; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 67509b007bb1d1..f5edc3409bb71e 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -195,6 +195,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:runfiles_supplier", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/util/io:out-err", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", @@ -422,9 +423,9 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/profiler:google-auto-profiler-utils", - "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:output_service", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java index a1de4b2e463509..462740478e4d7d 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java +++ b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java @@ -13,12 +13,15 @@ // limitations under the License. package com.google.devtools.build.lib.exec; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.base.Throwables; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.analysis.RunfilesSupport; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.DigestUtils; @@ -71,9 +74,10 @@ public RunfilesTreeUpdater(Path execRoot, BinTools binTools, XattrProvider xattr public void updateRunfiles( RunfilesSupplier runfilesSupplier, ImmutableMap env, OutErr outErr) throws ExecException, IOException, InterruptedException { - for (Map.Entry> runfiles : + for (Map.Entry> entry : runfilesSupplier.getMappings().entrySet()) { - PathFragment runfilesDir = runfiles.getKey(); + PathFragment runfilesDir = entry.getKey(); + Map runfilesMapping = entry.getValue(); if (runfilesSupplier.isBuildRunfileLinks(runfilesDir)) { continue; } @@ -85,7 +89,11 @@ public void updateRunfiles( // We are the first attempt; update the runfiles tree and mark the future complete. try { updateRunfilesTree( - runfilesDir, env, outErr, !runfilesSupplier.isRunfileLinksEnabled(runfilesDir)); + runfilesDir, + runfilesMapping, + env, + outErr, + checkNotNull(runfilesSupplier.getRunfileSymlinksMode(runfilesDir))); freshFuture.complete(null); } catch (Exception e) { freshFuture.completeExceptionally(e); @@ -111,9 +119,10 @@ public void updateRunfiles( private void updateRunfilesTree( PathFragment runfilesDir, + Map runfilesMapping, ImmutableMap env, OutErr outErr, - boolean manifestOnly) + RunfileSymlinksMode runfileSymlinksMode) throws IOException, ExecException, InterruptedException { Path runfilesDirPath = execRoot.getRelative(runfilesDir); Path inputManifest = RunfilesSupport.inputManifestPath(runfilesDirPath); @@ -144,6 +153,18 @@ private void updateRunfilesTree( SymlinkTreeHelper helper = new SymlinkTreeHelper(inputManifest, runfilesDirPath, /* filesetTree= */ false); - helper.createSymlinks(execRoot, outErr, binTools, env, manifestOnly); + + switch (runfileSymlinksMode) { + case SKIP: + helper.copyManifest(); + break; + case EXTERNAL: + helper.createSymlinksUsingCommand(execRoot, binTools, env, outErr); + break; + case INTERNAL: + helper.createSymlinksDirectly(runfilesDirPath, runfilesMapping); + outputManifest.createSymbolicLink(inputManifest); + break; + } } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java index c95e51bf596ea8..aa42f70fd4e3ad 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java @@ -117,25 +117,6 @@ public void createSymlinksDirectly(Path symlinkTreeRoot, Map shellEnvironment, - boolean manifestOnly) - throws ExecException, InterruptedException { - if (manifestOnly) { - copyManifest(); - } else { - createSymlinksUsingCommand(execRoot, binTools, shellEnvironment, outErr); - } - } - /** Copies the input manifest to the output manifest. */ public void copyManifest() throws ExecException { // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest. diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java index 5aeb59a0aba635..1be078c15fe8e2 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.actions.RunningActionEvent; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeActionContext; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.profiler.AutoProfiler; import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils; import com.google.devtools.build.lib.server.FailureDetails.Execution; @@ -68,6 +69,7 @@ public void createSymlinks( actionExecutionContext.getEventHandler().post(new RunningActionEvent(action, "local")); try (AutoProfiler p = GoogleAutoProfilerUtils.logged("running " + action.prettyPrint(), MIN_LOGGING)) { + // TODO(tjgq): Respect RunfileSymlinksMode.SKIP even in the presence of an OutputService. try { if (outputService != null && outputService.canCreateSymlinkTree()) { Path inputManifest = actionExecutionContext.getInputPath(action.getInputManifest()); @@ -99,10 +101,11 @@ public void createSymlinks( action.getOutputManifest().getExecPath().getParentDirectory()); createOutput(action, actionExecutionContext, inputManifest); - } else if (!action.isRunfilesEnabled()) { + } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.SKIP) { createSymlinkTreeHelper(action, actionExecutionContext).copyManifest(); } else if (action.getInputManifest() == null - || (action.inprocessSymlinkCreation() && !action.isFilesetTree())) { + || (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL + && !action.isFilesetTree())) { try { Map runfiles = runfilesToMap(action); createSymlinkTreeHelper(action, actionExecutionContext) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java index 16860431436982..ecc78aaefa0cfb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java @@ -152,8 +152,8 @@ public Object addEnv(StarlarkRuleContext ruleContext, String runfilesStr, Runfil runfiles, /* manifest= */ null, /* repoMappingManifest= */ null, - ruleContext.getConfiguration().buildRunfileLinks(), - ruleContext.getConfiguration().runfilesEnabled()); + ruleContext.getConfiguration().getRunfileSymlinksMode(), + ruleContext.getConfiguration().buildRunfileLinks()); } // TODO(rlevasseur): Remove once Starlark exposes this directly, see diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index 75e2ac0bbb5b47..4e598021d7632b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -421,7 +421,7 @@ private static BuiltTargets getBuiltTargets( configuration = result.getBuildConfiguration(); } - if (!configuration.buildRunfilesManifests()) { + if (!configuration.buildRunfileManifests()) { throw new RunCommandException( reportAndCreateFailureResult( env, @@ -530,7 +530,7 @@ private static RunCommandLine getCommandLineInfo( // ensureRunfilesBuilt does build the runfiles, but an extra consistency check won't hurt. Preconditions.checkState( settings.getRunfilesSymlinksCreated() - == options.getOptions(CoreOptions.class).buildRunfiles); + == options.getOptions(CoreOptions.class).buildRunfileLinks); ExecutionOptions executionOptions = options.getOptions(ExecutionOptions.class); Path tmpDirRoot = diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplierTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplierTest.java index fc0a713e1495af..65a6277fa301e5 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplierTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplierTest.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; @@ -55,10 +56,10 @@ public void testGetArtifactsWithSingleMapping() { new SingleRunfilesSupplier( PathFragment.create("notimportant"), mkRunfiles(artifacts), - /*manifest=*/ null, - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* manifest= */ null, + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); assertThat(underTest.getArtifacts().toList()).containsExactlyElementsIn(artifacts); } @@ -69,10 +70,10 @@ public void testGetManifestsWhenNone() { new SingleRunfilesSupplier( PathFragment.create("ignored"), Runfiles.EMPTY, - /*manifest=*/ null, - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* manifest= */ null, + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); assertThat(underTest.getManifests()).isEmpty(); } @@ -84,9 +85,9 @@ public void testGetManifestsWhenSupplied() { PathFragment.create("ignored"), Runfiles.EMPTY, manifest, - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); assertThat(underTest.getManifests()).containsExactly(manifest); } @@ -97,9 +98,9 @@ public void withOverriddenRunfilesDir() { PathFragment.create("old"), Runfiles.EMPTY, ActionsTestUtil.createArtifact(rootDir, "manifest"), - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); PathFragment newDir = PathFragment.create("new"); RunfilesSupplier overridden = original.withOverriddenRunfilesDir(newDir); @@ -119,9 +120,9 @@ public void withOverriddenRunfilesDir_noChange_sameObject() { dir, Runfiles.EMPTY, ActionsTestUtil.createArtifact(rootDir, "manifest"), - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); assertThat(original.withOverriddenRunfilesDir(dir)).isSameInstanceAs(original); } @@ -133,9 +134,9 @@ public void cachedMappings() { SingleRunfilesSupplier.createCaching( dir, runfiles, - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); Map> mappings1 = underTest.getMappings(); Map> mappings2 = underTest.getMappings(); @@ -154,9 +155,9 @@ public void cachedMappings_sharedAcrossDirOverrides() { SingleRunfilesSupplier.createCaching( oldDir, runfiles, - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); SingleRunfilesSupplier overridden = original.withOverriddenRunfilesDir(newDir); Map> mappingsOld = original.getMappings(); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java index b6cd9a53dbe1f2..bb0743a6b849f0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.SingleRunfilesSupplier; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.analysis.util.ActionTester; import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory; import com.google.devtools.build.lib.analysis.util.AnalysisTestUtil; @@ -378,8 +379,8 @@ public void testInputManifestsRemovedIfSupplied() throws Exception { Runfiles.EMPTY, manifest, /* repoMappingManifest= */ null, - /* buildRunfileLinks= */ false, - /* runfileLinksEnabled= */ false)) + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false)) .addOutput(getBinArtifactWithNoOwner("output")) .setExecutable(scratch.file("/bin/xxx").asFragment()) .setProgressMessage("Test") @@ -590,8 +591,8 @@ private static RunfilesSupplier runfilesSupplier(Artifact manifest, PathFragment Runfiles.EMPTY, manifest, /* repoMappingManifest= */ null, - /* buildRunfileLinks= */ false, - /* runfileLinksEnabled= */ false); + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); } private ActionOwner nullOwnerWithTargetConfig() { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java index 1e615ec0f90e94..415988777fe5c9 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.Runfiles; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.analysis.util.ActionTester; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import org.junit.Test; @@ -31,16 +32,12 @@ @RunWith(JUnit4.class) public class SymlinkTreeActionTest extends BuildViewTestCase { private enum FilesetActionAttributes { - ENABLE_RUNFILES, - INPROCESS_SYMLINKS, FIXED_ENVIRONMENT, VARIABLE_ENVIRONMENT } private enum RunfilesActionAttributes { RUNFILES, - ENABLE_RUNFILES, - INPROCESS_SYMLINKS, FIXED_ENVIRONMENT, VARIABLE_ENVIRONMENT } @@ -52,40 +49,45 @@ public void testComputeKey() throws Exception { final Artifact runfile = getBinArtifactWithNoOwner("dir/runfile"); final Artifact runfile2 = getBinArtifactWithNoOwner("dir/runfile2"); - new ActionTester(actionKeyContext) - .combinations( - RunfilesActionAttributes.class, - (attributesToFlip) -> - new SymlinkTreeAction( - ActionsTestUtil.NULL_ACTION_OWNER, - inputManifest, - /* runfiles= */ attributesToFlip.contains(RunfilesActionAttributes.RUNFILES) - ? new Runfiles.Builder("TESTING", false).addArtifact(runfile).build() - : new Runfiles.Builder("TESTING", false).addArtifact(runfile2).build(), - outputManifest, - /* repoMappingManifest= */ null, - /* filesetRoot= */ null, - createActionEnvironment( - attributesToFlip.contains(RunfilesActionAttributes.FIXED_ENVIRONMENT), - attributesToFlip.contains(RunfilesActionAttributes.VARIABLE_ENVIRONMENT)), - attributesToFlip.contains(RunfilesActionAttributes.ENABLE_RUNFILES), - attributesToFlip.contains(RunfilesActionAttributes.INPROCESS_SYMLINKS))) - .combinations( - FilesetActionAttributes.class, - (attributesToFlip) -> - new SymlinkTreeAction( - ActionsTestUtil.NULL_ACTION_OWNER, - inputManifest, - /* runfiles= */ null, - outputManifest, - /* repoMappingManifest= */ null, - /* filesetRoot= */ "root", - createActionEnvironment( - attributesToFlip.contains(FilesetActionAttributes.FIXED_ENVIRONMENT), - attributesToFlip.contains(FilesetActionAttributes.VARIABLE_ENVIRONMENT)), - attributesToFlip.contains(FilesetActionAttributes.ENABLE_RUNFILES), - attributesToFlip.contains(FilesetActionAttributes.INPROCESS_SYMLINKS))) - .runTest(); + ActionTester tester = new ActionTester(actionKeyContext); + + for (RunfileSymlinksMode runfileSymlinksMode : RunfileSymlinksMode.values()) { + tester = + tester.combinations( + RunfilesActionAttributes.class, + (attributesToFlip) -> + new SymlinkTreeAction( + ActionsTestUtil.NULL_ACTION_OWNER, + inputManifest, + /* runfiles= */ attributesToFlip.contains(RunfilesActionAttributes.RUNFILES) + ? new Runfiles.Builder("TESTING", false).addArtifact(runfile).build() + : new Runfiles.Builder("TESTING", false).addArtifact(runfile2).build(), + outputManifest, + /* repoMappingManifest= */ null, + /* filesetRoot= */ null, + createActionEnvironment( + attributesToFlip.contains(RunfilesActionAttributes.FIXED_ENVIRONMENT), + attributesToFlip.contains(RunfilesActionAttributes.VARIABLE_ENVIRONMENT)), + runfileSymlinksMode)); + + tester = + tester.combinations( + FilesetActionAttributes.class, + (attributesToFlip) -> + new SymlinkTreeAction( + ActionsTestUtil.NULL_ACTION_OWNER, + inputManifest, + /* runfiles= */ null, + outputManifest, + /* repoMappingManifest= */ null, + /* filesetRoot= */ "root", + createActionEnvironment( + attributesToFlip.contains(FilesetActionAttributes.FIXED_ENVIRONMENT), + attributesToFlip.contains(FilesetActionAttributes.VARIABLE_ENVIRONMENT)), + runfileSymlinksMode)); + } + + tester.runTest(); } private static ActionEnvironment createActionEnvironment(boolean fixed, boolean variable) { @@ -109,7 +111,6 @@ public void testNullRunfilesThrows() { /* repoMappingManifest= */ null, /* filesetRoot= */ null, createActionEnvironment(false, false), - /* enableRunfiles= */ true, - /* inprocessSymlinkCreation= */ false)); + RunfileSymlinksMode.SKIP)); } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java index 33018518572c62..8089d5e7b73be3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Options; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.BuildOptionsView; import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; @@ -544,10 +545,10 @@ public static RunfilesSupplier createRunfilesSupplier( return new SingleRunfilesSupplier( runfilesDir, runfiles, - /*manifest=*/ null, - /*repoMappingManifest=*/ null, - /*buildRunfileLinks=*/ false, - /*runfileLinksEnabled=*/ false); + /* manifest= */ null, + /* repoMappingManifest= */ null, + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false); } public static BuildOptions execOptions(BuildOptions targetOptions, EventHandler handler) diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD index a5f46be0eb79b5..025561243a5e11 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD @@ -33,6 +33,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/analysis/platform", @@ -61,13 +62,11 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:test_log_helper", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/shell", - "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/testing/vfs:spied_filesystem", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:exit_code", - "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/util/io:out-err", "//src/main/java/com/google/devtools/build/lib/vfs", diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java index d7d28c82d5beab..3c94ee7881a175 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeActionContext; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.util.Fingerprint; @@ -107,8 +108,7 @@ public void fingerprint(Fingerprint fingerprint) {} /* repoMappingManifest= */ null, /* filesetRoot= */ null, ActionEnvironment.EMPTY, - /* enableRunfiles= */ true, - /* inprocessSymlinkCreation= */ false); + RunfileSymlinksMode.EXTERNAL); action.execute(context); @@ -163,8 +163,7 @@ public void fingerprint(Fingerprint fingerprint) {} /* repoMappingManifest= */ null, /* filesetRoot= */ null, ActionEnvironment.EMPTY, - /* enableRunfiles= */ true, - /* inprocessSymlinkCreation= */ true); + RunfileSymlinksMode.INTERNAL); action.execute(context); // Check that the OutputService is not used. diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD index ad2deffdfa6701..2ce11e69d3dc62 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -597,6 +597,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:thread_state_receiver", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//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/exec:bin_tools", diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java index b6229b7f5aa89b..08164de32c87d1 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.SingleRunfilesSupplier; import com.google.devtools.build.lib.analysis.actions.SpawnAction; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode; import com.google.devtools.build.lib.analysis.util.ActionTester; import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory; import com.google.devtools.build.lib.analysis.util.AnalysisTestUtil; @@ -217,8 +218,8 @@ public Action generate(ImmutableSet attributesToFlip) { Runfiles.EMPTY, artifactA, /* repoMappingManifest= */ null, - /* buildRunfileLinks= */ false, - /* runfileLinksEnabled= */ false)); + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false)); } else { builder.addRunfilesSupplier( new SingleRunfilesSupplier( @@ -226,8 +227,8 @@ public Action generate(ImmutableSet attributesToFlip) { Runfiles.EMPTY, artifactB, /* repoMappingManifest= */ null, - /* buildRunfileLinks= */ false, - /* runfileLinksEnabled= */ false)); + RunfileSymlinksMode.SKIP, + /* buildRunfileLinks= */ false)); } if (attributesToFlip.contains(KeyAttributes.INPUT)) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java index c0df9d8ca09def..41b49ed2b09d82 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java @@ -2450,6 +2450,6 @@ private boolean keepGoing() { } final boolean buildRunfileManifests() { - return testCase.getRuntimeWrapper().getOptions(CoreOptions.class).buildRunfilesManifests; + return testCase.getRuntimeWrapper().getOptions(CoreOptions.class).buildRunfileManifests; } } From e3e24487484a94332a66c3a490eed67d128df86a Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 18 Aug 2023 05:45:03 -0700 Subject: [PATCH 15/19] Remove optimisation on single, no change split transitions The optimisation breaks Apple rules, becuase the code needs the information provided in the key. PiperOrigin-RevId: 558121414 Change-Id: I936dc86d2aae93df1ffd5e2412f3a7f3523037e3 --- .../analysis/producers/TransitionApplier.java | 20 +------ .../rules/objc/MultiArchBinarySupport.java | 5 +- .../StarlarkAttrTransitionProviderTest.java | 56 +++++++++++++++++++ .../rules/android/AndroidStarlarkTest.java | 7 +-- 4 files changed, 64 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java index 06d686893bc403..c067912de6819f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java @@ -13,8 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.analysis.producers; -import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY; - import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -98,7 +96,7 @@ public StateMachine step(Tasks tasks) throws InterruptedException { return runAfter; } if (!doesStarlarkTransition) { - return convertOptionsToKeys( + return new PlatformMappingApplier( transition.apply( TransitionUtil.restrict(transition, fromConfiguration.getOptions()), eventHandler)); } @@ -144,22 +142,6 @@ private StateMachine applyStarlarkTransition(Tasks tasks) throws InterruptedExce sink.acceptTransitionError(e); return runAfter; } - return convertOptionsToKeys(transitionedOptions); - } - - private StateMachine convertOptionsToKeys(Map transitionedOptions) { - // If there is a single, unchanged value, just outputs the original configuration, stripping any - // transition key. - if (transitionedOptions.size() == 1) { - BuildOptions options = transitionedOptions.values().iterator().next(); - if (options.checksum().equals(fromConfiguration.getOptionsChecksum())) { - sink.acceptTransitionedConfigurations( - ImmutableMap.of(PATCH_TRANSITION_KEY, fromConfiguration)); - return runAfter; - } - } - - // Otherwise, applies a platform mapping to the results. return new PlatformMappingApplier(transitionedOptions); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java index 4159c69d246115..5cf03dd963babb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java @@ -708,9 +708,12 @@ public static AppleLinkingOutputs.TargetTriplet getTargetTriplet(BuildConfigurat * their target triplet (architecture, platform, environment) */ public static Dict getSplitTargetTripletFromCtads( - Map, List> ctads) { + Map, List> ctads) throws EvalException { Dict.Builder result = Dict.builder(); for (Optional splitTransitionKey : ctads.keySet()) { + if (!splitTransitionKey.isPresent()) { + throw new EvalException("unexpected empty key in split transition"); + } TargetTriplet targetTriplet = getTargetTriplet( Iterables.getOnlyElement(ctads.get(splitTransitionKey)).getConfiguration()); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java index 4a0526fcfed75c..d8396d9eecd2ef 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java @@ -330,6 +330,62 @@ public void testStarlarkConfigSplitAttr() throws Exception { getConfiguredTarget("//test/starlark:test"); } + /** + * Tests that split transition key is preserved even when there's a single split with no change. + * + *

Starlark implementation may depend on the value of the key. + */ + @Test + public void testStarlarkSplitTransitionSplitAttrSingleUnchanged() throws Exception { + writeAllowlistFile(); + useConfiguration("--foo=stroopwafel"); + scratch.file( + "test/starlark/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "def transition_func(settings, attr):", + " return {", + " 'amsterdam': {'//command_line_option:foo': 'stroopwafel'},", + " }", + "my_transition = transition(", + " implementation = transition_func,", + " inputs = [],", + " outputs = ['//command_line_option:foo']", + ")", + "def _impl(ctx): ", + " return MyInfo(split_attr_dep = ctx.split_attr.dep)", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'dep': attr.label(cfg = my_transition),", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " }", + ")", + "def _s_impl_e(ctx):", + " return []", + "simple_rule = rule(_s_impl_e)"); + + scratch.file( + "test/starlark/BUILD", + "load('//test/starlark:rules.bzl', 'simple_rule', 'my_rule')", + "my_rule(name = 'test', dep = ':dep')", + "simple_rule(name = 'dep')"); + + @SuppressWarnings("unchecked") + Map splitAttr = + (Map) + getMyInfoFromTarget(getConfiguredTarget("//test/starlark:test")) + .getValue("split_attr_dep"); + assertThat(splitAttr.keySet()).containsExactly("amsterdam"); + assertThat( + getConfiguration(splitAttr.get("amsterdam")) + .getOptions() + .get(DummyTestOptions.class) + .foo) + .isEqualTo("stroopwafel"); + } + @Test public void testFunctionSplitTransitionCheckAttrDeps() throws Exception { writeBasicTestFiles(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java index edbf70b710512b..4fe86fa25bb06c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.testutil.TestConstants; import java.util.List; import java.util.Map; -import net.starlark.java.eval.Starlark; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -153,9 +152,9 @@ public void testAndroidSplitTransition_legacy_no_flags() throws Exception { getMyInfoFromTarget(target).getValue("split_attr_deps"); // Split transition isn't in effect, so the deps are compiled normally (i.e. using --cpu). - assertThat(splitDeps.get(Starlark.NONE)).hasSize(2); - assertThat(getConfiguration(splitDeps.get(Starlark.NONE).get(0)).getCpu()).isEqualTo("k8"); - assertThat(getConfiguration(splitDeps.get(Starlark.NONE).get(1)).getCpu()).isEqualTo("k8"); + assertThat(splitDeps.get("k8")).hasSize(2); + assertThat(getConfiguration(splitDeps.get("k8").get(0)).getCpu()).isEqualTo("k8"); + assertThat(getConfiguration(splitDeps.get("k8").get(1)).getCpu()).isEqualTo("k8"); } } From d443688cc1c1e9f1bf9b1b1bd8cc1057d12917ec Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 18 Aug 2023 06:06:52 -0700 Subject: [PATCH 16/19] Rollforward of https://github.com/bazelbuild/bazel/commit/ad6d4d7f839687548776c7f5f79704bdb6af7d0e: Merge java_binary/java_test with launcher into a single rule Add `//tools/build_defs/java:launcher_label_alias` target that reads native java_launcher/host_java_launcher flag. The target will be later replaced with label_flag. The implementation returns no providers, when flag is unset. Later on this can be solved, by implementing a "sentinel" launcher, that's used when the flag is unset. label_flag acts as a proper alias. Use `//tools/build_defs/java:launcher` as default value for launcher field in Java rules, remove computed default. This replicates the previous behavior. First the value of launcher attribute is used if set. Then the value of the flag is used. If the flag is also not set, the default is used. Handle launcher = None in the macro part. attr.label has a weird behaviour: If set to None, the default value is used (and that is a flag). Drop additional flavours of java_binary/java_tests. They are not needed. Update mocks for unit and integration tests There are further cleanup of the code possible, however probably better to do them in a separate change. NEW: - fixed edge case where launcher is set to None PiperOrigin-RevId: 558125242 Change-Id: I777d9116c6fcf56084b8eb80c022adbb1b347c75 --- .../bazel/java/bazel_java_binary.bzl | 22 ++++---------- .../bazel_java_binary_custom_launcher.bzl | 29 ------------------ .../java/bazel_java_binary_nolauncher.bzl | 29 ------------------ .../bazel/java/bazel_java_binary_nonexec.bzl | 2 +- .../bazel/java/bazel_java_binary_wrapper.bzl | 6 ---- .../builtins_bzl/common/cc/cc_common.bzl | 2 ++ .../builtins_bzl/common/java/java_binary.bzl | 7 +++-- .../common/java/java_binary_wrapper.bzl | 18 +++++------ .../builtins_bzl/common/java/java_helper.bzl | 5 +--- .../common/java/java_semantics.bzl | 1 + .../lib/analysis/mock/BazelAnalysisMock.java | 30 ++++++++++++++++++- 11 files changed, 52 insertions(+), 99 deletions(-) delete mode 100644 src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_custom_launcher.bzl delete mode 100644 src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nolauncher.bzl diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl index 3f1a3b85cc2c67..75e40c3093c1c8 100644 --- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl +++ b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl @@ -247,6 +247,8 @@ def _create_windows_exe_launcher(ctx, java_executable, classpath, main_class, jv launch_info.add_joined(jvm_flags_for_launcher, join_with = "\t", format_joined = "jvm_flags=%s", omit_if_empty = False) jar_bin_path = semantics.find_java_runtime_toolchain(ctx).java_home + "/bin/jar.exe" launch_info.add(jar_bin_path, format = "jar_bin_path=%s") + + # TODO(b/295221112): Change to use the "launcher" attribute (only windows use a fixed _launcher attribute) launcher_artifact = ctx.executable._launcher ctx.actions.run( executable = ctx.executable._windows_launcher_maker, @@ -306,18 +308,12 @@ _BASE_BINARY_ATTRS = merge_attrs( }, ) -def make_java_binary(executable, resolve_launcher_flag, has_launcher = False): +def make_java_binary(executable): return _make_binary_rule( _bazel_java_binary_impl, merge_attrs( _BASE_BINARY_ATTRS, { - "_java_launcher": attr.label( - default = configuration_field( - fragment = "java", - name = "launcher", - ) if resolve_launcher_flag else (_compute_launcher_attr if has_launcher else None), - ), "_use_auto_exec_groups": attr.bool(default = True), }, ({} if executable else { @@ -329,21 +325,15 @@ def make_java_binary(executable, resolve_launcher_flag, has_launcher = False): executable = executable, ) -java_binary = make_java_binary(executable = True, resolve_launcher_flag = True) +java_binary = make_java_binary(executable = True) -def make_java_test(resolve_launcher_flag, has_launcher = False): +def make_java_test(): return _make_binary_rule( _bazel_java_test_impl, merge_attrs( BASE_TEST_ATTRIBUTES, _BASE_BINARY_ATTRS, { - "_java_launcher": attr.label( - default = configuration_field( - fragment = "java", - name = "launcher", - ) if resolve_launcher_flag else (_compute_launcher_attr if has_launcher else None), - ), "_lcov_merger": attr.label( cfg = "exec", default = configuration_field( @@ -366,4 +356,4 @@ def make_java_test(resolve_launcher_flag, has_launcher = False): test = True, ) -java_test = make_java_test(True) +java_test = make_java_test() diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_custom_launcher.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_custom_launcher.bzl deleted file mode 100644 index ccfead1710d736..00000000000000 --- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_custom_launcher.bzl +++ /dev/null @@ -1,29 +0,0 @@ -# Copyright 2022 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. - -"""Defines a java_binary rule class that is executable, and has a custom launcher, -so doesn't have launcher flag resolution. - -There are three physical rule classes for java_binary and we want all of them -to have a name string of "java_binary" because various tooling expects that. -But we also need the rule classes to be defined in separate files. That way the -hash of their bzl environments will be different. See http://b/226379109, -specifically #20, for details. -""" - -load(":bazel/java/bazel_java_binary.bzl", "make_java_binary", "make_java_test") - -java_binary = make_java_binary(executable = True, resolve_launcher_flag = False, has_launcher = True) - -java_test = make_java_test(resolve_launcher_flag = False, has_launcher = True) diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nolauncher.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nolauncher.bzl deleted file mode 100644 index 3d5458d60165e1..00000000000000 --- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nolauncher.bzl +++ /dev/null @@ -1,29 +0,0 @@ -# Copyright 2022 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. - -"""Defines a java_binary rule class that is executable but doesn't have launcher - flag resolution. - -There are three physical rule classes for java_binary and we want all of them -to have a name string of "java_binary" because various tooling expects that. -But we also need the rule classes to be defined in separate files. That way the -hash of their bzl environments will be different. See http://b/226379109, -specifically #20, for details. -""" - -load(":bazel/java/bazel_java_binary.bzl", "make_java_binary", "make_java_test") - -java_binary = make_java_binary(executable = True, resolve_launcher_flag = False) - -java_test = make_java_test(resolve_launcher_flag = False) diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nonexec.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nonexec.bzl index 34247066c696e7..e0b8e373a7df51 100644 --- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nonexec.bzl +++ b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_nonexec.bzl @@ -23,4 +23,4 @@ specifically #20, for details. load(":bazel/java/bazel_java_binary.bzl", "make_java_binary") -java_binary = make_java_binary(executable = False, resolve_launcher_flag = False) +java_binary = make_java_binary(executable = False) diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_wrapper.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_wrapper.bzl index 104e387f1631d5..8d3e700b489350 100644 --- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_wrapper.bzl +++ b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary_wrapper.bzl @@ -19,8 +19,6 @@ the supplied value of the `create_executable` attribute. """ load(":bazel/java/bazel_java_binary.bzl", _java_test = "java_test", java_bin_exec = "java_binary") -load(":bazel/java/bazel_java_binary_nolauncher.bzl", java_bin_exec_no_launcher_flag = "java_binary", java_test_no_launcher = "java_test") -load(":bazel/java/bazel_java_binary_custom_launcher.bzl", java_bin_exec_custom_launcher = "java_binary", java_test_custom_launcher = "java_test") load(":bazel/java/bazel_java_binary_nonexec.bzl", java_bin_nonexec = "java_binary") load(":bazel/java/bazel_java_binary_deploy_jar.bzl", "deploy_jars", "deploy_jars_nonexec") load(":common/java/java_binary_wrapper.bzl", "register_java_binary_rules") @@ -29,8 +27,6 @@ def java_binary(**kwargs): register_java_binary_rules( java_bin_exec, java_bin_nonexec, - java_bin_exec_no_launcher_flag, - java_bin_exec_custom_launcher, rule_deploy_jars = deploy_jars, rule_deploy_jars_nonexec = deploy_jars_nonexec, **kwargs @@ -40,8 +36,6 @@ def java_test(**kwargs): register_java_binary_rules( _java_test, _java_test, - java_test_no_launcher, - java_test_custom_launcher, rule_deploy_jars = deploy_jars, rule_deploy_jars_nonexec = deploy_jars_nonexec, is_test_rule_class = True, diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl index 1b1c6a80de1236..e7b484d8f2fb9d 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl @@ -856,6 +856,8 @@ cc_common = struct( create_lto_backend_artifacts = _create_lto_backend_artifacts, # Google internal methods. create_cc_launcher_info = _create_cc_launcher_info, + # TODO(b/295221112): Remove after migrating launchers to Starlark flags + launcher_provider = _builtins.internal.cc_internal.launcher_provider, objcopy = _objcopy, objcopy_tool_path = _objcopy_tool_path, ld_tool_path = _ld_tool_path, diff --git a/src/main/starlark/builtins_bzl/common/java/java_binary.bzl b/src/main/starlark/builtins_bzl/common/java/java_binary.bzl index 676eb77f895ec4..6786527f579381 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_binary.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_binary.bzl @@ -29,6 +29,7 @@ load( "collect_native_deps_dirs", "get_runtime_classpath_for_archive", ) +load(":common/cc/cc_common.bzl", "cc_common") CcLauncherInfo = _builtins.internal.cc_internal.launcher_provider @@ -97,9 +98,9 @@ def basic_java_binary( ) """ - if not ctx.attr.create_executable and ctx.attr.launcher: + if not ctx.attr.create_executable and (ctx.attr.launcher and cc_common.launcher_provider in ctx.attr.launcher): fail("launcher specified but create_executable is false") - if not ctx.attr.use_launcher and ctx.attr.launcher: + if not ctx.attr.use_launcher and (ctx.attr.launcher and cc_common.launcher_provider in ctx.attr.launcher): fail("launcher specified but use_launcher is false") if not ctx.attr.srcs and ctx.attr.deps: @@ -519,7 +520,7 @@ BASIC_JAVA_BINARY_ATTRIBUTES = merge_attrs( ), "launcher": attr.label( allow_files = False, - providers = [CcLauncherInfo], + # TODO(b/295221112): add back CcLauncherInfo ), "neverlink": attr.bool(), "javacopts": attr.string_list(), diff --git a/src/main/starlark/builtins_bzl/common/java/java_binary_wrapper.bzl b/src/main/starlark/builtins_bzl/common/java/java_binary_wrapper.bzl index d03bcce80bf49d..011f5f844890a8 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_binary_wrapper.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_binary_wrapper.bzl @@ -18,13 +18,13 @@ This is needed since the `executable` nature of the target must be computed from the supplied value of the `create_executable` attribute. """ +load(":common/java/java_semantics.bzl", "semantics") + _DEPLOY_JAR_RULE_NAME_SUFFIX = "_deployjars_internal_rule" def register_java_binary_rules( rule_exec, rule_nonexec, - rule_nolauncher, - rule_customlauncher, rule_deploy_jars = None, rule_deploy_jars_nonexec = None, is_test_rule_class = False, @@ -34,10 +34,6 @@ def register_java_binary_rules( Args: rule_exec: (Rule) The executable java_binary rule rule_nonexec: (Rule) The non-executable java_binary rule - rule_nolauncher: (Rule) The executable java_binary rule without launcher flag resolution - rule_customlauncher: (Rule) The executable java_binary rule with a custom launcher attr set - rule_deploy_jars: (Rule) The auxiliary deploy jars rule for create_executable = True - rule_deploy_jars_nonexec: (Rule) The auxiliary deploy jars rule for create_executable = False is_test_rule_class: (bool) If this is a test rule **kwargs: Actual args to instantiate the rule """ @@ -49,11 +45,13 @@ def register_java_binary_rules( kwargs["stamp"] = 1 if kwargs["stamp"] else 0 if not create_executable: rule_nonexec(**kwargs) - elif "use_launcher" in kwargs and not kwargs["use_launcher"]: - rule_nolauncher(**kwargs) - elif "launcher" in kwargs and type(kwargs["launcher"]) == type(""): - rule_customlauncher(**kwargs) else: + if "use_launcher" in kwargs and not kwargs["use_launcher"]: + kwargs["launcher"] = None + else: + # If launcher is not set or None, set it to config flag + if "launcher" not in kwargs or not kwargs["launcher"]: + kwargs["launcher"] = semantics.LAUNCHER_FLAG_LABEL rule_exec(**kwargs) if not create_executable: diff --git a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl index 2895829093ca75..c19b00f66f521e 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl @@ -47,12 +47,9 @@ def _filter_launcher_for_target(ctx): return None # BUILD rule "launcher" attribute - if hasattr(ctx.attr, "launcher") and ctx.attr.launcher: + if ctx.attr.launcher and cc_common.launcher_provider in ctx.attr.launcher: return ctx.attr.launcher - # Blaze flag --java_launcher - if hasattr(ctx.attr, "_java_launcher") and ctx.attr._java_launcher: - return ctx.attr._java_launcher return None def _launcher_artifact_for_target(ctx): diff --git a/src/main/starlark/builtins_bzl/common/java/java_semantics.bzl b/src/main/starlark/builtins_bzl/common/java/java_semantics.bzl index 597f99d1c5cf36..de08bb0e99be9a 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_semantics.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_semantics.bzl @@ -72,4 +72,5 @@ semantics = struct( IS_BAZEL = True, get_default_resource_path = _get_default_resource_path, compatible_javac_options = _compatible_javac_options, + LAUNCHER_FLAG_LABEL = Label("@bazel_tools//tools/jdk:launcher_flag_alias"), ) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index 5ab335c17ee305..876146a829c3b5 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -148,6 +148,29 @@ public void setupMockClient(MockToolsConfig config, List workspaceConten } config.create("embedded_tools/" + filename, MoreFiles.asCharSource(path, UTF_8).read()); } + config.create( + "embedded_tools/tools/jdk/launcher_flag_alias.bzl", + "_providers = [CcInfo, cc_common.launcher_provider]", + "def _impl(ctx):", + " if not ctx.attr._launcher:", + " return None", + " launcher = ctx.attr._launcher", + " providers = [ctx.attr._launcher[p] for p in _providers]", + " providers.append(DefaultInfo(files = launcher[DefaultInfo].files, runfiles =" + + " launcher[DefaultInfo].default_runfiles))", + " return providers", + "launcher_flag_alias = rule(", + " implementation = _impl,", + " attrs = {", + " '_launcher': attr.label(", + " default = configuration_field(", + " fragment = 'java',", + " name = 'launcher',", + " ),", + " providers = _providers,", + " ),", + " },", + ")"); config.create( "embedded_tools/tools/jdk/BUILD", "load(", @@ -156,6 +179,7 @@ public void setupMockClient(MockToolsConfig config, List workspaceConten " 'java_runtime_alias',", " 'java_host_runtime_alias',", ")", + "load(':launcher_flag_alias.bzl', 'launcher_flag_alias')", "package(default_visibility=['//visibility:public'])", "java_toolchain(", " name = 'toolchain',", @@ -234,7 +258,11 @@ public void setupMockClient(MockToolsConfig config, List workspaceConten " toolchain_type = ':runtime_toolchain_type',", " toolchain = ':jdk',", ")", - "java_plugins_flag_alias(name = 'java_plugins_flag_alias')"); + "java_plugins_flag_alias(name = 'java_plugins_flag_alias')", + "launcher_flag_alias(", + " name = 'launcher_flag_alias',", + " visibility = ['//visibility:public'],", + ")"); config.create( TestConstants.CONSTRAINTS_PATH + "/android/BUILD", From 03b8616b4fdd088c5ad9a9959c2a365e87728d52 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 18 Aug 2023 07:28:53 -0700 Subject: [PATCH 17/19] Add platforms to apple crosstool transition This is needed to enable C++ toolchainisation in Bazel. PiperOrigin-RevId: 558140728 Change-Id: Id0b532de76c9e403790107f6916e8a3a8be23d95 --- src/MODULE.tools | 3 ++ .../builtins_bzl/common/objc/semantics.bzl | 23 +++++++++++ .../builtins_bzl/common/objc/transitions.bzl | 4 +- .../lib/analysis/mock/BazelAnalysisMock.java | 6 ++- .../lib/packages/util/MockObjcSupport.java | 39 ++++++++++++------- .../ApplePlatformsToolchainSelectionTest.java | 4 +- .../lib/rules/objc/J2ObjcLibraryTest.java | 2 - .../rules/objc/ObjcBuildVariablesTest.java | 2 - .../lib/rules/objc/ObjcRuleTestCase.java | 23 ----------- .../build/lib/testutil/TestConstants.java | 3 +- .../target_compatible_with_test.sh | 12 ++++++ 11 files changed, 74 insertions(+), 47 deletions(-) diff --git a/src/MODULE.tools b/src/MODULE.tools index ba2ae270cec29a..d7a7e0d469520a 100644 --- a/src/MODULE.tools +++ b/src/MODULE.tools @@ -37,3 +37,6 @@ use_repo(remote_coverage_tools_extension, "remote_coverage_tools") remote_android_extensions = use_extension("//tools/android:android_extensions.bzl", "remote_android_tools_extensions") use_repo(remote_android_extensions, "android_gmaven_r8", "android_tools") + +# Platforms used by transitions in builtins +bazel_dep(name = "apple_support", version = "1.5.0", repo_name = "build_bazel_apple_support") diff --git a/src/main/starlark/builtins_bzl/common/objc/semantics.bzl b/src/main/starlark/builtins_bzl/common/objc/semantics.bzl index 970e4d2b1612f6..9601361cbab934 100644 --- a/src/main/starlark/builtins_bzl/common/objc/semantics.bzl +++ b/src/main/starlark/builtins_bzl/common/objc/semantics.bzl @@ -16,6 +16,28 @@ load(":common/cc/cc_common.bzl", "cc_common") +_CPU_TO_PLATFORM = { + "darwin_x86_64": "@build_bazel_apple_support//platforms:darwin_x86_64", + "darwin_arm64": "@build_bazel_apple_support//platforms:darwin_arm64", + "darwin_arm64e": "@build_bazel_apple_support//platforms:darwin_arm64e", + "ios_x86_64": "@build_bazel_apple_support//platforms:ios_x86_64", + "ios_arm64": "@build_bazel_apple_support//platforms:ios_arm64", + "ios_sim_arm64": "@build_bazel_apple_support//platforms:ios_sim_arm64", + "ios_arm64e": "@build_bazel_apple_support//platforms:ios_arm64e", + "tvos_sim_arm64": "@build_bazel_apple_support//platforms:tvos_sim_arm64", + "tvos_arm64": "@build_bazel_apple_support//platforms:tvos_arm64", + "tvos_x86_64": "@build_bazel_apple_support//platforms:tvos_x86_64", + "visionos_arm64": "@build_bazel_apple_support//platforms:visionos_arm64", + "visionos_sim_arm64": "@build_bazel_apple_support//platforms:visionos_sim_arm64", + "visionos_x86_64": "@build_bazel_apple_support//platforms:visionos_x86_64", + "watchos_armv7k": "@build_bazel_apple_support//platforms:watchos_armv7k", + "watchos_arm64": "@build_bazel_apple_support//platforms:watchos_arm64", + "watchos_device_arm64": "@build_bazel_apple_support//platforms:watchos_arm64", + "watchos_device_arm64e": "@build_bazel_apple_support//platforms:watchos_arm64e", + "watchos_arm64_32": "@build_bazel_apple_support//platforms:watchos_arm64_32", + "watchos_x86_64": "@build_bazel_apple_support//platforms:watchos_x86_64", +} + def _check_toolchain_supports_objc_compile(ctx, cc_toolchain): feature_configuration = cc_common.configure_features( ctx = ctx, @@ -47,4 +69,5 @@ semantics = struct( get_semantics = _get_semantics, get_repo = _get_repo, get_licenses_attr = _get_licenses_attr, + cpu_to_platform = lambda cpu: _CPU_TO_PLATFORM[cpu], ) diff --git a/src/main/starlark/builtins_bzl/common/objc/transitions.bzl b/src/main/starlark/builtins_bzl/common/objc/transitions.bzl index 67fedd2a6a3700..5f535fdd170e66 100644 --- a/src/main/starlark/builtins_bzl/common/objc/transitions.bzl +++ b/src/main/starlark/builtins_bzl/common/objc/transitions.bzl @@ -14,6 +14,8 @@ """Definition of incoming apple crosstool transition.""" +load(":common/objc/semantics.bzl", "semantics") + transition = _builtins.toplevel.transition def _cpu_string(platform_type, settings): @@ -118,7 +120,7 @@ def _apple_crosstool_transition_impl(settings, attr): return {} # Ensure platforms aren't set so that platform mapping can take place. - return _output_dictionary(settings, cpu, platform_type, []) + return _output_dictionary(settings, cpu, platform_type, semantics.cpu_to_platform(cpu)) _apple_rule_base_transition_inputs = [ "//command_line_option:apple configuration distinguisher", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index 876146a829c3b5..65d8fcff859e45 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -63,6 +63,7 @@ public ImmutableList getWorkspaceContents(MockToolsConfig config) { String androidGmavenR8Workspace = config.getPath("android_gmaven_r8").getPathString(); String localConfigPlatformWorkspace = config.getPath("local_config_platform_workspace").getPathString(); + String appleSupport = config.getPath("build_bazel_apple_support").getPathString(); return ImmutableList.of( "# __SKIP_WORKSPACE_PREFIX__", @@ -85,6 +86,7 @@ public ImmutableList getWorkspaceContents(MockToolsConfig config) { "local_repository(name = 'rules_java', path = '" + rulesJavaWorkspace + "')", "local_repository(name = 'rules_java_builtin', path = '" + rulesJavaWorkspace + "')", "local_repository(name = 'android_gmaven_r8', path = '" + androidGmavenR8Workspace + "')", + "local_repository(name = 'build_bazel_apple_support', path = '" + appleSupport + "')", "register_toolchains('@rules_java//java/toolchains/runtime:all')", "register_toolchains('@rules_java//java/toolchains/javac:all')", "bind(name = 'android/sdk', actual='@bazel_tools//tools/android:sdk')", @@ -111,7 +113,8 @@ public ImmutableList getWorkspaceRepos() { "local_config_xcode", "platforms", "rules_java", - "rules_java_builtin"); + "rules_java_builtin", + "build_bazel_apple_support"); } @Override @@ -138,6 +141,7 @@ public void setupMockClient(MockToolsConfig config, List workspaceConten "local_config_platform_workspace/WORKSPACE", "workspace(name = 'local_config_platform')"); config.create( "local_config_platform_workspace/MODULE.bazel", "module(name = 'local_config_platform')"); + config.create("build_bazel_apple_support/WORKSPACE", "workspace(name = 'apple_support')"); config.create("embedded_tools/WORKSPACE", "workspace(name = 'bazel_tools')"); Runfiles runfiles = Runfiles.create(); for (String filename : diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java index 03a5ae7424badb..6b3ee08c5bdfb3 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java @@ -141,7 +141,7 @@ public static void setup(MockToolsConfig config) throws IOException { // Any device, simulator or maccatalyst platforms created by Apple tests should consider // building on one of these targets as parents, to ensure that the proper constraints are set. config.create( - TestConstants.CONSTRAINTS_PATH + "/apple/BUILD", + TestConstants.APPLE_PLATFORM_PATH + "/BUILD", "package(default_visibility=['//visibility:public'])", "licenses(['notice'])", "platform(", @@ -157,22 +157,31 @@ public static void setup(MockToolsConfig config) throws IOException { " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "os:ios',", " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:arm64',", " ],", - ")", - "platform(", - " name = 'ios_x86_64',", - " constraint_values = [", - " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "os:ios',", - " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:x86_64',", - " ],", - ")", - "platform(", - " name = 'watchos_x86_64',", - " constraint_values = [", - " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "os:watchos',", - " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:x86_64',", - " ],", ")"); + String[] simulatorPlatforms = { + "platform(", + " name = 'ios_x86_64',", + " constraint_values = [", + " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "os:ios',", + " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:x86_64',", + " ],", + ")", + "platform(", + " name = 'watchos_x86_64',", + " constraint_values = [", + " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "os:watchos',", + " '" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:x86_64',", + " ],", + ")" + }; + + if (TestConstants.PRODUCT_NAME.equals("bazel")) { + config.append(TestConstants.APPLE_PLATFORM_PATH + "/BUILD", simulatorPlatforms); + } else { + config.create(TestConstants.APPLE_PLATFORM_PATH + "/simulator/BUILD", simulatorPlatforms); + } + for (String tool : ImmutableSet.of("objc_dummy.mm", "gcov", "testrunner", "mcov", "libtool")) { config.create(TestConstants.TOOLS_REPOSITORY_SCRATCH + "tools/objc/" + tool); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ApplePlatformsToolchainSelectionTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ApplePlatformsToolchainSelectionTest.java index f479cc34213f5a..d08b7bab5580c6 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ApplePlatformsToolchainSelectionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ApplePlatformsToolchainSelectionTest.java @@ -59,7 +59,7 @@ public void testMacOsToolchainSetup() throws Exception { // Verify the macOS platform. ConfiguredTarget darwinPlatform = - getConfiguredTarget(TestConstants.CONSTRAINTS_PACKAGE_ROOT + "apple:darwin_x86_64"); + getConfiguredTarget(TestConstants.APPLE_PLATFORM_PACKAGE_ROOT + ":darwin_x86_64"); PlatformInfo darwinPlatformInfo = PlatformProviderUtils.platform(darwinPlatform); assertThat(darwinPlatformInfo).isNotNull(); } @@ -81,7 +81,7 @@ public void testIosDeviceToolchainSetup() throws Exception { // Verify the iOS 64 bit device platform. ConfiguredTarget iosDevicePlatform = - getConfiguredTarget(TestConstants.CONSTRAINTS_PACKAGE_ROOT + "apple:ios_arm64"); + getConfiguredTarget(TestConstants.APPLE_PLATFORM_PACKAGE_ROOT + ":ios_arm64"); PlatformInfo iosDevicePlatformInfo = PlatformProviderUtils.platform(iosDevicePlatform); assertThat(iosDevicePlatformInfo).isNotNull(); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/J2ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/J2ObjcLibraryTest.java index d071df3bce1e49..192bff4719abe2 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/J2ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/J2ObjcLibraryTest.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.packages.util.MockJ2ObjcSupport; -import com.google.devtools.build.lib.packages.util.MockObjcSupport; import com.google.devtools.build.lib.packages.util.MockProtoSupport; import com.google.devtools.build.lib.testutil.TestConstants; import java.util.Collection; @@ -74,7 +73,6 @@ public final void setup() throws Exception { " name = 'transpile',", " tags = ['__J2OBJC_LIBRARY_MIGRATION_DO_NOT_USE_WILL_BREAK__'],", " deps = ['test'])"); - MockObjcSupport.setup(mockToolsConfig); MockJ2ObjcSupport.setup(mockToolsConfig); MockProtoSupport.setup(mockToolsConfig); diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBuildVariablesTest.java index a0b7003859c3db..f752f01f5c9a81 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBuildVariablesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBuildVariablesTest.java @@ -74,7 +74,6 @@ protected void useConfiguration(String... args) throws Exception { @Test public void testAppleBuildVariablesIos() throws Exception { - MockObjcSupport.setup(mockToolsConfig); useConfiguration( "--crosstool_top=//tools/osx/crosstool", "--xcode_version=5.8", "--ios_minimum_os=12.345", "--watchos_minimum_os=11.111", @@ -145,7 +144,6 @@ public void testAppleBuildVariablesWatchos() throws Exception { @Test public void testDefaultBuildVariablesIos() throws Exception { - MockObjcSupport.setup(mockToolsConfig); useConfiguration( "--apple_platform_type=ios", "--crosstool_top=//tools/osx/crosstool", "--cpu=ios_x86_64"); scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java index 20a3b8068f1f7b..8e74338c4202d0 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java @@ -136,29 +136,6 @@ protected String configurationBin( + "bin/"; } - /** - * Returns the genfiles dir for artifacts built for a given Apple architecture and minimum OS - * version (as set by a configuration transition) and configuration distinguisher but the global - * default for {@code --cpu}. - * - * @param arch the given Apple architecture which artifacts are built under this configuration. - * Note this will likely be different than the value of {@code --cpu}. - * @param configurationDistinguisher the configuration distinguisher used to describe the a - * configuration transition - * @param minOsVersion the minimum os version for which to compile artifacts in the configuration - */ - protected String configurationGenfiles( - String arch, - ConfigurationDistinguisher configurationDistinguisher, - DottedVersion minOsVersion) { - return configurationDir( - arch, configurationDistinguisher, minOsVersion, CompilationMode.FASTBUILD) - + getTargetConfiguration() - .getGenfilesDirectory(RepositoryName.MAIN) - .getExecPath() - .getBaseName(); - } - @SuppressWarnings("MissingCasesInEnumSwitch") private String configurationDir( String arch, diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java b/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java index 4a6659c2367617..f89e204eec3dcc 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java @@ -140,7 +140,8 @@ private TestConstants() { + " - deps(" + TOOLS_REPOSITORY + "//tools/cpp:current_cc_toolchain)" + " - deps(" + TOOLS_REPOSITORY + "//tools/cpp:grep-includes)"; - public static final String PLATFORM_PACKAGE_ROOT = "@bazel_tools//platforms"; + public static final String APPLE_PLATFORM_PATH = "build_bazel_apple_support/platforms"; + public static final String APPLE_PLATFORM_PACKAGE_ROOT = "@build_bazel_apple_support//platforms"; public static final String CONSTRAINTS_PACKAGE_ROOT = "@platforms//"; public static final String LOCAL_CONFIG_PLATFORM_PACKAGE_ROOT = "@local_config_platform//"; diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh index aaba612646a223..19dc829d41880c 100755 --- a/src/test/shell/integration/target_compatible_with_test.sh +++ b/src/test/shell/integration/target_compatible_with_test.sh @@ -1107,6 +1107,18 @@ EOF # for https://github.com/bazelbuild/bazel/issues/12897. function test_incompatible_with_missing_toolchain() { set_up_custom_toolchain + cat >> WORKSPACE <<'EOF' +local_repository(name = 'build_bazel_apple_support', path = 'build_bazel_apple_support') +EOF + mkdir -p build_bazel_apple_support/platforms + touch build_bazel_apple_support/WORKSPACE + cat > build_bazel_apple_support/platforms/BUILD <<'EOF' +package(default_visibility=["//visibility:public"]) +platform( + name = "darwin_x86_64", + constraint_values = ["@platforms//os:macos", "@platforms//cpu:x86_64"], +) +EOF cat >> target_skipping/BUILD <<'EOF' load( From cc5889c4325f2f8bb5c18faa672fd10e3cc59f3a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 18 Aug 2023 09:14:59 -0700 Subject: [PATCH 18/19] Make module extension tag's `debugPrint` useful for error messages When passed to `print` or `fail`, a module extension tag now results in a string such as `'foo' tag at /ws/MODULE.bazel:3:4`, which can be used to form error messages referencing tags without leaking non-hermetic information to the extension implementation function. Closes #19274. PiperOrigin-RevId: 558163928 Change-Id: If20086c62356bb0635d0a4bdf91029267122f62b --- .../lib/bazel/bzlmod/StarlarkBazelModule.java | 7 +++- .../lib/bazel/bzlmod/TypeCheckedTag.java | 24 +++++++++++-- .../bzlmod/ModuleExtensionResolutionTest.java | 36 +++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java index 3f28dbe288bc34..af13cff8902421 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java @@ -54,7 +54,12 @@ public class StarlarkBazelModule implements StarlarkValue { "Contains the tags in a module for the module extension currently being processed. This" + " object has a field for each tag class of the extension, and the value of the" + " field is a list containing an object for each tag instance. This \"tag instance\"" - + " object in turn has a field for each attribute of the tag class.") + + " object in turn has a field for each attribute of the tag class.\n\n" + + "When passed as positional arguments to print() or fail()" + + ", tag instance objects turn into a meaningful string representation of the" + + " form \"'install' tag at /home/user/workspace/MODULE.bazel:3:4\". This can be used" + + " to construct error messages that point to the location of the tag in the module" + + " file, e.g. fail(\"Conflict between\", tag1, \"and\", tag2).") static class Tags implements Structure { private final ImmutableMap> typeCheckedTags; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java index 55595a67ef58f0..b9601be57ff8b9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java @@ -23,8 +23,11 @@ import javax.annotation.Nullable; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Printer; +import net.starlark.java.eval.StarlarkSemantics; import net.starlark.java.eval.Structure; import net.starlark.java.spelling.SpellChecker; +import net.starlark.java.syntax.Location; /** * A {@link Tag} whose attribute values have been type-checked against the attribute schema define @@ -36,10 +39,21 @@ public class TypeCheckedTag implements Structure { private final Object[] attrValues; private final boolean devDependency; - private TypeCheckedTag(TagClass tagClass, Object[] attrValues, boolean devDependency) { + // The properties below are only used for error reporting. + private final Location location; + private final String tagClassName; + + private TypeCheckedTag( + TagClass tagClass, + Object[] attrValues, + boolean devDependency, + Location location, + String tagClassName) { this.tagClass = tagClass; this.attrValues = attrValues; this.devDependency = devDependency; + this.location = location; + this.tagClassName = tagClassName; } /** Creates a {@link TypeCheckedTag}. */ @@ -97,7 +111,8 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked()); } } - return new TypeCheckedTag(tagClass, attrValues, tag.isDevDependency()); + return new TypeCheckedTag( + tagClass, attrValues, tag.isDevDependency(), tag.getLocation(), tag.getTagName()); } /** @@ -133,4 +148,9 @@ public ImmutableCollection getFieldNames() { public String getErrorMessageForUnknownField(String field) { return "unknown attribute " + field; } + + @Override + public void debugPrint(Printer printer, StarlarkSemantics semantics) { + printer.append(String.format("'%s' tag at %s", tagClassName, location)); + } } 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 d8759fb0d8593f..b5b4259beb80eb 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 @@ -2353,4 +2353,40 @@ public void isDevDependency_usages() throws Exception { assertThat(result.get(skyKey).getModule().getGlobal("ext2_data")).isEqualTo("ext2: False"); assertThat(result.get(skyKey).getModule().getGlobal("ext3_data")).isEqualTo("ext3: True"); } + + @Test + public void printAndFailOnTag() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "ext = use_extension('//:defs.bzl', 'ext')", + "ext.foo()", + "ext.foo()"); + scratch.file( + workspaceRoot.getRelative("defs.bzl").getPathString(), + "repo = repository_rule(lambda ctx: True)", + "def _ext_impl(ctx):", + " tag1 = ctx.modules[0].tags.foo[0]", + " tag2 = ctx.modules[0].tags.foo[1]", + " print('Conflict between', tag1, 'and', tag2)", + " fail('Fatal conflict between', tag1, 'and', tag2)", + "foo = tag_class()", + "ext = module_extension(implementation=_ext_impl,tag_classes={'foo':foo})"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + + ModuleExtensionId extensionId = + ModuleExtensionId.create(Label.parseCanonical("//:defs.bzl"), "ext", Optional.empty()); + reporter.removeHandler(failFastHandler); + var result = + evaluator.evaluate( + ImmutableList.of(SingleExtensionEvalValue.key(extensionId)), evaluationContext); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "Fatal conflict between 'foo' tag at /ws/MODULE.bazel:2:8 and 'foo' tag at " + + "/ws/MODULE.bazel:3:8", + ImmutableSet.of(EventKind.ERROR)); + assertContainsEvent( + "Conflict between 'foo' tag at /ws/MODULE.bazel:2:8 and 'foo' tag at /ws/MODULE.bazel:3:8", + ImmutableSet.of(EventKind.DEBUG)); + } } From 77a295485de60ffee57446ec52a74fe639e49628 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 18 Aug 2023 09:15:51 -0700 Subject: [PATCH 19/19] Internal change. PiperOrigin-RevId: 558164163 Change-Id: Ie10e5c10de483fb99f560219e0a55f9eacd094dc --- .../devtools/build/skyframe/IncrementalInMemoryNodeEntry.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java index 060c07dfbacf3c..ce4e784507cf7f 100644 --- a/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java @@ -157,6 +157,7 @@ public synchronized Set getInProgressReverseDeps() { * and the value. */ @Override + @CanIgnoreReturnValue public synchronized Set setValue( SkyValue value, Version graphVersion, @Nullable Version maxTransitiveSourceVersion) throws InterruptedException {