From d0459bb83a781f989aff29ced782a9fb5745160a Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Sun, 10 Oct 2021 15:47:56 -0700 Subject: [PATCH] feat(core): patch bazel-skylib; core can use npm https://github.com/bazelbuild/bazel-skylib/pull/323 is patched into our vendored copy of copy_file rule. That lets an action run with minimal inputs. This is obviously not ergonomic for users yet, just a step in the right direction. --- e2e/core/BUILD.bazel | 90 ++++++++++++- e2e/core/WORKSPACE | 21 +++ .../rules/private/copy_file_private.bzl | 125 +++++++++++++----- 3 files changed, 203 insertions(+), 33 deletions(-) diff --git a/e2e/core/BUILD.bazel b/e2e/core/BUILD.bazel index 6b991ce5dd..b6852f4ba3 100644 --- a/e2e/core/BUILD.bazel +++ b/e2e/core/BUILD.bazel @@ -46,7 +46,7 @@ write_file( genrule( name = "use_node_toolchain", srcs = ["some.js"], - outs = ["genrule_out"], + outs = ["actual1"], cmd = "$(NODE_PATH) $(execpath some.js) $@", toolchains = ["@node16_toolchains//:resolved_toolchain"], tools = ["@node16_toolchains//:resolved_toolchain"], @@ -55,7 +55,7 @@ genrule( diff_test( name = "test_genrule", file1 = "expected", - file2 = "genrule_out", + file2 = "actual1", ) # Here, my_nodejs is a fake for something like nodejs_binary or @@ -72,3 +72,89 @@ diff_test( file1 = "expected", file2 = "thing", ) + +########################################################## +# Call a program from npm to transform inputs to bazel-out + +# For using acorn as our test fixture, this is +# the serialized AST for the JS program just containing a literal "1" +write_file( + name = "write_expected_ast", + out = "expected_ast.json", + content = [ + """{"type":"Program","start":0,"end":1,"body":[{"type":"ExpressionStatement","start":0,"end":1,"expression":{"type":"Literal","start":0,"end":1,"value":1,"raw":"1"}}],"sourceType":"script"}""", + "", + ], +) + +write_file( + name = "write_one", + out = "one.js", + content = ["1"], +) + +genrule( + name = "call_acorn", + srcs = ["one.js"], + outs = ["actual2"], + cmd = """ + $(NODE_PATH) \\ + ./$(execpath @npm_acorn-8.5.0)/bin/acorn \\ + --compact \\ + $(execpath one.js) \\ + > $@""", + toolchains = ["@node16_toolchains//:resolved_toolchain"], + tools = [ + "@node16_toolchains//:resolved_toolchain", + "@npm_acorn-8.5.0", + ], +) + +diff_test( + name = "test_acorn", + file1 = "actual2", + file2 = "expected_ast.json", +) + +################################################ +# Run a program that requires a package from npm + +write_file( + name = "write_program", + out = "require_acorn.js", + content = [ + "const fs = require('fs')", + "const acorn = require('acorn')", + "fs.writeFileSync(process.argv[2], JSON.stringify(acorn.parse('1', {ecmaVersion: 2020})) + '\\n')", + ], +) + +genrule( + name = "require_acorn", + srcs = ["require_acorn.js"], + outs = ["actual3"], + # Note: confusingly, node uses an environment variable NODE_PATH as a "global" + # location for module resolutions, but we used the same name for the Make + # variable exposed by the nodejs tooling. + # One is interpreted by the bash shell, while the other is interpreted by + # bazel, so it doesn't cause any problems. + # Note, the trailing "/.." on the NODE_PATH variable is because our target + # points to the output directory we wrote, named "acorn", but node needs + # to start its module search in a directory *containing" one called "acorn" + cmd = """ + NODE_PATH=./$(execpath @npm_acorn-8.5.0)/.. \\ + $(NODE_PATH) \\ + ./$(execpath require_acorn.js) \\ + $@""", + toolchains = ["@node16_toolchains//:resolved_toolchain"], + tools = [ + "@node16_toolchains//:resolved_toolchain", + "@npm_acorn-8.5.0", + ], +) + +diff_test( + name = "test_require_acorn", + file1 = "actual3", + file2 = "expected_ast.json", +) diff --git a/e2e/core/WORKSPACE b/e2e/core/WORKSPACE index 187fb4bbee..b3dcffc680 100644 --- a/e2e/core/WORKSPACE +++ b/e2e/core/WORKSPACE @@ -17,3 +17,24 @@ nodejs_register_toolchains( name = "node16", node_version = "16.9.0", ) + +http_archive( + name = "npm_acorn-8.5.0", + build_file_content = """ +load("@rules_nodejs//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_file.bzl", "copy_file") + +# Turn a source directory into a TreeArtifact for RBE-compat +copy_file( + name = "npm_acorn-8.5.0", + src = "package", + # This attribute comes from rules_nodejs patch of + # https://github.com/bazelbuild/bazel-skylib/pull/323 + is_directory = True, + # We must give this as the directory in order for it to appear on NODE_PATH + out = "acorn", + visibility = ["//visibility:public"], +) +""", + sha256 = "d8f9d40c4656537a60bf0c6daae6f0553f54df5ff2518f86464b7c785f20376b", + urls = ["https://registry.npmjs.org/acorn/-/acorn-8.5.0.tgz"], +) 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 63b86ff874..307242d881 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 @@ -12,38 +12,58 @@ # See the License for the specific language governing permissions and # limitations under the License. +# LOCAL MODIFICATIONS: +# this has two PRs patched in: +# https://github.com/bazelbuild/bazel-skylib/pull/323 +# https://github.com/bazelbuild/bazel-skylib/pull/324 + """Implementation of copy_file macro and underlying rules. -These rules copy a file to another location using Bash (on Linux/macOS) or -cmd.exe (on Windows). '_copy_xfile' marks the resulting file executable, -'_copy_file' does not. +These rules copy a file or directory to another location using Bash (on Linux/macOS) or +cmd.exe (on Windows). `_copy_xfile` marks the resulting file executable, +`_copy_file` does not. """ -# BEGIN LOCAL MOD +# 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-exec": "1", +} + def _hash_file(file): return str(hash(file.path)) -# END LOCAL MOD - +# buildifier: disable=function-docstring def copy_cmd(ctx, src, dst): # 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. # To fix that we write the command to a .bat file so no command line # quoting or escaping is required. - - # BEGIN LOCAL MOD # Put a hash of the file name into the name of the generated batch file to - # make it unique within the pacakge since rules such as copy_to_bin and - # js_library will use copy_cmd multiple times for multiple files. + # make it unique within the package, so that users can define multiple copy_file's. bat = ctx.actions.declare_file("%s-%s-cmd.bat" % (ctx.label.name, _hash_file(src))) - # END LOCAL MOD + + # Flags are documented at + # https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/copy + # https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/xcopy + if dst.is_directory: + cmd_tmpl = "@xcopy \"%s\" \"%s\\\" /V /E /H /Y /Q >NUL" + mnemonic = "CopyDirectory" + progress_message = "Copying directory" + else: + cmd_tmpl = "@copy /Y \"%s\" \"%s\" >NUL" + mnemonic = "CopyFile" + progress_message = "Copying file" ctx.actions.write( output = bat, # Do not use lib/shell.bzl's shell.quote() method, because that uses # Bash quoting syntax, which is different from cmd.exe's syntax. - content = "@copy /Y \"%s\" \"%s\" >NUL" % ( + content = cmd_tmpl % ( src.path.replace("/", "\\"), dst.path.replace("/", "\\"), ), @@ -55,75 +75,116 @@ def copy_cmd(ctx, src, dst): outputs = [dst], executable = "cmd.exe", arguments = ["/C", bat.path.replace("/", "\\")], - mnemonic = "CopyFile", - progress_message = "Copying files", + mnemonic = mnemonic, + progress_message = progress_message, use_default_shell_env = True, + execution_requirements = _execution_requirements, ) +# buildifier: disable=function-docstring def copy_bash(ctx, src, dst): + if dst.is_directory: + cmd_tmpl = "rm -rf \"$2\" && cp -rf \"$1/\" \"$2\"" + mnemonic = "CopyDirectory" + progress_message = "Copying directory" + else: + cmd_tmpl = "cp -f \"$1\" \"$2\"" + mnemonic = "CopyFile" + progress_message = "Copying file" + ctx.actions.run_shell( tools = [src], outputs = [dst], - command = "cp -f \"$1\" \"$2\"", + command = cmd_tmpl, arguments = [src.path, dst.path], - mnemonic = "CopyFile", - progress_message = "Copying files", + mnemonic = mnemonic, + progress_message = progress_message, use_default_shell_env = True, + execution_requirements = _execution_requirements, ) def _copy_file_impl(ctx): + # When creating a directory, declare that to Bazel so downstream rules + # see it as a TreeArtifact and handle correctly, e.g. for remote execution + if getattr(ctx.attr, "is_directory", False): + output = ctx.actions.declare_directory(ctx.attr.out) + else: + output = ctx.outputs.out if ctx.attr.allow_symlink: + if output.is_directory: + fail("Cannot use both is_directory and allow_symlink") ctx.actions.symlink( - output = ctx.outputs.out, + output = output, target_file = ctx.file.src, is_executable = ctx.attr.is_executable, ) elif ctx.attr.is_windows: - copy_cmd(ctx, ctx.file.src, ctx.outputs.out) + copy_cmd(ctx, ctx.file.src, output) else: - copy_bash(ctx, ctx.file.src, ctx.outputs.out) + copy_bash(ctx, ctx.file.src, output) - files = depset(direct = [ctx.outputs.out]) - runfiles = ctx.runfiles(files = [ctx.outputs.out]) + files = depset(direct = [output]) + runfiles = ctx.runfiles(files = [output]) if ctx.attr.is_executable: - return [DefaultInfo(files = files, runfiles = runfiles, executable = ctx.outputs.out)] + return [DefaultInfo(files = files, runfiles = runfiles, executable = output)] else: return [DefaultInfo(files = files, runfiles = runfiles)] -# @unsorted-dict-items _ATTRS = { "src": attr.label(mandatory = True, allow_single_file = True), - "out": attr.output(mandatory = True), "is_windows": attr.bool(mandatory = True), "is_executable": attr.bool(mandatory = True), "allow_symlink": attr.bool(mandatory = True), } +_copy_directory = rule( + implementation = _copy_file_impl, + provides = [DefaultInfo], + attrs = dict(_ATTRS, **{ + "is_directory": attr.bool(default = True), + # Cannot declare out as an output here, because there's no API for declaring + # TreeArtifact outputs. + "out": attr.string(mandatory = True), + }), +) + _copy_file = rule( implementation = _copy_file_impl, provides = [DefaultInfo], - attrs = _ATTRS, + attrs = dict(_ATTRS, **{ + "out": attr.output(mandatory = True), + }), ) _copy_xfile = rule( implementation = _copy_file_impl, executable = True, provides = [DefaultInfo], - attrs = _ATTRS, + attrs = dict(_ATTRS, **{ + "out": attr.output(mandatory = True), + }), ) -def copy_file(name, src, out, is_executable = False, allow_symlink = False, **kwargs): - """Copies a file to another location. +def copy_file(name, src, out, is_directory = False, is_executable = False, allow_symlink = False, **kwargs): + """Copies a file or directory to another location. `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. This rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command on Windows (no Bash is required). + If using this rule with source directories, it is recommended that you use the + `--host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1` startup option so that changes + to files within source directories are detected. See + https://github.com/bazelbuild/bazel/commit/c64421bc35214f0414e4f4226cc953e8c55fa0d2 + for more context. + Args: name: Name of the rule. - src: A Label. The file to make a copy of. (Can also be the label of a rule - that generates a file.) + src: A Label. The file or directory to make a copy of. + (Can also be the label of a rule that generates a file or directory.) out: Path of the output file, relative to this package. + is_directory: treat the source file as a directory + Workaround for https://github.com/bazelbuild/bazel/issues/12954 is_executable: A boolean. Whether to make the output file executable. When True, the rule's output can be executed using `bazel run` and can be in the srcs of binary and test rules that require executable sources. @@ -140,6 +201,8 @@ def copy_file(name, src, out, is_executable = False, allow_symlink = False, **kw copy_file_impl = _copy_file if is_executable: copy_file_impl = _copy_xfile + elif is_directory: + copy_file_impl = _copy_directory copy_file_impl( name = name,