From a47eebfa65c4f0a44d0f18d21253f79a9a9faa68 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Tue, 31 Oct 2023 13:38:32 -0700 Subject: [PATCH] fix: fix execution requirements for 'build without the bytes' (#639) --- .aspect/bazelrc/performance.bazelrc | 9 --- lib/private/copy_common.bzl | 55 ++----------------- .../bazelrc_presets/all/performance.bazelrc | 9 --- .../copy_to_directory_bin_action/BUILD.bazel | 6 ++ 4 files changed, 11 insertions(+), 68 deletions(-) diff --git a/.aspect/bazelrc/performance.bazelrc b/.aspect/bazelrc/performance.bazelrc index fff4c7c..26dbbe1 100644 --- a/.aspect/bazelrc/performance.bazelrc +++ b/.aspect/bazelrc/performance.bazelrc @@ -27,12 +27,3 @@ build --experimental_reuse_sandbox_directories # author. # Docs: https://bazel.build/reference/command-line-reference#flag--legacy_external_runfiles build --nolegacy_external_runfiles - -# Some actions are always IO-intensive but require little compute. It's wasteful to put the output -# in the remote cache, it just saturates the network and fills the cache storage causing earlier -# evictions. It's also not worth sending them for remote execution. -# For actions like PackageTar it's usually faster to just re-run the work locally every time. -# You'll have to look at an execution log to figure out what other action mnemonics you care about. -# In some cases you may need to patch rulesets to add a mnemonic to actions that don't have one. -# https://bazel.build/reference/command-line-reference#flag--modify_execution_info -build --modify_execution_info=PackageTar=+no-remote diff --git a/lib/private/copy_common.bzl b/lib/private/copy_common.bzl index b17d86f..a6f50a8 100644 --- a/lib/private/copy_common.bzl +++ b/lib/private/copy_common.bzl @@ -17,63 +17,18 @@ def execution_requirements_for_copy(ctx): # If the rule ctx doesn't expose the CopyOptions, the default is to run locally return COPY_EXECUTION_REQUIREMENTS_LOCAL -# When applied to execution_requirements of an action, these prevent the action from being -# sandboxed or remotely cached, for performance of builds that don't rely on RBE and build-without-bytes. +# When applied to execution_requirements of an action, these prevent the action from being sandboxed +# for improved performance. COPY_EXECUTION_REQUIREMENTS_LOCAL = { - # ----------------+----------------------------------------------------------------------------- - # no-remote | Prevents the action or test from being executed remotely or cached remotely. - # | This is equivalent to using both `no-remote-cache` and `no-remote-exec`. - # ----------------+----------------------------------------------------------------------------- - # no-remote-cache | Results in the action or test never being cached remotely (but it may - # | be cached locally; it may also be executed remotely). Note: for the purposes - # | of this tag, the disk-cache is considered a local cache, whereas the http - # | and gRPC caches are considered remote. If a combined cache is specified - # | (i.e. a cache with local and remote components), it's treated as a remote - # | cache and disabled entirely unless --incompatible_remote_results_ignore_disk - # | is set in which case the local components will be used. - # ----------------+----------------------------------------------------------------------------- - # no-remote-exec | Results in the action or test never being executed remotely (but it may be - # | cached remotely). - # ----------------+----------------------------------------------------------------------------- - # no-cache | Results in the action or test never being cached (remotely or locally) # ----------------+----------------------------------------------------------------------------- # no-sandbox | Results in the action or test never being sandboxed; it can still be cached - # | or run remotely - use no-cache or no-remote to prevent either or both of - # | those. - # ----------------+----------------------------------------------------------------------------- - # local | Precludes the action or test from being remotely cached, remotely executed, - # | or run inside the sandbox. For genrules and tests, marking the rule with the - # | local = True attribute has the same effect. + # | or run remotely. # ----------------+----------------------------------------------------------------------------- # See https://bazel.google.cn/reference/be/common-definitions?hl=en&authuser=0#common-attributes # - # Copying file & directories is entirely IO-bound and there is no point doing this work - # remotely. - # - # Also, remote-execution does not allow source directory inputs, see - # https://github.com/bazelbuild/bazel/commit/c64421bc35214f0414e4f4226cc953e8c55fa0d2 So we must - # not attempt to execute remotely in that case. - # - # There is also no point pulling the output file or directory from the remote cache since the - # bytes to copy are already available locally. Conversely, no point in writing to the cache if - # no one has any reason to check it for this action. - # - # Read and writing to disk cache is disabled as well primarily to reduce disk usage on the local - # machine. A disk cache hit of a directory copy could be slghtly faster than a copy since the - # disk cache stores the directory artifact as a single entry, but the slight performance bump - # comes at the cost of heavy disk cache usage, which is an unmanaged directory that grow beyond - # the bounds of the physical disk. - # TODO: run benchmarks to measure the impact on copy_directory - # - # Sandboxing for this action is wasteful as well since there is a 1:1 mapping of input - # file/directory to output file/directory and no room for non-hermetic inputs to sneak in to the - # input. - "no-remote": "1", - "no-remote-cache": "1", - "no-remote-exec": "1", - "no-cache": "1", + # Sandboxing for this action is wasteful since there is a 1:1 mapping of input file/directory to + # output file/directory so little room for non-hermetic inputs to sneak in to the execution. "no-sandbox": "1", - "local": "1", } def progress_path(f): diff --git a/lib/tests/bazelrc_presets/all/performance.bazelrc b/lib/tests/bazelrc_presets/all/performance.bazelrc index fff4c7c..26dbbe1 100644 --- a/lib/tests/bazelrc_presets/all/performance.bazelrc +++ b/lib/tests/bazelrc_presets/all/performance.bazelrc @@ -27,12 +27,3 @@ build --experimental_reuse_sandbox_directories # author. # Docs: https://bazel.build/reference/command-line-reference#flag--legacy_external_runfiles build --nolegacy_external_runfiles - -# Some actions are always IO-intensive but require little compute. It's wasteful to put the output -# in the remote cache, it just saturates the network and fills the cache storage causing earlier -# evictions. It's also not worth sending them for remote execution. -# For actions like PackageTar it's usually faster to just re-run the work locally every time. -# You'll have to look at an execution log to figure out what other action mnemonics you care about. -# In some cases you may need to patch rulesets to add a mnemonic to actions that don't have one. -# https://bazel.build/reference/command-line-reference#flag--modify_execution_info -build --modify_execution_info=PackageTar=+no-remote diff --git a/lib/tests/copy_to_directory_bin_action/BUILD.bazel b/lib/tests/copy_to_directory_bin_action/BUILD.bazel index ba23b0a..44fea2f 100644 --- a/lib/tests/copy_to_directory_bin_action/BUILD.bazel +++ b/lib/tests/copy_to_directory_bin_action/BUILD.bazel @@ -14,6 +14,8 @@ copy_directory( name = "tree_artifact", src = "ds", out = "dsc", + # RBE not happy with the symlinks in this test case + tags = ["no-remote"], ) lib( @@ -43,6 +45,8 @@ pkg( ":tree_artifact", ], out = "pkg", + # RBE not happy with the symlinks in this test case + tags = ["no-remote"], target_compatible_with = select({ # D:/a/bazel-lib/bazel-lib/lib/tests/copy_to_directory_bin_action/BUILD.bazel:36:4: # declared output 'lib/tests/copy_to_directory_bin_action/pkg_symlink_0' is not a symlink @@ -59,6 +63,8 @@ diff_test( name = "test", file1 = ":pkg", file2 = ":expected_pkg", + # RBE not happy with the symlinks in this test case + tags = ["no-remote"], ) bzl_library(