From bbfb74c8e64f9534b2334e796de24182fe61d0e6 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Mon, 12 Sep 2022 20:45:48 -0700 Subject: [PATCH] fix: fix host platform detection on all copy actions which always run locally (#241) --- WORKSPACE | 7 ++++ docs/copy_directory.md | 2 +- docs/copy_file.md | 2 +- docs/copy_to_bin.md | 4 +- docs/copy_to_directory.md | 2 +- lib/private/BUILD.bazel | 7 +++- lib/private/copy_common.bzl | 5 +++ lib/private/copy_directory.bzl | 18 ++++---- lib/private/copy_file.bzl | 19 +++++---- lib/private/copy_to_bin.bzl | 17 ++++---- lib/private/copy_to_directory.bzl | 17 ++++---- lib/private/local_config_platform.bzl | 49 ++++++++++++++++++++++ lib/tests/copy_to_directory_action/pkg.bzl | 6 +-- 13 files changed, 114 insertions(+), 41 deletions(-) create mode 100644 lib/private/local_config_platform.bzl diff --git a/WORKSPACE b/WORKSPACE index 8e2c4c2d3..817f3986e 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -35,3 +35,10 @@ go_rules_dependencies() go_register_toolchains(version = "1.17.2") gazelle_dependencies() + +# buildifier: disable=bzl-visibility +load("//lib/private:local_config_platform.bzl", "local_config_platform") + +local_config_platform( + name = "local_config_platform", +) diff --git a/docs/copy_directory.md b/docs/copy_directory.md index 1177d20e2..6ffac440a 100644 --- a/docs/copy_directory.md +++ b/docs/copy_directory.md @@ -58,6 +58,6 @@ other rule implementations. | ctx | The rule context. | none | | src | The directory to make a copy of. Can be a source directory or TreeArtifact. | none | | dst | The directory to copy to. Must be a TreeArtifact. | none | -| is_windows | If true, an cmd.exe action is created so there is no bash dependency. | False | +| is_windows | Deprecated and unused | None | diff --git a/docs/copy_file.md b/docs/copy_file.md index c6f6d04f1..021eeef9c 100644 --- a/docs/copy_file.md +++ b/docs/copy_file.md @@ -72,6 +72,6 @@ other rule implementations. | src | The source file to copy or TreeArtifact to copy a single file out of. | none | | dst | The destination file. | none | | dir_path | If src is a TreeArtifact, the path within the TreeArtifact to the file to copy. | None | -| is_windows | If true, an cmd.exe action is created so there is no bash dependency. | False | +| is_windows | Deprecated and unused | None | diff --git a/docs/copy_to_bin.md b/docs/copy_to_bin.md index 23ba2d4b3..d1b733d53 100755 --- a/docs/copy_to_bin.md +++ b/docs/copy_to_bin.md @@ -33,7 +33,7 @@ without a copy action. | :------------- | :------------- | :------------- | | ctx | The rule context. | none | | file | The file to copy. | none | -| is_windows | If true, an cmd.exe action is created so there is no bash dependency. | False | +| is_windows | Deprecated and unused | None | **RETURNS** @@ -64,7 +64,7 @@ directly to the result without a copy action. | :------------- | :------------- | :------------- | | ctx | The rule context. | none | | files | List of File objects. | none | -| is_windows | If true, an cmd.exe action is created so there is no bash dependency. | False | +| is_windows | Deprecated and unused | None | **RETURNS** diff --git a/docs/copy_to_directory.md b/docs/copy_to_directory.md index 05f5e5472..acf74aea1 100644 --- a/docs/copy_to_directory.md +++ b/docs/copy_to_directory.md @@ -72,7 +72,7 @@ other rule implementations where additional_files can also be passed in. | exclude_prefixes | List of path prefixes to exclude from output directory. | [] | | replace_prefixes | Map of paths prefixes to replace in the output directory path when copying files.

See copy_to_directory rule documentation for more details. | {} | | 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 | -| is_windows | If true, an cmd.exe action is created so there is no bash dependency. | False | +| is_windows | Deprecated and unused | None | diff --git a/lib/private/BUILD.bazel b/lib/private/BUILD.bazel index 9ae6782eb..5a01d6020 100644 --- a/lib/private/BUILD.bazel +++ b/lib/private/BUILD.bazel @@ -8,13 +8,18 @@ exports_files( ) exports_files( - ["diff_test_tmpl.sh", "diff_test_tmpl.bat", "parse_status_file.jq"], + [ + "diff_test_tmpl.sh", + "diff_test_tmpl.bat", + "parse_status_file.jq", + ], visibility = ["//visibility:public"], ) bzl_library( name = "copy_common", srcs = ["copy_common.bzl"], + deps = ["@local_config_platform//:constraints"], ) bzl_library( diff --git a/lib/private/copy_common.bzl b/lib/private/copy_common.bzl index ac2a7b06b..cb3cc7182 100644 --- a/lib/private/copy_common.bzl +++ b/lib/private/copy_common.bzl @@ -1,5 +1,7 @@ "Helpers for copy rules" +load("@local_config_platform//:constraints.bzl", "HOST_CONSTRAINTS") + # Hints for Bazel spawn strategy COPY_EXECUTION_REQUIREMENTS = { # ----------------+----------------------------------------------------------------------------- @@ -57,3 +59,6 @@ COPY_EXECUTION_REQUIREMENTS = { "no-sandbox": "1", "local": "1", } + +def is_windows_host(): + return "@platforms//os:windows" in HOST_CONSTRAINTS diff --git a/lib/private/copy_directory.bzl b/lib/private/copy_directory.bzl index a9a6dc5ba..0b578b8ba 100644 --- a/lib/private/copy_directory.bzl +++ b/lib/private/copy_directory.bzl @@ -4,7 +4,7 @@ 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") +load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", _is_windows_host = "is_windows_host") def _copy_cmd(ctx, src, dst): # Most Windows binaries built with MSVC use a certain argument quoting @@ -61,7 +61,7 @@ def _copy_bash(ctx, src, dst): execution_requirements = _COPY_EXECUTION_REQUIREMENTS, ) -def copy_directory_action(ctx, src, dst, is_windows = False): +def copy_directory_action(ctx, src, dst, is_windows = None): """Helper function that creates an action to copy a directory from src to dst. This helper is used by copy_directory. It is exposed as a public API so it can be used within @@ -71,22 +71,27 @@ def copy_directory_action(ctx, src, dst, is_windows = False): ctx: The rule context. src: The directory to make a copy of. Can be a source directory or TreeArtifact. dst: The directory to copy to. Must be a TreeArtifact. - is_windows: If true, an cmd.exe action is created so there is no bash dependency. + is_windows: Deprecated and unused """ + + # TODO(2.0): remove depcreated & unused is_windows parameter if not src.is_source and not dst.is_directory: fail("src must be a source directory or TreeArtifact") if dst.is_source or not dst.is_directory: fail("dst must be a TreeArtifact") + + # Because copy actions have "local" execution requirements, we can safely assume + # the execution is the same as the host platform and generate different actions for Windows + # and non-Windows host platforms + is_windows = _is_windows_host() if is_windows: _copy_cmd(ctx, src, dst) else: _copy_bash(ctx, src, dst) def _copy_directory_impl(ctx): - is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]) - dst = ctx.actions.declare_directory(ctx.attr.out) - copy_directory_action(ctx, ctx.file.src, dst, is_windows) + copy_directory_action(ctx, ctx.file.src, dst) files = depset(direct = [dst]) runfiles = ctx.runfiles(files = [dst]) @@ -101,7 +106,6 @@ _copy_directory = rule( # Cannot declare out as an output here, because there's no API for declaring # TreeArtifact outputs. "out": attr.string(mandatory = True), - "_windows_constraint": attr.label(default = "@platforms//os:windows"), }, ) diff --git a/lib/private/copy_file.bzl b/lib/private/copy_file.bzl index 9ad96c0b3..e4836d187 100644 --- a/lib/private/copy_file.bzl +++ b/lib/private/copy_file.bzl @@ -24,7 +24,7 @@ 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") +load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", _is_windows_host = "is_windows_host") load(":directory_path.bzl", "DirectoryPathInfo") def _copy_cmd(ctx, src, src_path, dst): @@ -83,7 +83,7 @@ def _copy_bash(ctx, src, src_path, dst): execution_requirements = _COPY_EXECUTION_REQUIREMENTS, ) -def copy_file_action(ctx, src, dst, dir_path = None, is_windows = False): +def copy_file_action(ctx, src, dst, dir_path = None, is_windows = None): """Helper function that creates an action to copy a file from src to dst. If src is a TreeArtifact, dir_path must be specified as the path within @@ -97,8 +97,10 @@ def copy_file_action(ctx, src, dst, dir_path = None, is_windows = False): src: The source file to copy or TreeArtifact to copy a single file out of. dst: The destination file. dir_path: If src is a TreeArtifact, the path within the TreeArtifact to the file to copy. - is_windows: If true, an cmd.exe action is created so there is no bash dependency. + is_windows: Deprecated and unused """ + + # TODO(2.0): remove depcreated & unused is_windows parameter if dst.is_directory: fail("dst must not be a TreeArtifact") if src.is_directory: @@ -107,14 +109,17 @@ def copy_file_action(ctx, src, dst, dir_path = None, is_windows = False): src_path = "/".join([src.path, dir_path]) else: src_path = src.path + + # Because copy actions have "local" execution requirements, we can safely assume + # the execution is the same as the host platform and generate different actions for Windows + # and non-Windows host platforms + is_windows = _is_windows_host() if is_windows: _copy_cmd(ctx, src, src_path, dst) else: _copy_bash(ctx, src, src_path, dst) def _copy_file_impl(ctx): - is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]) - if ctx.attr.allow_symlink: if len(ctx.files.src) != 1: fail("src must be a single file when allow_symlink is True") @@ -131,14 +136,13 @@ def _copy_file_impl(ctx): ctx.attr.src[DirectoryPathInfo].directory, ctx.outputs.out, dir_path = ctx.attr.src[DirectoryPathInfo].path, - is_windows = is_windows, ) else: if len(ctx.files.src) != 1: fail("src must be a single file or a target that provides a DirectoryPathInfo") if ctx.files.src[0].is_directory: fail("cannot use copy_file on a directory; try copy_directory instead") - copy_file_action(ctx, ctx.files.src[0], ctx.outputs.out, is_windows = is_windows) + copy_file_action(ctx, ctx.files.src[0], ctx.outputs.out) files = depset(direct = [ctx.outputs.out]) runfiles = ctx.runfiles(files = [ctx.outputs.out]) @@ -152,7 +156,6 @@ _ATTRS = { "is_executable": attr.bool(mandatory = True), "allow_symlink": attr.bool(mandatory = True), "out": attr.output(mandatory = True), - "_windows_constraint": attr.label(default = "@platforms//os:windows"), } _copy_file = rule( diff --git a/lib/private/copy_to_bin.bzl b/lib/private/copy_to_bin.bzl index da6f564fc..2ac5a79f0 100644 --- a/lib/private/copy_to_bin.bzl +++ b/lib/private/copy_to_bin.bzl @@ -17,7 +17,7 @@ load("@bazel_skylib//lib:paths.bzl", "paths") load(":copy_file.bzl", "copy_file_action") -def copy_file_to_bin_action(ctx, file, is_windows = False): +def copy_file_to_bin_action(ctx, file, is_windows = None): """Helper function that creates an action to copy a file to the output tree. File are copied to the same workspace-relative path. The resulting files is @@ -29,11 +29,13 @@ def copy_file_to_bin_action(ctx, file, is_windows = False): Args: ctx: The rule context. file: The file to copy. - is_windows: If true, an cmd.exe action is created so there is no bash dependency. + is_windows: Deprecated and unused Returns: A File in the output tree. """ + + # TODO(2.0): remove depcreated & unused is_windows parameter if not file.is_source: return file if ctx.label.workspace_name != file.owner.workspace_name: @@ -89,7 +91,7 @@ target to {file_package} using: package = "%s//%s" % (curr_package_label.workspace_name, curr_package_label.package), ) -def copy_files_to_bin_actions(ctx, files, is_windows = False): +def copy_files_to_bin_actions(ctx, files, is_windows = None): """Helper function that creates actions to copy files to the output tree. Files are copied to the same workspace-relative path. The resulting list of @@ -101,17 +103,17 @@ def copy_files_to_bin_actions(ctx, files, is_windows = False): Args: ctx: The rule context. files: List of File objects. - is_windows: If true, an cmd.exe action is created so there is no bash dependency. + is_windows: Deprecated and unused Returns: List of File objects in the output tree. """ + + # TODO(2.0): remove depcreated & unused is_windows parameter return [copy_file_to_bin_action(ctx, file, is_windows = is_windows) for file in files] def _impl(ctx): - is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]) - - files = copy_files_to_bin_actions(ctx, ctx.files.srcs, is_windows = is_windows) + files = copy_files_to_bin_actions(ctx, ctx.files.srcs) return DefaultInfo( files = depset(files), runfiles = ctx.runfiles(files = files), @@ -122,7 +124,6 @@ _copy_to_bin = rule( provides = [DefaultInfo], attrs = { "srcs": attr.label_list(mandatory = True, allow_files = True), - "_windows_constraint": attr.label(default = "@platforms//os:windows"), }, ) diff --git a/lib/private/copy_to_directory.bzl b/lib/private/copy_to_directory.bzl index b9353c9d4..b6b2bd31e 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") +load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", _is_windows_host = "is_windows_host") load(":paths.bzl", "paths") load(":directory_path.bzl", "DirectoryPathInfo") load(":glob_match.bzl", "glob_match") @@ -279,7 +279,6 @@ _copy_to_directory_attr = { This setting has no effect on Windows where overwrites are always allowed.""", ), - "_windows_constraint": attr.label(default = "@platforms//os:windows"), } def _any_globs_match(exprs, path): @@ -580,11 +579,10 @@ if exist "{src}\\*" ( mnemonic = "CopyToDirectory", progress_message = "Copying files to directory", use_default_shell_env = True, + execution_requirements = _COPY_EXECUTION_REQUIREMENTS, ) def _copy_to_directory_impl(ctx): - is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]) - dst = ctx.actions.declare_directory(ctx.attr.out if ctx.attr.out else ctx.attr.name) copy_to_directory_action( @@ -600,7 +598,6 @@ def _copy_to_directory_impl(ctx): exclude_prefixes = ctx.attr.exclude_prefixes, replace_prefixes = ctx.attr.replace_prefixes, allow_overwrites = ctx.attr.allow_overwrites, - is_windows = is_windows, ) return [ @@ -640,7 +637,7 @@ def copy_to_directory_action( exclude_prefixes = [], replace_prefixes = {}, allow_overwrites = False, - is_windows = False): + is_windows = None): """Helper function to copy files to a directory. This helper is used by copy_to_directory. It is exposed as a public API so it can be used within @@ -689,8 +686,10 @@ def copy_to_directory_action( See copy_to_directory rule documentation for more details. - is_windows: If true, an cmd.exe action is created so there is no bash dependency. + is_windows: Deprecated and unused """ + + # TODO(2.0): remove depcreated & unused is_windows parameter if not srcs: fail("srcs must not be empty") @@ -762,6 +761,10 @@ def copy_to_directory_action( if not copy_paths: fail("There are no files or directories to copy after applying filters. Are your 'include_srcs_patterns' and 'exclude_srcs_patterns' attributes correct?") + # Because copy actions have "local" execution requirements, we can safely assume + # the execution is the same as the host platform and generate different actions for Windows + # and non-Windows host platforms + is_windows = _is_windows_host() if is_windows: _copy_to_dir_cmd(ctx, copy_paths, dst) else: diff --git a/lib/private/local_config_platform.bzl b/lib/private/local_config_platform.bzl new file mode 100644 index 000000000..0f6b0e010 --- /dev/null +++ b/lib/private/local_config_platform.bzl @@ -0,0 +1,49 @@ +"""Work-around for getting a bzl_library for @local_config_platform//:constraints.bzl load + +For internal use only +""" + +load(":repo_utils.bzl", "repo_utils") + +def _impl(rctx): + rctx.file("BUILD.bazel", """load(':constraints.bzl', 'HOST_CONSTRAINTS') +load("@bazel_skylib//:bzl_library.bzl", "bzl_library") + +package(default_visibility = ['//visibility:public']) + +platform(name = 'host', + # Auto-detected host platform constraints. + constraint_values = HOST_CONSTRAINTS, +) + +bzl_library( + name = "constraints", + srcs = ["constraints.bzl"], + visibility = ["//visibility:public"], +) +""") + + # TODO: we can detect the host CPU in the future as well if needed; + # see the repo_utils.platform(rctx) function for an example of this + if repo_utils.is_darwin(rctx): + rctx.file("constraints.bzl", content = """HOST_CONSTRAINTS = [ + '@platforms//cpu:x86_64', + '@platforms//os:osx', +] +""") + elif repo_utils.is_windows(rctx): + rctx.file("constraints.bzl", content = """HOST_CONSTRAINTS = [ + '@platforms//cpu:x86_64', + '@platforms//os:windows', +] +""") + else: + rctx.file("constraints.bzl", content = """HOST_CONSTRAINTS = [ + '@platforms//cpu:x86_64', + '@platforms//os:linux', +] +""") + +local_config_platform = repository_rule( + implementation = _impl, +) diff --git a/lib/tests/copy_to_directory_action/pkg.bzl b/lib/tests/copy_to_directory_action/pkg.bzl index 413f146da..906ca5337 100644 --- a/lib/tests/copy_to_directory_action/pkg.bzl +++ b/lib/tests/copy_to_directory_action/pkg.bzl @@ -8,12 +8,9 @@ load(":other_info.bzl", "OtherInfo") _attrs = { "srcs": attr.label_list(allow_files = True), "out": attr.string(mandatory = True), - "_windows_constraint": attr.label(default = "@platforms//os:windows"), } def _impl(ctx): - is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]) - dst = ctx.actions.declare_directory(ctx.attr.out) additional_files_depsets = [] @@ -27,8 +24,7 @@ def _impl(ctx): ctx, srcs = ctx.attr.srcs, dst = dst, - additional_files = depset(transitive = additional_files_depsets).to_list(), - is_windows = is_windows, + additional_files = depset(transitive = additional_files_depsets), ) return [