From 00410b7452fe39056b6d487b25b2f91abf975301 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 05:47:10 -0700 Subject: [PATCH 001/710] [Skymeld] Make skymeld incompatible with dumping an aquery dump after builds. We may decide to remove this feature altogether. Let's make them incompatible for now. PiperOrigin-RevId: 529065302 Change-Id: Ib0036ffc84c26870bb86438e704455a86da1040a --- .../devtools/build/lib/skyframe/SkymeldModule.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldModule.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldModule.java index 5b57a41637e3aa..9a269ef0268d04 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldModule.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldModule.java @@ -84,6 +84,16 @@ boolean determineIfMergingAnalysisExecution(CommandEnvironment env) { + " and its value will be ignored.")); effectiveValue = false; } + if (effectiveValue + && (buildRequestOptions.aqueryDumpAfterBuildFormat != null + || buildRequestOptions.aqueryDumpAfterBuildOutputFile != null)) { + env.getReporter() + .handle( + Event.warn( + "--experimental_merged_skyframe_analysis_execution is incompatible with" + + " generating an aquery dump after builds and its value will be ignored.")); + effectiveValue = false; + } return effectiveValue; } From 29058527ed12118dded48a9a985c431147b98bd4 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 05:54:29 -0700 Subject: [PATCH 002/710] Delay loading the async_profiler JNI until --experimental_command_profile is first used. Note that AsyncProfiler.getInstance() already returns a singleton, so we don't need to memoize it in CommandProfilerModule. PiperOrigin-RevId: 529066488 Change-Id: I6f676a59bec921d6f1fdae134d4ee8154353246c --- .../lib/profiler/CommandProfilerModule.java | 40 ++++++++++++------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/profiler/CommandProfilerModule.java b/src/main/java/com/google/devtools/build/lib/profiler/CommandProfilerModule.java index c3d9a13e5488df..666a27bbfa0b17 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/CommandProfilerModule.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/CommandProfilerModule.java @@ -36,19 +36,6 @@ public class CommandProfilerModule extends BlazeModule { private static final Duration PROFILING_INTERVAL = Duration.ofMillis(10); - @Nullable private static final AsyncProfiler profiler; - - static { - AsyncProfiler profilerInstance = null; - try { - profilerInstance = AsyncProfiler.getInstance(); - } catch (Throwable t) { - // Loading the JNI must be allowed to fail, as we might be running on an unsupported platform. - logger.atWarning().withCause(t).log("Failed to load async_profiler JNI"); - } - profiler = profilerInstance; - } - /** CommandProfilerModule options. */ public static final class Options extends OptionsBase { @Option( @@ -80,7 +67,13 @@ public void beforeCommand(CommandEnvironment env) { outputBase = env.getBlazeWorkspace().getOutputBase(); reporter = env.getReporter(); - if (profiler == null || !captureCommandProfile) { + if (!captureCommandProfile) { + // Early exit so we don't attempt to load the JNI unless necessary. + return; + } + + AsyncProfiler profiler = getProfiler(); + if (profiler == null) { return; } @@ -101,7 +94,13 @@ public void beforeCommand(CommandEnvironment env) { @Override public void afterCommand() { - if (profiler == null || !captureCommandProfile) { + if (!captureCommandProfile) { + // Early exit so we don't attempt to load the JNI unless necessary. + return; + } + + AsyncProfiler profiler = getProfiler(); + if (profiler == null) { return; } @@ -113,6 +112,17 @@ public void afterCommand() { outputPath = null; } + @Nullable + private static AsyncProfiler getProfiler() { + try { + return AsyncProfiler.getInstance(); + } catch (Throwable t) { + // Loading the JNI must be allowed to fail, as we might be running on an unsupported platform. + logger.atWarning().withCause(t).log("Failed to load async_profiler JNI"); + } + return null; + } + private static String getProfilerCommand(Path outputPath) { // See https://github.com/async-profiler/async-profiler/blob/master/src/arguments.cpp. return String.format( From e36c949d8ae8362fe622eb3ab6f7a6192f1a847c Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 06:14:35 -0700 Subject: [PATCH 003/710] Remove unused method `disableSandboxing` on `SpawnAction.Builder`. PiperOrigin-RevId: 529070032 Change-Id: I4518e07c119f96933be0bf8d67180e97f46f59a4 --- .../build/lib/analysis/actions/SpawnAction.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index cc75fab4e4c96a..608499fbb20516 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -617,7 +617,6 @@ public static class Builder { private CharSequence progressMessage; private String mnemonic = "Unknown"; - private boolean disableSandboxing = false; private String execGroup = DEFAULT_EXEC_GROUP_NAME; private boolean stripOutputPaths = false; @@ -722,13 +721,6 @@ private SpawnAction buildSpawnAction( .addTransitive(tools) .build(); - if (disableSandboxing) { - ImmutableMap.Builder builder = ImmutableMap.builder(); - builder.putAll(executionInfo); - builder.put("nosandbox", "1"); - executionInfo = builder.buildOrThrow(); - } - return createSpawnAction( owner, tools, @@ -1292,12 +1284,6 @@ public Builder stripOutputPaths(boolean stripPaths) { return this; } - @CanIgnoreReturnValue - public Builder disableSandboxing() { - this.disableSandboxing = true; - return this; - } - /** * Sets the exec group for this action by name. This does not check that {@code execGroup} is * being set to a valid exec group (i.e. one that actually exists). This method expects callers From cc7848ad865d58c8342af17e6f0a29d4a2f64a48 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 06:25:06 -0700 Subject: [PATCH 004/710] Disable the most time-consuming tests in presubmit on macOS arm64 platform. Waiting for an available macOS arm64 machine is often the bottleneck of Bazel presubmit during busy time because we have limited number of machines. This change disabled the top 50 most time-consuming tests in presubmit, but they are still enabled in postsubmit where we can catch breakages after the submitting the change. PiperOrigin-RevId: 529071647 Change-Id: Ia62a81afcd1053831bf6a60f9ba6872967585e94 --- .bazelci/presubmit.yml | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index b45ef7fd8962fe..84a688c4c6d4d5 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -302,6 +302,60 @@ tasks: - "-//scripts/docs:rewriter_test" # https://github.com/bazelbuild/bazel/issues/17007 - "-//src/test/java/com/google/devtools/build/lib/platform:SystemMemoryPressureEventTest" + # Disable the most time-consuming tests on macOS arm64 platform in presubmit. + # To run any of the following test in presubmit, just comment out the corresponding line. + # TODO(pcloudy): Re-enable when Apple Silicon machine waiting time is no longer the major + # bottleneck for presubmit. + - "-//src/test/shell/bazel:bazel_determinism_test" + - "-//src/test/shell/bazel:jdeps_test" + - "-//src/test/shell/bazel:starlark_git_repository_test" + - "-//src/test/shell/bazel:bazel_bootstrap_distfile_test" + - "-//src/test/shell/bazel:bazel_bootstrap_distfile_tar_test" + - "-//src/test/shell/integration:bazel_json_worker_test" + - "-//src/test/py/bazel:runfiles_test" + - "-//src/test/py/bazel:launcher_test" + - "-//src/test/py/bazel:bazel_module_test" + - "-//src/test/shell/bazel:bazel_java_test_jdk11_toolchain_head" + - "-//src/test/shell/integration:target_compatible_with_test" + - "-//src/test/py/bazel:py_test" + - "-//src/test/py/bazel:bzlmod_query_test" + - "-//src/test/py/bazel:cc_import_test" + - "-//src/test/shell/bazel/remote:build_without_the_bytes_test" + - "-//src/test/shell/bazel:bazel_coverage_java_test" + - "-//src/test/py/bazel:bazel_yanked_versions_test" + - "-//src/test/shell/bazel:bazel_coverage_java_jdk17_toolchain_head_test" + - "-//src/test/shell/bazel:bazel_proto_library_test" + - "-//src/test/shell/bazel:bazel_java_tools_test" + - "-//src/test/shell/integration:build_event_stream_test" + - "-//src/test/py/bazel:bazel_overrides_test" + - "-//src/test/shell/bazel:cc_integration_test" + - "-//src/test/shell/bazel:bazel_java_test" + - "-//src/test/java/com/google/devtools/build/lib/rules/config:ConfigRulesTests" + - "-//src/test/shell/bazel:bazel_java_test_jdk17_toolchain_head" + - "-//src/test/shell/integration:bazel_worker_test" + - "-//src/test/shell/bazel:tags_propagation_native_test" + - "-//src/test/shell/bazel:bazel_coverage_java_jdk11_toolchain_head_test" + - "-//src/test/shell/integration:bazel_sandboxed_worker_test" + - "-//src/test/py/bazel:bazel_repo_mapping_test" + - "-//src/test/shell/bazel:bazel_coverage_java_jdk11_toolchain_released_test" + - "-//src/test/py/bazel:bazel_workspace_test" + - "-//src/test/shell/bazel:execroot_test" + - "-//src/test/shell/bazel:starlark_repository_test" + - "-//src/test/shell/bazel:bazel_coverage_java_jdk17_toolchain_released_test" + - "-//src/test/shell/bazel/remote:remote_execution_test" + - "-//src/test/shell/integration:toolchain_test" + - "-//src/test/shell/bazel:bazel_test_test" + - "-//src/test/shell/integration:test_test" + - "-//src/test/shell/bazel:bazel_execlog_test" + - "-//src/test/shell/integration:modify_execution_info_test" + - "-//src/test/shell/bazel:external_integration_test" + - "-//src/test/py/bazel:bazel_external_repository_test" + - "-//src/test/shell/bazel:bazel_workspaces_test" + - "-//src/test/shell/integration:client_test" + - "-//src/test/shell/bazel:workspace_resolved_test" + - "-//src/test/shell/bazel:bazel_repository_cache_test" + - "-//src/test/shell/integration:aquery_test" + - "-//src/test/shell/integration:py_args_escaping_test" include_json_profile: - build - test From 0c724d465a8a2199bd091d66e0982a99513aaed0 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 06:39:16 -0700 Subject: [PATCH 005/710] Move the `sandbox_stash` directory into `outputBase` and clean it on `clean`. This prevents the `afterCommand` sandbox cleaning from interfering with the stash, but lets it be cleaned properly when running `bazel clean`. This might also help with some interactions with the async cleaner. PiperOrigin-RevId: 529074192 Change-Id: I1d43f97448f6e3b61b3e547d7d6a6a377236bcda --- .../google/devtools/build/lib/sandbox/BUILD | 1 + .../build/lib/sandbox/SandboxModule.java | 8 +- .../build/lib/sandbox/SandboxStash.java | 43 ++++++++-- src/test/shell/integration/sandboxing_test.sh | 86 +++++++++++++++++++ 4 files changed, 129 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD index 5f550ca376da4d..f297f772d5be2c 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD +++ b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD @@ -125,6 +125,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/exec/local:options", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/runtime/commands/events", "//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", diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java index a24a551c237a4b..31b95e11e3d39d 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.ProcessWrapper; +import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Sandbox; import com.google.devtools.build.lib.util.AbruptExitException; @@ -218,7 +219,7 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil treeDeleter = new AsynchronousTreeDeleter(); } } - SandboxStash.initialize(env.getWorkspaceName(), sandboxBase, options); + SandboxStash.initialize(env.getWorkspaceName(), env.getOutputBase(), options); Path mountPoint = sandboxBase.getRelative("sandboxfs"); if (sandboxfsProcess != null) { @@ -570,6 +571,11 @@ public void buildInterrupted(@SuppressWarnings("unused") BuildInterruptedEvent e unmountSandboxfs(); } + @Subscribe + public void cleanStarting(@SuppressWarnings("unused") CleanStartingEvent event) { + SandboxStash.clean(treeDeleter, env.getOutputBase()); + } + /** * Best-effort cleanup of the sandbox base assuming all per-spawn contents have been removed. * diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java index ec10ca909a9a07..4c2e735b8290f5 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.sandbox; import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.exec.TreeDeleter; import com.google.devtools.build.lib.vfs.Path; import java.io.FileNotFoundException; import java.io.IOException; @@ -34,7 +35,7 @@ public class SandboxStash { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); /** An incrementing count of stashes to avoid filename clashes. */ - static AtomicInteger stash = new AtomicInteger(0); + static final AtomicInteger stash = new AtomicInteger(0); /** If true, we have already warned about an error causing us to turn off reuse. */ private final AtomicBoolean warnedAboutTurningOffReuse = new AtomicBoolean(); @@ -47,11 +48,11 @@ public class SandboxStash { private static SandboxStash instance; private final String workspaceName; - private final Path sandboxBase; + private final Path outputBase; - public SandboxStash(String workspaceName, Path sandboxBase) { + public SandboxStash(String workspaceName, Path outputBase) { this.workspaceName = workspaceName; - this.sandboxBase = sandboxBase; + this.outputBase = outputBase; } static boolean takeStashedSandbox(Path sandboxPath, String mnemonic) { @@ -128,7 +129,7 @@ private boolean stashSandboxInternal(Path path, String mnemonic) { */ @Nullable private Path getSandboxStashDir(String mnemonic) { - Path stashDir = getStashBase(); + Path stashDir = getStashBase(this.outputBase); try { stashDir.createDirectory(); if (!maybeClearExistingStash(stashDir)) { @@ -150,8 +151,8 @@ private Path getSandboxStashDir(String mnemonic) { } } - private Path getStashBase() { - return this.sandboxBase.getChild("sandbox_stash"); + private static Path getStashBase(Path outputBase1) { + return outputBase1.getChild("sandbox_stash"); } /** @@ -189,7 +190,7 @@ public static void initialize(String workspaceName, Path sandboxBase, SandboxOpt if (instance == null) { instance = new SandboxStash(workspaceName, sandboxBase); } else if (!Objects.equals(workspaceName, instance.workspaceName)) { - Path stashBase = instance.getStashBase(); + Path stashBase = getStashBase(instance.outputBase); try { for (Path directoryEntry : stashBase.getDirectoryEntries()) { directoryEntry.deleteTree(); @@ -204,4 +205,30 @@ public static void initialize(String workspaceName, Path sandboxBase, SandboxOpt instance = null; } } + + /** Cleans up the entire current stash, if any. Cleaning may be asynchronous. */ + static void clean(TreeDeleter treeDeleter, Path outputBase) { + Path stashDir = getStashBase(outputBase); + if (!stashDir.isDirectory()) { + return; + } + Path stashTrashDir = stashDir.getChild("__trash"); + try { + stashDir.renameTo(stashTrashDir); + } catch (IOException e) { + // If we couldn't move the stashdir away for deletion, we need to delete it synchronously + // in place, so we can't use the treeDeleter. + treeDeleter = null; + stashTrashDir = stashDir; + } + try { + if (treeDeleter != null) { + treeDeleter.deleteTree(stashTrashDir); + } else { + stashTrashDir.deleteTree(); + } + } catch (IOException e) { + logger.atWarning().withCause(e).log("Failed to clean sandbox stash %s", stashDir); + } + } } diff --git a/src/test/shell/integration/sandboxing_test.sh b/src/test/shell/integration/sandboxing_test.sh index 332d4645dc599a..7ea0162501f1b9 100755 --- a/src/test/shell/integration/sandboxing_test.sh +++ b/src/test/shell/integration/sandboxing_test.sh @@ -870,4 +870,90 @@ EOF [[ -f "${temp_dir}/file" ]] || fail "Expected ${temp_dir}/file to exist" } +function test_sandbox_reuse_stashes_sandbox() { + mkdir pkg + cat >pkg/BUILD <<'EOF' +genrule( + name = "a", + srcs = [ "a.txt" ], + outs = [ "aout.txt" ], + cmd = "wc $(location :a.txt) > $@", +) +genrule( + name = "b", + srcs = [ "b.txt" ], + outs = [ "bout.txt" ], + cmd = "wc $(location :b.txt) > $@", +) +EOF + echo A > pkg/a.txt + echo BB > pkg/b.txt + local output_base="$(bazel info output_base)" + local execroot="$(bazel info execution_root)" + local execroot_reldir="${execroot#$output_base}" + + bazel build --reuse_sandbox_directories //pkg:a >"${TEST_log}" 2>&1 \ + || fail "Expected build to succeed" + + local sandbox_stash="${output_base}/sandbox_stash" + [[ -d "${sandbox_stash}" ]] \ + || fail "${sandbox_stash} not present" + [[ -d "${sandbox_stash}/_NoMnemonic_/3" ]] \ + || fail "${sandbox_stash} did not stash anything" + [[ -L "${sandbox_stash}/_NoMnemonic_/3/$execroot_reldir/pkg/a.txt" ]] \ + || fail "${sandbox_stash} did not have a link to a.txt" + + bazel build --reuse_sandbox_directories //pkg:b >"${TEST_log}" 2>&1 \ + || fail "Expected build to succeed" + ls -R "${sandbox_stash}/_NoMnemonic_/" + [[ ! -L "${sandbox_stash}/_NoMnemonic_/6/$execroot_reldir/pkg/a.txt" ]] \ + || fail "${sandbox_stash} should no longer have a link to a.txt" + [[ -L "${sandbox_stash}/_NoMnemonic_/6/$execroot_reldir/pkg/b.txt" ]] \ + || fail "${sandbox_stash} should now have a link to b.txt" + + bazel clean + [[ ! -d "${sandbox_stash}" ]] \ + || fail "${sandbox_stash} present after clean" + + bazel build --reuse_sandbox_directories //pkg:a >"${TEST_log}" 2>&1 \ + || fail "Expected build to succeed" +} + +function test_sandbox_reuse_clean() { + mkdir pkg + cat >pkg/BUILD <<'EOF' +genrule( + name = "a", + srcs = [ "a.txt" ], + outs = [ "aout.txt" ], + cmd = "wc $(location :a.txt) > $@", +) +EOF + echo A > pkg/a.txt + local output_base="$(bazel info output_base)" + + bazel build --reuse_sandbox_directories //pkg:a >"${TEST_log}" 2>&1 \ + || fail "Expected build to succeed" + + local sandbox_stash="${output_base}/sandbox_stash" + [[ -d "${sandbox_stash}" ]] \ + || fail "${sandbox_stash} not present" + [[ -d "${sandbox_stash}/_NoMnemonic_/3" ]] \ + || fail "${sandbox_stash} did not stash anything" + + bazel clean --reuse_sandbox_directories + [[ ! -d "${sandbox_stash}" ]] \ + || fail "${sandbox_stash} present after clean" + + bazel build --experimental_sandbox_async_tree_delete_idle_threads=2 \ + --reuse_sandbox_directories //pkg:a >"${TEST_log}" 2>&1 \ + || fail "Expected build to succeed" + [[ -d "${sandbox_stash}/_NoMnemonic_/6" ]] \ + || fail "${sandbox_stash} did not stash anything" + + bazel clean + [[ ! -d "${sandbox_stash}" ]] \ + || fail "${sandbox_stash} present after non-reusing clean" +} + run_suite "sandboxing" From 38e08c2a477c36c6f41933d5e04912eb3840e618 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 07:46:43 -0700 Subject: [PATCH 006/710] Cleanup of google_cpp fragment PiperOrigin-RevId: 529088221 Change-Id: I473868f0a48e69da42ad6f472aea904845eaf60b --- src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl | 2 +- src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl | 2 +- src/main/starlark/builtins_bzl/common/cc/cc_test.bzl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl index f34170afc663bb..7768cf4bdb72a8 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl @@ -902,7 +902,7 @@ def make_cc_binary(cc_binary_attrs, **kwargs): "stripped_binary": "%{name}.stripped", "dwp_file": "%{name}.dwp", }, - fragments = ["google_cpp", "cpp"], + fragments = ["cpp"] + semantics.additional_fragments(), exec_groups = { "cpp_link": exec_group(toolchains = cc_helper.use_cpp_toolchain()), }, diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl index a973826681577d..4cdfd61b8e499c 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl @@ -723,7 +723,7 @@ cc_shared_library = rule( ), }, toolchains = cc_helper.use_cpp_toolchain(), - fragments = ["google_cpp", "cpp"], + fragments = ["cpp"] + semantics.additional_fragments(), incompatible_use_toolchain_transition = True, ) diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl index 5ac09bc8bb7fb8..ca8aad88b21e82 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl @@ -130,7 +130,7 @@ def make_cc_test(with_linkstatic = False, with_aspects = False): "stripped_binary": "%{name}.stripped", "dwp_file": "%{name}.dwp", }, - fragments = ["google_cpp", "cpp", "coverage"], + fragments = ["cpp", "coverage"] + semantics.additional_fragments(), exec_groups = { "cpp_link": exec_group(toolchains = cc_helper.use_cpp_toolchain()), # testing.ExecutionInfo defaults to an exec_group of "test". From 78cb7d5b652ee155a0d1ad5cef3a3131e9705152 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 08:17:36 -0700 Subject: [PATCH 007/710] Allow overrides in non-root modules They're simply ignored. RELNOTES: Overrides specified by non-root modules no longer cause an error, and are silently ignored instead. They were originally treated as an error to allow for the future possibility of overrides in the transitive dependency graph working together; but we've deemed that infeasible (and even if it was, it'd be so complicated and confusing to users that it would not be a good addition). PiperOrigin-RevId: 529095596 Change-Id: I8b9b7b570b405ee757554accf791d8e4c1ff6528 --- site/en/external/module.md | 8 +++---- .../lib/bazel/bzlmod/ModuleFileFunction.java | 3 --- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 24 +++++++++---------- .../build/lib/bazel/bzlmod/DiscoveryTest.java | 5 +++- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/site/en/external/module.md b/site/en/external/module.md index a273acb221defd..c07a299aa041d5 100644 --- a/site/en/external/module.md +++ b/site/en/external/module.md @@ -108,12 +108,12 @@ exist in the resolved dependency graph. ## Overrides Specify overrides in the `MODULE.bazel` file to alter the behavior of Bazel -module resolution. Only the root module can specify overrides — Bazel throws an -error for dependency modules with overrides. +module resolution. Only the root module's overrides take effect — if a module is +used as a dependency, its overrides are ignored. Each override is specified for a certain module name, affecting all of its -versions in the dependency graph. Although only the root module can specify -overrides, they can be for transitive dependencies that the root module does not +versions in the dependency graph. Although only the root module's overrides take +effect, they can be for transitive dependencies that the root module does not directly depend on. ### Single-version override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java index 18efa92425018a..b8406793a7ffd9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java @@ -138,9 +138,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) moduleKey, module.getVersion()); } - if (!moduleFileGlobals.buildOverrides().isEmpty()) { - throw errorf(Code.BAD_MODULE, "The MODULE.bazel file of %s declares overrides", moduleKey); - } return NonRootModuleFileValue.create(module, moduleFileHash); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index c711c83f0fabf4..108b2b59dfcb85 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -626,8 +626,8 @@ private void addOverride(String moduleName, ModuleOverride override) throws Eval doc = "Specifies that a dependency should still come from a registry, but its version should" + " be pinned, or its registry overridden, or a list of patches applied. This" - + " directive can only be used by the root module; in other words, if a module" - + " specifies any overrides, it cannot be used as a dependency by others.", + + " directive only takes effect in the root module; in other words, if a module" + + " is used as a dependency by others, its own overrides are ignored.", parameters = { @Param( name = "module_name", @@ -708,9 +708,9 @@ public void singleVersionOverride( "Specifies that a dependency should still come from a registry, but multiple versions of" + " it should be allowed to coexist. See the documentation for" - + " more details. This directive can only be used by the root module; in other words," - + " if a module specifies any overrides, it cannot be used as a dependency by" - + " others.", + + " more details. This" + + " directive only takes effect in the root module; in other words, if a module" + + " is used as a dependency by others, its own overrides are ignored.", parameters = { @Param( name = "module_name", @@ -760,9 +760,9 @@ public void multipleVersionOverride(String moduleName, Iterable versions, Str name = "archive_override", doc = "Specifies that this dependency should come from an archive file (zip, gzip, etc) at a" - + " certain location, instead of from a registry. This directive can only be used by" - + " the root module; in other words, if a module specifies any overrides, it cannot" - + " be used as a dependency by others.", + + " certain location, instead of from a registry. This" + + " directive only takes effect in the root module; in other words, if a module" + + " is used as a dependency by others, its own overrides are ignored.", parameters = { @Param( name = "module_name", @@ -844,8 +844,8 @@ public void archiveOverride( name = "git_override", doc = "Specifies that a dependency should come from a certain commit of a Git repository. This" - + " directive can only be used by the root module; in other words, if a module" - + " specifies any overrides, it cannot be used as a dependency by others.", + + " directive only takes effect in the root module; in other words, if a module" + + " is used as a dependency by others, its own overrides are ignored.", parameters = { @Param( name = "module_name", @@ -911,8 +911,8 @@ public void gitOverride( name = "local_path_override", doc = "Specifies that a dependency should come from a certain directory on local disk. This" - + " directive can only be used by the root module; in other words, if a module" - + " specifies any overrides, it cannot be used as a dependency by others.", + + " directive only takes effect in the root module; in other words, if a module" + + " is used as a dependency by others, its own overrides are ignored.", parameters = { @Param( name = "module_name", diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java index e8307458e2ef93..fda410324d68c6 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java @@ -215,7 +215,10 @@ public void testSimpleDiamond() throws Exception { .addModule( createModuleKey("ccc", "2.0"), "module(name='ccc', version='2.0');bazel_dep(name='ddd',version='3.0')") - .addModule(createModuleKey("ddd", "3.0"), "module(name='ddd', version='3.0')"); + .addModule( + createModuleKey("ddd", "3.0"), + // Add a random override here; it should be ignored + "module(name='ddd', version='3.0');local_path_override(module_name='ff',path='f')"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); EvaluationResult result = From 37b8e1b37d5676b3d047b528149546bf2bfefdd1 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 09:06:44 -0700 Subject: [PATCH 008/710] Delete Bazel's apple bitcode support, and the --apple_bitcode flag. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apple now prohibits (instead of requires!) bitcode in app-store uploads, so the bitcode embedding options no longer serve any purpose. This also deletes the starlark type `apple_bitcode_mode`, and deprecates and stubs out the starlark APIs `ctx.fragments.cpp.apple_bitcode_mode` and `ctx.fragments.apple.bitcode_mode`. The latter now always return "none", for compatibility with the current rules_apple/rules_swift libraries. PiperOrigin-RevId: 529106847 Change-Id: I27338dbb7907eab0046821b58a41e90e7e9333f3 --- .../rules/apple/AppleBitcodeConverter.java | 105 ---------- .../rules/apple/AppleCommandLineOptions.java | 79 ------- .../lib/rules/apple/AppleConfiguration.java | 65 +----- .../build/lib/rules/apple/XcodeConfig.java | 15 -- .../build/lib/rules/cpp/CcCommon.java | 3 +- .../build/lib/rules/cpp/CppConfiguration.java | 31 +-- .../build/lib/rules/objc/AppleBinary.java | 7 - .../lib/rules/objc/AppleDebugOutputsInfo.java | 8 +- .../lib/rules/objc/AppleLinkingOutputs.java | 5 - .../lib/rules/objc/AppleStarlarkCommon.java | 1 - .../lib/rules/objc/CompilationSupport.java | 9 - .../lib/rules/objc/IntermediateArtifacts.java | 5 - .../rules/objc/ObjcVariablesExtension.java | 25 --- .../apple/AppleBitcodeModeApi.java | 35 ---- .../apple/AppleConfigurationApi.java | 7 +- .../cpp/CppConfigurationApi.java | 8 +- .../lib/rules/apple/XcodeConfigTest.java | 14 -- .../objc/AppleBinaryStarlarkApiTest.java | 23 +- .../build/lib/rules/objc/CcLibraryTest.java | 152 -------------- .../build/lib/rules/objc/ObjcLibraryTest.java | 196 ------------------ .../lib/rules/objc/ObjcStarlarkTest.java | 3 +- 21 files changed, 17 insertions(+), 779 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/rules/apple/AppleBitcodeConverter.java delete mode 100644 src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleBitcodeModeApi.java diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleBitcodeConverter.java b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleBitcodeConverter.java deleted file mode 100644 index 9fc053e4b25210..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleBitcodeConverter.java +++ /dev/null @@ -1,105 +0,0 @@ -// Copyright 2020 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.apple; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Maps; -import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions.AppleBitcodeMode; -import com.google.devtools.common.options.Converter; -import com.google.devtools.common.options.OptionsParsingException; -import java.util.List; -import java.util.Map; - -/** - * Converts the {@code --apple_bitcode} command line option to a pair containing an optional - * platform type and the bitcode mode to apply to builds targeting that platform. - */ -public final class AppleBitcodeConverter - extends Converter.Contextless>> { - /** Used to convert Bitcode mode strings to their enum value. */ - private static final AppleBitcodeMode.Converter MODE_CONVERTER = new AppleBitcodeMode.Converter(); - - /** Used to convert Apple platform type strings to their enum value. */ - private static final AppleCommandLineOptions.PlatformTypeConverter PLATFORM_TYPE_CONVERTER = - new AppleCommandLineOptions.PlatformTypeConverter(); - - private static final String TYPE_DESCRIPTION = - String.format( - "'mode' or 'platform=mode', where 'mode' is %s, and 'platform' is %s", - MODE_CONVERTER.getTypeDescription(), PLATFORM_TYPE_CONVERTER.getTypeDescription()); - - @VisibleForTesting - public static final String INVALID_APPLE_BITCODE_OPTION_FORMAT = - "Apple Bitcode mode must be in the form " + TYPE_DESCRIPTION; - - @Override - public ImmutableList> convert( - String input) throws OptionsParsingException { - ApplePlatform.PlatformType platformType; - AppleBitcodeMode mode; - - int pos = input.indexOf('='); - if (pos < 0) { - // If there was no '=', then parse it as a Bitcode mode and apply it to all platforms (by - // using a null key in the entry). - platformType = null; - mode = convertAppleBitcodeMode(input); - } else { - // If there was a '=', then parse the platform type from the left side, the Bitcode mode from - // the right side, and apply it to just that platform. - String platformTypeName = input.substring(0, pos); - String modeName = input.substring(pos + 1); - - platformType = convertPlatformType(platformTypeName); - mode = convertAppleBitcodeMode(modeName); - } - - return ImmutableList.of(Maps.immutableEntry(platformType, mode)); - } - - @Override - public String getTypeDescription() { - return TYPE_DESCRIPTION; - } - - /** - * Returns the {@code AppleBitcodeMode} value that is equivalent to the given string. - * - * @throws OptionsParsingException if the string was not a valid Apple Bitcode mode - */ - private static AppleBitcodeMode convertAppleBitcodeMode(String input) - throws OptionsParsingException { - try { - return MODE_CONVERTER.convert(input, /*conversionContext=*/ null); - } catch (OptionsParsingException e) { - throw new OptionsParsingException(INVALID_APPLE_BITCODE_OPTION_FORMAT, e); - } - } - - /** - * Returns the {@code ApplePlatform.PlatformType} value that is equivalent to the given string. - * - * @throws OptionsParsingException if any of the strings was not a valid Apple platform type - */ - private static ApplePlatform.PlatformType convertPlatformType(String input) - throws OptionsParsingException { - try { - return PLATFORM_TYPE_CONVERTER.convert(input, /*conversionContext=*/ null); - } catch (OptionsParsingException e) { - throw new OptionsParsingException(INVALID_APPLE_BITCODE_OPTION_FORMAT, e); - } - } -} diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java index f0aa08973ffaaa..487ad2118a0814 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java @@ -15,18 +15,15 @@ package com.google.devtools.build.lib.rules.apple; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelConverter; import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelListConverter; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.rules.apple.AppleConfiguration.ConfigurationDistinguisher; import com.google.devtools.build.lib.rules.apple.ApplePlatform.PlatformType; import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; -import com.google.devtools.build.lib.starlarkbuildapi.apple.AppleBitcodeModeApi; import com.google.devtools.build.lib.util.CPU; import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter; import com.google.devtools.common.options.EnumConverter; @@ -38,8 +35,6 @@ import com.google.protobuf.CodedOutputStream; import java.io.IOException; import java.util.List; -import java.util.Map; -import net.starlark.java.eval.Printer; /** Command-line options for building for Apple platforms. */ public class AppleCommandLineOptions extends FragmentOptions { @@ -371,22 +366,6 @@ public class AppleCommandLineOptions extends FragmentOptions { @VisibleForTesting public static final String DEFAULT_XCODE_VERSION_CONFIG_LABEL = "//tools/objc:host_xcodes"; - @Option( - name = "apple_bitcode", - allowMultiple = true, - converter = AppleBitcodeConverter.class, - defaultValue = "null", - documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, - effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE}, - help = - "Specify the Apple bitcode mode for compile steps targeting device architectures. Values" - + " are of the form '[platform=]mode', where the platform (which must be 'ios'," - + " 'macos', 'tvos', or 'watchos') is optional. If provided, the bitcode mode is" - + " applied for that platform specifically; if omitted, it is applied for all" - + " platforms. The mode must be 'none', 'embedded_markers', or 'embedded'. This" - + " option may be provided multiple times.") - public List> appleBitcodeMode; - @Option( name = "incompatible_enable_apple_toolchain_resolution", defaultValue = "false", @@ -430,64 +409,6 @@ public DottedVersion getMinimumOsVersion() { return DottedVersion.maybeUnwrap(option); } - /** - * Represents the Apple Bitcode mode for compilation steps. - * - *

Bitcode is an intermediate representation of a compiled program. For many platforms, Apple - * requires app submissions to contain bitcode in order to be uploaded to the app store. - * - *

This is a build-wide value, as bitcode mode needs to be consistent among a target and its - * compiled dependencies. - */ - @Immutable - public enum AppleBitcodeMode implements AppleBitcodeModeApi { - - /** Do not compile bitcode. */ - NONE("none", ImmutableList.of()), - /** - * Compile the minimal set of bitcode markers. This is often the best option for developer/debug - * builds. - */ - EMBEDDED_MARKERS("embedded_markers", ImmutableList.of("bitcode_embedded_markers")), - /** Fully embed bitcode in compiled files. This is often the best option for release builds. */ - EMBEDDED("embedded", ImmutableList.of("bitcode_embedded")); - - private final String mode; - private final ImmutableList featureNames; - - private AppleBitcodeMode(String mode, ImmutableList featureNames) { - this.mode = mode; - this.featureNames = featureNames; - } - - @Override - public boolean isImmutable() { - return true; // immutable and Starlark-hashable - } - - @Override - public String toString() { - return mode; - } - - @Override - public void repr(Printer printer) { - printer.append(mode); - } - - /** Returns the names of any crosstool features that correspond to this bitcode mode. */ - public ImmutableList getFeatureNames() { - return featureNames; - } - - /** Converts to {@link AppleBitcodeMode}. */ - public static class Converter extends EnumConverter { - public Converter() { - super(AppleBitcodeMode.class, "apple bitcode mode"); - } - } - } - @Override public FragmentOptions getExec() { AppleCommandLineOptions exec = (AppleCommandLineOptions) super.getExec(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java index 512b985902a3fa..1f5d43087f461e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java @@ -30,15 +30,11 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; -import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions.AppleBitcodeMode; import com.google.devtools.build.lib.rules.apple.ApplePlatform.PlatformType; import com.google.devtools.build.lib.starlarkbuildapi.apple.AppleConfigurationApi; import com.google.devtools.build.lib.util.CPU; import java.util.ArrayList; -import java.util.Arrays; -import java.util.EnumMap; import java.util.List; -import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Module; @@ -87,7 +83,6 @@ public class AppleConfiguration extends Fragment implements AppleConfigurationAp private final PlatformType applePlatformType; private final ConfigurationDistinguisher configurationDistinguisher; - private final EnumMap platformBitcodeModes; private final Label xcodeConfigLabel; private final AppleCommandLineOptions options; private final AppleCpus appleCpus; @@ -100,7 +95,6 @@ public AppleConfiguration(BuildOptions buildOptions) { this.applePlatformType = Preconditions.checkNotNull(options.applePlatformType, "applePlatformType"); this.configurationDistinguisher = options.configurationDistinguisher; - this.platformBitcodeModes = collectBitcodeModes(options.appleBitcodeMode); this.xcodeConfigLabel = Preconditions.checkNotNull(options.xcodeVersionConfig, "xcodeConfigLabel"); this.mandatoryMinimumVersion = options.mandatoryMinimumVersion; @@ -364,35 +358,9 @@ public ApplePlatform getMultiArchPlatform(PlatformType platformType) { } } - /** - * Returns the bitcode mode to use for compilation steps. This should only be invoked in - * single-architecture contexts. - * - *

Users can control bitcode mode using the {@code apple_bitcode} build flag, but bitcode - * will be disabled for all simulator architectures regardless of this flag. - * - * @see AppleBitcodeMode - */ @Override - public AppleBitcodeMode getBitcodeMode() { - return getAppleBitcodeMode(applePlatformType, appleCpus, platformBitcodeModes); - } - - /** Returns the bitcode mode to use for compilation steps. */ - public static AppleBitcodeMode getAppleBitcodeMode( - PlatformType applePlatformType, - AppleCpus appleCpus, - EnumMap platformBitcodeModes) { - String architecture = - getSingleArchitecture(applePlatformType, appleCpus, /* removeEnvironmentPrefix= */ false); - String cpuString = ApplePlatform.cpuStringForTarget(applePlatformType, architecture); - if (ApplePlatform.isApplePlatform(cpuString)) { - ApplePlatform platform = ApplePlatform.forTarget(applePlatformType, architecture); - if (platform.isDevice()) { - return platformBitcodeModes.get(applePlatformType); - } - } - return AppleBitcodeMode.NONE; + public String getBitcodeMode() { + return "none"; } /** @@ -463,35 +431,6 @@ public int hashCode() { return options.hashCode(); } - /** - * Compute the platform-type-to-bitcode-mode mapping from the pairs that were passed on the - * command line. - */ - public static EnumMap collectBitcodeModes( - List> platformModeMappings) { - EnumMap modes = - new EnumMap<>(ApplePlatform.PlatformType.class); - ApplePlatform.PlatformType[] allPlatforms = ApplePlatform.PlatformType.values(); - - // Seed the map with the default mode for every key so that there is a valid mode for every - // platform. - // TODO(blaze-team): Default to embedded_markers when fully implemented. - Arrays.stream(allPlatforms).forEach(platform -> modes.put(platform, AppleBitcodeMode.NONE)); - - // Process the entries in order. If we encounter one with a null key, apply the mode to all - // platforms; otherwise, apply it only to that specific platform. This ensures that the later - // options override the earlier options. - for (Map.Entry entry : platformModeMappings) { - if (entry.getKey() == null) { - Arrays.stream(allPlatforms).forEach(platform -> modes.put(platform, entry.getValue())); - } else { - modes.put(entry.getKey(), entry.getValue()); - } - } - - return modes; - } - /** * Value used to avoid multiple configurations from conflicting. No two instances of this * transition may exist with the same value in a single Bazel invocation. diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java index 5df242200393a5..b22bbf7fc21041 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java @@ -33,7 +33,6 @@ import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.XcodeConfigEvent; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions.AppleBitcodeMode; import com.google.devtools.build.lib.rules.apple.XcodeConfigInfo.Availability; import com.google.devtools.build.lib.xcode.proto.XcodeConfig.XcodeConfigRuleInfo; import com.google.devtools.build.lib.xcode.proto.XcodeConfig.XcodeVersionInfo; @@ -46,9 +45,6 @@ public class XcodeConfig implements RuleConfiguredTargetFactory { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - private static final DottedVersion MINIMUM_BITCODE_XCODE_VERSION = - DottedVersion.fromStringUnchecked("7"); - /** An exception that signals that an Xcode config setup was invalid. */ public static class XcodeConfigException extends Exception { @@ -166,17 +162,6 @@ public ConfiguredTarget create(RuleContext ruleContext) appleOptions.xcodeVersion, appleOptions.includeXcodeExecutionRequirements); - AppleBitcodeMode bitcodeMode = appleConfig.getBitcodeMode(); - DottedVersion xcodeVersion = xcodeVersions.getXcodeVersion(); - if (bitcodeMode != AppleBitcodeMode.NONE - && xcodeVersion != null - && xcodeVersion.compareTo(MINIMUM_BITCODE_XCODE_VERSION) < 0) { - ruleContext.throwWithRuleError( - String.format( - "apple_bitcode mode '%s' is unsupported for xcode version '%s'", - bitcodeMode, xcodeVersion)); - } - return new RuleConfiguredTargetBuilder(ruleContext) .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY) .addNativeDeclaredProvider(xcodeVersions) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index 80595169c33c38..90828e762ce2c2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java @@ -732,8 +732,7 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException( .addAll(ImmutableSet.of(cppConfiguration.getCompilationMode().toString())) .addAll(DEFAULT_ACTION_CONFIGS) .addAll(requestedFeatures) - .addAll(toolchain.getFeatures().getDefaultFeaturesAndActionConfigs()) - .addAll(cppConfiguration.getAppleBitcodeMode().getFeatureNames()); + .addAll(toolchain.getFeatures().getDefaultFeaturesAndActionConfigs()); if (language == Language.OBJC || language == Language.OBJCPP) { allFeatures.addAll(OBJC_ACTIONS); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index d851b60468910a..e0f52d77827078 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -16,7 +16,6 @@ import static com.google.devtools.build.lib.rules.cpp.CcModule.isBuiltIn; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildOptions; @@ -34,14 +33,9 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions; -import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions.AppleBitcodeMode; -import com.google.devtools.build.lib.rules.apple.AppleConfiguration; -import com.google.devtools.build.lib.rules.apple.AppleConfiguration.AppleCpus; -import com.google.devtools.build.lib.rules.apple.ApplePlatform; import com.google.devtools.build.lib.starlarkbuildapi.cpp.CppConfigurationApi; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.EnumMap; import javax.annotation.Nullable; import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.EvalException; @@ -186,7 +180,6 @@ public String toString() { private final boolean isToolConfigurationDoNotUseWillBeRemovedFor129045294; private final boolean appleGenerateDsym; - private final AppleBitcodeMode appleBitcodeMode; public CppConfiguration(BuildOptions options) throws InvalidConfigurationException { CppOptions cppOptions = options.get(CppOptions.class); @@ -299,20 +292,6 @@ public CppConfiguration(BuildOptions options) throws InvalidConfigurationExcepti this.appleGenerateDsym = (cppOptions.appleGenerateDsym || (cppOptions.appleEnableAutoDsymDbg && compilationMode == CompilationMode.DBG)); - this.appleBitcodeMode = - computeAppleBitcodeMode(options.get(AppleCommandLineOptions.class), commonOptions); - } - - private static AppleBitcodeMode computeAppleBitcodeMode( - AppleCommandLineOptions options, CoreOptions commonOptions) { - ApplePlatform.PlatformType applePlatformType = - Preconditions.checkNotNull(options.applePlatformType, "applePlatformType"); - AppleCpus appleCpus = AppleCpus.create(options, commonOptions); - EnumMap platformBitcodeModes = - AppleConfiguration.collectBitcodeModes(options.appleBitcodeMode); - - return AppleConfiguration.getAppleBitcodeMode( - applePlatformType, appleCpus, platformBitcodeModes); } /** Returns the label of the cc_compiler rule for the C++ configuration. */ @@ -922,15 +901,9 @@ public boolean getExperimentalPlatformCcTest(StarlarkThread thread) throws EvalE return experimentalPlatformCcTest(); } - /** - * Returns the bitcode mode to use for compilation. - * - *

Users can control bitcode mode using the {@code apple_bitcode} build flag, but bitcode will - * be disabled for all simulator architectures regardless of this flag. - */ @Override - public AppleBitcodeMode getAppleBitcodeMode() { - return appleBitcodeMode; + public String getAppleBitcodeMode() { + return "none"; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java index af9b183875b0b2..5549b9c6c04969 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java @@ -28,7 +28,6 @@ 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.RuleClass.ConfiguredTargetFactory.RuleErrorException; -import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions.AppleBitcodeMode; import com.google.devtools.build.lib.rules.cpp.CcInfo; import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CppSemantics; @@ -148,12 +147,6 @@ public static AppleLinkingOutputs linkMultiArchBinary( .setTargetTriplet(childTriplet) .setBinary(binaryArtifact); - if (childCppConfig.getAppleBitcodeMode() == AppleBitcodeMode.EMBEDDED) { - Artifact bitcodeSymbols = intermediateArtifacts.bitcodeSymbolMap(); - outputBuilder.setBitcodeSymbols(bitcodeSymbols); - legacyDebugOutputsBuilder.addOutput( - childTriplet.architecture(), OutputType.BITCODE_SYMBOLS, bitcodeSymbols); - } if (childCppConfig.appleGenerateDsym()) { Artifact dsymBinary = childCppConfig.objcShouldStripBinary() diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java index 8b1a1f3182232f..8baa8e5639efc4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java @@ -34,10 +34,9 @@ * Artifact, output_type: Artifact, ... } } * *

Where {@code arch} is any Apple architecture such as "arm64" or "armv7", {@code output_type} - * can currently be "bitcode_symbols" or "dsym_binary", and the artifact is an instance of the - * {@link Artifact} class. + * can currently be "dsym_binary", and the artifact is an instance of the {@link Artifact} class. * - *

Example: { "arm64": { "bitcode_symbols": Artifact, "dsym_binary": Artifact } } + *

Example: { "arm64": { "dsym_binary": Artifact } } */ @Immutable public final class AppleDebugOutputsInfo extends NativeInfo @@ -46,9 +45,6 @@ public final class AppleDebugOutputsInfo extends NativeInfo /** Expected types of debug outputs. */ enum OutputType { - /** A Bitcode symbol map, per architecture. */ - BITCODE_SYMBOLS, - /** A single-architecture DWARF binary with debug symbols. */ DSYM_BINARY, diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleLinkingOutputs.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleLinkingOutputs.java index 471d367ce54658..d50a096ef8a71f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleLinkingOutputs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleLinkingOutputs.java @@ -72,9 +72,6 @@ public abstract static class LinkingOutput { abstract Artifact getBinary(); - @Nullable - abstract Artifact getBitcodeSymbols(); - @Nullable abstract Artifact getDsymBinary(); @@ -92,8 +89,6 @@ public abstract static class Builder { abstract Builder setBinary(Artifact binary); - abstract Builder setBitcodeSymbols(Artifact bitcodeSymbols); - abstract Builder setDsymBinary(Artifact dsymBinary); abstract Builder setLinkmap(Artifact linkmap); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java index 081864ab439038..2f9ddace26945d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java @@ -351,7 +351,6 @@ private StructImpl createStarlarkLinkingOutputs( .put("architecture", targetTriplet.architecture()) .put("environment", targetTriplet.environment()) .put("binary", linkingOutput.getBinary()) - .put("bitcode_symbols", valueOrNone(linkingOutput.getBitcodeSymbols())) .put("dsym_binary", valueOrNone(linkingOutput.getDsymBinary())) .put("linkmap", valueOrNone(linkingOutput.getLinkmap())) .buildOrThrow(), diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index 25de6766f71e3d..6d3609a74527a7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -56,7 +56,6 @@ import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.packages.TargetUtils; -import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions.AppleBitcodeMode; import com.google.devtools.build.lib.rules.apple.AppleConfiguration; import com.google.devtools.build.lib.rules.apple.XcodeConfigInfo; import com.google.devtools.build.lib.rules.cpp.CcCommon; @@ -694,14 +693,6 @@ public CompilationSupport registerLinkActions( linkerOutputs.add(linkmap); } - if (cppConfiguration.getAppleBitcodeMode() == AppleBitcodeMode.EMBEDDED) { - Artifact bitcodeSymbolMap = intermediateArtifacts.bitcodeSymbolMap(); - extensionBuilder - .setBitcodeSymbolMap(bitcodeSymbolMap) - .addVariableCategory(VariableCategory.BITCODE_VARIABLES); - linkerOutputs.add(bitcodeSymbolMap); - } - executableLinkingHelper.addVariableExtension(extensionBuilder.build()); executableLinkingHelper.addLinkerOutputs(linkerOutputs.build()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java index 99b0ce7a90424f..55a919283f903c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java @@ -223,11 +223,6 @@ private Artifact dsymSymbol(String suffix) { return appendExtension(String.format("_%s.dwarf", suffix)); } - /** Bitcode symbol map generated for a linked binary, for a specific architecture. */ - public Artifact bitcodeSymbolMap() { - return appendExtension(".bcsymbolmap"); - } - /** Representation for a specific architecture. */ private Artifact architectureRepresentation(String arch, String suffix) { return appendExtension(String.format("_%s%s", arch, suffix)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java index 7b37e1dfc777d7..d04fe627833635 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcVariablesExtension.java @@ -36,7 +36,6 @@ class ObjcVariablesExtension implements VariablesExtension { static final String OBJC_MODULE_CACHE_KEY = "modules_cache_path"; static final String ARCHIVE_PATH_VARIABLE_NAME = "archive_path"; static final String LINKMAP_EXEC_PATH = "linkmap_exec_path"; - static final String BITCODE_SYMBOL_MAP_PATH_VARAIBLE_NAME = "bitcode_symbol_map_path"; // executable linking variables static final String FRAMEWORK_NAMES_VARIABLE_NAME = "framework_names"; @@ -68,7 +67,6 @@ class ObjcVariablesExtension implements VariablesExtension { private final ImmutableSet activeVariableCategories; private final Artifact dsymSymbol; private final Artifact linkmap; - private final Artifact bitcodeSymbolMap; private boolean arcEnabled = true; private ObjcVariablesExtension( @@ -85,7 +83,6 @@ private ObjcVariablesExtension( ImmutableSet activeVariableCategories, Artifact dsymSymbol, Artifact linkmap, - Artifact bitcodeSymbolMap, boolean arcEnabled) { this.ruleContext = ruleContext; this.intermediateArtifacts = intermediateArtifacts; @@ -100,7 +97,6 @@ private ObjcVariablesExtension( this.activeVariableCategories = activeVariableCategories; this.dsymSymbol = dsymSymbol; this.linkmap = linkmap; - this.bitcodeSymbolMap = bitcodeSymbolMap; this.arcEnabled = arcEnabled; } @@ -109,7 +105,6 @@ public enum VariableCategory { EXECUTABLE_LINKING_VARIABLES, DSYM_VARIABLES, LINKMAP_VARIABLES, - BITCODE_VARIABLES, MODULE_MAP_VARIABLES } @@ -128,9 +123,6 @@ public void addVariables(CcToolchainVariables.Builder builder) { if (activeVariableCategories.contains(VariableCategory.LINKMAP_VARIABLES)) { addLinkmapVariables(builder); } - if (activeVariableCategories.contains(VariableCategory.BITCODE_VARIABLES)) { - addBitcodeVariables(builder); - } if (arcEnabled) { builder.addStringVariable(OBJC_ARC_VARIABLE_NAME, ""); } else { @@ -187,11 +179,6 @@ private void addLinkmapVariables(CcToolchainVariables.Builder builder) { builder.addStringVariable(LINKMAP_EXEC_PATH, linkmap.getExecPathString()); } - private void addBitcodeVariables(CcToolchainVariables.Builder builder) { - builder.addStringVariable( - BITCODE_SYMBOL_MAP_PATH_VARAIBLE_NAME, bitcodeSymbolMap.getExecPathString()); - } - /** A Builder for {@link ObjcVariablesExtension}. */ static class Builder { private RuleContext ruleContext; @@ -206,7 +193,6 @@ static class Builder { private ImmutableList attributeLinkopts; private Artifact dsymSymbol; private Artifact linkmap; - private Artifact bitcodeSymbolMap; private boolean arcEnabled = true; private final ImmutableSet.Builder activeVariableCategoriesBuilder = @@ -303,13 +289,6 @@ public Builder setLinkmap(Artifact linkmap) { return this; } - /** Sets the Artifact for the bitcode symbol map. */ - @CanIgnoreReturnValue - public Builder setBitcodeSymbolMap(Artifact bitcodeSymbolMap) { - this.bitcodeSymbolMap = bitcodeSymbolMap; - return this; - } - /** Sets whether ARC is enabled. */ @CanIgnoreReturnValue public Builder setArcEnabled(boolean enabled) { @@ -340,9 +319,6 @@ public ObjcVariablesExtension build() { if (activeVariableCategories.contains(VariableCategory.LINKMAP_VARIABLES)) { Preconditions.checkNotNull(linkmap, "missing linkmap artifact"); } - if (activeVariableCategories.contains(VariableCategory.BITCODE_VARIABLES)) { - Preconditions.checkNotNull(bitcodeSymbolMap, "missing bitcode symbol map artifact"); - } return new ObjcVariablesExtension( ruleContext, @@ -358,7 +334,6 @@ public ObjcVariablesExtension build() { activeVariableCategories, dsymSymbol, linkmap, - bitcodeSymbolMap, arcEnabled); } } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleBitcodeModeApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleBitcodeModeApi.java deleted file mode 100644 index f62d0e7d10af99..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleBitcodeModeApi.java +++ /dev/null @@ -1,35 +0,0 @@ -// 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.lib.starlarkbuildapi.apple; - -import com.google.devtools.build.docgen.annot.DocCategory; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.eval.StarlarkValue; - -/** - * Interface for an enum describing the bitcode mode to use when compiling Objective-C and Swift - * code on Apple platforms. - */ -@StarlarkBuiltin( - name = "apple_bitcode_mode", - category = DocCategory.BUILTIN, - doc = - "The Bitcode mode to use when compiling Objective-C and Swift code on Apple platforms. " - + "Possible values are:

    " - + "
  • 'none'
    • " - + "
  • 'embedded'
    • " - + "
  • 'embedded_markers'
    • " - + "
") -public interface AppleBitcodeModeApi extends StarlarkValue {} diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleConfigurationApi.java index 44ded3403a864d..9f58c03ea075df 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleConfigurationApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleConfigurationApi.java @@ -64,13 +64,14 @@ public interface AppleConfigurationApiThis field is only valid for" - + " device builds; for simulator builds, it always returns 'none'.", + "Deprecated: Returns the Bitcode mode to use for compilation steps. " + + "Always returns 'none'.", structField = true) - AppleBitcodeModeApi getBitcodeMode(); + String getBitcodeMode(); @StarlarkMethod(name = "mandatory_minimum_version", documented = false, useStarlarkThread = true) boolean isMandatoryMinimumVersionForStarlark(StarlarkThread thread) throws EvalException; diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CppConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CppConfigurationApi.java index 5054a6c32f3959..3dc4c482393d84 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CppConfigurationApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CppConfigurationApi.java @@ -17,7 +17,6 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.starlarkbuildapi.apple.AppleBitcodeModeApi; import javax.annotation.Nullable; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; @@ -151,11 +150,10 @@ boolean fissionActiveForCurrentCompilationModeStarlark(StarlarkThread thread) @StarlarkMethod( name = "apple_bitcode_mode", doc = - "Returns the Bitcode mode to use for compilation steps.

This field is only valid for" - + " Apple, and only for device builds; for simulator builds, it always returns " - + "'none'.", + "Deprecated: Returns the Bitcode mode to use for compilation steps. " + + "Always returns 'none'.", structField = true) - AppleBitcodeModeApi getAppleBitcodeMode(); + String getAppleBitcodeMode(); @StarlarkMethod( name = "apple_generate_dsym", diff --git a/src/test/java/com/google/devtools/build/lib/rules/apple/XcodeConfigTest.java b/src/test/java/com/google/devtools/build/lib/rules/apple/XcodeConfigTest.java index 2cb7a3c249ca59..105b67a7cd9977 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/apple/XcodeConfigTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/apple/XcodeConfigTest.java @@ -1218,20 +1218,6 @@ public void testVersionDoesNotContainDefault() throws Exception { assertContainsEvent("must be contained in versions attribute"); } - @Test - public void testInvalidBitcodeVersion() throws Exception { - new BuildFileBuilder() - .addExplicitVersion("version512", "5.1.2", true) - .write(scratch, "xcode/BUILD"); - - useConfiguration( - "--apple_platform_type=ios", "--apple_bitcode=embedded", "--apple_split_cpu=arm64"); - - reporter.removeHandler(failFastHandler); - getConfiguredTarget("//xcode:foo"); - assertContainsEvent("apple_bitcode mode 'embedded' is unsupported"); - } - // Verifies that the --xcode_version_config configuration value can be accessed via the // configuration_field() Starlark method and used in a Starlark rule. @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryStarlarkApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryStarlarkApiTest.java index 902c3ff5ceeabe..8b14d33d2cf152 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryStarlarkApiTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryStarlarkApiTest.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.OutputGroupInfo; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; import com.google.devtools.build.lib.analysis.config.CompilationMode; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo; @@ -376,18 +375,6 @@ public void testObjcLibraryLinkoptsArePropagatedToLinkActionPostMigration() thro getRuleType(), /* linkingInfoMigration= */ true); } - /** Returns the bcsymbolmap artifact for given architecture and compilation mode. */ - protected Artifact bitcodeSymbol(String arch, CompilationMode mode) throws Exception { - SpawnAction lipoAction = (SpawnAction) lipoBinAction("//examples/apple_starlark:bin"); - - String bin = - configurationBin(arch, ConfigurationDistinguisher.APPLEBIN_IOS, null, mode) - + "examples/apple_starlark/bin_bin"; - Artifact binArtifact = getFirstArtifactEndingWith(lipoAction.getInputs(), bin); - CommandAction linkAction = (CommandAction) getGeneratingAction(binArtifact); - return getFirstArtifactEndingWith(linkAction.getOutputs(), "bcsymbolmap"); - } - /** Returns the path to the dSYM binary artifact for given architecture and compilation mode. */ protected String dsymBinaryPath(String arch, CompilationMode mode) throws Exception { return configurationBin(arch, ConfigurationDistinguisher.APPLEBIN_IOS, null, mode) @@ -827,18 +814,14 @@ private void checkAppleDebugSymbolProviderDsymEntries( assertThat(outputMap).containsKey("armv7"); Map arm64 = outputMap.get("arm64"); - assertThat(arm64).containsEntry("bitcode_symbols", bitcodeSymbol("arm64", compilationMode)); String expectedArm64Path = dsymBinaryPath("arm64", compilationMode); assertThat(arm64.get("dsym_binary").getExecPathString()).isEqualTo(expectedArm64Path); Map armv7 = outputMap.get("armv7"); - assertThat(armv7).containsEntry("bitcode_symbols", bitcodeSymbol("armv7", compilationMode)); String expectedArmv7Path = dsymBinaryPath("armv7", compilationMode); assertThat(armv7.get("dsym_binary").getExecPathString()).isEqualTo(expectedArmv7Path); Map x8664 = outputMap.get("x86_64"); - // Simulator build has bitcode disabled. - assertThat(x8664).doesNotContainKey("bitcode_symbols"); String expectedx8664Path = dsymBinaryPath("x86_64", compilationMode); assertThat(x8664.get("dsym_binary").getExecPathString()).isEqualTo(expectedx8664Path); } @@ -860,8 +843,7 @@ private void checkAppleDebugSymbolProviderLinkMapEntries( @Test public void testAppleDebugSymbolProviderWithDsymsExposedToStarlark() throws Exception { - useConfiguration( - "--apple_bitcode=embedded", "--apple_generate_dsym", "--ios_multi_cpus=armv7,arm64,x86_64"); + useConfiguration("--apple_generate_dsym", "--ios_multi_cpus=armv7,arm64,x86_64"); checkAppleDebugSymbolProviderDsymEntries( generateAppleDebugOutputsStarlarkProviderMap(), CompilationMode.FASTBUILD); } @@ -870,7 +852,6 @@ public void testAppleDebugSymbolProviderWithDsymsExposedToStarlark() throws Exce public void testAppleDebugSymbolProviderWithAutoDsymDbgAndDsymsExposedToStarlark() throws Exception { useConfiguration( - "--apple_bitcode=embedded", "--compilation_mode=dbg", "--apple_enable_auto_dsym_dbg", "--ios_multi_cpus=armv7,arm64,x86_64"); @@ -881,7 +862,6 @@ public void testAppleDebugSymbolProviderWithAutoDsymDbgAndDsymsExposedToStarlark @Test public void testAppleDebugSymbolProviderWithLinkMapsExposedToStarlark() throws Exception { useConfiguration( - "--apple_bitcode=embedded", "--objc_generate_linkmap", "--ios_multi_cpus=armv7,arm64,x86_64"); checkAppleDebugSymbolProviderLinkMapEntries(generateAppleDebugOutputsStarlarkProviderMap()); @@ -890,7 +870,6 @@ public void testAppleDebugSymbolProviderWithLinkMapsExposedToStarlark() throws E @Test public void testAppleDebugSymbolProviderWithDsymsAndLinkMapsExposedToStarlark() throws Exception { useConfiguration( - "--apple_bitcode=embedded", "--objc_generate_linkmap", "--apple_generate_dsym", "--ios_multi_cpus=armv7,arm64,x86_64"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/CcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/CcLibraryTest.java index cdaafceddfddc0..6a227b37268734 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/CcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/CcLibraryTest.java @@ -30,158 +30,6 @@ protected ScratchAttributeWriter createLibraryTargetWriter(String labelString) { return ScratchAttributeWriter.fromLabelString(this, "cc_library", labelString); } - @Test - public void testCompilationActionsWithEmbeddedBitcode() throws Exception { - useConfiguration("--apple_platform_type=ios", "--cpu=ios_arm64", "--apple_bitcode=embedded"); - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode"); - } - - @Test - public void testCompilationActionsWithEmbeddedBitcodeMarkers() throws Exception { - useConfiguration( - "--apple_platform_type=ios", "--cpu=ios_arm64", "--apple_bitcode=embedded_markers"); - - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testCompilationActionsWithNoBitcode() throws Exception { - useConfiguration("--apple_platform_type=ios", "--cpu=ios_arm64", "--apple_bitcode=none"); - - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode-marker"); - } - - /** Tests that bitcode is disabled for simulator builds even if enabled by flag. */ - @Test - public void testCompilationActionsWithBitcode_simulator() throws Exception { - useConfiguration("--apple_platform_type=ios", "--cpu=ios_x86_64", "--apple_bitcode=embedded"); - - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode-marker"); - } - - @Test - public void testCompilationActionsWithEmbeddedBitcodeForMultiplePlatformsWithMatch() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--cpu=ios_arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=watchos=embedded"); - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode"); - } - - @Test - public void testCompilationActionsWithEmbeddedBitcodeForMultiplePlatformsWithoutMatch() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--cpu=ios_arm64", - "--apple_bitcode=tvos=embedded", - "--apple_bitcode=watchos=embedded"); - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode-marker"); - } - - @Test - public void testLaterBitcodeOptionsOverrideEarlierOptionsForSamePlatform() throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--cpu=ios_arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=ios=embedded_markers"); - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testLaterBitcodeOptionWithoutPlatformOverridesEarlierOptionWithPlatform() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--cpu=ios_arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=embedded_markers"); - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testLaterPlatformBitcodeOptionWithPlatformOverridesEarlierOptionWithoutPlatform() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--cpu=ios_arm64", - "--apple_bitcode=embedded", - "--apple_bitcode=ios=embedded_markers"); - createLibraryTargetWriter("//cc:lib") - .setAndCreateFiles("srcs", "a.cc", "b.cc", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//cc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - @Test public void testGenerateDsymFlagPropagatesToCcLibraryFeature() throws Exception { useConfiguration("--apple_generate_dsym"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java index 3dc01a4cb9e705..33a237c2fb6b2b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java @@ -19,7 +19,6 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.baseArtifactNames; -import static com.google.devtools.build.lib.rules.apple.AppleBitcodeConverter.INVALID_APPLE_BITCODE_OPTION_FORMAT; import static com.google.devtools.build.lib.rules.objc.CompilationSupport.ABSOLUTE_INCLUDES_PATH_FORMAT; import static com.google.devtools.build.lib.rules.objc.CompilationSupport.BOTH_MODULE_NAME_AND_MODULE_MAP_SPECIFIED; import static com.google.devtools.build.lib.rules.objc.ObjcProvider.CC_LIBRARY; @@ -682,201 +681,6 @@ public void testBothModuleNameAndModuleMapGivesError() throws Exception { "objc_library( name = 'x', module_name = 'x', module_map = 'x.modulemap' )"); } - @Test - public void testCompilationActionsWithEmbeddedBitcode() throws Exception { - useConfiguration( - "--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode=embedded"); - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode"); - } - - @Test - public void testCompilationActionsWithEmbeddedBitcodeMarkers() throws Exception { - useConfiguration( - "--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode=embedded_markers"); - - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testCompilationActionsWithNoBitcode() throws Exception { - useConfiguration("--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode=none"); - - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode-marker"); - } - - /** - * Tests that bitcode is disabled for simulator builds even if enabled by flag. - */ - @Test - public void testCompilationActionsWithBitcode_simulator() throws Exception { - useConfiguration( - "--apple_platform_type=ios", "--ios_multi_cpus=x86_64", "--apple_bitcode=embedded"); - - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode-marker"); - } - - @Test - public void testCompilationActionsWithEmbeddedBitcodeForMultiplePlatformsWithMatch() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--ios_multi_cpus=arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=watchos=embedded"); - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode"); - } - - @Test - public void testCompilationActionsWithEmbeddedBitcodeForMultiplePlatformsWithoutMatch() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--ios_multi_cpus=arm64", - "--apple_bitcode=tvos=embedded", - "--apple_bitcode=watchos=embedded"); - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode-marker"); - } - - @Test - public void testLaterBitcodeOptionsOverrideEarlierOptionsForSamePlatform() throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--ios_multi_cpus=arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=ios=embedded_markers"); - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testLaterBitcodeOptionWithoutPlatformOverridesEarlierOptionWithPlatform() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--ios_multi_cpus=arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=embedded_markers"); - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testLaterPlatformBitcodeOptionWithPlatformOverridesEarlierOptionWithoutPlatform() - throws Exception { - useConfiguration( - "--apple_platform_type=ios", - "--ios_multi_cpus=arm64", - "--apple_bitcode=embedded", - "--apple_bitcode=ios=embedded_markers"); - createLibraryTargetWriter("//objc:lib") - .setAndCreateFiles("srcs", "a.m", "b.m", "private.h") - .setAndCreateFiles("hdrs", "c.h") - .write(); - - CommandAction compileActionA = compileAction("//objc:lib", "a.o"); - - assertThat(compileActionA.getArguments()).doesNotContain("-fembed-bitcode"); - assertThat(compileActionA.getArguments()).contains("-fembed-bitcode-marker"); - } - - @Test - public void testAppleBitcode_invalidPlatformNameGivesError() { - checkBitcodeModeError( - "--apple_platform_type=ios", - "--ios_multi_cpus=arm64", - "--apple_bitcode=ios=embedded", - "--apple_bitcode=nachos=embedded"); - } - - @Test - public void testAppleBitcode_invalidBitcodeModeGivesError() { - checkBitcodeModeError( - "--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode=indebted"); - } - - @Test - public void testAppleBitcode_invalidBitcodeModeWithPlatformGivesError() { - checkBitcodeModeError( - "--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode=ios=indebted"); - } - - @Test - public void testAppleBitcode_emptyBitcodeModeGivesError() { - checkBitcodeModeError( - "--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode=ios="); - } - - @Test - public void testAppleBitcode_emptyValueGivesError() { - checkBitcodeModeError( - "--apple_platform_type=ios", "--ios_multi_cpus=arm64", "--apple_bitcode="); - } - - private void checkBitcodeModeError(String... args) { - OptionsParsingException thrown = - assertThrows(OptionsParsingException.class, () -> useConfiguration(args)); - assertThat(thrown).hasMessageThat().contains(INVALID_APPLE_BITCODE_OPTION_FORMAT); - } - @Test public void testCompilationActionsWithCoptFmodules() throws Exception { createLibraryTargetWriter("//objc:lib") diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkTest.java index 52dc789c8422e8..6e7a8ff662031e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkTest.java @@ -425,8 +425,9 @@ public void testStarlarkCanAccessAppleConfiguration() throws Exception { assertThat(myInfo.getValue("single_arch_platform")).isEqualTo("IOS_SIMULATOR"); assertThat(myInfo.getValue("single_arch_cpu")).isEqualTo("i386"); assertThat(myInfo.getValue("platform_type")).isEqualTo("ios"); - assertThat(myInfo.getValue("bitcode_mode")).isEqualTo("none"); assertThat(myInfo.getValue("dead_code_report")).isEqualTo("None"); + // bitcode_mode is deprecated, but ensure it still correctly returns none. + assertThat(myInfo.getValue("bitcode_mode")).isEqualTo("none"); } @Test From 3c1aac19b1fef734fed2450ab3f13d248e90246b Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 09:26:30 -0700 Subject: [PATCH 009/710] Remove `CallablePathFragment` class to save memory. `CallablePathFragment` served two purposes: 1. It implemented `PathStrippable` and applied path stripping to derived artifacts. Instead, implement `PathStrippable` directly on `DerivedArtifact`. This works because if path stripping is not enabled for the action, the mapper will be no-op. `PathStrippable` is branched to a top-level interface to avoid a circular dependency. 2. It used `PathFragment#getCallablePathString()` for command line expansion. This method is identical to `Artifact#expandToCommandLine()` so long as the exec path contains a path separator, so it is only expected to make a difference for source artifacts in the root package. Use the `Artifact` itself to save memory if there's no difference. PiperOrigin-RevId: 529111858 Change-Id: Ib7d1d7b168060b0bebc1644914490e8a5bda9783 --- .../devtools/build/lib/actions/Artifact.java | 8 ++- .../google/devtools/build/lib/actions/BUILD | 10 +++ .../build/lib/actions/CommandLines.java | 7 --- .../build/lib/actions/PathStrippable.java | 25 ++++++++ .../lib/analysis/actions/SpawnAction.java | 62 ++++++++----------- 5 files changed, 69 insertions(+), 43 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 3d59a556765b35..ee637cf2c340be 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -59,6 +59,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.function.UnaryOperator; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Printer; @@ -326,7 +327,7 @@ private Artifact(ArtifactRoot root, PathFragment execPath, int hashCodeWithOwner } /** An artifact corresponding to a file in the output tree, generated by an {@link Action}. */ - public static class DerivedArtifact extends Artifact { + public static class DerivedArtifact extends Artifact implements PathStrippable { /** * An {@link ActionLookupKey} until {@link #setGeneratingActionKey} is set, at which point it is @@ -460,6 +461,11 @@ final boolean ownersEqual(Artifact other) { public boolean contentBasedPath() { return contentBasedPath; } + + @Override + public String expand(UnaryOperator stripPaths) { + return stripPaths.apply(getExecPath()).getPathString(); + } } /** Supplies {@link SourceArtifact} instances and allows for interning of derived artifacts. */ 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 811612bcfa8b6f..3fd934b1893fc6 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -225,6 +225,7 @@ java_library( ":commandline_item", ":fileset_output_symlink", ":package_roots", + ":path_strippable", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", @@ -380,6 +381,15 @@ java_library( ], ) +java_library( + name = "path_strippable", + srcs = ["PathStrippable.java"], + deps = [ + ":commandline_item", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + ], +) + java_library( name = "resource_manager", srcs = [ diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java index 556340e1927af8..d02774b697a0ef 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java @@ -46,13 +46,6 @@ * to expand the command lines into a master argument list + any param files needed to be written. */ public class CommandLines { - /** - * An object that can apply the {@code stripPaths} map to optionally strip config prefixes before - * returning output artifact exec paths - */ - public interface PathStrippable { - String expand(Function stripPaths); - } // A (hopefully) conservative estimate of how much long each param file arg would be // eg. the length of '@path/to/param_file'. diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java b/src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java new file mode 100644 index 00000000000000..e63efa03a5ed87 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java @@ -0,0 +1,25 @@ +// 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.lib.actions; + +import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.function.UnaryOperator; + +/** + * A {@link CommandLineItem} that can apply the {@code stripPaths} map to optionally strip config + * prefixes before returning output artifact exec paths. + */ +public interface PathStrippable extends CommandLineItem { + String expand(UnaryOperator stripPaths); +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 608499fbb20516..17eff32a231340 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -14,13 +14,14 @@ package com.google.devtools.build.lib.analysis.actions; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps; import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CharMatcher; -import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -43,6 +44,7 @@ import com.google.devtools.build.lib.actions.CommandAction; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; +import com.google.devtools.build.lib.actions.CommandLineItem; import com.google.devtools.build.lib.actions.CommandLineLimits; import com.google.devtools.build.lib.actions.CommandLines; import com.google.devtools.build.lib.actions.CommandLines.CommandLineAndParamFileInfo; @@ -89,7 +91,6 @@ import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.function.Function; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Sequence; @@ -969,8 +970,7 @@ public Builder setExecutable(PathFragment executable) { @CanIgnoreReturnValue public Builder setExecutable(Artifact executable) { addTool(executable); - this.executableArg = - new CallablePathFragment(executable.getExecPath(), executable.hasKnownGeneratingAction()); + this.executableArg = ensureCallable(executable); this.executableArgs = null; return this; } @@ -984,8 +984,7 @@ public Builder setExecutable(Artifact executable) { */ @CanIgnoreReturnValue public Builder setExecutable(TransitiveInfoCollection executable) { - FilesToRunProvider provider = executable.getProvider(FilesToRunProvider.class); - Preconditions.checkArgument(provider != null); + FilesToRunProvider provider = checkNotNull(executable.getProvider(FilesToRunProvider.class)); return setExecutable(provider); } @@ -998,12 +997,10 @@ public Builder setExecutable(TransitiveInfoCollection executable) { */ @CanIgnoreReturnValue public Builder setExecutable(FilesToRunProvider executableProvider) { - Preconditions.checkArgument( - executableProvider.getExecutable() != null, "The target does not have an executable"); - this.executableArg = - new CallablePathFragment( - executableProvider.getExecutable().getExecPath(), - executableProvider.getExecutable().hasKnownGeneratingAction()); + Artifact executable = + checkNotNull( + executableProvider.getExecutable(), "The target does not have an executable"); + this.executableArg = ensureCallable(executable); this.executableArgs = null; return addTool(executableProvider); } @@ -1125,8 +1122,7 @@ public Builder addExecutableArguments(String... arguments) { executableArgs = CustomCommandLine.builder().addObject(executableArg); executableArg = null; } - Preconditions.checkState(executableArgs != null); - this.executableArgs.addAll(ImmutableList.copyOf(arguments)); + executableArgs.addAll(ImmutableList.copyOf(arguments)); return this; } @@ -1270,7 +1266,7 @@ public Builder setProgressMessageFromStarlark(String progressMessage) { @CanIgnoreReturnValue public Builder setMnemonic(String mnemonic) { - Preconditions.checkArgument( + checkArgument( !mnemonic.isEmpty() && CharMatcher.javaLetterOrDigit().matchesAllOf(mnemonic), "mnemonic must only contain letters and/or digits, and have non-zero length, was: \"%s\"", mnemonic); @@ -1296,25 +1292,21 @@ public Builder setExecGroup(String execGroup) { } } - /** A {@link PathFragment} that is expanded with {@link PathFragment#getCallablePathString()}. */ - private static final class CallablePathFragment implements CommandLines.PathStrippable { - - final PathFragment fragment; - private final boolean isDerived; - - CallablePathFragment(PathFragment fragment, boolean isDerived) { - this.fragment = fragment; - this.isDerived = isDerived; - } - - @Override - public String expand(Function stripPaths) { - return isDerived ? stripPaths.apply(fragment).getCallablePathString() : toString(); - } - - @Override - public String toString() { - return fragment.getCallablePathString(); - } + /** + * Returns a {@link CommandLineItem} for the given executable. + * + *

In the common case that the executable's exec path is already {@linkplain + * PathFragment#getCallablePathString callable} (contains {@link PathFragment#SEPARATOR_CHAR}), + * returns the executable as-is to avoid creating a new object. + * + *

The only time this method can't return {@code executable} as-is is for source artifacts in + * the root package, since their exec path contains no path separator. Note that derived artifacts + * are necessarily callable since they are always under an output directory. + */ + private static CommandLineItem ensureCallable(Artifact executable) { + PathFragment execPath = executable.getExecPath(); + return execPath.getCallablePathString().equals(executable.expandToCommandLine()) + ? executable + : execPath::getCallablePathString; } } From c73d4aa20ebda7198320edc06d3fbcdfd63abadd Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 10:16:27 -0700 Subject: [PATCH 010/710] Remove empty `IjarBaseRule` class And move its children rules to use its ancestor `JavaToolchainBaseRule` PiperOrigin-RevId: 529125544 Change-Id: Ib3f12e4e81bfba1c324f700bc6c3e333ced55639 --- .../build/lib/bazel/rules/JavaRules.java | 2 -- .../bazel/rules/java/BazelJavaImportRule.java | 4 ++-- .../bazel/rules/java/BazelJavaRuleClasses.java | 4 ++-- .../build/lib/rules/java/JavaRuleClasses.java | 17 ----------------- 4 files changed, 4 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/JavaRules.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/JavaRules.java index 4b5f9e7abc7a0e..f74b286cba0f6c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/JavaRules.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/JavaRules.java @@ -36,7 +36,6 @@ import com.google.devtools.build.lib.rules.java.JavaPackageConfigurationRule; import com.google.devtools.build.lib.rules.java.JavaPluginInfo; import com.google.devtools.build.lib.rules.java.JavaPluginsFlagAliasRule; -import com.google.devtools.build.lib.rules.java.JavaRuleClasses.IjarBaseRule; import com.google.devtools.build.lib.rules.java.JavaRuleClasses.JavaRuntimeBaseRule; import com.google.devtools.build.lib.rules.java.JavaRuleClasses.JavaToolchainBaseRule; import com.google.devtools.build.lib.rules.java.JavaRuntimeRule; @@ -65,7 +64,6 @@ public void init(ConfiguredRuleClassProvider.Builder builder) { builder.addBuildInfoFactory(new BazelJavaBuildInfoFactory()); builder.addRuleDefinition(new BazelJavaRuleClasses.BaseJavaBinaryRule()); - builder.addRuleDefinition(new IjarBaseRule()); builder.addRuleDefinition(new JavaToolchainBaseRule()); builder.addRuleDefinition(new JavaRuntimeBaseRule()); builder.addRuleDefinition(new BazelJavaRuleClasses.JavaBaseRule()); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaImportRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaImportRule.java index 8f857cf78e019e..494cd857e850fd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaImportRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaImportRule.java @@ -24,7 +24,7 @@ import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.rules.java.JavaImportBaseRule; -import com.google.devtools.build.lib.rules.java.JavaRuleClasses.IjarBaseRule; +import com.google.devtools.build.lib.rules.java.JavaRuleClasses.JavaToolchainBaseRule; import com.google.devtools.build.lib.rules.java.JavaSemantics; /** @@ -77,7 +77,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) public Metadata getMetadata() { return RuleDefinition.Metadata.builder() .name("java_import") - .ancestors(JavaImportBaseRule.class, IjarBaseRule.class) + .ancestors(JavaImportBaseRule.class, JavaToolchainBaseRule.class) .factoryClass(BaseRuleClasses.EmptyRuleConfiguredTargetFactory.class) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaRuleClasses.java index cb6d9de64b6454..19e67fabc8cc12 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaRuleClasses.java @@ -48,8 +48,8 @@ import com.google.devtools.build.lib.rules.java.JavaConfiguration; import com.google.devtools.build.lib.rules.java.JavaInfo; import com.google.devtools.build.lib.rules.java.JavaPluginInfo; -import com.google.devtools.build.lib.rules.java.JavaRuleClasses.IjarBaseRule; import com.google.devtools.build.lib.rules.java.JavaRuleClasses.JavaRuntimeBaseRule; +import com.google.devtools.build.lib.rules.java.JavaRuleClasses.JavaToolchainBaseRule; import com.google.devtools.build.lib.rules.java.JavaSemantics; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.util.FileTypeSet; @@ -107,7 +107,7 @@ public Metadata getMetadata() { return RuleDefinition.Metadata.builder() .name("$java_base_rule") .type(RuleClassType.ABSTRACT) - .ancestors(IjarBaseRule.class, JavaRuntimeBaseRule.class) + .ancestors(JavaToolchainBaseRule.class, JavaRuntimeBaseRule.class) .build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuleClasses.java index ed9a1a73d51f4e..06a23fdcd82198 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuleClasses.java @@ -53,23 +53,6 @@ public static ToolchainTypeRequirement javaRuntimeToolchainTypeRequirement( .build(); } - /** Common attributes for rules that depend on ijar. */ - public static final class IjarBaseRule implements RuleDefinition { - @Override - public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) { - return builder.setPreferredDependencyPredicate(JavaSemantics.JAVA_SOURCE).build(); - } - - @Override - public Metadata getMetadata() { - return RuleDefinition.Metadata.builder() - .name("$ijar_base_rule") - .type(RuleClassType.ABSTRACT) - .ancestors(JavaToolchainBaseRule.class) - .build(); - } - } - /** Common attributes for rules that use the Java toolchain. */ public static final class JavaToolchainBaseRule implements RuleDefinition { @Override From c0e7d04f36f8487e7f0b6a1d61290ff917bcce11 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 10:23:04 -0700 Subject: [PATCH 011/710] Add ProxyConfiguredTargetKey. This key creates a layer of indirection between ConfiguredTargetKey and ConfiguredTargetValue. It will be used to deduplicate convergent keys once Rule transitions are moved from the dependency resolver to the top of the ConfiguredTargetFunction. PiperOrigin-RevId: 529127498 Change-Id: I73212ccc9ed4887ac0021384fabe646eb2174e35 --- .../build/lib/actions/ActionLookupData.java | 34 +-- .../build/lib/actions/ActionLookupKey.java | 38 +-- .../lib/actions/ActionLookupKeyOrProxy.java | 62 ++++ .../build/lib/actions/ActionRegistry.java | 2 +- .../devtools/build/lib/actions/Actions.java | 6 +- .../devtools/build/lib/actions/Artifact.java | 32 +- .../build/lib/actions/ArtifactFactory.java | 5 +- .../google/devtools/build/lib/actions/BUILD | 6 +- .../lib/analysis/AnalysisFailureEvent.java | 6 +- .../build/lib/analysis/AspectResolver.java | 6 +- .../google/devtools/build/lib/analysis/BUILD | 2 - .../build/lib/analysis/BuildView.java | 8 +- .../analysis/CachingAnalysisEnvironment.java | 8 +- .../build/lib/analysis/RuleContext.java | 10 +- .../IncompatibleTargetChecker.java | 6 +- .../ConfiguredTargetAndDataProducer.java | 2 +- .../producers/PlatformInfoProducer.java | 4 +- .../starlark/StarlarkActionFactory.java | 4 +- .../query2/PostAnalysisQueryEnvironment.java | 8 +- .../aquery/ActionGraphQueryEnvironment.java | 5 +- .../aquery/ConfiguredTargetValueAccessor.java | 2 +- .../cquery/ConfiguredTargetAccessor.java | 3 +- .../ConfiguredTargetQueryEnvironment.java | 8 +- .../skyframe/AbstractLabelCycleReporter.java | 12 +- .../skyframe/ActionArtifactCycleReporter.java | 3 +- .../lib/skyframe/ActionInputMapHelper.java | 6 +- .../ActionLookupConflictFindingFunction.java | 6 +- .../ActionLookupConflictFindingValue.java | 10 +- .../skyframe/ActionLookupValuesTraversal.java | 4 +- .../ActionTemplateExpansionFunction.java | 2 +- .../ActionTemplateExpansionValue.java | 15 +- .../build/lib/skyframe/ArtifactFunction.java | 7 +- .../build/lib/skyframe/AspectFunction.java | 9 +- .../build/lib/skyframe/AspectKeyCreator.java | 2 +- .../google/devtools/build/lib/skyframe/BUILD | 8 +- .../lib/skyframe/BuildDriverFunction.java | 21 +- .../build/lib/skyframe/BuildDriverKey.java | 9 +- .../skyframe/BuildInfoCollectionValue.java | 2 +- .../lib/skyframe/BzlLoadCycleReporter.java | 7 +- .../lib/skyframe/BzlmodRepoCycleReporter.java | 5 +- .../lib/skyframe/CompletionFunction.java | 2 +- .../skyframe/ConfiguredTargetFunction.java | 3 +- .../lib/skyframe/ConfiguredTargetKey.java | 289 ++++++++++++++---- .../skyframe/ConstraintValueLookupUtil.java | 7 +- .../lib/skyframe/CoverageReportValue.java | 2 +- .../lib/skyframe/PlatformLookupUtil.java | 6 +- .../lib/skyframe/PrerequisiteProducer.java | 30 +- .../RegisteredExecutionPlatformsFunction.java | 7 +- .../RegisteredToolchainsCycleReporter.java | 18 +- .../RegisteredToolchainsFunction.java | 11 +- .../skyframe/SequencedSkyframeExecutor.java | 6 +- .../build/lib/skyframe/SkyframeBuildView.java | 29 +- .../lib/skyframe/SkyframeErrorProcessor.java | 4 +- .../build/lib/skyframe/SkyframeExecutor.java | 88 +++--- .../lib/skyframe/TargetCycleReporter.java | 8 +- .../lib/skyframe/TestCompletionFunction.java | 2 +- .../lib/skyframe/ToolchainTypeLookupUtil.java | 8 +- ...elActionLookupConflictFindingFunction.java | 5 +- .../TopLevelActionLookupKeyWrapper.java | 4 +- .../lib/skyframe/WorkspaceStatusValue.java | 2 +- .../actions/util/InjectedActionLookupKey.java | 2 +- .../build/lib/analysis/RunfilesTest.java | 2 +- .../lib/analysis/util/AnalysisTestCase.java | 3 +- .../lib/analysis/util/AnalysisTestUtil.java | 3 +- .../analysis/util/BuildViewForTesting.java | 4 +- .../lib/analysis/util/BuildViewTestCase.java | 6 +- .../lib/rules/cpp/CcBinaryThinLtoTest.java | 3 +- .../rules/objc/ObjcLibraryAnalysisTest.java | 2 +- ...ValueTransformSharedTreeArtifactsTest.java | 8 + .../lib/skyframe/ArtifactFunctionTest.java | 2 +- .../lib/skyframe/BuildDriverFunctionTest.java | 12 +- .../lib/skyframe/ConfiguredTargetKeyTest.java | 139 +++++++++ .../skyframe/SkyframeErrorProcessorTest.java | 2 +- .../lib/skyframe/TargetCycleReporterTest.java | 2 +- .../skyframe/TreeArtifactMetadataTest.java | 2 +- .../util/SkyframeExecutorTestUtils.java | 3 +- 76 files changed, 776 insertions(+), 345 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/ActionLookupKeyOrProxy.java create mode 100644 src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKeyTest.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java index 9dc46b0f5eb62c..ec7b03f52402ab 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java @@ -36,7 +36,7 @@ public abstract class ActionLookupData implements ExecutionPhaseSkyKey { * Creates a key for the result of action execution. Does not intern its results, so should * only be called once per {@code (actionLookupKey, actionIndex)} pair. */ - public static ActionLookupData create(ActionLookupKey actionLookupKey, int actionIndex) { + public static ActionLookupData create(ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { if (!actionLookupKey.mayOwnShareableActions()) { return createUnshareable(actionLookupKey, actionIndex); } @@ -71,17 +71,17 @@ public static ActionLookupData create(ActionLookupKey actionLookupKey, int actio * #valueIsShareable}. */ public static ActionLookupData createUnshareable( - ActionLookupKey actionLookupKey, int actionIndex) { + ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { return new UnshareableActionLookupData(actionLookupKey, actionIndex); } - private final ActionLookupKey actionLookupKey; + private final ActionLookupKeyOrProxy actionLookupKey; - private ActionLookupData(ActionLookupKey actionLookupKey) { + private ActionLookupData(ActionLookupKeyOrProxy actionLookupKey) { this.actionLookupKey = Preconditions.checkNotNull(actionLookupKey); } - public final ActionLookupKey getActionLookupKey() { + public ActionLookupKeyOrProxy getActionLookupKey() { return actionLookupKey; } @@ -132,7 +132,7 @@ public final SkyFunctionName functionName() { } private static final class ActionLookupData0 extends ActionLookupData { - private ActionLookupData0(ActionLookupKey actionLookupKey) { + private ActionLookupData0(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -143,7 +143,7 @@ public int getActionIndex() { } private static final class ActionLookupData1 extends ActionLookupData { - private ActionLookupData1(ActionLookupKey actionLookupKey) { + private ActionLookupData1(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -154,7 +154,7 @@ public int getActionIndex() { } private static final class ActionLookupData2 extends ActionLookupData { - private ActionLookupData2(ActionLookupKey actionLookupKey) { + private ActionLookupData2(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -165,7 +165,7 @@ public int getActionIndex() { } private static final class ActionLookupData3 extends ActionLookupData { - private ActionLookupData3(ActionLookupKey actionLookupKey) { + private ActionLookupData3(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -176,7 +176,7 @@ public int getActionIndex() { } private static final class ActionLookupData4 extends ActionLookupData { - private ActionLookupData4(ActionLookupKey actionLookupKey) { + private ActionLookupData4(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -187,7 +187,7 @@ public int getActionIndex() { } private static final class ActionLookupData5 extends ActionLookupData { - private ActionLookupData5(ActionLookupKey actionLookupKey) { + private ActionLookupData5(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -198,7 +198,7 @@ public int getActionIndex() { } private static final class ActionLookupData6 extends ActionLookupData { - private ActionLookupData6(ActionLookupKey actionLookupKey) { + private ActionLookupData6(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -209,7 +209,7 @@ public int getActionIndex() { } private static final class ActionLookupData7 extends ActionLookupData { - private ActionLookupData7(ActionLookupKey actionLookupKey) { + private ActionLookupData7(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -220,7 +220,7 @@ public int getActionIndex() { } private static final class ActionLookupData8 extends ActionLookupData { - private ActionLookupData8(ActionLookupKey actionLookupKey) { + private ActionLookupData8(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -231,7 +231,7 @@ public int getActionIndex() { } private static final class ActionLookupData9 extends ActionLookupData { - private ActionLookupData9(ActionLookupKey actionLookupKey) { + private ActionLookupData9(ActionLookupKeyOrProxy actionLookupKey) { super(actionLookupKey); } @@ -244,7 +244,7 @@ public int getActionIndex() { private static class ActionLookupDataN extends ActionLookupData { private final int actionIndex; - private ActionLookupDataN(ActionLookupKey actionLookupKey, int actionIndex) { + private ActionLookupDataN(ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { super(actionLookupKey); this.actionIndex = actionIndex; } @@ -256,7 +256,7 @@ public final int getActionIndex() { } private static final class UnshareableActionLookupData extends ActionLookupDataN { - private UnshareableActionLookupData(ActionLookupKey actionLookupKey, int actionIndex) { + private UnshareableActionLookupData(ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { super(actionLookupKey, actionIndex); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java index 23f07b1121b764..d522718af238a8 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java @@ -13,49 +13,23 @@ // limitations under the License. package com.google.devtools.build.lib.actions; -import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; import com.google.devtools.build.skyframe.CPUHeavySkyKey; import com.google.devtools.build.skyframe.SkyKey; -import javax.annotation.Nullable; /** * {@link SkyKey} for an "analysis object": either an {@link ActionLookupValue} or a {@link * com.google.devtools.build.lib.analysis.ConfiguredTargetValue}. * - *

Whether a configured target creates actions cannot be inferred from its {@link - * com.google.devtools.build.lib.cmdline.Label} without performing analysis, so this class is used - * for both types. Non-{@link ActionLookupValue} nodes are not accessed during the execution phase. - * - *

All subclasses of {@link ActionLookupValue} "own" artifacts with {@link ArtifactOwner}s that - * are subclasses of {@link ActionLookupKey}. This allows callers to easily find the value key, - * while remaining agnostic to what action lookup values actually exist. + *

See {@link ActionLookupKeyOrProxy}. */ -public abstract class ActionLookupKey implements ArtifactOwner, CPUHeavySkyKey { - - /** - * Returns the {@link BuildConfigurationKey} for the configuration associated with this key, or - * {@code null} if this key has no associated configuration. - */ - @Nullable - public abstract BuildConfigurationKey getConfigurationKey(); - - /** - * Returns {@code true} if this key may own shareable actions, as determined by {@link - * ActionLookupData#valueIsShareable}. - * - *

Returns {@code false} for some non-standard keys such as the build info key and coverage - * report key. - * - *

A return of {@code true} still requires checking {@link ActionLookupData#valueIsShareable} - * to determine whether the individual action can be shared - notably, for a test target, - * compilation actions are shareable, but test actions are not. - */ - public boolean mayOwnShareableActions() { - return getLabel() != null; +public interface ActionLookupKey extends ActionLookupKeyOrProxy, CPUHeavySkyKey { + @Override + default ActionLookupKey toKey() { + return this; } @Override - public boolean hasLowFanout() { + default boolean hasLowFanout() { return false; // May have >10k deps. } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKeyOrProxy.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKeyOrProxy.java new file mode 100644 index 00000000000000..92b189cf5e8c3b --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKeyOrProxy.java @@ -0,0 +1,62 @@ +// 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.lib.actions; + +import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; +import com.google.devtools.build.skyframe.SkyKey; +import javax.annotation.Nullable; + +/** + * Supplies a {@link SkyKey} for an "analysis object": either an {@link ActionLookupValue} or a + * {@link com.google.devtools.build.lib.analysis.ConfiguredTargetValue}. + * + *

Whether a configured target creates actions cannot be inferred from its {@link + * com.google.devtools.build.lib.cmdline.Label} without performing analysis, so this class is used + * for both types. Non-{@link ActionLookupValue} nodes are not accessed during the execution phase. + * + *

All subclasses of {@link ActionLookupValue} "own" artifacts with {@link ArtifactOwner}s that + * are subclasses of {@link ActionLookupKeyOrProxy}. This allows callers to easily find the value + * key, while remaining agnostic to what action lookup values actually exist. + */ +public interface ActionLookupKeyOrProxy extends ArtifactOwner { + /** + * Returns the {@link BuildConfigurationKey} for the configuration associated with this key, or + * {@code null} if this key has no associated configuration. + */ + @Nullable + BuildConfigurationKey getConfigurationKey(); + + /** + * Returns {@code true} if {@link #toKey} may own shareable actions, as determined by + * {@link ActionLookupData#valueIsShareable}. + * + *

Returns {@code false} for some non-standard keys such as the build info key and coverage + * report key. + * + *

A return of {@code true} still requires checking {@link ActionLookupData#valueIsShareable} + * to determine whether the individual action can be shared - notably, for a test target, + * compilation actions are shareable, but test actions are not. + */ + default boolean mayOwnShareableActions() { + return getLabel() != null; + } + + /** + * Returns the key that owns this key's value in Skyframe. + * + *

Usually returns {@code this} but returns a delegate for {@link + * com.google.devtools.build.lib.skyframe.ConfiguredTargetKey.ProxyConfiguredTargetKey}. + */ + ActionLookupKey toKey(); +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionRegistry.java b/src/main/java/com/google/devtools/build/lib/actions/ActionRegistry.java index c0bb184b5904f6..2b1cd56cec4355 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionRegistry.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionRegistry.java @@ -20,5 +20,5 @@ public interface ActionRegistry { void registerAction(ActionAnalysisMetadata action); /** Get the key of the ConfiguredTarget/Aspect ultimately responsible for all these actions. */ - ActionLookupKey getOwner(); + ActionLookupKeyOrProxy getOwner(); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java index b55f977c6cca19..44a4d1a7842665 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java @@ -156,7 +156,7 @@ private static boolean artifactsEqualWithoutOwner( public static GeneratingActions assignOwnersAndFindAndThrowActionConflict( ActionKeyContext actionKeyContext, ImmutableList actions, - ActionLookupKey actionLookupKey) + ActionLookupKeyOrProxy actionLookupKey) throws ActionConflictException, InterruptedException, ArtifactGeneratedByOtherRuleException { return Actions.assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict( actionKeyContext, @@ -182,7 +182,7 @@ public static GeneratingActions assignOwnersAndFindAndThrowActionConflict( public static GeneratingActions assignOwnersAndFilterSharedActionsAndThrowActionConflict( ActionKeyContext actionKeyContext, ImmutableList actions, - ActionLookupKey actionLookupKey, + ActionLookupKeyOrProxy actionLookupKey, @Nullable Collection outputFiles) throws ActionConflictException, InterruptedException, ArtifactGeneratedByOtherRuleException { return Actions.assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict( @@ -232,7 +232,7 @@ private static void verifyGeneratingActionKeys( private static GeneratingActions assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict( ActionKeyContext actionKeyContext, ImmutableList actions, - ActionLookupKey actionLookupKey, + ActionLookupKeyOrProxy actionLookupKey, boolean allowSharedAction, @Nullable Collection outputFiles) throws ActionConflictException, InterruptedException, ArtifactGeneratedByOtherRuleException { diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index ee637cf2c340be..49afdc4bba7c34 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -330,9 +330,9 @@ private Artifact(ArtifactRoot root, PathFragment execPath, int hashCodeWithOwner public static class DerivedArtifact extends Artifact implements PathStrippable { /** - * An {@link ActionLookupKey} until {@link #setGeneratingActionKey} is set, at which point it is - * an {@link ActionLookupData}, whose {@link ActionLookupData#getActionLookupKey} will be the - * same as the original value of owner. + * An {@link ActionLookupKeyOrProxy} until {@link #setGeneratingActionKey} is set, at which + * point it is an {@link ActionLookupData}, whose {@link ActionLookupData#getActionLookupKey} + * will be the same as the original value of owner. * *

We overload this field in order to save memory. */ @@ -348,17 +348,20 @@ public static class DerivedArtifact extends Artifact implements PathStrippable { /** Standard factory method for derived artifacts. */ public static DerivedArtifact create( - ArtifactRoot root, PathFragment execPath, ActionLookupKey owner) { + ArtifactRoot root, PathFragment execPath, ActionLookupKeyOrProxy owner) { return create(root, execPath, owner, /*contentBasedPath=*/ false); } /** - * Same as {@link #create(ArtifactRoot, PathFragment, ActionLookupKey)} but includes the option - * to use a content-based path for this artifact (see {@link + * Same as {@link #create(ArtifactRoot, PathFragment, ActionLookupKeyOrOwner)} but includes the + * option to use a content-based path for this artifact (see {@link * com.google.devtools.build.lib.analysis.config.BuildConfigurationValue#useContentBasedOutputPaths}). */ public static DerivedArtifact create( - ArtifactRoot root, PathFragment execPath, ActionLookupKey owner, boolean contentBasedPath) { + ArtifactRoot root, + PathFragment execPath, + ActionLookupKeyOrProxy owner, + boolean contentBasedPath) { return new DerivedArtifact(root, execPath, owner, contentBasedPath); } @@ -384,7 +387,7 @@ public final void setGeneratingActionKey(ActionLookupData generatingActionKey) { Preconditions.checkState( this.owner != OMITTED_FOR_SERIALIZATION, "Owner was omitted for serialization: %s", this); Preconditions.checkState( - this.owner instanceof ActionLookupKey, + this.owner instanceof ActionLookupKeyOrProxy, "Already set generating action key: %s (%s %s)", this, this.owner, @@ -410,12 +413,12 @@ public final ActionLookupData getGeneratingActionKey() { } @Override - public final ActionLookupKey getArtifactOwner() { + public final ActionLookupKeyOrProxy getArtifactOwner() { Preconditions.checkState( this.owner != OMITTED_FOR_SERIALIZATION, "Owner was omitted for serialization: %s", this); return owner instanceof ActionLookupData ? getGeneratingActionKey().getActionLookupKey() - : (ActionLookupKey) owner; + : (ActionLookupKeyOrProxy) owner; } /** @@ -1050,7 +1053,10 @@ public static final class SpecialArtifact extends DerivedArtifact { @VisibleForTesting public static SpecialArtifact create( - ArtifactRoot root, PathFragment execPath, ActionLookupKey owner, SpecialArtifactType type) { + ArtifactRoot root, + PathFragment execPath, + ActionLookupKeyOrProxy owner, + SpecialArtifactType type) { return new SpecialArtifact(root, execPath, owner, type); } @@ -1413,8 +1419,8 @@ public boolean isChildOfDeclaredDirectory() { return !isActionTemplateExpansionKey(getArtifactOwner()); } - private static boolean isActionTemplateExpansionKey(ActionLookupKey key) { - return SkyFunctions.ACTION_TEMPLATE_EXPANSION.equals(key.functionName()); + private static boolean isActionTemplateExpansionKey(ActionLookupKeyOrProxy key) { + return SkyFunctions.ACTION_TEMPLATE_EXPANSION.equals(key.toKey().functionName()); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java index 0be8fa2239a08b..e9c7d0e9fe2b12 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java @@ -353,9 +353,10 @@ private static Artifact createArtifact( if (type == null) { return root.isSourceRoot() ? new Artifact.SourceArtifact(root, execPath, owner) - : DerivedArtifact.create(root, execPath, (ActionLookupKey) owner, contentBasedPath); + : DerivedArtifact.create( + root, execPath, (ActionLookupKeyOrProxy) owner, contentBasedPath); } else { - return SpecialArtifact.create(root, execPath, (ActionLookupKey) owner, type); + return SpecialArtifact.create(root, execPath, (ActionLookupKeyOrProxy) owner, type); } } 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 3fd934b1893fc6..ce619757a54523 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -104,6 +104,7 @@ java_library( "//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/shell", + "//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration", "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions", "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_aware_action", @@ -180,7 +181,10 @@ java_library( java_library( name = "action_lookup_key", - srcs = ["ActionLookupKey.java"], + srcs = [ + "ActionLookupKey.java", + "ActionLookupKeyOrProxy.java", + ], deps = [ ":artifact_owner", "//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java index a2a537adce6333..7e721026b31ea7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java @@ -19,7 +19,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.buildeventstream.BuildEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventContext; import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; @@ -46,7 +46,9 @@ public class AnalysisFailureEvent implements BuildEvent { private final NestedSet rootCauses; public AnalysisFailureEvent( - ActionLookupKey failedTarget, BuildEventId configuration, NestedSet rootCauses) { + ActionLookupKeyOrProxy failedTarget, + BuildEventId configuration, + NestedSet rootCauses) { Preconditions.checkArgument( failedTarget instanceof ConfiguredTargetKey || failedTarget instanceof AspectKey); if (failedTarget instanceof ConfiguredTargetKey) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java index c596065a0e6dde..78f4a1a2e9c431 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java @@ -51,7 +51,7 @@ public final class AspectResolver { @Nullable public static OrderedSetMultimap resolveAspectDependencies( SkyFunction.Environment env, - Map configuredTargetMap, + Map configuredTargetMap, Iterable deps, @Nullable NestedSetBuilder transitivePackages) throws AspectCreationException, InterruptedException { @@ -117,7 +117,7 @@ public static OrderedSetMultimap resolveAspectDepe */ public static OrderedSetMultimap mergeAspects( OrderedSetMultimap depValueNames, - Map depConfiguredTargetMap, + Map depConfiguredTargetMap, OrderedSetMultimap depAspectMap) throws DuplicateException { OrderedSetMultimap result = @@ -125,7 +125,7 @@ public static OrderedSetMultimap mergeA for (Map.Entry entry : depValueNames.entries()) { Dependency dep = entry.getValue(); - SkyKey depKey = dep.getConfiguredTargetKey(); + ConfiguredTargetKey depKey = dep.getConfiguredTargetKey(); ConfiguredTargetAndData depConfiguredTarget = depConfiguredTargetMap.get(depKey); result.put( 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 7175dbfba1e914..06ac2948092b33 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -380,7 +380,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib:runtime/build_event_streamer_utils", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//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", @@ -626,7 +625,6 @@ java_library( ":view_creation_failed_exception", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:package_roots", "//src/main/java/com/google/devtools/build/lib/actions:resource_manager", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 29be53e217354e..ee06d26894adb1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -31,7 +31,7 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionLookupData; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; @@ -648,7 +648,9 @@ public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { ((Artifact.DerivedArtifact) artifact).getGeneratingActionKey(); ActionLookupValue val; try { - val = (ActionLookupValue) graph.getValue(generatingActionKey.getActionLookupKey()); + val = + (ActionLookupValue) + graph.getValue(generatingActionKey.getActionLookupKey().toKey()); } catch (InterruptedException e) { throw new IllegalStateException( "Interruption not expected from this graph: " + generatingActionKey, e); @@ -813,7 +815,7 @@ private static ImmutableList filterTransitiveExtraActions( // might have injected. for (Artifact.DerivedArtifact artifact : provider.getTransitiveExtraActionArtifacts().toList()) { - ActionLookupKey owner = artifact.getArtifactOwner(); + ActionLookupKeyOrProxy owner = artifact.getArtifactOwner(); if (owner instanceof AspectKey) { if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) { artifacts.add(artifact); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java index d9c9d5d171ffbb..b09a22a1b13dcc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java @@ -21,7 +21,7 @@ import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionKeyContext; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactFactory; @@ -60,7 +60,7 @@ public final class CachingAnalysisEnvironment implements AnalysisEnvironment { private final ArtifactFactory artifactFactory; - private final ActionLookupKey owner; + private final ActionLookupKeyOrProxy owner; private final boolean extendedSanityChecks; private final boolean allowAnalysisFailures; private final ActionKeyContext actionKeyContext; @@ -91,7 +91,7 @@ public final class CachingAnalysisEnvironment implements AnalysisEnvironment { public CachingAnalysisEnvironment( ArtifactFactory artifactFactory, ActionKeyContext actionKeyContext, - ActionLookupKey owner, + ActionLookupKeyOrProxy owner, boolean extendedSanityChecks, boolean allowAnalysisFailures, ExtendedEventHandler errorEventListener, @@ -386,7 +386,7 @@ public ImmutableList getBuildInfo( } @Override - public ActionLookupKey getOwner() { + public ActionLookupKeyOrProxy getOwner() { return owner; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index bed39fb1465dd4..6f3f6950b2ad83 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -36,7 +36,7 @@ import com.google.common.collect.Multimaps; import com.google.common.collect.Streams; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionRegistry; import com.google.devtools.build.lib.actions.Artifact; @@ -170,7 +170,7 @@ void validate( /** Map of exec group names to ActionOwners. */ private final Map actionOwners = new HashMap<>(); - private final SymbolGenerator actionOwnerSymbolGenerator; + private final SymbolGenerator actionOwnerSymbolGenerator; /* lazily computed cache for Make variables, computed from the above. See get... method */ private transient ConfigurationMakeVariableContext configurationMakeVariableContext = null; @@ -491,7 +491,7 @@ public boolean isLegalFragment(Class fragment) { } @Override - public ActionLookupKey getOwner() { + public ActionLookupKeyOrProxy getOwner() { return getAnalysisEnvironment().getOwner(); } @@ -1656,7 +1656,7 @@ public static final class Builder implements RuleErrorConsumer { private final RuleErrorConsumer reporter; private ConfiguredRuleClassProvider ruleClassProvider; private ConfigurationFragmentPolicy configurationFragmentPolicy; - private ActionLookupKey actionOwnerSymbol; + private ActionLookupKeyOrProxy actionOwnerSymbol; private OrderedSetMultimap prerequisiteMap; private ConfigConditions configConditions; private Mutability mutability; @@ -1788,7 +1788,7 @@ public Builder setConfigurationFragmentPolicy(ConfigurationFragmentPolicy policy } @CanIgnoreReturnValue - public Builder setActionOwnerSymbol(ActionLookupKey actionOwnerSymbol) { + public Builder setActionOwnerSymbol(ActionLookupKeyOrProxy actionOwnerSymbol) { this.actionOwnerSymbol = actionOwnerSymbol; return this; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java index 4030580586b39c..7fac66471e79c5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java @@ -166,7 +166,11 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) { } for (Label label : targetCompatibleWith) { tasks.lookUp( - ConfiguredTargetKey.builder().setLabel(label).setConfiguration(configuration).build(), + ConfiguredTargetKey.builder() + .setLabel(label) + .setConfiguration(configuration) + .build() + .toKey(), this); } return this::processResult; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/ConfiguredTargetAndDataProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/ConfiguredTargetAndDataProducer.java index 3f0c726f9558f2..a8ad363c22f7b8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/ConfiguredTargetAndDataProducer.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/ConfiguredTargetAndDataProducer.java @@ -79,7 +79,7 @@ public ConfiguredTargetAndDataProducer( @Override public StateMachine step(Tasks tasks, ExtendedEventHandler listener) { tasks.lookUp( - key, + key.toKey(), ConfiguredValueCreationException.class, (ValueOrExceptionSink) this); return this::fetchConfigurationAndPackage; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/PlatformInfoProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/PlatformInfoProducer.java index 3d543969138627..0f5662f83dfe8b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/PlatformInfoProducer.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/PlatformInfoProducer.java @@ -108,7 +108,9 @@ private StateMachine lookupPlatform(Tasks tasks, ExtendedEventHandler listener) } tasks.lookUp( - platformKey, ConfiguredValueCreationException.class, this::acceptPlatformValueOrError); + platformKey.toKey(), + ConfiguredValueCreationException.class, + this::acceptPlatformValueOrError); return this::retrievePlatformInfo; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java index b263f447d55c22..63c54b74eb47b6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java @@ -24,7 +24,7 @@ import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionRegistry; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; @@ -142,7 +142,7 @@ public void registerAction(ActionAnalysisMetadata action) { } @Override - public ActionLookupKey getOwner() { + public ActionLookupKeyOrProxy getOwner() { return starlarkActionFactory .getActionConstructionContext() .getAnalysisEnvironment() diff --git a/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java index 94257f047610cf..77224f5d613bb6 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java @@ -218,7 +218,7 @@ public ConfiguredTargetValue getConfiguredTargetValue(SkyKey key) throws Interru } private boolean isAliasConfiguredTarget(ConfiguredTargetKey key) throws InterruptedException { - return AliasProvider.isAlias(getConfiguredTargetValue(key).getConfiguredTarget()); + return AliasProvider.isAlias(getConfiguredTargetValue(key.toKey()).getConfiguredTarget()); } public InterruptibleSupplier> @@ -248,7 +248,7 @@ public RepositoryMapping getMainRepoMapping() { public ThreadSafeMutableSet getFwdDeps(Iterable targets) throws InterruptedException { Map targetsByKey = Maps.newHashMapWithExpectedSize(Iterables.size(targets)); for (T target : targets) { - targetsByKey.put(getSkyKey(target), target); + targetsByKey.put(getConfiguredTargetKey(target).toKey(), target); } Map>> directDeps = targetifyValues(targetsByKey, graph.getDirectDeps(targetsByKey.keySet())); @@ -285,7 +285,7 @@ public Collection getReverseDeps(Iterable targets, QueryExpressionContext< throws InterruptedException { Map targetsByKey = Maps.newHashMapWithExpectedSize(Iterables.size(targets)); for (T target : targets) { - targetsByKey.put(getSkyKey(target), target); + targetsByKey.put(getConfiguredTargetKey(target).toKey(), target); } Map>> reverseDepsByKey = targetifyValues(targetsByKey, graph.getReverseDeps(targetsByKey.keySet())); @@ -500,7 +500,7 @@ private static ImmutableList getDependencies( @Nullable protected abstract BuildConfigurationValue getConfiguration(T target); - protected abstract ConfiguredTargetKey getSkyKey(T target); + protected abstract ConfiguredTargetKey getConfiguredTargetKey(T target); @Override public ThreadSafeMutableSet getTransitiveClosure( diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java index a27ef0eca4c1ec..5bb81e84b6f964 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java @@ -197,7 +197,7 @@ public Label getCorrectLabel(KeyedConfiguredTargetValue keyedConfiguredTargetVal @Nullable private KeyedConfiguredTargetValue createKeyedConfiguredTargetValueFromKey( ConfiguredTargetKey key) throws InterruptedException { - ConfiguredTargetValue configuredTargetValue = getConfiguredTargetValue(key); + ConfiguredTargetValue configuredTargetValue = getConfiguredTargetValue(key.toKey()); return configuredTargetValue == null ? null : KeyedConfiguredTargetValue.create(configuredTargetValue, key); @@ -271,7 +271,8 @@ protected BuildConfigurationValue getConfiguration( } @Override - protected ConfiguredTargetKey getSkyKey(KeyedConfiguredTargetValue keyedConfiguredTargetValue) { + protected ConfiguredTargetKey getConfiguredTargetKey( + KeyedConfiguredTargetValue keyedConfiguredTargetValue) { return keyedConfiguredTargetValue.getConfiguredTargetKey(); } diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java index 49eee82d586457..e3fc3a937a6f20 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java @@ -164,7 +164,7 @@ private Target getTargetFromConfiguredTargetValue( public Set getAspectValues(KeyedConfiguredTargetValue keyedConfiguredTargetValue) throws InterruptedException { Set result = new HashSet<>(); - SkyKey skyKey = configuredTargetKeyExtractor.extractKey(keyedConfiguredTargetValue); + SkyKey skyKey = configuredTargetKeyExtractor.extractKey(keyedConfiguredTargetValue).toKey(); Iterable revDeps = Iterables.concat(walkableGraph.getReverseDeps(ImmutableList.of(skyKey)).values()); Label label = keyedConfiguredTargetValue.getConfiguredTarget().getLabel(); diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java index 6dbb208d6265b8..f356d3e0d46288 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java @@ -192,7 +192,8 @@ RuleConfiguredTarget getGeneratingConfiguredTarget(KeyedConfiguredTarget kct) .getGeneratingRule() .getLabel()) .setConfiguration(queryEnvironment.getConfiguration(kct)) - .build())) + .build() + .toKey())) .getConfiguredTarget(); } diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java index 7fef980aa6a42d..1beeae8e8f7be7 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java @@ -325,7 +325,11 @@ public QueryTaskFuture getTargetsMatchingPattern( private KeyedConfiguredTarget getConfiguredTarget( Label label, BuildConfigurationValue configuration) throws InterruptedException { return getValueFromKey( - ConfiguredTargetKey.builder().setLabel(label).setConfiguration(configuration).build()); + ConfiguredTargetKey.builder() + .setLabel(label) + .setConfiguration(configuration) + .build() + .toKey()); } @Override @@ -515,7 +519,7 @@ protected BuildConfigurationValue getConfiguration(KeyedConfiguredTarget target) } @Override - protected ConfiguredTargetKey getSkyKey(KeyedConfiguredTarget target) { + protected ConfiguredTargetKey getConfiguredTargetKey(KeyedConfiguredTarget target) { return target.getConfiguredTargetKey(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java index 493a971c1c69c8..d0eaf0a1f00ce1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java @@ -18,6 +18,7 @@ import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.Uninterruptibles; import com.google.devtools.build.lib.events.Event; @@ -48,8 +49,11 @@ abstract class AbstractLabelCycleReporter implements CyclesReporter.SingleCycleR protected abstract boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo); /** Returns the String representation of the {@code SkyKey}. */ - protected String prettyPrint(SkyKey key) { - return getLabel(key).toString(); + protected String prettyPrint(Object rawKey) { + if (rawKey instanceof ActionLookupKeyOrProxy) { + return ((ActionLookupKeyOrProxy) rawKey).getLabel().toString(); + } + return getLabel((SkyKey) rawKey).toString(); } /** Can be used to skip individual keys on the path to the cycle. */ @@ -130,14 +134,14 @@ public boolean maybeReportCycle( static SkyKey printCycle( ImmutableList cycle, StringBuilder cycleMessage, - Function printFunction) { + Function printFunction) { return printCycle(cycle, cycleMessage, printFunction, Predicates.alwaysFalse()); } private static SkyKey printCycle( ImmutableList cycle, StringBuilder cycleMessage, - Function printFunction, + Function printFunction, Predicate shouldSkipIntermediateKey) { Preconditions.checkArgument(!cycle.isEmpty()); SkyKey cycleValue = null; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java index daeae5352cf7b6..4ce2d234c30be8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java @@ -44,7 +44,8 @@ public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter { } @Override - protected String prettyPrint(SkyKey key) { + protected String prettyPrint(Object untypedKey) { + SkyKey key = (SkyKey) untypedKey; return prettyPrint(key.functionName(), key.argument()); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java index 8971039c3a0a69..223d66490d8527 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java @@ -21,7 +21,7 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionInputMapSink; import com.google.devtools.build.lib.actions.ActionLookupData; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; @@ -160,10 +160,10 @@ private static ImmutableList getFilesets( Environment env, SpecialArtifact actionInput) throws InterruptedException { checkState(actionInput.isFileset(), actionInput); ActionLookupData generatingActionKey = actionInput.getGeneratingActionKey(); - ActionLookupKey filesetActionLookupKey = generatingActionKey.getActionLookupKey(); + ActionLookupKeyOrProxy filesetActionLookupKey = generatingActionKey.getActionLookupKey(); ActionLookupValue filesetActionLookupValue = - (ActionLookupValue) env.getValue(filesetActionLookupKey); + (ActionLookupValue) env.getValue(filesetActionLookupKey.toKey()); ActionAnalysisMetadata generatingAction = filesetActionLookupValue.getAction(generatingActionKey.getActionIndex()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingFunction.java index c3723c7260c883..5199a6601c21c8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingFunction.java @@ -17,7 +17,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.bugreport.BugReport; @@ -44,8 +44,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { ImmutableMap actionConflicts = ACTION_CONFLICTS.get(env); - ActionLookupKey lookupKey = ((ActionLookupConflictFindingValue.Key) skyKey).argument(); - ActionLookupValue alValue = (ActionLookupValue) env.getValue(lookupKey); + ActionLookupKeyOrProxy lookupKey = ((ActionLookupConflictFindingValue.Key) skyKey).argument(); + ActionLookupValue alValue = (ActionLookupValue) env.getValue(lookupKey.toKey()); if (env.valuesMissing()) { if (!CoverageReportValue.COVERAGE_REPORT_KEY.equals(lookupKey)) { BugReport.sendBugReport( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java index fe5ee5dd2c72f3..348d0fdf5813b2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupConflictFindingValue.java @@ -15,7 +15,7 @@ import static com.google.common.base.Preconditions.checkArgument; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -35,7 +35,7 @@ public class ActionLookupConflictFindingValue implements SkyValue { private ActionLookupConflictFindingValue() {} - public static Key key(ActionLookupKey lookupKey) { + public static Key key(ActionLookupKeyOrProxy lookupKey) { return Key.create(lookupKey); } @@ -47,16 +47,16 @@ public static Key key(Artifact artifact) { @AutoCodec.VisibleForSerialization @AutoCodec - static class Key extends AbstractSkyKey { + static class Key extends AbstractSkyKey { private static final SkyKeyInterner interner = SkyKey.newInterner(); - private Key(ActionLookupKey arg) { + private Key(ActionLookupKeyOrProxy arg) { super(arg); } @AutoCodec.VisibleForSerialization @AutoCodec.Instantiator - static Key create(ActionLookupKey arg) { + static Key create(ActionLookupKeyOrProxy arg) { return interner.intern(new Key(arg)); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValuesTraversal.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValuesTraversal.java index 647bec50cc2d25..7d61c7ccba37f5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValuesTraversal.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValuesTraversal.java @@ -15,7 +15,7 @@ import static com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.NUM_JOBS; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; @@ -43,7 +43,7 @@ public class ActionLookupValuesTraversal { public ActionLookupValuesTraversal() {} - void accumulate(ActionLookupKey keyForDebugging, SkyValue value) { + void accumulate(ActionLookupKeyOrProxy keyForDebugging, SkyValue value) { boolean isConfiguredTarget = value instanceof ConfiguredTargetValue; boolean isActionLookupValue = value instanceof ActionLookupValue; if (!isConfiguredTarget && !isActionLookupValue) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java index c6820da6256e56..3b4668b7eb7e93 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java @@ -73,7 +73,7 @@ public class ActionTemplateExpansionFunction implements SkyFunction { public SkyValue compute(SkyKey skyKey, Environment env) throws ActionTemplateExpansionFunctionException, InterruptedException { ActionTemplateExpansionKey key = (ActionTemplateExpansionKey) skyKey.argument(); - ActionLookupValue value = (ActionLookupValue) env.getValue(key.getActionLookupKey()); + ActionLookupValue value = (ActionLookupValue) env.getValue(key.getActionLookupKey().toKey()); if (value == null) { // Because of the phase boundary separating analysis and execution, all needed // ActionLookupValues must have already been evaluated, so a missing ActionLookupValue is diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java index 2298a072965d53..402dc955f78af7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java @@ -17,6 +17,7 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.Actions.GeneratingActions; import com.google.devtools.build.lib.actions.BasicActionLookupValue; import com.google.devtools.build.lib.cmdline.Label; @@ -30,27 +31,29 @@ public final class ActionTemplateExpansionValue extends BasicActionLookupValue { super(generatingActions); } - public static ActionTemplateExpansionKey key(ActionLookupKey actionLookupKey, int actionIndex) { + public static ActionTemplateExpansionKey key( + ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { return ActionTemplateExpansionKey.of(actionLookupKey, actionIndex); } /** Key for {@link ActionTemplateExpansionValue} nodes. */ @AutoCodec - public static final class ActionTemplateExpansionKey extends ActionLookupKey { + public static final class ActionTemplateExpansionKey implements ActionLookupKey { private static final Interner interner = BlazeInterners.newWeakInterner(); - private final ActionLookupKey actionLookupKey; + private final ActionLookupKeyOrProxy actionLookupKey; private final int actionIndex; - private ActionTemplateExpansionKey(ActionLookupKey actionLookupKey, int actionIndex) { + private ActionTemplateExpansionKey(ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { this.actionLookupKey = actionLookupKey; this.actionIndex = actionIndex; } @VisibleForTesting @AutoCodec.Instantiator - public static ActionTemplateExpansionKey of(ActionLookupKey actionLookupKey, int actionIndex) { + public static ActionTemplateExpansionKey of( + ActionLookupKeyOrProxy actionLookupKey, int actionIndex) { return interner.intern(new ActionTemplateExpansionKey(actionLookupKey, actionIndex)); } @@ -69,7 +72,7 @@ public BuildConfigurationKey getConfigurationKey() { return actionLookupKey.getConfigurationKey(); } - public ActionLookupKey getActionLookupKey() { + public ActionLookupKeyOrProxy getActionLookupKey() { return actionLookupKey; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index 8c2a0689d1e26b..c3c317d42b9b80 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -23,7 +23,7 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionLookupData; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.ActionTemplate; import com.google.devtools.build.lib.actions.Artifact; @@ -409,8 +409,9 @@ public String extractTag(SkyKey skyKey) { @Nullable static ActionLookupValue getActionLookupValue( - ActionLookupKey actionLookupKey, SkyFunction.Environment env) throws InterruptedException { - ActionLookupValue value = (ActionLookupValue) env.getValue(actionLookupKey); + ActionLookupKeyOrProxy actionLookupKey, SkyFunction.Environment env) + throws InterruptedException { + ActionLookupValue value = (ActionLookupValue) env.getValue(actionLookupKey.toKey()); if (value == null) { Preconditions.checkState( actionLookupKey == CoverageReportValue.COVERAGE_REPORT_KEY, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index 69cbe73e926cd3..c64d876ff3542c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; +import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.AliasProvider; import com.google.devtools.build.lib.analysis.AspectResolver; @@ -459,11 +460,11 @@ static BzlLoadValue.Key bzlLoadKeyForStarlarkAspect(StarlarkAspectClass starlark private static InitialValues getInitialValues( PrerequisiteProducer.State state, AspectKey key, Environment env) throws AspectFunctionException, InterruptedException { - ConfiguredTargetKey baseConfiguredTargetKey = key.getBaseConfiguredTargetKey(); + ActionLookupKey configuredTargetLookupKey = key.getBaseConfiguredTargetKey().toKey(); PackageIdentifier basePackageKey = key.getBaseConfiguredTargetKey().getLabel().getPackageIdentifier(); var initialKeys = - ImmutableSet.builder().add(baseConfiguredTargetKey).add(basePackageKey); + ImmutableSet.builder().add(configuredTargetLookupKey).add(basePackageKey); BuildConfigurationKey configurationKey = key.getConfigurationKey(); if (configurationKey != null) { @@ -492,10 +493,10 @@ private static InitialValues getInitialValues( var baseConfiguredTargetValue = (ConfiguredTargetValue) initialValues.getOrThrow( - baseConfiguredTargetKey, ConfiguredValueCreationException.class); + configuredTargetLookupKey, ConfiguredValueCreationException.class); if (baseConfiguredTargetValue == null) { BugReport.logUnexpected( - "Unexpected exception with %s and AspectKey %s", baseConfiguredTargetKey, key); + "Unexpected exception with %s and AspectKey %s", key.getBaseConfiguredTargetKey(), key); return null; } baseConfiguredTarget = baseConfiguredTargetValue.getConfiguredTarget(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java index fc4929ba9e8276..b71215c463d2ee 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java @@ -59,7 +59,7 @@ public static TopLevelAspectsKey createTopLevelAspectsKey( } /** Common superclass for {@link AspectKey} and {@link TopLevelAspectsKey}. */ - public abstract static class AspectBaseKey extends ActionLookupKey { + public abstract static class AspectBaseKey implements ActionLookupKey { private final ConfiguredTargetKey baseConfiguredTargetKey; private final int hashCode; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 21c6ab79d79403..1d9de768d79c0d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -377,6 +377,7 @@ java_library( name = "abstract_label_cycle_reporter", srcs = ["AbstractLabelCycleReporter.java"], deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", @@ -483,7 +484,6 @@ java_library( ":tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", @@ -501,9 +501,7 @@ java_library( deps = [ ":sky_functions", "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", - "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", @@ -666,7 +664,6 @@ java_library( ":tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:middleman_type", @@ -865,7 +862,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_key", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", @@ -912,7 +908,6 @@ java_library( ":top_level_conflict_exception", ":top_level_status_events", "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", @@ -1156,6 +1151,7 @@ java_library( "//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/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/lib/util:hash_codes", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", "//third_party:jsr305", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java index ce7b4701782f38..8637f856501301 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java @@ -28,7 +28,7 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.AspectValue; import com.google.devtools.build.lib.analysis.ConfiguredAspect; @@ -146,7 +146,7 @@ public void setShouldCheckForConflict(Supplier shouldCheckForConflict) public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { BuildDriverKey buildDriverKey = (BuildDriverKey) skyKey; - ActionLookupKey actionLookupKey = buildDriverKey.getActionLookupKey(); + ActionLookupKeyOrProxy actionLookupKey = buildDriverKey.getActionLookupKey(); TopLevelArtifactContext topLevelArtifactContext = buildDriverKey.getTopLevelArtifactContext(); State state = env.getState(State::new); @@ -157,7 +157,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // Why SkyValue and not ActionLookupValue? The evaluation of some ActionLookupKey can result in // classes that don't implement ActionLookupValue // (e.g. ConfiguredTargetKey -> NonRuleConfiguredTargetValue). - SkyValue topLevelSkyValue = env.getValue(actionLookupKey); + SkyValue topLevelSkyValue = env.getValue(actionLookupKey.toKey()); if (env.valuesMissing()) { return null; @@ -396,7 +396,7 @@ private static Target getTarget(Environment env, Label label) private void requestConfiguredTargetExecution( ConfiguredTarget configuredTarget, BuildDriverKey buildDriverKey, - ActionLookupKey actionLookupKey, + ActionLookupKeyOrProxy actionLookupKey, BuildConfigurationValue buildConfigurationValue, Environment env, TopLevelArtifactContext topLevelArtifactContext, @@ -509,7 +509,8 @@ private static void declareDependenciesAndCheckValues( @VisibleForTesting ImmutableMap checkActionConflicts( - ActionLookupKey actionLookupKey, boolean strictConflictCheck) throws InterruptedException { + ActionLookupKeyOrProxy actionLookupKey, boolean strictConflictCheck) + throws InterruptedException { ActionLookupValuesCollectionResult transitiveValueCollectionResult = transitiveActionLookupValuesHelper.collect(actionLookupKey); @@ -566,20 +567,22 @@ interface TransitiveActionLookupValuesHelper { * Perform the traversal of the transitive closure of the {@code key} and collect the * corresponding ActionLookupValues. */ - ActionLookupValuesCollectionResult collect(ActionLookupKey key) throws InterruptedException; + ActionLookupValuesCollectionResult collect(ActionLookupKeyOrProxy key) + throws InterruptedException; /** Register with the helper that the {@code keys} are conflict-free. */ - void registerConflictFreeKeys(ImmutableSet keys); + void registerConflictFreeKeys(ImmutableSet keys); } @AutoValue abstract static class ActionLookupValuesCollectionResult { abstract ImmutableCollection collectedValues(); - abstract ImmutableSet visitedKeys(); + abstract ImmutableSet visitedKeys(); static ActionLookupValuesCollectionResult create( - ImmutableCollection collectedValues, ImmutableSet visitedKeys) { + ImmutableCollection collectedValues, + ImmutableSet visitedKeys) { return new AutoValue_BuildDriverFunction_ActionLookupValuesCollectionResult( collectedValues, visitedKeys); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverKey.java index f182416cdf05f0..d126acb7345457 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverKey.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.skyframe.CPUHeavySkyKey; import com.google.devtools.build.skyframe.SkyFunctionName; @@ -24,7 +25,7 @@ * the {@link ActionLookupKey} and executing the associated actions. */ public final class BuildDriverKey implements CPUHeavySkyKey { - private final ActionLookupKey actionLookupKey; + private final ActionLookupKeyOrProxy actionLookupKey; private final TopLevelArtifactContext topLevelArtifactContext; private final TestType testType; private final boolean strictActionConflictCheck; @@ -33,7 +34,7 @@ public final class BuildDriverKey implements CPUHeavySkyKey { private final boolean isTopLevelAspectDriver; private BuildDriverKey( - ActionLookupKey actionLookupKey, + ActionLookupKeyOrProxy actionLookupKey, TopLevelArtifactContext topLevelArtifactContext, boolean strictActionConflictCheck, boolean explicitlyRequested, @@ -66,7 +67,7 @@ public static BuildDriverKey ofTopLevelAspect( } public static BuildDriverKey ofConfiguredTarget( - ActionLookupKey actionLookupKey, + ActionLookupKeyOrProxy actionLookupKey, TopLevelArtifactContext topLevelArtifactContext, boolean strictActionConflictCheck, boolean explicitlyRequested, @@ -86,7 +87,7 @@ public TopLevelArtifactContext getTopLevelArtifactContext() { return topLevelArtifactContext; } - public ActionLookupKey getActionLookupKey() { + public ActionLookupKeyOrProxy getActionLookupKey() { return actionLookupKey; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java index 266b67c2cc45f7..e83e719ca1ec59 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java @@ -56,7 +56,7 @@ public static BuildInfoKeyAndConfig key( /** Key for BuildInfoCollectionValues. */ @AutoCodec - public static final class BuildInfoKeyAndConfig extends ActionLookupKey { + public static final class BuildInfoKeyAndConfig implements ActionLookupKey { private static final SkyKeyInterner keyInterner = SkyKey.newInterner(); private final BuildInfoKey infoKey; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java index c2c73e29dd055f..aa5ec38d08f17e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java @@ -88,8 +88,9 @@ public boolean maybeReportCycle( || IS_WORKSPACE_FILE.apply(lastPathElement) || IS_BZLMOD_EXTENSION.apply(lastPathElement))) { - Function printer = - input -> { + Function printer = + rawInput -> { + SkyKey input = (SkyKey) rawInput; if (input.argument() instanceof BzlLoadValue.Key) { return ((BzlLoadValue.Key) input.argument()).getLabel().toString(); } @@ -135,7 +136,7 @@ public boolean maybeReportCycle( StringBuilder cycleMessage = new StringBuilder().append("Circular definition of repositories:"); Iterable repos = Iterables.filter(cycle, IS_REPOSITORY_DIRECTORY); - Function printer = + Function printer = input -> { if (input instanceof RepositoryDirectoryValue.Key) { return ((RepositoryDirectoryValue.Key) input).argument().getNameWithAt(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java index ec5f9bf2e54503..2224251270b4bb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java @@ -100,8 +100,9 @@ public boolean maybeReportCycle( Iterable repos = Iterables.filter( cycle, Predicates.or(IS_REPOSITORY_DIRECTORY, IS_EXTENSION_IMPL, IS_BZL_LOAD)); - Function printer = - input -> { + Function printer = + rawInput -> { + SkyKey input = (SkyKey) rawInput; if (input instanceof RepositoryDirectoryValue.Key) { return ((RepositoryDirectoryValue.Key) input).argument().getNameWithAt(); } else if (input.argument() instanceof ModuleExtensionId) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index 8c15870a9ef752..74f6b02bf97da3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java @@ -328,7 +328,7 @@ private void handleSourceFileError( Pair getValueAndArtifactsToBuild( TopLevelActionLookupKeyWrapper key, Environment env) throws InterruptedException { @SuppressWarnings("unchecked") - ValueT value = (ValueT) env.getValue(key.actionLookupKey()); + ValueT value = (ValueT) env.getValue(key.actionLookupKey().toKey()); if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 3a71a9656e69e0..15606b74f4a531 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -182,12 +182,13 @@ public SkyValue compute(SkyKey key, Environment env) throws ReportedException, UnreportedException, InterruptedException { State state = env.getState(() -> new State(storeTransitivePackages)); ConfiguredTargetKey configuredTargetKey = (ConfiguredTargetKey) key.argument(); + Preconditions.checkArgument(!configuredTargetKey.isProxy(), configuredTargetKey); SkyframeBuildView view = buildViewProvider.getSkyframeBuildView(); PrerequisiteProducer prereqs = new PrerequisiteProducer(); if (shouldUnblockCpuWorkWhenFetchingDeps) { // Fetching blocks on other resources, so we don't want to hold on to the semaphore meanwhile. - // TODO(b/194319860): remove this and DepedencyGraphBuilder.SemaphoreAcquirer when we no need + // TODO(b/194319860): remove this and PrerequisiteProducer.SemaphoreAcquirer when we no need // semaphore locking. env = new StateInformingSkyFunctionEnvironment( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java index ea4c2c535fd42d..c67d4f7d8c938c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java @@ -14,17 +14,27 @@ package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.devtools.build.lib.util.HashCodes.hashObjects; import com.google.common.base.MoreObjects; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; +import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; +import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; +import com.google.devtools.build.lib.skyframe.serialization.SerializationException; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.errorprone.annotations.Keep; +import com.google.protobuf.CodedInputStream; +import com.google.protobuf.CodedOutputStream; +import java.io.IOException; import java.util.Objects; import javax.annotation.Nullable; @@ -53,76 +63,56 @@ * *

Note that this key may be used to look up the generating action of an artifact. * + *

The {@link ConfiguredTargetKey} is not a {@link SkyKey} and must be cast to one using {@link + * ActionLookupKeyOrProxy#toKey}. + * *

TODO(blaze-configurability-team): Consider just using BuildOptions over a * BuildConfigurationKey. */ -@AutoCodec -public class ConfiguredTargetKey extends ActionLookupKey { +public abstract class ConfiguredTargetKey implements ActionLookupKeyOrProxy { /** * Cache so that the number of ConfiguredTargetKey instances is {@code O(configured targets)} and * not {@code O(edges between configured targets)}. */ - private static final SkyKeyInterner interner = SkyKey.newInterner(); + private static final SkyKey.SkyKeyInterner interner = SkyKey.newInterner(); - private final Label label; @Nullable private final BuildConfigurationKey configurationKey; - private final transient int hashCode; - ConfiguredTargetKey(Label label, @Nullable BuildConfigurationKey configurationKey, int hashCode) { - this.label = checkNotNull(label); + private ConfiguredTargetKey(@Nullable BuildConfigurationKey configurationKey, int hashCode) { this.configurationKey = configurationKey; this.hashCode = hashCode; } - @AutoCodec.VisibleForSerialization - @AutoCodec.Instantiator - static ConfiguredTargetKey create(Label label, @Nullable BuildConfigurationKey configurationKey) { - int hashCode = computeHashCode(label, configurationKey, /*executionPlatformLabel=*/ null); - return interner.intern(new ConfiguredTargetKey(label, configurationKey, hashCode)); - } - public Builder toBuilder() { return builder() .setConfigurationKey(configurationKey) - .setLabel(label) + .setLabel(getLabel()) .setExecutionPlatformLabel(getExecutionPlatformLabel()); } - @Override - public final Label getLabel() { - return label; - } - - @Override - public final SkyFunctionName functionName() { - return SkyFunctions.CONFIGURED_TARGET; - } - @Nullable @Override public final BuildConfigurationKey getConfigurationKey() { return configurationKey; } - @Nullable - public Label getExecutionPlatformLabel() { - return null; - } + public abstract Label getExecutionPlatformLabel(); @Override public final int hashCode() { return hashCode; } + public boolean isProxy() { + return false; + } + private static int computeHashCode( Label label, @Nullable BuildConfigurationKey configurationKey, @Nullable Label executionPlatformLabel) { - int configVal = configurationKey == null ? 79 : configurationKey.hashCode(); - int executionPlatformLabelVal = - executionPlatformLabel == null ? 47 : executionPlatformLabel.hashCode(); - return 31 * label.hashCode() + configVal + executionPlatformLabelVal; + return hashObjects(label, configurationKey, executionPlatformLabel); } @Override @@ -135,42 +125,74 @@ public final boolean equals(Object obj) { } ConfiguredTargetKey other = (ConfiguredTargetKey) obj; return hashCode == other.hashCode - && label.equals(other.label) + && getLabel().equals(other.getLabel()) && Objects.equals(configurationKey, other.configurationKey) && Objects.equals(getExecutionPlatformLabel(), other.getExecutionPlatformLabel()); } - public final String prettyPrint() { - if (label == null) { + public String prettyPrint() { + if (getLabel() == null) { return "null"; } - return String.format( - "%s (%s)", - label, configurationKey == null ? "null" : configurationKey.getOptions().checksum()); + return String.format("%s (%s)", getLabel(), formatConfigurationKey(configurationKey)); + } + + private static ConfiguredTargetKey intern(ConfiguredTargetKey key) { + return (ConfiguredTargetKey) interner.intern((SkyKey) key); } @Override - public final String toString() { + public String toString() { // TODO(b/162809183): consider reverting to less verbose toString when bug is resolved. MoreObjects.ToStringHelper helper = - MoreObjects.toStringHelper(this).add("label", label).add("config", configurationKey); + MoreObjects.toStringHelper(this).add("label", getLabel()).add("config", configurationKey); if (getExecutionPlatformLabel() != null) { helper.add("executionPlatformLabel", getExecutionPlatformLabel()); } return helper.toString(); } - @Override - public SkyKeyInterner getSkyKeyInterner() { - return interner; - } + private static final class RealConfiguredTargetKey extends ConfiguredTargetKey + implements ActionLookupKey { + private final Label label; + + private RealConfiguredTargetKey( + Label label, @Nullable BuildConfigurationKey configurationKey, int hashCode) { + super(configurationKey, hashCode); + this.label = label; + } + + static ConfiguredTargetKey create( + Label label, @Nullable BuildConfigurationKey configurationKey) { + int hashCode = computeHashCode(label, configurationKey, /* executionPlatformLabel= */ null); + return intern(new RealConfiguredTargetKey(label, configurationKey, hashCode)); + } + + @Override + public final SkyFunctionName functionName() { + return SkyFunctions.CONFIGURED_TARGET; + } - @AutoCodec.VisibleForSerialization - @AutoCodec - static class ToolchainDependencyConfiguredTargetKey extends ConfiguredTargetKey { - private static final SkyKeyInterner - toolchainDependencyConfiguredTargetKeyInterner = SkyKey.newInterner(); + @Override + public SkyKeyInterner getSkyKeyInterner() { + return interner; + } + @Override + public Label getLabel() { + return label; + } + + @Nullable + @Override + public Label getExecutionPlatformLabel() { + return null; + } + } + + private static final class ToolchainDependencyConfiguredTargetKey extends ConfiguredTargetKey + implements ActionLookupKey { + private final Label label; private final Label executionPlatformLabel; private ToolchainDependencyConfiguredTargetKey( @@ -178,30 +200,118 @@ private ToolchainDependencyConfiguredTargetKey( @Nullable BuildConfigurationKey configurationKey, int hashCode, Label executionPlatformLabel) { - super(label, configurationKey, hashCode); + super(configurationKey, hashCode); + this.label = label; this.executionPlatformLabel = checkNotNull(executionPlatformLabel); } - @AutoCodec.VisibleForSerialization - @AutoCodec.Instantiator - static ToolchainDependencyConfiguredTargetKey create( + private static ConfiguredTargetKey create( Label label, @Nullable BuildConfigurationKey configurationKey, Label executionPlatformLabel) { int hashCode = computeHashCode(label, configurationKey, executionPlatformLabel); - return toolchainDependencyConfiguredTargetKeyInterner.intern( + return intern( new ToolchainDependencyConfiguredTargetKey( label, configurationKey, hashCode, executionPlatformLabel)); } @Override - public final Label getExecutionPlatformLabel() { + public SkyFunctionName functionName() { + return SkyFunctions.CONFIGURED_TARGET; + } + + @Override + public Label getLabel() { + return label; + } + + @Override + public Label getExecutionPlatformLabel() { return executionPlatformLabel; } @Override - public final SkyKeyInterner getSkyKeyInterner() { - return toolchainDependencyConfiguredTargetKeyInterner; + public SkyKeyInterner getSkyKeyInterner() { + return interner; + } + } + + // This class implements SkyKey only so that it can share the interner. It should never be used as + // a SkyKey. + private static final class ProxyConfiguredTargetKey extends ConfiguredTargetKey + implements SkyKey { + private final ConfiguredTargetKey delegate; + + private static ConfiguredTargetKey create( + ConfiguredTargetKey delegate, @Nullable BuildConfigurationKey configurationKey) { + int hashCode = + computeHashCode( + delegate.getLabel(), configurationKey, delegate.getExecutionPlatformLabel()); + return intern(new ProxyConfiguredTargetKey(delegate, configurationKey, hashCode)); + } + + private ProxyConfiguredTargetKey( + ConfiguredTargetKey delegate, + @Nullable BuildConfigurationKey configurationKey, + int hashCode) { + super(configurationKey, hashCode); + checkArgument( + !delegate.isProxy(), "Proxy keys must not be nested: %s %s", delegate, configurationKey); + this.delegate = delegate; + } + + @Override + public SkyFunctionName functionName() { + // ProxyConfiguredTargetKey is never used directly by Skyframe. It must always be cast using + // toKey. + throw new UnsupportedOperationException(); + } + + @Override + public Label getLabel() { + return delegate.getLabel(); + } + + @Override + @Nullable + public Label getExecutionPlatformLabel() { + return delegate.getExecutionPlatformLabel(); + } + + @Override + public ActionLookupKey toKey() { + return (ActionLookupKey) delegate; + } + + @Override + public boolean isProxy() { + return true; + } + + @Override + public Builder toBuilder() { + return new Builder().setDelegate(delegate).setConfigurationKey(getConfigurationKey()); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("delegate", delegate) + .add("config", getConfigurationKey()) + .toString(); + } + + @Override + public String prettyPrint() { + return super.prettyPrint() + + " virtual(" + + formatConfigurationKey(getConfigurationKey()) + + ")"; + } + + @Override + public SkyKeyInterner getSkyKeyInterner() { + return interner; } } @@ -223,6 +333,7 @@ public static final class Builder { private Label label = null; private BuildConfigurationKey configurationKey = null; private Label executionPlatformLabel = null; + private ConfiguredTargetKey delegate; private Builder() {} @@ -256,13 +367,71 @@ public Builder setExecutionPlatformLabel(@Nullable Label executionPlatformLabel) return this; } + /** + * If set, creates a {@link ProxyConfiguredTargetKey}. + * + *

It's invalid to set a label or execution platform label if this is set. Those will be + * defined by the corresponding values of {@code delegate}. + */ + @CanIgnoreReturnValue + public Builder setDelegate(ConfiguredTargetKey delegate) { + this.delegate = delegate; + return this; + } + /** Builds a new {@link ConfiguredTargetKey} based on the supplied data. */ public ConfiguredTargetKey build() { + if (this.delegate != null) { + checkArgument(label == null); + checkArgument(executionPlatformLabel == null); + return ProxyConfiguredTargetKey.create(delegate, configurationKey); + } if (this.executionPlatformLabel != null) { return ToolchainDependencyConfiguredTargetKey.create( label, configurationKey, executionPlatformLabel); } - return create(label, configurationKey); + return RealConfiguredTargetKey.create(label, configurationKey); + } + } + + private static String formatConfigurationKey(@Nullable BuildConfigurationKey key) { + if (key == null) { + return "null"; + } + return key.getOptions().checksum(); + } + + /** + * Codec for all {@link ConfiguredTargetKey} subtypes. + * + *

By design, {@link ProxyConfiguredTargetKey} serializes as a key without delegation. Upon + * deserialization, if the key is locally delegated, it becomes delegating again due to interning. + * If not, it deserializes to the appropriate non-delegating key. + */ + @Keep + private static class ConfiguredTargetKeyCodec implements ObjectCodec { + @Override + public Class getEncodedClass() { + return ConfiguredTargetKey.class; + } + + @Override + public void serialize( + SerializationContext context, ConfiguredTargetKey key, CodedOutputStream codedOut) + throws SerializationException, IOException { + context.serialize(key.getLabel(), codedOut); + context.serialize(key.getConfigurationKey(), codedOut); + context.serialize(key.getExecutionPlatformLabel(), codedOut); + } + + @Override + public ConfiguredTargetKey deserialize(DeserializationContext context, CodedInputStream codedIn) + throws SerializationException, IOException { + return builder() + .setLabel(context.deserialize(codedIn)) + .setConfigurationKey(context.deserialize(codedIn)) + .setExecutionPlatformLabel(context.deserialize(codedIn)) + .build(); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConstraintValueLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConstraintValueLookupUtil.java index b1b12163fb5a85..3b36b919db3ad9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConstraintValueLookupUtil.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConstraintValueLookupUtil.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; @@ -36,7 +37,9 @@ public static List getConstraintValueInfo( Iterable constraintValueKeys, Environment env) throws InterruptedException, InvalidConstraintValueException { - SkyframeLookupResult values = env.getValuesAndExceptions(constraintValueKeys); + SkyframeLookupResult values = + env.getValuesAndExceptions( + Iterables.transform(constraintValueKeys, ConfiguredTargetKey::toKey)); boolean valuesMissing = env.valuesMissing(); List constraintValues = valuesMissing ? null : new ArrayList<>(); for (ConfiguredTargetKey key : constraintValueKeys) { @@ -64,7 +67,7 @@ private static ConstraintValueInfo findConstraintValueInfo( ConfiguredTargetValue ctv = (ConfiguredTargetValue) values.getOrThrow( - key, + key.toKey(), ConfiguredValueCreationException.class, NoSuchThingException.class, ActionConflictException.class); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java index fd474db5f5b17d..e7001dfb305540 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java @@ -32,7 +32,7 @@ public final class CoverageReportValue extends BasicActionLookupValue { super(generatingActions); } - private static final class CoverageReportKey extends ActionLookupKey { + private static final class CoverageReportKey implements ActionLookupKey { private CoverageReportKey() {} @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java index 45ec7f66fdce64..764cdcdf67ad5b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; @@ -51,7 +52,8 @@ public static Map getPlatformInfo( return null; } - SkyframeLookupResult values = env.getValuesAndExceptions(platformKeys); + SkyframeLookupResult values = + env.getValuesAndExceptions(Lists.transform(platformKeys, ConfiguredTargetKey::toKey)); boolean valuesMissing = env.valuesMissing(); Map platforms = valuesMissing ? null : new HashMap<>(); for (ConfiguredTargetKey key : platformKeys) { @@ -130,7 +132,7 @@ private static PlatformInfo findPlatformInfo(ConfiguredTargetKey key, SkyframeLo ConfiguredTargetValue ctv = (ConfiguredTargetValue) values.getOrThrow( - key, + key.toKey(), ConfiguredValueCreationException.class, NoSuchThingException.class, ActionConflictException.class); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java index ce34664f0b6862..b26aa0842526bb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java @@ -155,7 +155,8 @@ public static class State implements SkyKeyComputeState, DependencyContextProduc /** Null if not yet computed or if {@link #computeDependenciesResult} is non-null. */ @Nullable - private Map resolveConfiguredTargetDependenciesResult; + private Map + resolveConfiguredTargetDependenciesResult; /** * Non-null if all the work in {@link #computeDependencies} is already done. This field contains @@ -726,7 +727,7 @@ static OrderedSetMultimap computeDepend } // Resolve configured target dependencies and handle errors. - Map depValues; + Map depValues; if (state.resolveConfiguredTargetDependenciesResult != null) { depValues = state.resolveConfiguredTargetDependenciesResult; } else { @@ -892,13 +893,14 @@ private static BuildConfigurationKey computeToolchainConfigurationKey( *

Returns null if not all instances are available yet. */ @Nullable - private static Map resolveConfiguredTargetDependencies( - Environment env, - TargetAndConfiguration ctgValue, - Collection deps, - @Nullable NestedSetBuilder transitivePackages, - NestedSetBuilder transitiveRootCauses) - throws DependencyEvaluationException, InterruptedException { + private static Map + resolveConfiguredTargetDependencies( + Environment env, + TargetAndConfiguration ctgValue, + Collection deps, + @Nullable NestedSetBuilder transitivePackages, + NestedSetBuilder transitiveRootCauses) + throws DependencyEvaluationException, InterruptedException { boolean missedValues = env.valuesMissing(); ConfiguredValueCreationException rootError = null; DetailedExitCode detailedExitCode = null; @@ -912,22 +914,24 @@ private static Map resolveConfiguredTargetDepen Iterables.transform(deps, input -> input.getLabel().getPackageIdentifier())); Iterable depKeys = Iterables.concat( - Iterables.transform(deps, Dependency::getConfiguredTargetKey), packageKeys); + Iterables.transform(deps, dep -> dep.getConfiguredTargetKey().toKey()), packageKeys); SkyframeLookupResult depValuesOrExceptions = env.getValuesAndExceptions(depKeys); boolean depValuesMissingForDebugging = env.valuesMissing(); - Map result = Maps.newHashMapWithExpectedSize(deps.size()); + Map result = + Maps.newHashMapWithExpectedSize(deps.size()); Set aliasPackagesToFetch = new HashSet<>(); List aliasDepsToRedo = new ArrayList<>(); SkyframeLookupResult aliasPackageValues = null; Collection depsToProcess = deps; for (int i = 0; i < 2; i++) { for (Dependency dep : depsToProcess) { - SkyKey key = dep.getConfiguredTargetKey(); + ConfiguredTargetKey key = dep.getConfiguredTargetKey(); ConfiguredTargetValue depValue; try { depValue = (ConfiguredTargetValue) - depValuesOrExceptions.getOrThrow(key, ConfiguredValueCreationException.class); + depValuesOrExceptions.getOrThrow( + key.toKey(), ConfiguredValueCreationException.class); } catch (ConfiguredValueCreationException e) { transitiveRootCauses.addTransitive(e.getRootCauses()); detailedExitCode = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java index e933d5b637f9e4..e0ca8354271001 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java @@ -18,6 +18,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.analysis.PlatformConfiguration; @@ -200,13 +201,15 @@ private static ImmutableList configureRegisteredExecutionPl .build()) .collect(toImmutableList()); - SkyframeLookupResult values = env.getValuesAndExceptions(keys); + SkyframeLookupResult values = + env.getValuesAndExceptions(Lists.transform(keys, ConfiguredTargetKey::toKey)); ImmutableList.Builder validPlatformKeys = new ImmutableList.Builder<>(); boolean valuesMissing = false; for (ConfiguredTargetKey platformKey : keys) { Label platformLabel = platformKey.getLabel(); try { - SkyValue value = values.getOrThrow(platformKey, ConfiguredValueCreationException.class); + SkyValue value = + values.getOrThrow(platformKey.toKey(), ConfiguredValueCreationException.class); if (value == null) { valuesMissing = true; continue; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsCycleReporter.java index f75e6a06613a3f..c6af24c194206a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsCycleReporter.java @@ -74,22 +74,22 @@ public boolean maybeReportCycle( return false; } - Function printer = + Function printer = input -> { - if (input.argument() instanceof ConfiguredTargetKey) { - Label label = ((ConfiguredTargetKey) input.argument()).getLabel(); + if (input instanceof ConfiguredTargetKey) { + Label label = ((ConfiguredTargetKey) input).getLabel(); return label.toString(); } - if (input.argument() instanceof RegisteredToolchainsValue.Key) { + if (input instanceof RegisteredToolchainsValue.Key) { return "RegisteredToolchains"; } - if (input.argument() instanceof SingleToolchainResolutionKey) { + if (input instanceof SingleToolchainResolutionKey) { Label toolchainType = - ((SingleToolchainResolutionKey) input.argument()).toolchainType().toolchainType(); + ((SingleToolchainResolutionKey) input).toolchainType().toolchainType(); return String.format("toolchain type %s", toolchainType); } - if (input.argument() instanceof ToolchainContextKey) { - ToolchainContextKey toolchainContextKey = (ToolchainContextKey) input.argument(); + if (input instanceof ToolchainContextKey) { + ToolchainContextKey toolchainContextKey = (ToolchainContextKey) input; String toolchainTypes = toolchainContextKey.toolchainTypes().stream() .map(ToolchainTypeRequirement::toolchainType) @@ -98,7 +98,7 @@ public boolean maybeReportCycle( return String.format("toolchain types %s", toolchainTypes); } - throw new UnsupportedOperationException(); + throw new UnsupportedOperationException(input.toString()); }; StringBuilder cycleMessage = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java index c0c5187efd3c82..81130b2ce9de32 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java @@ -18,6 +18,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.analysis.PlatformConfiguration; @@ -179,22 +180,22 @@ private static ImmutableList getBzlmodToolchains( private static ImmutableList configureRegisteredToolchains( Environment env, BuildConfigurationValue configuration, List

This is only used in skymeld mode AND when we don't keep the incremental state. */ - public ImmutableMap getBatchedActionLookupValuesForConflictChecking() { + public ImmutableMap + getBatchedActionLookupValuesForConflictChecking() { checkState( checkNotNull(mergedSkyframeAnalysisExecutionSupplier).get() && !tracksStateForIncrementality()); synchronized (this) { - ImmutableMap result = + ImmutableMap result = ImmutableMap.copyOf(batchedActionLookupValuesForConflictChecking); batchedActionLookupValuesForConflictChecking = Maps.newConcurrentMap(); return result; @@ -3373,7 +3381,7 @@ final ActionLookupValuesTraversal collectActionLookupValuesInBuild( return alvTraversal; } - Map foundActions = + Map foundActions = new TransitiveActionLookupKeysCollector(SkyframeExecutorWrappingWalkableGraph.of(this)) .collect(Iterables.concat(topLevelCtKeys, aspectKeys)); foundActions.forEach(alvTraversal::accumulate); @@ -3386,11 +3394,11 @@ final ActionLookupValuesTraversal collectActionLookupValuesInBuild( * set of visited ALKs this build for pruning. */ private ActionLookupValuesCollectionResult collectTransitiveActionLookupValuesOfKey( - ActionLookupKey key) { + ActionLookupKeyOrProxy key) { try (SilentCloseable c = Profiler.instance().profile("SkyframeExecutor.collectTransitiveActionLookupValuesOfKey")) { if (tracksStateForIncrementality()) { - ImmutableMap foundTransitiveActionLookupEntities = + ImmutableMap foundTransitiveActionLookupEntities = incrementalTransitiveActionLookupKeysCollector.collect(key); return ActionLookupValuesCollectionResult.create( foundTransitiveActionLookupEntities.values(), @@ -3398,7 +3406,7 @@ private ActionLookupValuesCollectionResult collectTransitiveActionLookupValuesOf } // No graph edges available when there's no incrementality. We get the ALVs collected // since the last time this method was called. - ImmutableMap batchOfActionLookupValues = + ImmutableMap batchOfActionLookupValues = progressReceiver.getBatchedActionLookupValuesForConflictChecking(); return ActionLookupValuesCollectionResult.create( batchOfActionLookupValues.values(), batchOfActionLookupValues.keySet()); @@ -3768,12 +3776,12 @@ private static class TransitiveActionLookupKeysCollector { * ActionLookupKey -> SkyValue for the keys previously UNVISITED by this * TransitiveActionLookupKeysCollector instance. */ - Map collect(Iterable visitationRoots) + Map collect(Iterable visitationRoots) throws InterruptedException { try { - Map collected = Maps.newConcurrentMap(); + Map collected = Maps.newConcurrentMap(); List> futures = Lists.newArrayListWithCapacity(Iterables.size(visitationRoots)); - for (ActionLookupKey actionLookupKey : visitationRoots) { + for (ActionLookupKeyOrProxy actionLookupKey : visitationRoots) { futures.add(executorService.submit(new VisitActionLookupKey(actionLookupKey, collected))); } for (Future future : futures) { @@ -3787,7 +3795,7 @@ Map collect(Iterable visitationRoots } } - boolean seen(ActionLookupKey key, Map collected) { + boolean seen(ActionLookupKeyOrProxy key, Map collected) { return collected.containsKey(key); } @@ -3798,10 +3806,11 @@ void shutdown() throws InterruptedException { } protected final class VisitActionLookupKey extends RecursiveAction { - private final ActionLookupKey key; - private final Map collected; + private final ActionLookupKeyOrProxy key; + private final Map collected; - private VisitActionLookupKey(ActionLookupKey key, Map collected) { + private VisitActionLookupKey( + ActionLookupKeyOrProxy key, Map collected) { this.key = key; this.collected = collected; } @@ -3810,7 +3819,7 @@ private VisitActionLookupKey(ActionLookupKey key, Map public void compute() { SkyValue value; try { - value = walkableGraph.getValue(key); + value = walkableGraph.getValue(key.toKey()); } catch (InterruptedException e) { return; } @@ -3820,7 +3829,7 @@ public void compute() { collected.put(key, value); Iterable directDeps; try { - directDeps = walkableGraph.getDirectDeps(key); + directDeps = walkableGraph.getDirectDeps(key.toKey()); } catch (InterruptedException e) { return; } @@ -3848,16 +3857,17 @@ public void compute() { */ private static final class IncrementalTransitiveActionLookupKeysCollector extends TransitiveActionLookupKeysCollector { - private final Set globalVisitedSet; + private final Set globalVisitedSet; private IncrementalTransitiveActionLookupKeysCollector( - WalkableGraph walkableGraph, Set globalVisitedSet) { + WalkableGraph walkableGraph, Set globalVisitedSet) { super(walkableGraph); this.globalVisitedSet = globalVisitedSet; } @Override - Map collect(Iterable visitationRoots) { + Map collect( + Iterable visitationRoots) { throw new UnsupportedOperationException( "IncrementalTransitiveActionLookupKeysCollector expects only a single visitation root."); } @@ -3869,8 +3879,8 @@ Map collect(Iterable visitationRoots *

IMPORTANT: due to pruning, the set of returned ActionLookupKey is a SUBSET of the set of * elements in the transitive closure of the visitationRoot. */ - ImmutableMap collect(ActionLookupKey visitationRoot) { - Map collected = Maps.newConcurrentMap(); + ImmutableMap collect(ActionLookupKeyOrProxy visitationRoot) { + Map collected = Maps.newConcurrentMap(); if (seen(visitationRoot, collected)) { return ImmutableMap.copyOf(collected); } @@ -3879,7 +3889,7 @@ ImmutableMap collect(ActionLookupKey visitationRoot) } @Override - boolean seen(ActionLookupKey key, Map collected) { + boolean seen(ActionLookupKeyOrProxy key, Map collected) { return globalVisitedSet.contains(key) || collected.containsKey(key); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCycleReporter.java index 8f67f412d73d05..69fdac108f7ac7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCycleReporter.java @@ -64,13 +64,13 @@ protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) { } @Override - public String prettyPrint(SkyKey key) { + public String prettyPrint(Object key) { if (key instanceof ConfiguredTargetKey) { - return ((ConfiguredTargetKey) key.argument()).prettyPrint(); + return ((ConfiguredTargetKey) key).prettyPrint(); } else if (key instanceof AspectKey) { - return ((AspectKey) key.argument()).prettyPrint(); + return ((AspectKey) key).prettyPrint(); } else { - return getLabel(key).toString(); + return getLabel((SkyKey) key).toString(); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java index 8b482faf3f803a..bcd29b5d273c0d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java @@ -43,7 +43,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept return null; } - ConfiguredTargetValue ctValue = (ConfiguredTargetValue) env.getValue(ctKey); + ConfiguredTargetValue ctValue = (ConfiguredTargetValue) env.getValue(ctKey.toKey()); if (ctValue == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java index 18bfc476c8095a..c9f584606758c7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java @@ -14,8 +14,8 @@ package com.google.devtools.build.lib.skyframe; - import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; @@ -37,7 +37,9 @@ public class ToolchainTypeLookupUtil { public static ImmutableMap resolveToolchainTypes( Environment env, Iterable toolchainTypeKeys) throws InterruptedException, InvalidToolchainTypeException { - SkyframeLookupResult values = env.getValuesAndExceptions(toolchainTypeKeys); + SkyframeLookupResult values = + env.getValuesAndExceptions( + Iterables.transform(toolchainTypeKeys, ConfiguredTargetKey::toKey)); boolean valuesMissing = env.valuesMissing(); Map results = valuesMissing ? null : new HashMap<>(); for (ConfiguredTargetKey key : toolchainTypeKeys) { @@ -63,7 +65,7 @@ private static ToolchainTypeInfo findToolchainTypeInfo( ConfiguredTargetValue ctv = (ConfiguredTargetValue) values.getOrThrow( - key, + key.toKey(), ConfiguredValueCreationException.class, NoSuchThingException.class, ActionConflictException.class); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java index 087a2bba25e9c5..d29a51c154a2a0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupConflictFindingFunction.java @@ -18,6 +18,7 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.analysis.ConfiguredObjectValue; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; @@ -64,14 +65,14 @@ public String extractTag(SkyKey skyKey) { } static Iterable keys( - Iterable keys, TopLevelArtifactContext topLevelArtifactContext) { + Iterable keys, TopLevelArtifactContext topLevelArtifactContext) { return Iterables.transform(keys, k -> Key.create(k, topLevelArtifactContext)); } @AutoValue abstract static class Key implements TopLevelActionLookupKeyWrapper { static Key create( - ActionLookupKey actionLookupKey, TopLevelArtifactContext topLevelArtifactContext) { + ActionLookupKeyOrProxy actionLookupKey, TopLevelArtifactContext topLevelArtifactContext) { return new AutoValue_TopLevelActionLookupConflictFindingFunction_Key( actionLookupKey, topLevelArtifactContext); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupKeyWrapper.java b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupKeyWrapper.java index 36fdee2c31fc66..d211c553c5bba0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupKeyWrapper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelActionLookupKeyWrapper.java @@ -14,13 +14,13 @@ package com.google.devtools.build.lib.skyframe; -import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.skyframe.SkyKey; /** The common interface for keys that wraps a top level action lookup keys. */ public interface TopLevelActionLookupKeyWrapper extends SkyKey { - ActionLookupKey actionLookupKey(); + ActionLookupKeyOrProxy actionLookupKey(); TopLevelArtifactContext topLevelArtifactContext(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java index ceadba42632436..653cc485351394 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java @@ -54,7 +54,7 @@ public Artifact getVolatileArtifact() { } /** {@link com.google.devtools.build.skyframe.SkyKey} for {@link WorkspaceStatusValue}. */ - public static final class BuildInfoKey extends ActionLookupKey { + public static final class BuildInfoKey implements ActionLookupKey { private BuildInfoKey() {} @Override diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java b/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java index 68b2e9f670bd6e..c0ffa286400abb 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java @@ -24,7 +24,7 @@ * An {@link ActionLookupKey} with a non-hermetic {@link SkyFunctionName} so that its value can be * directly injected during tests. */ -public final class InjectedActionLookupKey extends ActionLookupKey { +public final class InjectedActionLookupKey implements ActionLookupKey { public static final SkyFunctionName INJECTED_ACTION_LOOKUP = SkyFunctionName.createNonHermetic("INJECTED_ACTION_LOOKUP"); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java index 948606007b7855..9bbb4bf801eac4 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java @@ -155,7 +155,7 @@ private void checkConflictError() { assertThat(Iterables.getOnlyElement(eventCollector).getKind()).isEqualTo(EventKind.ERROR); } - private static final class SimpleActionLookupKey extends ActionLookupKey { + private static final class SimpleActionLookupKey implements ActionLookupKey { private final String name; SimpleActionLookupKey(String name) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java index 277a8ff0109bf4..50f998edc03c7d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java @@ -637,7 +637,8 @@ protected Artifact getBinArtifact(String packageRelativePath, ConfiguredTarget o ConfiguredTargetKey.builder() .setLabel(label) .setConfigurationKey(owner.getConfigurationKey()) - .build(); + .build() + .toKey(); ActionLookupValue actionLookupValue; try { actionLookupValue = 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 b3ec3f79d293a8..cf1cf2070e4694 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 @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; import com.google.devtools.build.lib.actions.Artifact; @@ -212,7 +213,7 @@ public ImmutableList getBuildInfo( } @Override - public ActionLookupKey getOwner() { + public ActionLookupKeyOrProxy getOwner() { return original.getOwner(); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index 36371d895ebde6..ea1c2e7b8dc997 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java @@ -311,7 +311,7 @@ public Collection getDirectPrerequisitesForTesting( // Load the keys of the dependencies of the target, based on data currently in skyframe. Iterable directPrerequisites = - walkableGraph.getDirectDeps(ConfiguredTargetKey.fromConfiguredTarget(ct)); + walkableGraph.getDirectDeps(ConfiguredTargetKey.fromConfiguredTarget(ct).toKey()); // Turn the keys back into ConfiguredTarget instances, possibly merging in aspects that were // propagated from the original target. @@ -334,7 +334,7 @@ public Collection getDirectPrerequisitesForTesting( private static ConfiguredTargetAndData getConfiguredTarget( WalkableGraph graph, ConfiguredTargetKey key) { try { - ConfiguredTargetValue value = (ConfiguredTargetValue) graph.getValue(key); + ConfiguredTargetValue value = (ConfiguredTargetValue) graph.getValue(key.toKey()); if (value != null) { ConfiguredTarget ct = value.getConfiguredTarget(); BuildConfigurationValue config = null; diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index f02ce8bec74596..641743f17046ad 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; @@ -1445,7 +1446,8 @@ protected final Artifact.DerivedArtifact getDerivedArtifact( if ((owner instanceof ActionLookupKey)) { SkyValue skyValue; try { - skyValue = skyframeExecutor.getEvaluator().getExistingValue((SkyKey) owner); + skyValue = + skyframeExecutor.getEvaluator().getExistingValue(((ActionLookupKey) owner).toKey()); } catch (InterruptedException e) { throw new IllegalStateException(e); } @@ -1472,7 +1474,7 @@ protected final Artifact.DerivedArtifact getDerivedArtifact( * "foo.o". */ protected final Artifact getTreeArtifact(String packageRelativePath, ConfiguredTarget owner) { - ActionLookupKey actionLookupKey = ConfiguredTargetKey.fromConfiguredTarget(owner); + ActionLookupKeyOrProxy actionLookupKey = ConfiguredTargetKey.fromConfiguredTarget(owner); return getDerivedArtifact( owner.getLabel().getPackageFragment().getRelative(packageRelativePath), getConfiguration(owner).getBinDirectory(RepositoryName.MAIN), diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java index 3b31261d0aefc4..0ce69bfe370904 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java @@ -217,7 +217,8 @@ public void testActionGraph() throws Exception { ConfiguredTargetKey.builder() .setLabel(pkg.getLabel()) .setConfiguration(getConfiguration(pkg)) - .build()) + .build() + .toKey()) .getValue(); ImmutableList linkstampCompileActions = configuredTargetValue.getActions().stream() diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryAnalysisTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryAnalysisTest.java index da538e90b9f1ac..7e4c1a1927d6fd 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryAnalysisTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryAnalysisTest.java @@ -70,7 +70,7 @@ public void libraryToLinkStaysInSyncWithConfiguredTarget() throws Exception { (ActionLookupValue) skyframeExecutor .getEvaluator() - .getExistingValue(generatingActionKey.getActionLookupKey()); + .getExistingValue(generatingActionKey.getActionLookupKey().toKey()); Action generatingAction = actionLookupValue.getAction(generatingActionKey.getActionIndex()); assertWithMessage(context).that(generatingAction).isInstanceOf(CppLinkAction.class); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java index b6b2e01efc59f8..c1de1a58bf83d9 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java @@ -15,6 +15,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -42,6 +43,7 @@ import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; @@ -62,6 +64,12 @@ public final class ActionExecutionValueTransformSharedTreeArtifactsTest { private final Scratch scratch = new Scratch(); private ArtifactRoot derivedRoot; + @BeforeClass + public static void initMocks() { + when(KEY_1.toKey()).thenReturn(KEY_1); + when(KEY_2.toKey()).thenReturn(KEY_2); + } + @Before public void createDerivedRoot() throws IOException { derivedRoot = diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java index a619ddd37ddcfb..912530fccc717d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java @@ -460,7 +460,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept Map treeArtifactData = new HashMap<>(); ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); ActionLookupValue actionLookupValue = - (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey()); + (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey().toKey()); Action action = actionLookupValue.getAction(actionLookupData.getActionIndex()); Artifact output = Iterables.getOnlyElement(action.getOutputs()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BuildDriverFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BuildDriverFunctionTest.java index a60538bfb784b7..d823f5fbe46c4a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BuildDriverFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BuildDriverFunctionTest.java @@ -15,13 +15,14 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy; import com.google.devtools.build.lib.actions.MutableActionGraph; import com.google.devtools.build.lib.skyframe.BuildDriverFunction.ActionLookupValuesCollectionResult; import com.google.devtools.build.lib.skyframe.BuildDriverFunction.TransitiveActionLookupValuesHelper; -import com.google.devtools.build.skyframe.SkyKey; import java.util.HashSet; import java.util.Set; import org.junit.Test; @@ -34,23 +35,24 @@ public class BuildDriverFunctionTest { @Test public void checkActionConflicts_noConflict_conflictFreeKeysRegistered() throws Exception { - Set globalSet = new HashSet<>(); + Set globalSet = new HashSet<>(); IncrementalArtifactConflictFinder dummyConflictFinder = IncrementalArtifactConflictFinder.createWithActionGraph(mock(MutableActionGraph.class)); TransitiveActionLookupValuesHelper fakeHelper = new TransitiveActionLookupValuesHelper() { @Override - public ActionLookupValuesCollectionResult collect(ActionLookupKey key) { + public ActionLookupValuesCollectionResult collect(ActionLookupKeyOrProxy key) { return ActionLookupValuesCollectionResult.create( ImmutableSet.of(), ImmutableSet.of(key)); } @Override - public void registerConflictFreeKeys(ImmutableSet keys) { + public void registerConflictFreeKeys(ImmutableSet keys) { globalSet.addAll(keys); } }; ActionLookupKey dummyKey = mock(ActionLookupKey.class); + when(dummyKey.toKey()).thenReturn(dummyKey); BuildDriverFunction function = new BuildDriverFunction( fakeHelper, @@ -58,7 +60,7 @@ public void registerConflictFreeKeys(ImmutableSet keys) { /*ruleContextConstraintSemantics=*/ null, /*extraActionFilterSupplier=*/ null); - function.checkActionConflicts(dummyKey, /*strictConflictCheck=*/ true); + var unused = function.checkActionConflicts(dummyKey, /* strictConflictCheck= */ true); assertThat(globalSet).containsExactly(dummyKey); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKeyTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKeyTest.java new file mode 100644 index 00000000000000..c44c5b4b2d3e80 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKeyTest.java @@ -0,0 +1,139 @@ +// 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.lib.skyframe; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache; +import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache; +import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(TestParameterInjector.class) +public final class ConfiguredTargetKeyTest extends BuildViewTestCase { + private static final AtomicInteger nextId = new AtomicInteger(); + + @Test + public void testDelegation( + @TestParameter boolean useNullConfig, @TestParameter boolean isToolchainKey) { + var baseKey = createKey(useNullConfig, isToolchainKey); + + assertThat(baseKey.isProxy()).isFalse(); + assertThat(baseKey.toKey()).isSameInstanceAs(baseKey); + + BuildConfigurationKey newConfigurationKey = getNewUniqueConfigurationKey(); + var delegatingKey = + ConfiguredTargetKey.builder() + .setDelegate(baseKey) + .setConfigurationKey(newConfigurationKey) + .build(); + assertThat(delegatingKey.isProxy()).isTrue(); + assertThat(delegatingKey.toKey()).isSameInstanceAs(baseKey); + assertThat(delegatingKey.getLabel()).isSameInstanceAs(baseKey.getLabel()); + assertThat(delegatingKey.getConfigurationKey()).isSameInstanceAs(newConfigurationKey); + assertThat(delegatingKey.getExecutionPlatformLabel()) + .isSameInstanceAs(baseKey.getExecutionPlatformLabel()); + + // Building a key with the same parameters as the delegating key returns the delegating key. + var similarKey = + ConfiguredTargetKey.builder() + .setLabel(delegatingKey.getLabel()) + .setConfigurationKey(delegatingKey.getConfigurationKey()) + .setExecutionPlatformLabel(delegatingKey.getExecutionPlatformLabel()) + .build(); + assertThat(similarKey).isSameInstanceAs(delegatingKey); + } + + @Test + public void existingKey_inhibitsDelegation( + @TestParameter boolean useNullConfig, @TestParameter boolean isToolchainKey) { + var baseKey = createKey(useNullConfig, isToolchainKey); + + BuildConfigurationKey newConfigurationKey = getNewUniqueConfigurationKey(); + + var existingKey = + ConfiguredTargetKey.builder() + .setLabel(baseKey.getLabel()) + .setConfigurationKey(newConfigurationKey) + .setExecutionPlatformLabel(baseKey.getExecutionPlatformLabel()) + .build(); + + var delegatingKey = + ConfiguredTargetKey.builder() + .setDelegate(baseKey) + .setConfigurationKey(newConfigurationKey) + .build(); + + assertThat(delegatingKey).isSameInstanceAs(existingKey); + } + + @Test + public void testCodec() throws Exception { + var nullConfigKey = createKey(/* useNullConfig= */ true, /* isToolchainKey= */ false); + var keyWithConfig = createKey(/* useNullConfig= */ false, /* isToolchainKey= */ false); + var toolchainKey = createKey(/* useNullConfig= */ false, /* isToolchainKey= */ true); + + var delegatingToNullConfig = + ConfiguredTargetKey.builder() + .setDelegate(nullConfigKey) + .setConfigurationKey(targetConfigKey) + .build(); + var delegatingToKeyWithConfig = + ConfiguredTargetKey.builder().setDelegate(keyWithConfig).build(); + var delegatingToToolchainKey = + ConfiguredTargetKey.builder() + .setDelegate(toolchainKey) + .setConfigurationKey(getNewUniqueConfigurationKey()) + .build(); + + new SerializationTester( + nullConfigKey, + keyWithConfig, + toolchainKey, + delegatingToNullConfig, + delegatingToKeyWithConfig, + delegatingToToolchainKey) + .addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache()) + .runTests(); + } + + private ConfiguredTargetKey createKey(boolean useNullConfig, boolean isToolchainKey) { + var key = ConfiguredTargetKey.builder().setLabel(Label.parseCanonicalUnchecked("//p:key")); + if (!useNullConfig) { + key.setConfigurationKey(targetConfigKey); + } + if (isToolchainKey) { + key.setExecutionPlatformLabel(Label.parseCanonicalUnchecked("//platforms:b")); + } + return key.build(); + } + + private BuildConfigurationKey getNewUniqueConfigurationKey() { + BuildOptions newOptions = targetConfigKey.getOptions().clone(); + var coreOptions = newOptions.get(CoreOptions.class); + coreOptions.affectedByStarlarkTransition = + ImmutableList.of("//fake:id" + nextId.getAndIncrement()); + assertThat(newOptions.checksum()).isNotEqualTo(targetConfigKey.getOptions().checksum()); + return BuildConfigurationKey.withoutPlatformMapping(newOptions); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessorTest.java index f010ad284cda8d..97e3b8f7525bbd 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessorTest.java @@ -61,7 +61,7 @@ public void testProcessErrors_analysisErrorNoKeepGoing_throwsException( /*isTransitivelyTransient=*/ false); EvaluationResult result = - EvaluationResult.builder().addError(analysisErrorKey, analysisErrorInfo).build(); + EvaluationResult.builder().addError(analysisErrorKey.toKey(), analysisErrorInfo).build(); ViewCreationFailedException thrown = assertThrows( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java index 1e3721dc5d63c7..5ea40e8da94123 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java @@ -58,7 +58,7 @@ public void loadingPhaseCycleWithDifferentTopLevelKeyTypes() throws Exception { .setLabel(Label.parseCanonicalUnchecked("//foo:a")) .setConfiguration(targetConfig) .build(); - assertThat(cycleReporter.getAdditionalMessageAboutCycle(reporter, ctKey, cycle)) + assertThat(cycleReporter.getAdditionalMessageAboutCycle(reporter, ctKey.toKey(), cycle)) .contains( "The cycle is caused by a visibility edge from //foo:b to the non-package_group " + "target //foo:c"); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index 2fa2e58e9fb4a0..678e141621f7d2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -265,7 +265,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); ActionLookupValue actionLookupValue = - (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey()); + (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey().toKey()); Action action = actionLookupValue.getAction(actionLookupData.getActionIndex()); SpecialArtifact output = (SpecialArtifact) Iterables.getOnlyElement(action.getOutputs()); TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(output); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java index 7fbbdc6951bafd..260a6b64beb56f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java @@ -88,7 +88,8 @@ public static EvaluationResult evaluate( public static ConfiguredTargetValue getExistingConfiguredTargetValue( SkyframeExecutor skyframeExecutor, Label label, BuildConfigurationValue config) throws InterruptedException { - SkyKey key = ConfiguredTargetKey.builder().setLabel(label).setConfiguration(config).build(); + SkyKey key = + ConfiguredTargetKey.builder().setLabel(label).setConfiguration(config).build().toKey(); return (ConfiguredTargetValue) getExistingValue(skyframeExecutor, key); } From d81ef2dfc5fefb5ef665ca134c7f1960eafb6ac0 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 10:37:31 -0700 Subject: [PATCH 012/710] Make the name of the remote strategy overrideable, for internal reasons. Necessary for flipping `--internal_spawn_scheduler` to true. PiperOrigin-RevId: 529131895 Change-Id: I081414c9507981d5f66cae9c82865b8ee14a7ef8 --- .../build/lib/dynamic/DynamicExecutionModule.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java index de8ad2784d1320..1715d15b4d5d96 100644 --- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java +++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.common.options.OptionsBase; +import com.google.errorprone.annotations.ForOverride; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -120,10 +121,15 @@ private ImmutableMap> getRemoteStrategies(DynamicExecutionO strategies.put(e.getKey(), e.getValue()); } return options.dynamicRemoteStrategy.isEmpty() - ? ImmutableMap.of("", ImmutableList.of("remote")) + ? ImmutableMap.of("", ImmutableList.of(remoteStrategyName())) : ImmutableMap.copyOf(strategies); } + @ForOverride + protected String remoteStrategyName() { + return "remote"; + } + @Override public void registerSpawnStrategies( SpawnStrategyRegistry.Builder registryBuilder, CommandEnvironment env) From 261cf200b8ff78550cfae155a471a08f975a6795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20K=C3=B6ppe?= Date: Wed, 3 May 2023 10:49:52 -0700 Subject: [PATCH 013/710] A new documentation page about iterative build performance. This document can be linked from the CLI warning when the analysis cache is discarded. PiperOrigin-RevId: 529135476 Change-Id: I8b389e883c468e0d22b41a2652f6190480515bd7 --- site/en/_book.yaml | 2 + .../advanced/performance/iteration-speed.md | 85 +++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 site/en/advanced/performance/iteration-speed.md diff --git a/site/en/_book.yaml b/site/en/_book.yaml index 3fa7c88d5d341b..3ad3764eb242c9 100644 --- a/site/en/_book.yaml +++ b/site/en/_book.yaml @@ -168,6 +168,8 @@ upper_tabs: path: /advanced/performance/json-trace-profile - title: Optimize memory path: /advanced/performance/memory + - title: Optimize build iteration speed + path: /advanced/performance/iteration-speed - name: Remote execution contents: - heading: Remote build execution (RBE) diff --git a/site/en/advanced/performance/iteration-speed.md b/site/en/advanced/performance/iteration-speed.md new file mode 100644 index 00000000000000..7a2a1fa626496c --- /dev/null +++ b/site/en/advanced/performance/iteration-speed.md @@ -0,0 +1,85 @@ +Project: /_project.yaml +Book: /_book.yaml + + +# Optimize Iteration Speed + +{% include "_buttons.html" %} + +This page describes how to optimize Bazel's build performance when running Bazel +repeatedly. + +## Bazel's Runtime State + +A Bazel invocation involves several interacting parts. + +* The `bazel` command line interface (CLI) is the user-facing front-end tool + and receives commands from the user. + +* The CLI tool starts a [*Bazel server*](https://bazel.build/run/client-server) + for each distinct [output base](https://bazel.build/remote/output-directories). + The Bazel server is generally persistent, but will shut down after some idle + time so as to not waste resources. + +* The Bazel server performs the loading and analysis steps for a given command + (`build`, `run`, `cquery`, etc.), in which it constructs the necessary parts + of the build graph in memory. The resulting data structures are retained in + the Bazel server as part of the *analysis cache*. + +* The Bazel server can also perform the action execution, or it can send + actions off for remote execution if it is set up to do so. The results of + action executions are also cached, namely in the *action cache* (or + *execution cache*, which may be either local or remote, and it may be shared + among Bazel servers). + +* The result of the Bazel invocation is made available in the output tree. + +## Running Bazel Iteratively + +In a typical developer workflow, it is common to build (or run) a piece of code +repeatedly, often at a very high frequency (e.g. to resolve some compilation +error or investigate a failing test). In this situation, it is important that +repeated invocations of `bazel` have as little overhead as possible relative to +the underlying, repeated action (e.g. invoking a compiler, or executing a test). + +With this in mind, we take another look at Bazel's runtime state: + +The analysis cache is a critical piece of data. A significant amount of time can +be spent just on the loading and analysis phases of a cold run (i.e. a run just +after the Bazel server was started or when the analysis cache was discarded). +For a single, successful cold build (e.g. for a production release) this cost is +bearable, but for repeatedly building the same target it is important that this +cost be amortized and not repeated on each invocation. + +The analysis cache is rather volatile. First off, it is part of the in-process +state of the Bazel server, so losing the server loses the cache. But the cache +is also *invalidated* very easily: for example, many `bazel` command line flags +cause the cache to be discarded. This is because many flags affect the build +graph (e.g. because of +[configurable attributes](https://bazel.build/configure/attributes)). Some flag +changes can also cause the Bazel server to be restarted (e.g. changing +[startup options](https://bazel.build/docs/user-manual#startup-options)). + +Bazel will print a warning if either the analysis cache was discarded or the +server was restarted. Either of these should be avoided during iterative use: + +* Be mindful of changing `bazel` flags in the middle of an iterative + workflow. For example, mixing a `bazel build -c opt` with a `bazel cquery` + causes each command to discard the analysis cache of the other. In general, + try to use a fixed set of flags for the duration of a particular workflow. + +* Losing the Bazel server loses the analysis cache. The Bazel server has a + [configurable](https://bazel.build/docs/user-manual#max-idle-secs) idle + time, after which it shuts down. You can configure this time via your + bazelrc file to suit your needs. The server also restarted when startup + flags change, so, again, avoid changing those flags if possible. + +* If you want to use multiple sets of flags from the same workspace, you can + use multiple, distinct output bases, switched with the `--output_base` + flag. Each output base gets its own Bazel server. + +A good execution cache is also valuable for build performance. An execution +cache can be kept locally +[on disk](https://bazel.build/remote/caching#disk-cache), or +[remotely](https://bazel.build/remote/caching). The cache can be shared among +Bazel servers, and indeed among developers. From d5a963e28d8ee5dff526f296f6bcbb26ab740090 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 10:50:30 -0700 Subject: [PATCH 014/710] Remove unused methods from `SpawnAction.Builder`. Some are completely unused and others are tested but unused in practice. Cleaning up to make it easier to uncover optimizations. PiperOrigin-RevId: 529135685 Change-Id: Iae01fba0519a1c5cd74ecbf75f8911885cc88df2 --- .../lib/analysis/actions/SpawnAction.java | 138 ++++-------------- .../lib/analysis/actions/SpawnActionTest.java | 92 ++++-------- .../lib/rules/cpp/LtoBackendActionTest.java | 7 +- 3 files changed, 54 insertions(+), 183 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 17eff32a231340..bd1bc680cbd4ef 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -83,7 +83,6 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.CompileTimeConstant; -import com.google.errorprone.annotations.DoNotCall; import com.google.errorprone.annotations.ForOverride; import com.google.errorprone.annotations.FormatMethod; import com.google.errorprone.annotations.FormatString; @@ -606,9 +605,7 @@ public static class Builder { private final List outputs = new ArrayList<>(); private final List inputRunfilesSuppliers = new ArrayList<>(); private ResourceSetOrBuilder resourceSetOrBuilder = AbstractAction.DEFAULT_RESOURCE_SET; - private ActionEnvironment actionEnvironment = null; private ImmutableMap environment = ImmutableMap.of(); - private ImmutableSet inheritedEnvironment = ImmutableSet.of(); private ImmutableMap executionInfo = ImmutableMap.of(); private boolean useDefaultShellEnvironment = false; protected boolean executeUnconditionally; @@ -631,7 +628,6 @@ public Builder() {} this.outputs.addAll(other.outputs); this.inputRunfilesSuppliers.addAll(other.inputRunfilesSuppliers); this.resourceSetOrBuilder = other.resourceSetOrBuilder; - this.actionEnvironment = other.actionEnvironment; this.environment = other.environment; this.executionInfo = other.executionInfo; this.useDefaultShellEnvironment = other.useDefaultShellEnvironment; @@ -676,11 +672,9 @@ public SpawnAction build(ActionOwner owner, BuildConfigurationValue configuratio } CommandLines commandLines = result.build(); ActionEnvironment env = - actionEnvironment != null - ? actionEnvironment - : useDefaultShellEnvironment - ? configuration.getActionEnvironment() - : ActionEnvironment.create(environment, inheritedEnvironment); + useDefaultShellEnvironment + ? configuration.getActionEnvironment() + : ActionEnvironment.create(environment); return buildSpawnAction(owner, commandLines, configuration, env); } @@ -695,11 +689,7 @@ SpawnAction buildForActionTemplate(ActionOwner owner) { for (CommandLineAndParamFileInfo pair : commandLines) { result.addCommandLine(pair.commandLine); } - return buildSpawnAction( - owner, - result.build(), - null, - ActionEnvironment.create(environment, inheritedEnvironment)); + return buildSpawnAction(owner, result.build(), null, ActionEnvironment.create(environment)); } /** @@ -820,19 +810,6 @@ public Builder addInputs(Iterable artifacts) { return this; } - /** - * @deprecated Use {@link #addTransitiveInputs} to avoid excessive memory use. - */ - @CanIgnoreReturnValue - @Deprecated - @DoNotCall - public final Builder addInputs(NestedSet artifacts) { - // Do not delete this method, or else addInputs(Iterable) calls with a NestedSet argument - // will not be flagged. - inputsBuilder.addAll(artifacts.toList()); - return this; - } - /** Adds transitive inputs to this action. */ @CanIgnoreReturnValue public Builder addTransitiveInputs(NestedSet artifacts) { @@ -868,13 +845,6 @@ public Builder setResources(ResourceSetOrBuilder resourceSetOrBuilder) { return this; } - /** Sets the action environment. */ - @CanIgnoreReturnValue - public Builder setEnvironment(ActionEnvironment actionEnvironment) { - this.actionEnvironment = actionEnvironment; - return this; - } - /** * Sets the map of environment variables. Do not use! This makes the builder ignore the 'default * shell environment', which is computed from the --action_env command line option. @@ -886,17 +856,6 @@ public Builder setEnvironment(Map environment) { return this; } - /** - * Sets the set of inherited environment variables. Do not use! This makes the builder ignore - * the 'default shell environment', which is computed from the --action_env command line option. - */ - @CanIgnoreReturnValue - public Builder setInheritedEnvironment(Iterable inheritedEnvironment) { - this.inheritedEnvironment = ImmutableSet.copyOf(inheritedEnvironment); - this.useDefaultShellEnvironment = false; - return this; - } - /** Sets the map of execution info. */ @CanIgnoreReturnValue public Builder setExecutionInfo(Map info) { @@ -938,7 +897,6 @@ public Builder setExecutionInfo(Map info) { @CanIgnoreReturnValue public Builder useDefaultShellEnvironment() { this.environment = null; - this.inheritedEnvironment = null; this.useDefaultShellEnvironment = true; return this; } @@ -951,8 +909,8 @@ public Builder useDefaultShellEnvironment() { * relative to PATH. See https://github.com/bazelbuild/bazel/issues/13189 for details. To avoid * that, use {@link #setExecutable(Artifact)} instead. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setExecutable(PathFragment executable) { @@ -964,8 +922,8 @@ public Builder setExecutable(PathFragment executable) { /** * Sets the executable as an artifact. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setExecutable(Artifact executable) { @@ -979,8 +937,8 @@ public Builder setExecutable(Artifact executable) { * Sets the executable as a configured target. Automatically adds the files to run to the tools * and inputs and uses the executable of the target as the executable. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setExecutable(TransitiveInfoCollection executable) { @@ -992,8 +950,8 @@ public Builder setExecutable(TransitiveInfoCollection executable) { * Sets the executable as a configured target. Automatically adds the files to run to the tools * and inputs and uses the executable of the target as the executable. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setExecutable(FilesToRunProvider executableProvider) { @@ -1013,8 +971,8 @@ public Builder setExecutable(FilesToRunProvider executableProvider) { * duplication when passing {@link PathFragment} to Starlark as a String and then executing with * it. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setExecutableAsString(String executable) { @@ -1023,54 +981,27 @@ public Builder setExecutableAsString(String executable) { return this; } - @CanIgnoreReturnValue - private Builder setJavaExecutable( - PathFragment javaExecutable, - Artifact deployJar, - NestedSet jvmArgs, - String... launchArgs) { - this.executableArgs = - CustomCommandLine.builder() - .addPath(javaExecutable) - .addAll(jvmArgs) - .addAll(ImmutableList.copyOf(launchArgs)); - this.executableArg = null; - toolsBuilder.add(deployJar); - return this; - } - - /** - * Sets the executable to be a java class executed from the given deploy jar. The deploy jar is - * automatically added to the action inputs. - * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. - */ - @CanIgnoreReturnValue - Builder setJavaExecutable( - PathFragment javaExecutable, - Artifact deployJar, - String javaMainClass, - NestedSet jvmArgs) { - return setJavaExecutable( - javaExecutable, deployJar, jvmArgs, "-cp", deployJar.getExecPathString(), javaMainClass); - } - /** * Sets the executable to be a jar executed from the given deploy jar. The deploy jar is * automatically added to the action inputs. * - *

This method is similar to {@link #setJavaExecutable} but it assumes that the Jar artifact - * declares a main class. + *

Assumes that the Jar artifact declares a main class. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setJarExecutable( PathFragment javaExecutable, Artifact deployJar, NestedSet jvmArgs) { - return setJavaExecutable(javaExecutable, deployJar, jvmArgs, "-jar", - deployJar.getExecPathString()); + this.executableArgs = + CustomCommandLine.builder() + .addPath(javaExecutable) + .addAll(jvmArgs) + .add("-jar") + .addExecPath(deployJar); + this.executableArg = null; + toolsBuilder.add(deployJar); + return this; } /** @@ -1079,8 +1010,8 @@ public Builder setJarExecutable( *

Note that this will not clear the arguments, so any arguments will be passed in addition * to the command given here. * - *

Calling this method overrides any previous values set via calls to {@link #setExecutable}, - * {@link #setJavaExecutable}, or {@link #setShellCommand}. + *

Calling this method overrides any previous values set via calls to {@link #setExecutable} + * or {@link #setShellCommand}. */ @CanIgnoreReturnValue public Builder setShellCommand(PathFragment shExecutable, String command) { @@ -1102,19 +1033,6 @@ public Builder setShellCommand(Iterable command) { return this; } - /** Returns a {@link CustomCommandLine.Builder} for executable arguments. */ - CustomCommandLine.Builder executableArguments() { - if (executableArgs == null) { - if (executableArg != null) { - executableArgs = CustomCommandLine.builder().addObject(executableArg); - executableArg = null; - } else { - executableArgs = CustomCommandLine.builder(); - } - } - return this.executableArgs; - } - /** Appends the arguments to the list of executable arguments. */ @CanIgnoreReturnValue public Builder addExecutableArguments(String... arguments) { 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 ae3d8fdb65c5b0..24da5e03ceedf9 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 @@ -71,7 +71,7 @@ public final class SpawnActionTest extends BuildViewTestCase { private Artifact jarArtifact; private AnalysisTestUtil.CollectingAnalysisEnvironment collectingAnalysisEnvironment; - private SpawnAction.Builder builder() { + private static SpawnAction.Builder builder() { return new SpawnAction.Builder(); } @@ -107,7 +107,7 @@ private SpawnAction createCopyFromWelcomeToDestination(Map envir @Test public void testWelcomeArtifactIsInput() { SpawnAction copyFromWelcomeToDestination = - createCopyFromWelcomeToDestination(ImmutableMap.of()); + createCopyFromWelcomeToDestination(ImmutableMap.of()); ImmutableList inputs = copyFromWelcomeToDestination.getInputs().toList(); assertThat(inputs).containsExactly(welcomeArtifact); } @@ -115,7 +115,7 @@ public void testWelcomeArtifactIsInput() { @Test public void testDestinationArtifactIsOutput() { SpawnAction copyFromWelcomeToDestination = - createCopyFromWelcomeToDestination(ImmutableMap.of()); + createCopyFromWelcomeToDestination(ImmutableMap.of()); Collection outputs = copyFromWelcomeToDestination.getOutputs(); assertThat(outputs).containsExactly(destinationArtifact); } @@ -214,24 +214,23 @@ public void testBuilderWithExecutableInRootPackage() throws Exception { } @Test - public void testBuilderWithJavaExecutable() throws Exception { + public void testBuilderWithJarExecutable() throws Exception { SpawnAction action = builder() .addOutput(destinationArtifact) - .setJavaExecutable( - scratch.file("/bin/java").asFragment(), + .setJarExecutable( + PathFragment.create("/bin/java"), jarArtifact, - "MyMainClass", NestedSetBuilder.create(Order.STABLE_ORDER, "-jvmarg")) .build(nullOwnerWithTargetConfig(), targetConfig); collectingAnalysisEnvironment.registerAction(action); assertThat(action.getArguments()) - .containsExactly("/bin/java", "-jvmarg", "-cp", "pkg/exe.jar", "MyMainClass") + .containsExactly("/bin/java", "-jvmarg", "-jar", "pkg/exe.jar") .inOrder(); } @Test - public void testBuilderWithJavaExecutableAndParameterFile2() throws Exception { + public void testBuilderWithJarExecutableAndParameterFile2() throws Exception { useConfiguration("--min_param_file_size=0", "--defer_param_files"); collectingAnalysisEnvironment = new AnalysisTestUtil.CollectingAnalysisEnvironment(getTestAnalysisEnvironment()); @@ -239,10 +238,9 @@ public void testBuilderWithJavaExecutableAndParameterFile2() throws Exception { SpawnAction action = builder() .addOutput(output) - .setJavaExecutable( - scratch.file("/bin/java").asFragment(), + .setJarExecutable( + PathFragment.create("/bin/java"), jarArtifact, - "MyMainClass", NestedSetBuilder.create(Order.STABLE_ORDER, "-jvmarg")) .addCommandLine( CustomCommandLine.builder().add("-X").build(), @@ -251,7 +249,7 @@ public void testBuilderWithJavaExecutableAndParameterFile2() throws Exception { // The action reports all arguments, including those inside the param file assertThat(action.getArguments()) - .containsExactly("/bin/java", "-jvmarg", "-cp", "pkg/exe.jar", "MyMainClass", "-X") + .containsExactly("/bin/java", "-jvmarg", "-jar", "pkg/exe.jar", "-X") .inOrder(); Spawn spawn = @@ -264,14 +262,7 @@ public void testBuilderWithJavaExecutableAndParameterFile2() throws Exception { String paramFileName = output.getExecPathString() + "-0.params"; // The spawn's primary arguments should reference the param file assertThat(spawn.getArguments()) - .containsExactly( - "/bin/java", - - "-jvmarg", - "-cp", - "pkg/exe.jar", - "MyMainClass", - "@" + paramFileName) + .containsExactly("/bin/java", "-jvmarg", "-jar", "pkg/exe.jar", "@" + paramFileName) .inOrder(); // Asserts that the inputs contain the param file virtual input @@ -289,10 +280,9 @@ public void testBuilderWithExtraExecutableArguments() throws Exception { SpawnAction action = builder() .addOutput(destinationArtifact) - .setJavaExecutable( - scratch.file("/bin/java").asFragment(), + .setJarExecutable( + PathFragment.create("/bin/java"), jarArtifact, - "MyMainClass", NestedSetBuilder.create(Order.STABLE_ORDER, "-jvmarg")) .addExecutableArguments("execArg1", "execArg2") .addCommandLine(CustomCommandLine.builder().add("arg1").build()) @@ -300,15 +290,7 @@ public void testBuilderWithExtraExecutableArguments() throws Exception { collectingAnalysisEnvironment.registerAction(action); assertThat(action.getArguments()) .containsExactly( - "/bin/java", - - "-jvmarg", - "-cp", - "pkg/exe.jar", - "MyMainClass", - "execArg1", - "execArg2", - "arg1"); + "/bin/java", "-jvmarg", "-jar", "pkg/exe.jar", "execArg1", "execArg2", "arg1"); } @Test @@ -361,7 +343,7 @@ public void testGetArgumentsWithParameterFiles() throws Exception { @Test public void testExtraActionInfo() throws Exception { - SpawnAction action = createCopyFromWelcomeToDestination(ImmutableMap.of()); + SpawnAction action = createCopyFromWelcomeToDestination(ImmutableMap.of()); ExtraActionInfo info = action.getExtraActionInfo(actionKeyContext).build(); assertThat(info.getMnemonic()).isEqualTo("Dummy"); @@ -457,8 +439,8 @@ public Action generate(ImmutableSet attributesToFlip) { if (attributesToFlip.contains(KeyAttributes.EXECUTABLE)) { builder.setExecutable(executable); } else { - builder.setJavaExecutable( - executable, jarArtifact, "Main", NestedSetBuilder.emptySet(Order.STABLE_ORDER)); + builder.setJarExecutable( + executable, jarArtifact, NestedSetBuilder.emptySet(Order.STABLE_ORDER)); } builder.setMnemonic(attributesToFlip.contains(KeyAttributes.MNEMONIC) ? "a" : "b"); @@ -572,22 +554,20 @@ private SpawnAction createWorkerSupportSpawn(Map executionInfoVa @Test public void testWorkerSupport() throws Exception { SpawnAction workerSupportSpawn = - createWorkerSupportSpawn(ImmutableMap.of("supports-workers", "1")); + createWorkerSupportSpawn(ImmutableMap.of("supports-workers", "1")); assertThat(Spawns.supportsWorkers(workerSupportSpawn.getSpawn())).isTrue(); } @Test public void testMultiplexWorkerSupport() throws Exception { SpawnAction multiplexWorkerSupportSpawn = - createWorkerSupportSpawn( - ImmutableMap.of("supports-multiplex-workers", "1")); + createWorkerSupportSpawn(ImmutableMap.of("supports-multiplex-workers", "1")); assertThat(Spawns.supportsMultiplexWorkers(multiplexWorkerSupportSpawn.getSpawn())).isTrue(); } @Test public void testWorkerProtocolFormat_defaultIsProto() throws Exception { - SpawnAction spawn = - createWorkerSupportSpawn(ImmutableMap.of("supports-workers", "1")); + SpawnAction spawn = createWorkerSupportSpawn(ImmutableMap.of("supports-workers", "1")); assertThat(Spawns.getWorkerProtocolFormat(spawn.getSpawn())) .isEqualTo(WorkerProtocolFormat.PROTO); } @@ -596,8 +576,7 @@ public void testWorkerProtocolFormat_defaultIsProto() throws Exception { public void testWorkerProtocolFormat_explicitProto() throws Exception { SpawnAction spawn = createWorkerSupportSpawn( - ImmutableMap.of( - "supports-workers", "1", "requires-worker-protocol", "proto")); + ImmutableMap.of("supports-workers", "1", "requires-worker-protocol", "proto")); assertThat(Spawns.getWorkerProtocolFormat(spawn.getSpawn())) .isEqualTo(WorkerProtocolFormat.PROTO); } @@ -606,15 +585,14 @@ public void testWorkerProtocolFormat_explicitProto() throws Exception { public void testWorkerProtocolFormat_explicitJson() throws Exception { SpawnAction spawn = createWorkerSupportSpawn( - ImmutableMap.of( - "supports-workers", "1", "requires-worker-protocol", "json")); + ImmutableMap.of("supports-workers", "1", "requires-worker-protocol", "json")); assertThat(Spawns.getWorkerProtocolFormat(spawn.getSpawn())) .isEqualTo(WorkerProtocolFormat.JSON); } @Test public void testWorkerMnemonicDefault() throws Exception { - SpawnAction defaultMnemonicSpawn = createWorkerSupportSpawn(ImmutableMap.of()); + SpawnAction defaultMnemonicSpawn = createWorkerSupportSpawn(ImmutableMap.of()); assertThat(Spawns.getWorkerKeyMnemonic(defaultMnemonicSpawn.getSpawn())) .isEqualTo("ActionToolMnemonic"); } @@ -622,31 +600,11 @@ public void testWorkerMnemonicDefault() throws Exception { @Test public void testWorkerMnemonicOverride() throws Exception { SpawnAction customMnemonicSpawn = - createWorkerSupportSpawn( - ImmutableMap.of("worker-key-mnemonic", "ToolPoolMnemonic")); + createWorkerSupportSpawn(ImmutableMap.of("worker-key-mnemonic", "ToolPoolMnemonic")); assertThat(Spawns.getWorkerKeyMnemonic(customMnemonicSpawn.getSpawn())) .isEqualTo("ToolPoolMnemonic"); } - @Test - public void testExecutableBuilder() throws Exception { - SpawnAction.Builder builder = builder().addOutput(destinationArtifact); - builder.executableArguments().add("binary").add("execArg1").add("execArg2"); - SpawnAction action = builder.build(nullOwnerWithTargetConfig(), targetConfig); - collectingAnalysisEnvironment.registerAction(action); - assertThat(action.getArguments()).containsExactly("binary", "execArg1", "execArg2").inOrder(); - } - - @Test - public void testExecutableBuilderAfterSetExecutable() throws Exception { - SpawnAction.Builder builder = builder().addOutput(destinationArtifact); - builder.setExecutable(PathFragment.create("binary")); - builder.executableArguments().add("execArg1").add("execArg2"); - SpawnAction action = builder.build(nullOwnerWithTargetConfig(), targetConfig); - collectingAnalysisEnvironment.registerAction(action); - assertThat(action.getArguments()).containsExactly("binary", "execArg1", "execArg2").inOrder(); - } - private static RunfilesSupplier runfilesSupplier(Artifact manifest, PathFragment dir) { return new SingleRunfilesSupplier( dir, 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 01df1d0f91095e..b6229b7f5aa89b 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 @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.SyscallCache; -import java.util.Arrays; import java.util.HashMap; import java.util.Map; import org.junit.Before; @@ -175,8 +174,7 @@ private enum KeyAttributes { MNEMONIC, RUNFILES_SUPPLIER, INPUT, - FIXED_ENVIRONMENT, - VARIABLE_ENVIRONMENT + FIXED_ENVIRONMENT } @Test @@ -243,9 +241,6 @@ public Action generate(ImmutableSet attributesToFlip) { env.put("foo", "bar"); } builder.setEnvironment(env); - if (attributesToFlip.contains(KeyAttributes.VARIABLE_ENVIRONMENT)) { - builder.setInheritedEnvironment(Arrays.asList("baz")); - } SpawnAction action = builder.build(ActionsTestUtil.NULL_ACTION_OWNER, targetConfig); collectingAnalysisEnvironment.registerAction(action); From b384a98a55e1ff4da0a650f5f782645581173358 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 11:51:19 -0700 Subject: [PATCH 015/710] Remove rules preferences from native rules definitions as they are translated to Starlark PiperOrigin-RevId: 529154073 Change-Id: I8c531f3d1b3032c052c1655f2f1368160e95477b --- .../bazel/rules/cpp/BazelCppRuleClasses.java | 2 - .../rules/python/BazelPyRuleClasses.java | 1 - .../build/lib/packages/RuleClass.java | 25 --------- .../build/lib/packages/RuleClassTest.java | 51 ------------------- 4 files changed, 79 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java index 1bb5d8844a485d..71f9612e9ff7ae 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java @@ -37,7 +37,6 @@ import static com.google.devtools.build.lib.rules.cpp.CppFileTypes.SHARED_LIBRARY; import static com.google.devtools.build.lib.rules.cpp.CppFileTypes.VERSIONED_SHARED_LIBRARY; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.PlatformConfiguration; @@ -103,7 +102,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .add( attr(CcToolchain.CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, NODEP_LABEL) .value(CppRuleClasses.ccToolchainTypeAttribute(env))) - .setPreferredDependencyPredicate(Predicates.or(CPP_SOURCE, C_SOURCE, CPP_HEADER)) .requiresConfigurationFragments(PlatformConfiguration.class) .addToolchainTypes(CppRuleClasses.ccToolchainTypeRequirement(env)) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java index c238f4eb85389e..7a98a23a9424b3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java @@ -115,7 +115,6 @@

Note that only the executable rules (py_binary and py_libr attr("srcs_version", STRING) .value(PythonVersion.DEFAULT_SRCS_VALUE.toString()) .allowedValues(new AllowedValueSet(PythonVersion.SRCS_STRINGS))) - .setPreferredDependencyPredicate(PyRuleClasses.PYTHON_SOURCE) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 2241e2c9b3b39e..7ec57b99e1bad0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -765,7 +765,6 @@ public String toString() { private TransitionFactory transitionFactory; private ConfiguredTargetFactory configuredTargetFactory = null; private PredicateWithMessage validityPredicate = PredicatesWithMessage.alwaysTrue(); - private Predicate preferredDependencyPredicate = Predicates.alwaysFalse(); private final AdvertisedProviderSet.Builder advertisedProviders = AdvertisedProviderSet.builder(); private StarlarkCallable configuredTargetFunction = null; @@ -813,9 +812,6 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p if (parent.getValidityPredicate() != PredicatesWithMessage.alwaysTrue()) { setValidityPredicate(parent.getValidityPredicate()); } - if (parent.preferredDependencyPredicate != Predicates.alwaysFalse()) { - setPreferredDependencyPredicate(parent.preferredDependencyPredicate); - } configurationFragmentPolicy .includeConfigurationFragmentsFrom(parent.getConfigurationFragmentPolicy()); supportsConstraintChecking = parent.supportsConstraintChecking; @@ -935,7 +931,6 @@ public RuleClass build(String name, String key) { transitionFactory, configuredTargetFactory, validityPredicate, - preferredDependencyPredicate, advertisedProviders.build(), configuredTargetFunction, externalBindingsFunction, @@ -1152,12 +1147,6 @@ public Builder setValidityPredicate(PredicateWithMessage predicate) { return this; } - @CanIgnoreReturnValue - public Builder setPreferredDependencyPredicate(Predicate predicate) { - this.preferredDependencyPredicate = predicate; - return this; - } - /** * State that the rule class being built possibly supplies the specified provider to its direct * dependencies. @@ -1615,11 +1604,6 @@ public Attribute.Builder copy(String name) { */ private final PredicateWithMessage validityPredicate; - /** - * See {@link #isPreferredDependency}. - */ - private final Predicate preferredDependencyPredicate; - /** * The list of transitive info providers this class advertises to aspects. */ @@ -1716,7 +1700,6 @@ public Attribute.Builder copy(String name) { TransitionFactory transitionFactory, ConfiguredTargetFactory configuredTargetFactory, PredicateWithMessage validityPredicate, - Predicate preferredDependencyPredicate, AdvertisedProviderSet advertisedProviders, @Nullable StarlarkCallable configuredTargetFunction, Function> externalBindingsFunction, @@ -1746,7 +1729,6 @@ public Attribute.Builder copy(String name) { this.transitionFactory = transitionFactory; this.configuredTargetFactory = configuredTargetFactory; this.validityPredicate = validityPredicate; - this.preferredDependencyPredicate = preferredDependencyPredicate; this.advertisedProviders = advertisedProviders; this.configuredTargetFunction = configuredTargetFunction; this.externalBindingsFunction = externalBindingsFunction; @@ -1944,13 +1926,6 @@ public PredicateWithMessage getValidityPredicate() { public AdvertisedProviderSet getAdvertisedProviders() { return advertisedProviders; } - /** - * For --compile_one_dependency: if multiple rules consume the specified target, - * should we choose this one over the "unpreferred" options? - */ - public boolean isPreferredDependency(String filename) { - return preferredDependencyPredicate.apply(filename); - } /** * Returns this rule's policy for configuration fragment access. diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index bab4a1dd81a847..913b171290065a 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java @@ -33,8 +33,6 @@ import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; -import com.google.common.base.Predicate; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -100,8 +98,6 @@ public final class RuleClassTest extends PackageLoadingTestCase { private static final class DummyFragment extends Fragment {} - private static final Predicate PREFERRED_DEPENDENCY_PREDICATE = Predicates.alwaysFalse(); - private static RuleClass createRuleClassA() throws LabelSyntaxException { return newRuleClass( "ruleA", @@ -115,7 +111,6 @@ private static RuleClass createRuleClassA() throws LabelSyntaxException { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -149,7 +144,6 @@ private static RuleClass createRuleClassB(RuleClass ruleClassA) { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -285,7 +279,6 @@ public void testDuplicatedDeps() throws Exception { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -329,7 +322,6 @@ public void testDuplicatedDepsWithinSingleSelectConditionError() throws Exceptio null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -367,7 +359,6 @@ public void testDuplicatedDepsWithinConditionMultipleSelectsErrors() throws Exce null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -417,7 +408,6 @@ public void testSameDepAcrossMultipleSelectsNoDuplicateNoError() throws Exceptio null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -461,7 +451,6 @@ public void testSameDepAcrossMultipleSelectsIsDuplicateNoError() throws Exceptio null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -505,7 +494,6 @@ public void testSameDepAcrossConditionsInSelectNoError() throws Exception { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -604,7 +592,6 @@ public void testImplicitOutputs() throws Exception { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -641,7 +628,6 @@ public void testImplicitOutsWithBasenameDirname() throws Exception { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -673,7 +659,6 @@ private static RuleClass getRuleClassWithComputedDefault(Attribute computedDefau null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -843,7 +828,6 @@ public void testOutputsAreOrdered() throws Exception { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -882,7 +866,6 @@ public void testSubstitutePlaceholderIntoTemplate() throws Exception { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(), @@ -1029,7 +1012,6 @@ private static RuleClass newRuleClass( TransitionFactory transitionFactory, ConfiguredTargetFactory configuredTargetFactory, PredicateWithMessage validityPredicate, - Predicate preferredDependencyPredicate, AdvertisedProviderSet advertisedProviders, @Nullable StarlarkFunction configuredTargetFunction, Set> allowedConfigurationFragments, @@ -1054,7 +1036,6 @@ private static RuleClass newRuleClass( transitionFactory, configuredTargetFactory, validityPredicate, - preferredDependencyPredicate, advertisedProviders, configuredTargetFunction, NO_EXTERNAL_BINDINGS, @@ -1093,7 +1074,6 @@ private static RuleClass createParentRuleClass() { null, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.alwaysTrue(), - PREFERRED_DEPENDENCY_PREDICATE, AdvertisedProviderSet.EMPTY, null, ImmutableSet.of(DummyFragment.class), @@ -1171,37 +1151,6 @@ public String checkValid(Rule from, String toRuleClass, Set toRuleTags) .isNull(); } - /** - * Tests structure for making certain rules "preferential choices" for certain files under - * --compile_one_dependency. - */ - @Test - public void testPreferredDependencyChecker() throws Exception { - final String cppFile = "file.cc"; - final String textFile = "file.txt"; - - // Default: not preferred for anything. - RuleClass defaultClass = - new RuleClass.Builder("defaultClass", RuleClassType.NORMAL, false) - .factory(DUMMY_CONFIGURED_TARGET_FACTORY) - .add(attr("tags", STRING_LIST)) - .build(); - Rule defaultRule = createRule(defaultClass, "defaultRule", ImmutableMap.of()); - assertThat(defaultRule.getRuleClassObject().isPreferredDependency(cppFile)).isFalse(); - assertThat(defaultRule.getRuleClassObject().isPreferredDependency(textFile)).isFalse(); - - // Make a rule that's preferred for C++ sources. - RuleClass cppClass = - new RuleClass.Builder("cppClass", RuleClassType.NORMAL, false) - .factory(DUMMY_CONFIGURED_TARGET_FACTORY) - .add(attr("tags", STRING_LIST)) - .setPreferredDependencyPredicate(filename -> filename.endsWith(".cc")) - .build(); - Rule cppRule = createRule(cppClass, "cppRule", ImmutableMap.of()); - assertThat(cppRule.getRuleClassObject().isPreferredDependency(cppFile)).isTrue(); - assertThat(cppRule.getRuleClassObject().isPreferredDependency(textFile)).isFalse(); - } - @Test public void testBadRuleClassNames() { expectError(RuleClassType.NORMAL, "8abc"); From 6a5b897169edae65fecb018b29875641cc6b224a Mon Sep 17 00:00:00 2001 From: Bazel Release System Date: Wed, 3 May 2023 21:05:14 +0200 Subject: [PATCH 016/710] Release 7.0.0-pre.20230426.1 (2023-05-03) Baseline: dc724e4e8041a9357c6b82af0749def4174f21f2 Important changes: - Added a new `max_compatibility_level` attribute to the `bazel_dep` directive, which allows version selection to upgrade a dependency up to the specified compatibility level. - `--experimental_remote_grpc_log` has been renamed to `--remote_grpc_log` - `--incompatible_remote_build_event_upload_respect_no_cache` is now a no-op. This release contains contributions from many people at Google, as well as Benjamin Peterson, Brentley Jones, Fabian Meumertzheim, George Gensure, Kai Zhang, Oscar Garzon. --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcd42483001c34..71d7d0c42e7482 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,21 @@ +## Release 7.0.0-pre.20230426.1 (2023-05-03) + +``` +Baseline: dc724e4e8041a9357c6b82af0749def4174f21f2 +``` + +Important changes: + + - Added a new `max_compatibility_level` attribute to the + `bazel_dep` directive, which allows version selection to upgrade + a dependency up to the specified compatibility level. + - `--experimental_remote_grpc_log` has been renamed to + `--remote_grpc_log` + - `--incompatible_remote_build_event_upload_respect_no_cache` is + now a no-op. + +This release contains contributions from many people at Google, as well as Benjamin Peterson, Brentley Jones, Fabian Meumertzheim, George Gensure, Kai Zhang, Oscar Garzon. + ## Release 7.0.0-pre.20230420.2 (2023-04-27) ``` From 15226ae1a7b881fad547815e92f41391608fd95c Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 May 2023 14:41:35 -0700 Subject: [PATCH 017/710] Slim down StarlarkLibrary This breaks business logic out of StarlarkLibrary in anticipation of making that class the entry point to getting the "fixed" starlark environment -- i.e. the part of the environment that's hardcoded and not dependent on the rule class provider or builtins injection. - SelectLibrary is moved to SelectorList - DepsetLibrary is moved to Depset. The Depset.depset factory method is made private since it's DepsetLibrary's implementation detail. Depset.fromDirectAndTransitive takes its place for java callers. - BuildLibrary and Proto are elevated to top-level classes. - I preferred to use singletons rather than having the client instantiate these helpers just to pass them to Starlark.addMethods(). PiperOrigin-RevId: 529199753 Change-Id: I33f97e8f304d46daa23bcca688da01f346784081 --- .../analysis/starlark/StarlarkModules.java | 4 +- .../build/lib/collect/nestedset/Depset.java | 93 +++- .../build/lib/packages/BuildGlobals.java | 156 ++++++ .../devtools/build/lib/packages/Proto.java | 212 ++++++++ .../build/lib/packages/SelectorList.java | 44 +- .../build/lib/packages/StarlarkLibrary.java | 472 +----------------- .../build/lib/packages/StructImpl.java | 2 +- .../build/lib/packages/WorkspaceFactory.java | 4 +- .../cpp/StarlarkDefinedLinkTimeLibrary.java | 11 +- .../build/lib/packages/SelectTest.java | 5 +- 10 files changed, 531 insertions(+), 472 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java create mode 100644 src/main/java/com/google/devtools/build/lib/packages/Proto.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java index a77ff194bb82dd..11ee1b9195150d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java @@ -19,8 +19,8 @@ import com.google.devtools.build.lib.analysis.DefaultInfo; import com.google.devtools.build.lib.analysis.OutputGroupInfo; import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; +import com.google.devtools.build.lib.packages.SelectorList; import com.google.devtools.build.lib.packages.StarlarkLibrary; -import com.google.devtools.build.lib.packages.StarlarkLibrary.SelectLibrary; import com.google.devtools.build.lib.packages.StructProvider; import net.starlark.java.eval.Starlark; @@ -40,7 +40,7 @@ public static void addPredeclared(ImmutableMap.Builder predeclar predeclared.putAll(StarlarkLibrary.COMMON); // e.g. select, depset Starlark.addMethods(predeclared, new BazelBuildApiGlobals()); // e.g. configuration_field Starlark.addMethods(predeclared, new StarlarkRuleClassFunctions()); // e.g. rule - Starlark.addMethods(predeclared, new SelectLibrary()); + Starlark.addMethods(predeclared, SelectorList.SelectLibrary.INSTANCE); predeclared.put("cmd_helper", new StarlarkCommandLine()); predeclared.put("attr", new StarlarkAttrModule()); predeclared.put("struct", StructProvider.STRUCT); diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java index 2e098dcb9df99f..13dd5cfa85ceb7 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java @@ -16,16 +16,21 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.docgen.annot.GlobalMethods; +import com.google.devtools.build.docgen.annot.GlobalMethods.Environment; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import java.util.List; import javax.annotation.Nullable; +import net.starlark.java.annot.Param; +import net.starlark.java.annot.ParamType; import net.starlark.java.annot.StarlarkAnnotations; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.Debug; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.NoneType; import net.starlark.java.eval.Printer; import net.starlark.java.eval.Sequence; import net.starlark.java.eval.Starlark; @@ -333,7 +338,7 @@ public StarlarkList toListForStarlark(StarlarkThread thread) throws Eval } /** Create a Depset from the given direct and transitive components. */ - static Depset fromDirectAndTransitive( + public static Depset fromDirectAndTransitive( Order order, List direct, List transitive, boolean strict) throws EvalException { NestedSetBuilder builder = new NestedSetBuilder<>(order); @@ -490,11 +495,8 @@ public boolean equals(Object that) { } } - /** - * Implementation of the build language's depset function, aka - * StarlarkLibrary.CommonLibrary.depset. - */ - public static Depset depset( + /** Implementation of the {@code depset()} callable. */ + private static Depset depset( String orderString, Object direct, Object transitive, StarlarkSemantics semantics) throws EvalException { Order order; @@ -533,4 +535,83 @@ public int hashCode() { public boolean equals(Object other) { return other instanceof Depset && set.equals(((Depset) other).set); } + + /** The user-facing API to the {@code depset} callable. */ + @GlobalMethods(environment = {Environment.BUILD, Environment.BZL}) + public static final class DepsetLibrary { + + private DepsetLibrary() {} + + public static final DepsetLibrary INSTANCE = new DepsetLibrary(); + + @StarlarkMethod( + name = "depset", + doc = + "Creates a depset. The direct" + + " parameter is a list of direct elements of the depset, and" + + " transitive parameter is a list of depsets whose elements become" + + " indirect elements of the created depset. The order in which elements are" + + " returned when the depset is converted to a list is specified by the" + + " order parameter. See the Depsets overview for more" + + " information.\n" // + + "

All" + + " elements (direct and indirect) of a depset must be of the same type, as" + + " obtained by the expression type(x).\n" // + + "

Because a hash-based set is used to eliminate duplicates during iteration," + + " all elements of a depset should be hashable. However, this invariant is not" + + " currently checked consistently in all constructors. Use the" + + " --incompatible_always_check_depset_elements flag to enable consistent" + + " checking; this will be the default behavior in future releases; see Issue 10313.\n" // + + "

In addition, elements must currently be immutable, though this restriction" + + " will be relaxed in future.\n" // + + "

The order of the created depset should be compatible with the order" + + " of its transitive depsets. \"default\" order is" + + " compatible with any other order, all other orders are only compatible with" + + " themselves.\n" // + + "

Note on backward/forward compatibility. This function currently accepts a" + + " positional items parameter. It is deprecated and will be removed" + + " in the future, and after its removal direct will become a sole" + + " positional parameter of the depset function. Thus, both of the" + + " following calls are equivalent and future-proof:
\n" // + + "

depset(['a', 'b'], transitive = [...])\n" //
+                + "depset(direct = ['a', 'b'], transitive = [...])\n" //
+                + "
", + parameters = { + // TODO(cparsons): Make 'order' keyword-only. + @Param( + name = "direct", + defaultValue = "None", + named = true, + allowedTypes = { + @ParamType(type = Sequence.class), + @ParamType(type = NoneType.class), + }, + doc = "A list of direct elements of a depset. "), + @Param( + name = "order", + defaultValue = "\"default\"", + doc = + "The traversal strategy for the new depset. See " + + "here for the possible values.", + named = true), + @Param( + name = "transitive", + named = true, + positional = false, + allowedTypes = { + @ParamType(type = Sequence.class, generic1 = Depset.class), + @ParamType(type = NoneType.class), + }, + doc = "A list of depsets whose elements will become indirect elements of the depset.", + defaultValue = "None"), + }, + useStarlarkThread = true) + public Depset depset( + Object direct, String orderString, Object transitive, StarlarkThread thread) + throws EvalException { + return Depset.depset(orderString, direct, transitive, thread.getSemantics()); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java new file mode 100644 index 00000000000000..3eb044873c549d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java @@ -0,0 +1,156 @@ +// 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.lib.packages; + +import com.google.devtools.build.docgen.annot.GlobalMethods; +import com.google.devtools.build.docgen.annot.GlobalMethods.Environment; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.packages.License.DistributionType; +import com.google.devtools.build.lib.packages.PackageFactory.PackageContext; +import com.google.devtools.build.lib.packages.Type.ConversionException; +import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; +import java.util.List; +import java.util.Set; +import net.starlark.java.annot.Param; +import net.starlark.java.annot.ParamType; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.NoneType; +import net.starlark.java.eval.Sequence; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkThread; +import net.starlark.java.syntax.Location; + +/** A set of miscellaneous APIs that are available to any BUILD file. */ +@GlobalMethods(environment = Environment.BUILD) +class BuildGlobals { + + private BuildGlobals() {} + + static final BuildGlobals INSTANCE = new BuildGlobals(); + + @StarlarkMethod( + name = "environment_group", + doc = + "Defines a set of related environments that can be tagged onto rules to prevent" + + "incompatible rules from depending on each other.", + parameters = { + @Param(name = "name", positional = false, named = true, doc = "The name of the rule."), + // Both parameter below are lists of label designators + @Param( + name = "environments", + allowedTypes = { + @ParamType(type = Sequence.class, generic1 = Label.class), + }, + positional = false, + named = true, + doc = "A list of Labels for the environments to be grouped, from the same package."), + @Param( + name = "defaults", + allowedTypes = { + @ParamType(type = Sequence.class, generic1 = Label.class), + }, + positional = false, + named = true, + doc = "A list of Labels.") + }, // TODO(bazel-team): document what that is + // Not documented by docgen, as this is only available in BUILD files. + // TODO(cparsons): Devise a solution to document BUILD functions. + documented = false, + useStarlarkThread = true) + public NoneType environmentGroup( + String name, + Sequence environmentsList, //