From 21b43020f52087f92afec978f7362d0fdc2214c0 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:15:39 +0100 Subject: [PATCH 1/8] fix(builtin): improve execution requirements for copy file operations --- internal/pkg_web/pkg_web.bzl | 10 +-- .../bazel-skylib/rules/copy_file.bzl | 2 + .../rules/private/copy_common.bzl | 73 +++++++++++++++++++ .../rules/private/copy_file_private.bzl | 15 +--- 4 files changed, 80 insertions(+), 20 deletions(-) create mode 100644 third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common.bzl diff --git a/internal/pkg_web/pkg_web.bzl b/internal/pkg_web/pkg_web.bzl index dbb2f23091..48bc8d1e71 100644 --- a/internal/pkg_web/pkg_web.bzl +++ b/internal/pkg_web/pkg_web.bzl @@ -15,6 +15,7 @@ """Contains the pkg_web rule. """ +load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_file.bzl", "copy_file", "COPY_EXECUTION_REQUIREMENTS") load("@rules_nodejs//nodejs:providers.bzl", "STAMP_ATTR", "StampSettingInfo") _DOC = """Assembles a web application from source files.""" @@ -42,13 +43,6 @@ See the section on stamping in the README.""", ), } -# Hints for Bazel spawn strategy -_execution_requirements = { - # Copying files is entirely IO-bound and there is no point doing this work - # remotely. - "no-remote-exec": "1", -} - def _move_files(ctx, root_paths): """Moves files into an output directory @@ -81,7 +75,7 @@ def _move_files(ctx, root_paths): outputs = [www_dir], executable = ctx.executable._assembler, arguments = [args], - execution_requirements = _execution_requirements, + execution_requirements = COPY_EXECUTION_REQUIREMENTS, env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, ) return depset([www_dir]) diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl index f3f689c698..a3499164ab 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl @@ -24,6 +24,8 @@ on Windows (no Bash is required). load( "//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_file_private.bzl", _copy_file = "copy_file", + _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", ) copy_file = _copy_file +COPY_EXECUTION_REQUIREMENTS = _COPY_EXECUTION_REQUIREMENTS diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common.bzl new file mode 100644 index 0000000000..588cd9c2be --- /dev/null +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common.bzl @@ -0,0 +1,73 @@ +# Copyright 2019 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. + +"Helpers for copy rules" + +# Hints for Bazel spawn strategy +COPY_EXECUTION_REQUIREMENTS = { + # ----------------+----------------------------------------------------------------------------- + # 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. + # ----------------+----------------------------------------------------------------------------- + # 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", + "no-sandbox": "1", + "local": "1", +} \ No newline at end of file diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl index 273322d06f..dffe07c45a 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl @@ -24,16 +24,7 @@ cmd.exe (on Windows). `_copy_xfile` marks the resulting file executable, `_copy_file` does not. """ -# Hints for Bazel spawn strategy -_execution_requirements = { - # Copying files 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. - # 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": "1", -} +load(":copy_common.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS") def _hash_file(file): return str(hash(file.path)) @@ -81,7 +72,7 @@ def copy_cmd(ctx, src, dst): mnemonic = mnemonic, progress_message = progress_message, use_default_shell_env = True, - execution_requirements = _execution_requirements, + execution_requirements = _COPY_EXECUTION_REQUIREMENTS, ) # buildifier: disable=function-docstring @@ -103,7 +94,7 @@ def copy_bash(ctx, src, dst): mnemonic = mnemonic, progress_message = progress_message, use_default_shell_env = True, - execution_requirements = _execution_requirements, + execution_requirements = _COPY_EXECUTION_REQUIREMENTS, ) def _copy_file_impl(ctx): From 8576f2ebe420f5d8eaf4051f1c80585d4f07b616 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:23:17 +0100 Subject: [PATCH 2/8] fix(builtin): import statements and private/public apis --- internal/pkg_web/pkg_web.bzl | 2 +- .../bazel-skylib/rules/copy_common.bzl | 29 +++++++++++++++++++ .../bazel-skylib/rules/copy_file.bzl | 2 -- ...opy_common.bzl => copy_common_private.bzl} | 0 .../rules/private/copy_file_private.bzl | 2 +- 5 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl rename third_party/github.com/bazelbuild/bazel-skylib/rules/private/{copy_common.bzl => copy_common_private.bzl} (100%) diff --git a/internal/pkg_web/pkg_web.bzl b/internal/pkg_web/pkg_web.bzl index 48bc8d1e71..b6d040c1c2 100644 --- a/internal/pkg_web/pkg_web.bzl +++ b/internal/pkg_web/pkg_web.bzl @@ -15,7 +15,7 @@ """Contains the pkg_web rule. """ -load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_file.bzl", "copy_file", "COPY_EXECUTION_REQUIREMENTS") +load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") load("@rules_nodejs//nodejs:providers.bzl", "STAMP_ATTR", "StampSettingInfo") _DOC = """Assembles a web application from source files.""" diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl new file mode 100644 index 0000000000..263856154e --- /dev/null +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl @@ -0,0 +1,29 @@ +# Copyright 2019 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. + +"""A rule that copies a file to another place. + +native.genrule() is sometimes used to copy files (often wishing to rename them). +The 'copy_file' rule does this with a simpler interface than genrule. + +The rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command +on Windows (no Bash is required). +""" + +load( + "//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_common_private.bzl", + _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", +) + +COPY_EXECUTION_REQUIREMENTS = _COPY_EXECUTION_REQUIREMENTS diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl index a3499164ab..f3f689c698 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_file.bzl @@ -24,8 +24,6 @@ on Windows (no Bash is required). load( "//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_file_private.bzl", _copy_file = "copy_file", - _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS", ) copy_file = _copy_file -COPY_EXECUTION_REQUIREMENTS = _COPY_EXECUTION_REQUIREMENTS diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common_private.bzl similarity index 100% rename from third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common.bzl rename to third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common_private.bzl diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl index dffe07c45a..7793c214ac 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.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_private.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS") def _hash_file(file): return str(hash(file.path)) From 3d25cb0db21280f425eb077a3800e5c240dab7dd Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:32:56 +0100 Subject: [PATCH 3/8] chore(builtin): test removing wrong import --- internal/pkg_web/pkg_web.bzl | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/pkg_web/pkg_web.bzl b/internal/pkg_web/pkg_web.bzl index b6d040c1c2..36d025bf4c 100644 --- a/internal/pkg_web/pkg_web.bzl +++ b/internal/pkg_web/pkg_web.bzl @@ -15,9 +15,16 @@ """Contains the pkg_web rule. """ -load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") +# load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") load("@rules_nodejs//nodejs:providers.bzl", "STAMP_ATTR", "StampSettingInfo") +# Hints for Bazel spawn strategy +_execution_requirements = { + # Copying files is entirely IO-bound and there is no point doing this work + # remotely. + "no-remote-exec": "1", +} + _DOC = """Assembles a web application from source files.""" _ATTRS = { @@ -75,7 +82,7 @@ def _move_files(ctx, root_paths): outputs = [www_dir], executable = ctx.executable._assembler, arguments = [args], - execution_requirements = COPY_EXECUTION_REQUIREMENTS, + execution_requirements = _execution_requirements, env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, ) return depset([www_dir]) From be8fef133c0f70f963e54d9b1a7bf70a13c1ae9d Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:34:02 +0100 Subject: [PATCH 4/8] Revert "chore(builtin): test removing wrong import" This reverts commit 3d25cb0db21280f425eb077a3800e5c240dab7dd. --- internal/pkg_web/pkg_web.bzl | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/internal/pkg_web/pkg_web.bzl b/internal/pkg_web/pkg_web.bzl index 36d025bf4c..b6d040c1c2 100644 --- a/internal/pkg_web/pkg_web.bzl +++ b/internal/pkg_web/pkg_web.bzl @@ -15,16 +15,9 @@ """Contains the pkg_web rule. """ -# load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") +load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") load("@rules_nodejs//nodejs:providers.bzl", "STAMP_ATTR", "StampSettingInfo") -# Hints for Bazel spawn strategy -_execution_requirements = { - # Copying files is entirely IO-bound and there is no point doing this work - # remotely. - "no-remote-exec": "1", -} - _DOC = """Assembles a web application from source files.""" _ATTRS = { @@ -82,7 +75,7 @@ def _move_files(ctx, root_paths): outputs = [www_dir], executable = ctx.executable._assembler, arguments = [args], - execution_requirements = _execution_requirements, + execution_requirements = COPY_EXECUTION_REQUIREMENTS, env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, ) return depset([www_dir]) From 7581d48b070e5781a9f98afa004bc7a3301d1ca3 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:34:56 +0100 Subject: [PATCH 5/8] fix(builtin): import statements inside private file --- .../bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl index 7793c214ac..8b19fba214 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_file_private.bzl @@ -24,7 +24,7 @@ cmd.exe (on Windows). `_copy_xfile` marks the resulting file executable, `_copy_file` does not. """ -load(":copy_common_private.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS") +load(":rules/private/copy_common_private.bzl", _COPY_EXECUTION_REQUIREMENTS = "COPY_EXECUTION_REQUIREMENTS") def _hash_file(file): return str(hash(file.path)) From 77d8013786fc027409fe8dd15a2229bb3c843cc6 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:38:22 +0100 Subject: [PATCH 6/8] chore(builtin): use buildifier on modified file --- .../bazel-skylib/rules/private/copy_common_private.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common_private.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common_private.bzl index 588cd9c2be..189591c384 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common_private.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/private/copy_common_private.bzl @@ -70,4 +70,4 @@ COPY_EXECUTION_REQUIREMENTS = { "no-cache": "1", "no-sandbox": "1", "local": "1", -} \ No newline at end of file +} From 3f0a6ec7a40d4109c4d03bd660f9d96a6de4766d Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 15 Apr 2022 17:52:39 +0100 Subject: [PATCH 7/8] fix(builtin): internal import path for pkg_web --- internal/pkg_web/pkg_web.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg_web/pkg_web.bzl b/internal/pkg_web/pkg_web.bzl index b6d040c1c2..e590d19978 100644 --- a/internal/pkg_web/pkg_web.bzl +++ b/internal/pkg_web/pkg_web.bzl @@ -15,7 +15,7 @@ """Contains the pkg_web rule. """ -load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") +load("//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_common.bzl", "COPY_EXECUTION_REQUIREMENTS") load("@rules_nodejs//nodejs:providers.bzl", "STAMP_ATTR", "StampSettingInfo") _DOC = """Assembles a web application from source files.""" From fd7daf6cfd68ca74876d5f56b88c90eb75a71037 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Mon, 18 Apr 2022 16:23:40 +0100 Subject: [PATCH 8/8] docs(builtin): updates header docs for coppy_common --- .../bazelbuild/bazel-skylib/rules/copy_common.bzl | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl index 263856154e..d3fd46e63c 100644 --- a/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl +++ b/third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl @@ -12,14 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""A rule that copies a file to another place. - -native.genrule() is sometimes used to copy files (often wishing to rename them). -The 'copy_file' rule does this with a simpler interface than genrule. - -The rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command -on Windows (no Bash is required). -""" +"""Exposes common helpers to help on copying files rules""" load( "//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_common_private.bzl",