From 8b06f31c46bea4e416bd199f67da68f8494a2ebd Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 9 Oct 2023 14:08:17 -0700 Subject: [PATCH] feat: expose a config_setting for copy execution_requirements (#606) (#607) * feat: expose a config_setting for copy execution_requirements Fixes #604 * chore: add user docs * chore: improve docs * chore: better link to copy_file --- docs/copy_directory.md | 8 +++++++- docs/copy_file.md | 34 +++++++++++++++++++++++++++++++ docs/copy_to_directory.md | 8 +++++++- lib/BUILD.bazel | 24 ++++++++++++++++++++++ lib/copy_directory.bzl | 4 ++++ lib/copy_file.bzl | 34 +++++++++++++++++++++++++++++++ lib/copy_to_directory.bzl | 4 ++++ lib/private/copy_common.bzl | 22 ++++++++++++++++++-- lib/private/copy_directory.bzl | 18 +++++++++------- lib/private/copy_file.bzl | 11 +++++----- lib/private/copy_to_directory.bzl | 18 +++++++++------- 11 files changed, 162 insertions(+), 23 deletions(-) diff --git a/docs/copy_directory.md b/docs/copy_directory.md index 66e550607..0a94e0874 100644 --- a/docs/copy_directory.md +++ b/docs/copy_directory.md @@ -5,6 +5,10 @@ A rule that copies a directory to another place. The rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command on Windows (no Bash is required). +NB: See notes on [copy_file](./copy_file.md#choosing-execution-requirements) +regarding `execution_requirements` settings for remote execution. +These settings apply to the rules below as well. + @@ -73,7 +77,8 @@ other rule implementations. ## copy_directory_bin_action 
-copy_directory_bin_action(ctx, src, dst, copy_directory_bin, hardlink, verbose)
+copy_directory_bin_action(ctx, src, dst, copy_directory_bin, hardlink, verbose,
+                          override_execution_requirements)
Factory function that creates an action to copy a directory from src to dst using a tool binary. @@ -96,5 +101,6 @@ within other rule implementations. | copy_directory_bin | Copy to directory tool binary. | none | | hardlink | Controls when to use hardlinks to files instead of making copies.

See copy_directory rule documentation for more details. | "auto" | | verbose | If true, prints out verbose logs to stdout | False | +| override_execution_requirements | specify execution_requirements for this action | None | diff --git a/docs/copy_file.md b/docs/copy_file.md index f9ea8440e..bfb2e9995 100644 --- a/docs/copy_file.md +++ b/docs/copy_file.md @@ -11,6 +11,40 @@ on Windows (no Bash is required). This fork of bazel-skylib's copy_file adds DirectoryPathInfo support and allows multiple copy_file in the same package. +Choosing execution requirements +------------------------------- + +Copy actions can be very numerous, especially when used on third-party dependency packages. + +By default, we set the `execution_requirements` of actions we spawn to be non-sandboxed and run +locally, not reading or writing to a remote cache. For the typical user this is the fastest, because +it avoids the overhead of creating a sandbox and making network calls for every file being copied. + +If you use Remote Execution and Build-without-the-bytes, then you'll want the copy action to +occur on the remote machine instead, since the inputs and outputs stay in the cloud and don't need +to be brought to the local machine where Bazel runs. + +Other reasons to allow copy actions to run remotely: +- Bazel prints an annoying warning "[action] uses implicit fallback from sandbox to local, which is deprecated because it is not hermetic" +- When the host and exec platforms have different architectures, toolchain resolution runs the wrong binary locally, + see https://github.com/aspect-build/bazel-lib/issues/466 + +To disable our `copy_use_local_execution` flag, put this in your `.bazelrc` file: + +``` +# with Bazel 6.4 or greater: + +# Disable default for execution_requirements of copy actions +common --@aspect_bazel_lib//lib:copy_use_local_execution=false + +# with Bazel 6.3 or earlier: + +# Disable default for execution_requirements of copy actions +build --@aspect_bazel_lib//lib:copy_use_local_execution=false +fetch --@aspect_bazel_lib//lib:copy_use_local_execution=false +query --@aspect_bazel_lib//lib:copy_use_local_execution=false +``` + diff --git a/docs/copy_to_directory.md b/docs/copy_to_directory.md index 36eff7957..f75553924 100644 --- a/docs/copy_to_directory.md +++ b/docs/copy_to_directory.md @@ -2,6 +2,10 @@ Copy files and directories to an output directory. +NB: See notes on [copy_file](./copy_file.md#choosing-execution-requirements) +regarding `execution_requirements` settings for remote execution. +These settings apply to the rules below as well. + @@ -119,7 +123,8 @@ other rule implementations where additional_files can also be passed in. copy_to_directory_bin_action(ctx, name, dst, copy_to_directory_bin, files, targets, root_paths, include_external_repositories, include_srcs_packages, exclude_srcs_packages, include_srcs_patterns, exclude_srcs_patterns, - exclude_prefixes, replace_prefixes, allow_overwrites, hardlink, verbose) + exclude_prefixes, replace_prefixes, allow_overwrites, hardlink, verbose, + override_execution_requirements) Factory function to copy files to a directory using a tool binary. @@ -153,6 +158,7 @@ other rule implementations where additional_files can also be passed in. | allow_overwrites | If True, allow files to be overwritten if the same output file is copied to twice.

See copy_to_directory rule documentation for more details. | False | | hardlink | Controls when to use hardlinks to files instead of making copies.

See copy_to_directory rule documentation for more details. | "auto" | | verbose | If true, prints out verbose logs to stdout | False | +| override_execution_requirements | specify execution_requirements for this action | None | diff --git a/lib/BUILD.bazel b/lib/BUILD.bazel index 0dbda46b3..3a525cce7 100644 --- a/lib/BUILD.bazel +++ b/lib/BUILD.bazel @@ -1,6 +1,7 @@ load("@bazel_skylib//:bzl_library.bzl", "bzl_library") load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") load("//lib:utils.bzl", "is_bzlmod_enabled") +load("//lib/private:copy_common.bzl", "copy_options") load("//lib/private:stamping.bzl", "stamp_build_setting") # Ensure that users building their own rules can dep on our bzl_library targets for their stardoc @@ -31,6 +32,29 @@ bool_flag( build_setting_default = True if is_bzlmod_enabled() else False, ) +# Users may set this flag with +bool_flag( + name = "copy_use_local_execution", + build_setting_default = True, + visibility = ["//visibility:public"], +) + +config_setting( + name = "copy_use_local_execution_setting", + flag_values = { + ":copy_use_local_execution": "true", + }, +) + +# Used in rules which spawn actions that copy files +copy_options( + name = "copy_options", + copy_use_local_execution = select({ + ":copy_use_local_execution_setting": True, + "//conditions:default": False, + }), +) + toolchain_type( name = "jq_toolchain_type", ) diff --git a/lib/copy_directory.bzl b/lib/copy_directory.bzl index 26d1ef465..1cbc9ba19 100644 --- a/lib/copy_directory.bzl +++ b/lib/copy_directory.bzl @@ -2,6 +2,10 @@ The rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command on Windows (no Bash is required). + +NB: See notes on [copy_file](./copy_file.md#choosing-execution-requirements) +regarding `execution_requirements` settings for remote execution. +These settings apply to the rules below as well. """ load( diff --git a/lib/copy_file.bzl b/lib/copy_file.bzl index 12d4695e7..1f853d87f 100644 --- a/lib/copy_file.bzl +++ b/lib/copy_file.bzl @@ -27,6 +27,40 @@ on Windows (no Bash is required). This fork of bazel-skylib's copy_file adds DirectoryPathInfo support and allows multiple copy_file in the same package. + +Choosing execution requirements +------------------------------- + +Copy actions can be very numerous, especially when used on third-party dependency packages. + +By default, we set the `execution_requirements` of actions we spawn to be non-sandboxed and run +locally, not reading or writing to a remote cache. For the typical user this is the fastest, because +it avoids the overhead of creating a sandbox and making network calls for every file being copied. + +If you use Remote Execution and Build-without-the-bytes, then you'll want the copy action to +occur on the remote machine instead, since the inputs and outputs stay in the cloud and don't need +to be brought to the local machine where Bazel runs. + +Other reasons to allow copy actions to run remotely: +- Bazel prints an annoying warning "[action] uses implicit fallback from sandbox to local, which is deprecated because it is not hermetic" +- When the host and exec platforms have different architectures, toolchain resolution runs the wrong binary locally, + see https://github.com/aspect-build/bazel-lib/issues/466 + +To disable our `copy_use_local_execution` flag, put this in your `.bazelrc` file: + +``` +# with Bazel 6.4 or greater: + +# Disable default for execution_requirements of copy actions +common --@aspect_bazel_lib//lib:copy_use_local_execution=false + +# with Bazel 6.3 or earlier: + +# Disable default for execution_requirements of copy actions +build --@aspect_bazel_lib//lib:copy_use_local_execution=false +fetch --@aspect_bazel_lib//lib:copy_use_local_execution=false +query --@aspect_bazel_lib//lib:copy_use_local_execution=false +``` """ load( diff --git a/lib/copy_to_directory.bzl b/lib/copy_to_directory.bzl index 4afaf2d05..a9c3ac68a 100644 --- a/lib/copy_to_directory.bzl +++ b/lib/copy_to_directory.bzl @@ -1,4 +1,8 @@ """Copy files and directories to an output directory. + +NB: See notes on [copy_file](./copy_file.md#choosing-execution-requirements) +regarding `execution_requirements` settings for remote execution. +These settings apply to the rules below as well. """ load( diff --git a/lib/private/copy_common.bzl b/lib/private/copy_common.bzl index aedb04ac6..b17d86f9c 100644 --- a/lib/private/copy_common.bzl +++ b/lib/private/copy_common.bzl @@ -1,7 +1,25 @@ "Helpers for copy rules" -# Hints for Bazel spawn strategy -COPY_EXECUTION_REQUIREMENTS = { +CopyOptionsInfo = provider("Options for running copy actions", fields = ["execution_requirements"]) + +def _copy_options_impl(ctx): + return CopyOptionsInfo( + execution_requirements = COPY_EXECUTION_REQUIREMENTS_LOCAL if ctx.attr.copy_use_local_execution else {}, + ) + +copy_options = rule(implementation = _copy_options_impl, attrs = {"copy_use_local_execution": attr.bool()}) + +# Helper function to be used when creating an action +def execution_requirements_for_copy(ctx): + if hasattr(ctx.attr, "_options") and CopyOptionsInfo in ctx.attr._options: + return ctx.attr._options[CopyOptionsInfo].execution_requirements + + # 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. +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`. diff --git a/lib/private/copy_directory.bzl b/lib/private/copy_directory.bzl index 790a66df5..0dc45f4e4 100644 --- a/lib/private/copy_directory.bzl +++ b/lib/private/copy_directory.bzl @@ -4,10 +4,10 @@ This rule copies a directory to another location using Bash (on Linux/macOS) or cmd.exe (on Windows). """ -load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", _progress_path = "progress_path") +load(":copy_common.bzl", "execution_requirements_for_copy", _progress_path = "progress_path") load(":platform_utils.bzl", _platform_utils = "platform_utils") -def _copy_cmd(ctx, src, dst): +def _copy_cmd(ctx, src, dst, override_execution_requirements = None): # Most Windows binaries built with MSVC use a certain argument quoting # scheme. Bazel uses that scheme too to quote arguments. However, # cmd.exe uses different semantics, so Bazel's quoting is wrong here. @@ -43,10 +43,10 @@ def _copy_cmd(ctx, src, dst): mnemonic = mnemonic, progress_message = progress_message, use_default_shell_env = True, - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) -def _copy_bash(ctx, src, dst): +def _copy_bash(ctx, src, dst, override_execution_requirements = None): cmd = "rm -Rf \"$2\" && cp -fR \"$1/\" \"$2\"" mnemonic = "CopyDirectory" progress_message = "Copying directory %s" % _progress_path(src) @@ -59,7 +59,7 @@ def _copy_bash(ctx, src, dst): mnemonic = mnemonic, progress_message = progress_message, use_default_shell_env = True, - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) # TODO(2.0): remove the legacy copy_directory_action helper @@ -103,7 +103,8 @@ def copy_directory_bin_action( dst, copy_directory_bin, hardlink = "auto", - verbose = False): + verbose = False, + override_execution_requirements = None): """Factory function that creates an action to copy a directory from src to dst using a tool binary. The tool binary will typically be the `@aspect_bazel_lib//tools/copy_directory` `go_binary` @@ -126,6 +127,8 @@ def copy_directory_bin_action( See copy_directory rule documentation for more details. verbose: If true, prints out verbose logs to stdout + + override_execution_requirements: specify execution_requirements for this action """ args = [ src.path, @@ -146,7 +149,7 @@ def copy_directory_bin_action( arguments = args, mnemonic = "CopyDirectory", progress_message = "Copying directory %s" % _progress_path(src), - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) def _copy_directory_impl(ctx): @@ -184,6 +187,7 @@ _copy_directory = rule( default = "auto", ), "verbose": attr.bool(), + "_options": attr.label(default = "//lib:copy_options"), # use '_tool' attribute for development only; do not commit with this attribute active since it # propagates a dependency on rules_go which would be breaking for users # "_tool": attr.label( diff --git a/lib/private/copy_file.bzl b/lib/private/copy_file.bzl index 76c36a176..2bb35e2ec 100644 --- a/lib/private/copy_file.bzl +++ b/lib/private/copy_file.bzl @@ -24,11 +24,11 @@ cmd.exe (on Windows). `_copy_xfile` marks the resulting file executable, `_copy_file` does not. """ -load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", _progress_path = "progress_path") +load(":copy_common.bzl", "execution_requirements_for_copy", _progress_path = "progress_path") load(":directory_path.bzl", "DirectoryPathInfo") load(":platform_utils.bzl", _platform_utils = "platform_utils") -def _copy_cmd(ctx, src, src_path, dst): +def _copy_cmd(ctx, src, src_path, dst, override_execution_requirements = None): # Most Windows binaries built with MSVC use a certain argument quoting # scheme. Bazel uses that scheme too to quote arguments. However, # cmd.exe uses different semantics, so Bazel's quoting is wrong here. @@ -65,10 +65,10 @@ def _copy_cmd(ctx, src, src_path, dst): mnemonic = mnemonic, progress_message = progress_message, use_default_shell_env = True, - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) -def _copy_bash(ctx, src, src_path, dst): +def _copy_bash(ctx, src, src_path, dst, override_execution_requirements = None): cmd_tmpl = "cp -f \"$1\" \"$2\"" mnemonic = "CopyFile" progress_message = "Copying file %s" % _progress_path(src) @@ -81,7 +81,7 @@ def _copy_bash(ctx, src, src_path, dst): mnemonic = mnemonic, progress_message = progress_message, use_default_shell_env = True, - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) def copy_file_action(ctx, src, dst, dir_path = None, is_windows = None): @@ -157,6 +157,7 @@ _ATTRS = { "is_executable": attr.bool(mandatory = True), "allow_symlink": attr.bool(mandatory = True), "out": attr.output(mandatory = True), + "_options": attr.label(default = "//lib:copy_options"), } _copy_file = rule( diff --git a/lib/private/copy_to_directory.bzl b/lib/private/copy_to_directory.bzl index 3bc020602..7b4ff5c7b 100644 --- a/lib/private/copy_to_directory.bzl +++ b/lib/private/copy_to_directory.bzl @@ -1,7 +1,7 @@ "copy_to_directory implementation" load("@bazel_skylib//lib:paths.bzl", skylib_paths = "paths") -load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", _progress_path = "progress_path") +load(":copy_common.bzl", "execution_requirements_for_copy", _progress_path = "progress_path") load(":directory_path.bzl", "DirectoryPathInfo") load(":glob_match.bzl", "glob_match", "is_glob") load(":paths.bzl", "paths") @@ -277,6 +277,7 @@ _copy_to_directory_attr = { "verbose": attr.bool( doc = _copy_to_directory_attr_doc["verbose"], ), + "_options": attr.label(default = "//lib:copy_options"), # use '_tool' attribute for development only; do not commit with this attribute active since it # propagates a dependency on rules_go which would be breaking for users # "_tool": attr.label( @@ -459,7 +460,7 @@ def _merge_into_copy_path(copy_paths, src_path, dst_path, src_file): return True return False -def _copy_to_dir_bash(ctx, copy_paths, dst_dir, allow_overwrites): +def _copy_to_dir_bash(ctx, copy_paths, dst_dir, allow_overwrites, override_execution_requirements = None): cmds = [ "set -o errexit -o nounset -o pipefail", "OUT_CAPTURE=$(mktemp)", @@ -520,10 +521,10 @@ fi mnemonic = "CopyToDirectory", progress_message = "Copying files to directory %s" % _progress_path(dst_dir), use_default_shell_env = True, - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) -def _copy_to_dir_cmd(ctx, copy_paths, dst_dir): +def _copy_to_dir_cmd(ctx, copy_paths, dst_dir, override_execution_requirements = None): # Most Windows binaries built with MSVC use a certain argument quoting # scheme. Bazel uses that scheme too to quote arguments. However, # cmd.exe uses different semantics, so Bazel's quoting is wrong here. @@ -588,7 +589,7 @@ if exist "{src}\\*" ( mnemonic = "CopyToDirectory", progress_message = "Copying files to directory %s" % _progress_path(dst_dir), use_default_shell_env = True, - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) def _copy_to_directory_impl(ctx): @@ -654,7 +655,8 @@ def copy_to_directory_bin_action( replace_prefixes = {}, allow_overwrites = False, hardlink = "auto", - verbose = False): + verbose = False, + override_execution_requirements = None): """Factory function to copy files to a directory using a tool binary. The tool binary will typically be the `@aspect_bazel_lib//tools/copy_to_directory` `go_binary` @@ -717,6 +719,8 @@ def copy_to_directory_bin_action( See copy_to_directory rule documentation for more details. verbose: If true, prints out verbose logs to stdout + + override_execution_requirements: specify execution_requirements for this action """ # Replace "." in root_paths with the package name of the target @@ -839,7 +843,7 @@ def copy_to_directory_bin_action( arguments = [config_file.path], mnemonic = "CopyToDirectory", progress_message = "Copying files to directory %s" % _progress_path(dst), - execution_requirements = _COPY_EXECUTION_REQUIREMENTS, + execution_requirements = override_execution_requirements or execution_requirements_for_copy(ctx), ) # TODO(2.0): remove the legacy copy_to_directory_action helper