Allow sandboxing for copy_* and fix copy_directory tests (#392)
After some thought, I have to say that forcing a local strategy for copy_file/copy_directory is inappropriate. The point of a sandbox is to catch hermeticity bugs; disabling the sandbox may be useful for performance, but it's up to the user to do it if they trust us - and they can do it via flag. The default should be paranoia and safety. And on the subject of strategies - using a genrule to create an empty directory fails in environments where genrules run remote by default (and thus, copy_directory tests fail). We could, of course, set local=1, but that disables the sandbox and causes scary warnings. Instead, add a proper empty_directory rule to test with.
This commit is contained in:
parent
42abf5cbf2
commit
908bf1431d
|
@ -22,10 +22,6 @@ COPY_EXECUTION_REQUIREMENTS = {
|
||||||
# ----------------+-----------------------------------------------------------------------------
|
# ----------------+-----------------------------------------------------------------------------
|
||||||
# no-cache | Results in the action or test never being cached (remotely or locally)
|
# no-cache | Results in the action or test never being cached (remotely or locally)
|
||||||
# ----------------+-----------------------------------------------------------------------------
|
# ----------------+-----------------------------------------------------------------------------
|
||||||
# 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.build/reference/be/common-definitions#common-attributes
|
# See https://bazel.build/reference/be/common-definitions#common-attributes
|
||||||
#
|
#
|
||||||
# Copying file & directories is entirely IO-bound and there is no point doing this work
|
# Copying file & directories is entirely IO-bound and there is no point doing this work
|
||||||
|
@ -44,11 +40,6 @@ COPY_EXECUTION_REQUIREMENTS = {
|
||||||
# disk cache stores the directory artifact as a single entry, but the slight performance bump
|
# 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
|
# comes at the cost of heavy disk cache usage, which is an unmanaged directory that grow beyond
|
||||||
# the bounds of the physical disk.
|
# the bounds of the physical disk.
|
||||||
#
|
|
||||||
# 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": "1",
|
||||||
"no-cache": "1",
|
"no-cache": "1",
|
||||||
"local": "1",
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -1,6 +1,7 @@
|
||||||
# This package aids testing the 'copy_directory' rule.
|
# This package aids testing the 'copy_directory' rule.
|
||||||
|
|
||||||
load("//rules:copy_directory.bzl", "copy_directory")
|
load("//rules:copy_directory.bzl", "copy_directory")
|
||||||
|
load(":empty_directory.bzl", "empty_directory")
|
||||||
|
|
||||||
licenses(["notice"])
|
licenses(["notice"])
|
||||||
|
|
||||||
|
@ -13,10 +14,8 @@ copy_directory(
|
||||||
out = "dir_copy",
|
out = "dir_copy",
|
||||||
)
|
)
|
||||||
|
|
||||||
genrule(
|
empty_directory(
|
||||||
name = "empty_directory",
|
name = "empty_dir",
|
||||||
outs = ["empty_dir"],
|
|
||||||
cmd = "mkdir $@",
|
|
||||||
)
|
)
|
||||||
|
|
||||||
copy_directory(
|
copy_directory(
|
||||||
|
|
|
@ -0,0 +1,34 @@
|
||||||
|
# Copyright 2022 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.
|
||||||
|
|
||||||
|
"""Creates an empty directory."""
|
||||||
|
|
||||||
|
def _empty_directory_impl(ctx):
|
||||||
|
out = ctx.actions.declare_directory(ctx.attr.name)
|
||||||
|
ctx.actions.run_shell(
|
||||||
|
outputs = [out],
|
||||||
|
command = "mkdir -p $@",
|
||||||
|
arguments = [out.path],
|
||||||
|
mnemonic = "EmptyDirectory",
|
||||||
|
progress_message = "Creating empty directory %s" % out.path,
|
||||||
|
use_default_shell_env = True,
|
||||||
|
execution_requirements = {"no-remote": "1"}, # see rules/private/copy_directory_private.bzl
|
||||||
|
)
|
||||||
|
return [DefaultInfo(files = depset(direct = [out]))]
|
||||||
|
|
||||||
|
empty_directory = rule(
|
||||||
|
implementation = _empty_directory_impl,
|
||||||
|
provides = [DefaultInfo],
|
||||||
|
doc = "Creates an empty directory with the same name as the target",
|
||||||
|
)
|
Loading…
Reference in New Issue