From 7009f1dfe4229d489f888992e7f10e2c0e1504a0 Mon Sep 17 00:00:00 2001 From: Noah S-C Date: Mon, 27 May 2024 17:53:51 +0100 Subject: [PATCH] bazel: add utility macro for wrapping single-file tools (#62930) Currently, we provide single-file tools such as `ctags`, `gsutil` etc via an `sh_binary` wrapper, to have a single target to reference that automatically does platform selection of the underlying tool. Due to some [unfortunate reason](https://github.com/bazelbuild/bazel/issues/11820), the underlying srcs (which is [a single file](https://bazel.build/reference/be/shell#sh_binary.srcs)) of an `sh_binary` are also exposed as outputs (rather than just as typical runfiles) alongside the script that wraps. This is _sometimes_ problematic when doing location expansion (e.g. `$(location ...)`) due to these only allowing a single output (dont ask why this works in some contexts but not others, I dont know). To address this, we create a wrapper macro + rule to replicate what we want from `sh_binary` (automatic platform selection + tool naming), while only exposing a singular file. See example of currently required approach to consuming a tool: [BUILD.bazel](https://github.com/sourcegraph/sourcegraph/pull/62801/files#diff-e2a562c2e13908933b2ee24f0ac596829b38a5325cc69a4aee05c383aaa2e494R8) & [main_test.go](https://github.com/sourcegraph/sourcegraph/pull/62801/files#diff-7a91cb5143064bfc8993ef97baf68b718ef49747ccc1d3c5e1150d4696b88305R66). With this change, `rlocationpath` (singular) can be used instead (or any of the other singular nouns in different contexts), as well as no `strings.Split/strings.Fields` being required ## Test plan `bazel cquery --output=files //dev/tools:dropdb` yields 1 vs 2 files. Also updated the rule behind `//internal/database:generate_schemas` due to the workaround in it for the fact that the underlying srcs was also exposed. The correctness is verified by running said target (locally + CI) --- dev/migrations.bzl | 15 +++---------- dev/tools/BUILD.bazel | 52 ++++++++++++++++++------------------------- dev/tools/tool.bzl | 47 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 42 deletions(-) create mode 100644 dev/tools/tool.bzl diff --git a/dev/migrations.bzl b/dev/migrations.bzl index 2744a746fd4ce..95fb49ad8c8f3 100644 --- a/dev/migrations.bzl +++ b/dev/migrations.bzl @@ -41,20 +41,12 @@ fi # Dumping the schema requires running the squash operation first, as it reuses the database, so we do all of those operations # in a single step. def _generate_schemas_impl(ctx): - # for every entry in pgutils filegroup, there's two files: - # - one in external e.g. external/createdb-linux-amd64/file/downloaded - # - one in output base e.g. bazel-out/k8-opt-exec-ST-13d3ddad9198/bin/dev/tools/createdb - # only the one in output base can be picked up by-name in PATH, so we need to filter out the ones in external. - pgutils_path = ":".join([ - f.path.rpartition("/")[0] - for f in ctx.attr._pg_utils[DefaultInfo].default_runfiles.files.to_list() - if not f.path.startswith("external") - ]) - + pgutils_path = ctx.attr._pg_utils[DefaultInfo].files.to_list()[0].path.rpartition("/")[0] runfiles = depset(direct = ctx.attr._sg[DefaultInfo].default_runfiles.files.to_list() + ctx.attr._pg_utils[DefaultInfo].default_runfiles.files.to_list()) ctx.actions.run_shell( inputs = ctx.files.srcs, + tools = runfiles, outputs = [ ctx.outputs.out_frontend_squash, ctx.outputs.out_codeintel_squash, @@ -78,7 +70,7 @@ def _generate_schemas_impl(ctx): }, command = """{cmd_preamble} - export PATH="{pgutils_path}:$PATH" + export PATH="{pgutils_path}" trap "dropdb --if-exists sg-squasher-frontend && echo 'temp db sg-squasher-frontend dropped'" EXIT trap "dropdb --if-exists sg-squasher-codeintel && echo 'temp db sg-squasher-codeintel dropped'" EXIT @@ -110,7 +102,6 @@ def _generate_schemas_impl(ctx): out_codeintel_schema_md = ctx.outputs.out_codeintel_schema_md.path, out_codeinsights_schema_md = ctx.outputs.out_codeinsights_schema_md.path, ), - tools = runfiles, ) return [ diff --git a/dev/tools/BUILD.bazel b/dev/tools/BUILD.bazel index 35ada05a878f4..493ffbd9b80e2 100644 --- a/dev/tools/BUILD.bazel +++ b/dev/tools/BUILD.bazel @@ -1,41 +1,39 @@ -sh_binary( +load(":tool.bzl", "tool") + +tool( name = "docsite", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@docsite_darwin_amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_arm64": ["@docsite_darwin_arm64//file:downloaded"], "@bazel_tools//src/conditions:linux_x86_64": ["@docsite_linux_amd64//file:downloaded"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "src-cli", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@src-cli-darwin-amd64//:src-cli-darwin-amd64"], "@bazel_tools//src/conditions:darwin_arm64": ["@src-cli-darwin-arm64//:src-cli-darwin-arm64"], "@bazel_tools//src/conditions:linux_x86_64": ["@src-cli-linux-amd64//:src-cli-linux-amd64"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "universal-ctags", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@universal-ctags-darwin-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_arm64": ["@universal-ctags-darwin-arm64//file:downloaded"], "@bazel_tools//src/conditions:linux_x86_64": ["@universal-ctags-linux-amd64//file:downloaded"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "gcloud", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@gcloud-darwin-amd64//:gcloud"], "@bazel_tools//src/conditions:darwin_arm64": ["@gcloud-darwin-arm64//:gcloud"], "@bazel_tools//src/conditions:linux_x86_64": ["@gcloud-linux-amd64//:gcloud"], }), - visibility = ["//visibility:public"], ) sh_binary( @@ -48,64 +46,58 @@ sh_binary( visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "packer", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@packer-darwin-amd64//:packer-darwin-amd64"], "@bazel_tools//src/conditions:darwin_arm64": ["@packer-darwin-arm64//:packer-darwin-arm64"], "@bazel_tools//src/conditions:linux_x86_64": ["@packer-linux-amd64//:packer-linux-amd64"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "p4-fusion", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@p4-fusion-darwin-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_arm64": ["@p4-fusion-darwin-arm64//file:downloaded"], "@bazel_tools//src/conditions:linux_x86_64": ["@p4-fusion-linux-amd64//file:downloaded"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "gh", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:darwin_x86_64": ["@gh_darwin-amd64//:gh"], "@bazel_tools//src/conditions:darwin_arm64": ["@gh_darwin-arm64//:gh"], "@bazel_tools//src/conditions:linux_x86_64": ["@gh_linux-amd64//:gh"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "pg_dump", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:linux_x86_64": ["@pg_dump-linux-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_x86_64": ["@pg_dump-darwin-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_arm64": ["@pg_dump-darwin-arm64//file:downloaded"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "createdb", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:linux_x86_64": ["@createdb-linux-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_x86_64": ["@createdb-darwin-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_arm64": ["@createdb-darwin-arm64//file:downloaded"], }), - visibility = ["//visibility:public"], ) -sh_binary( +tool( name = "dropdb", - srcs = select({ + src = select({ "@bazel_tools//src/conditions:linux_x86_64": ["@dropdb-linux-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_x86_64": ["@dropdb-darwin-amd64//file:downloaded"], "@bazel_tools//src/conditions:darwin_arm64": ["@dropdb-darwin-arm64//file:downloaded"], }), - visibility = ["//visibility:public"], ) alias( diff --git a/dev/tools/tool.bzl b/dev/tools/tool.bzl new file mode 100644 index 0000000000000..f0cfd8f24a1cb --- /dev/null +++ b/dev/tools/tool.bzl @@ -0,0 +1,47 @@ +load("@aspect_bazel_lib//lib:output_files.bzl", "make_output_files") + +# Convenience wrapper for rules that provide a single executable (binary/script/etc) that +# we want to rename. For example, files produced by http_file have the name `downloaded`, +# we may want to rename them with sh_binary, however this has a weird quirk whereby both +# the sh_binary wrapper "script" and the input binary/script itself are included in the outputs. +# This results in some less ergonomic usage when trying to use them in e.g. go tests, having to +# use $(rlocationpaths ...) and then filepath.Dir(runfiles.Rlocation(strings.Split(..., " ")[0])) in +# order to get the path. +# With this macro, its slightly simplified by being able to use $(rlocationpath ...) (singular) and +# not having to strings.Split(..., " ") as a result. +# See more: https://github.com/bazelbuild/bazel/issues/11820 +def tool(name, src, visibility = ["//visibility:public"]): + native.sh_binary( + name = name + "_sh", + srcs = src, + ) + make_bin_and_deps_available( + name = name, + out = name, + data = make_output_files( + name = name + "_out", + target = name + "_sh", + paths = [native.package_name() + "/" + name + "_sh"], + ), + visibility = visibility, + ) + +def _make_bin_and_deps_available_impl(ctx): + symlink = ctx.actions.declare_file(ctx.attr.out) + ctx.actions.symlink(output = symlink, target_file = ctx.file.data) + return [ + DefaultInfo( + executable = symlink, + files = depset(direct = [symlink]), + runfiles = ctx.runfiles(files = ctx.files.data), + ), + ] + +make_bin_and_deps_available = rule( + _make_bin_and_deps_available_impl, + executable = True, + attrs = { + "out": attr.string(mandatory = True), + "data": attr.label(mandatory = True, allow_single_file = True), + }, +)