Skip to content

Commit

Permalink
fix: fix host platform detection on all copy actions which always run…
Browse files Browse the repository at this point in the history
… locally (#241)
  • Loading branch information
gregmagolan authored Sep 13, 2022
1 parent aa42a92 commit bbfb74c
Show file tree
Hide file tree
Showing 13 changed files with 114 additions and 41 deletions.
7 changes: 7 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)
2 changes: 1 addition & 1 deletion docs/copy_directory.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion docs/copy_file.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions docs/copy_to_bin.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion docs/copy_to_directory.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion lib/private/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 5 additions & 0 deletions lib/private/copy_common.bzl
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"Helpers for copy rules"

load("@local_config_platform//:constraints.bzl", "HOST_CONSTRAINTS")

# Hints for Bazel spawn strategy
COPY_EXECUTION_REQUIREMENTS = {
# ----------------+-----------------------------------------------------------------------------
Expand Down Expand Up @@ -57,3 +59,6 @@ COPY_EXECUTION_REQUIREMENTS = {
"no-sandbox": "1",
"local": "1",
}

def is_windows_host():
return "@platforms//os:windows" in HOST_CONSTRAINTS
18 changes: 11 additions & 7 deletions lib/private/copy_directory.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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])
Expand All @@ -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"),
},
)

Expand Down
19 changes: 11 additions & 8 deletions lib/private/copy_file.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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")
Expand All @@ -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])
Expand All @@ -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(
Expand Down
17 changes: 9 additions & 8 deletions lib/private/copy_to_bin.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand All @@ -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),
Expand All @@ -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"),
},
)

Expand Down
17 changes: 10 additions & 7 deletions lib/private/copy_to_directory.bzl
Original file line number Diff line number Diff line change
@@ -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")
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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(
Expand All @@ -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 [
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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:
Expand Down
Loading

0 comments on commit bbfb74c

Please sign in to comment.