From 25bbc6d9d2d724b1184d292aa82745e76464d4f5 Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 08:48:33 +0200 Subject: [PATCH 1/7] ci: adding a buildifier job Adding a CI job that will help us keep the starlark files tidy --- .github/workflows/buildifier.yaml | 39 +++++++++++++++++++++++++++++++ BUILD.bazel | 11 +++++++++ Makefile | 2 +- WORKSPACE | 4 ++-- 4 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/buildifier.yaml diff --git a/.github/workflows/buildifier.yaml b/.github/workflows/buildifier.yaml new file mode 100644 index 0000000..9d1cd99 --- /dev/null +++ b/.github/workflows/buildifier.yaml @@ -0,0 +1,39 @@ +name: Buildifier + +# Controls when the action will run. +on: + # Triggers the workflow on push or pull request events but only for the main branch + push: + branches: [main] + pull_request: + branches: [main] + # Allows you to run this workflow manually from the Actions tab + workflow_dispatch: + +concurrency: + # Cancel previous actions from the same PR or branch except 'main' branch. + # See https://docs.github.com/en/actions/using-jobs/using-concurrency and https://docs.github.com/en/actions/learn-github-actions/contexts for more info. + group: concurrency-group::${{ github.workflow }}::${{ github.event.pull_request.number > 0 && format('pr-{0}', github.event.pull_request.number) || github.ref_name }}${{ github.ref_name == 'main' && format('::{0}', github.run_id) || ''}} + cancel-in-progress: ${{ github.ref_name != 'main' }} + + +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: bazel-contrib/setup-bazel@0.8.5 + with: + # Avoid downloading Bazel every time. + bazelisk-cache: true + # enable a disk cache + disk-cache: true + # Share repository cache between workflows. + repository-cache: true + bazelrc: | + import %workspace%/.aspect/bazelrc/ci.bazelrc + import %workspace%/.github/workflows/ci.bazelrc + # keep a cache for MODULE.bazel repos + external-cache: true + - name: buildifier + run: bazel run //:buildifier.check diff --git a/BUILD.bazel b/BUILD.bazel index dac6aa5..f78ec9d 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -12,6 +12,17 @@ buildifier( name = "buildifier", ) +buildifier( + name = "buildifier.check", + diff_command = "diff -u", + exclude_patterns = [ + "./.git/*", + "./**/testdata/*", + ], + lint_mode = "warn", + mode = "diff", +) + bazeldnf( name = "bazeldnf", ) diff --git a/Makefile b/Makefile index 64c51e9..e5a250d 100644 --- a/Makefile +++ b/Makefile @@ -11,7 +11,7 @@ test: gazelle e2e bazelisk build //... && bazelisk test //... buildifier: - bazelisk run //:buildifier + bazelisk run //:buildifier.check gofmt: gofmt -w pkg/.. cmd/.. diff --git a/WORKSPACE b/WORKSPACE index e270fbf..3a97edf 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -46,10 +46,10 @@ http_archive( ], ) -load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies") +load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") load("//:build_deps.bzl", "bazeldnf_build_dependencies") -load("//bazeldnf:deps.bzl", "bazeldnf_dependencies", "rpm") +load("//bazeldnf:deps.bzl", "bazeldnf_dependencies") # gazelle:repository_macro build_deps.bzl%bazeldnf_build_dependencies bazeldnf_build_dependencies() From 9b36db6bc1c862d6929d6d128c3695708a4e58ce Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 08:54:22 +0200 Subject: [PATCH 2/7] buildifier: document modules now most modules and methods are documented --- bazeldnf/toolchain.bzl | 2 ++ build_deps.bzl | 3 +++ internal/bazeldnf.bzl | 2 ++ internal/rpm.bzl | 2 ++ internal/rpmtree.bzl | 16 ++++++++++++++++ internal/xattrs.bzl | 2 ++ 6 files changed, 27 insertions(+) diff --git a/bazeldnf/toolchain.bzl b/bazeldnf/toolchain.bzl index 63a9854..1d85fdb 100644 --- a/bazeldnf/toolchain.bzl +++ b/bazeldnf/toolchain.bzl @@ -1,3 +1,5 @@ +"Wraps bazeldnf tool binary through a toolchain" + def _bazeldnf_toolchain(ctx): return [ platform_common.ToolchainInfo( diff --git a/build_deps.bzl b/build_deps.bzl index 8c87269..6f3e01a 100644 --- a/build_deps.bzl +++ b/build_deps.bzl @@ -1,9 +1,12 @@ +"Provides WORKSPACE build dependencies to build the bazeldnf binary" + load( "@bazel_gazelle//:deps.bzl", _go_repository = "go_repository", ) def bazeldnf_build_dependencies(): + "dependencies to build the bazeldnf binary" _maybe( build_external = "external", name = "co_honnef_go_tools", diff --git a/internal/bazeldnf.bzl b/internal/bazeldnf.bzl index 5cfb51a..ae49b2c 100644 --- a/internal/bazeldnf.bzl +++ b/internal/bazeldnf.bzl @@ -1,3 +1,5 @@ +"Provides a wrapper to run bazeldnf as a run target" + load("@bazel_skylib//lib:shell.bzl", "shell") load("//bazeldnf:toolchain.bzl", "BAZELDNF_TOOLCHAIN") diff --git a/internal/rpm.bzl b/internal/rpm.bzl index 42a0ca1..4a6d0fb 100644 --- a/internal/rpm.bzl +++ b/internal/rpm.bzl @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +"Exposes rpm files to a Bazel workspace" + load("@bazel_tools//tools/build_defs/repo:utils.bzl", "update_attrs") _HTTP_FILE_BUILD = """ diff --git a/internal/rpmtree.bzl b/internal/rpmtree.bzl index d020008..bd4fdd2 100644 --- a/internal/rpmtree.bzl +++ b/internal/rpmtree.bzl @@ -12,6 +12,13 @@ # See the License for the specific language governing permissions and # limitations under the License. +"""Provide helpers to convert rpm files into a single tar file + +This file exposes rpmtree and tar2files to convert a group of +rpm files into either a .tar or extract files from that tar to +make available to bazel +""" + load("//bazeldnf:toolchain.bzl", "BAZELDNF_TOOLCHAIN") def _rpm2tar_impl(ctx): @@ -98,6 +105,7 @@ _tar2files = rule( ) def rpmtree(**kwargs): + """Creates a tar file from a list of rpm files.""" kwargs.pop("files", None) basename = kwargs["name"] kwargs.pop("name", None) @@ -109,6 +117,14 @@ def rpmtree(**kwargs): ) def tar2files(**kwargs): + """Extracts files from a tar file. + + Args: + name: The name of the tar file to be processed. + files: A dictionary where each key-value pair represents a file to be extracted. + If not provided, the function will fail. + **kwargs: Additional keyword arguments to be passed to the _tar2files function. + """ files = kwargs["files"] kwargs.pop("files", None) basename = kwargs["name"] diff --git a/internal/xattrs.bzl b/internal/xattrs.bzl index 23103eb..c439a79 100644 --- a/internal/xattrs.bzl +++ b/internal/xattrs.bzl @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +"Modify xattrs on tar file members" + def _xattrs_impl(ctx): out = ctx.outputs.out args = ["xattr", "--input", ctx.files.tar[0].path, "--output", out.path] From 91f7d944a02f1ff4d2698c4a7ca5fe2d04747cad Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 08:54:53 +0200 Subject: [PATCH 3/7] buildifier: ignore some print statements We have a few cases where we want to warn the user about things, in those cases we want the warning statement to happen --- def.bzl | 3 +++ deps.bzl | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/def.bzl b/def.bzl index 119dd1f..f71dabf 100644 --- a/def.bzl +++ b/def.bzl @@ -1,8 +1,11 @@ +"legacy API" + load( "@bazeldnf//bazeldnf:defs.bzl", _bazeldnf = "bazeldnf", ) def bazeldnf(*args, **kwargs): + # buildifier: disable=print print("import this method from @bazeldnf//bazeldnf:defs.bzl") _bazeldnf(*args, **kwargs) diff --git a/deps.bzl b/deps.bzl index e85cea8..e7b2b38 100644 --- a/deps.bzl +++ b/deps.bzl @@ -2,7 +2,6 @@ load( "@bazeldnf//bazeldnf:defs.bzl", - _rpm = "rpm", _rpmtree = "rpmtree", _tar2files = "tar2files", _xattrs = "xattrs", @@ -10,25 +9,32 @@ load( load( "@bazeldnf//bazeldnf:deps.bzl", _bazeldnf_dependencies = "bazeldnf_dependencies", + _rpm = "rpm", ) def rpm(*args, **kwargs): + # buildifier: disable=print print("import rpm method from @bazeldnf//bazeldnf:defs.bzl") _rpm(*args, **kwargs) def rpmtree(*args, **kwargs): + # buildifier: disable=print print("import rpmtree method from @bazeldnf//bazeldnf:defs.bzl") _rpmtree(*args, **kwargs) def tar2files(*args, **kwargs): + # buildifier: disable=print print("import tar2files method from @bazeldnf//bazeldnf:defs.bzl") _tar2files(*args, **kwargs) def xattrs(*args, **kwargs): + # buildifier: disable=print print("import xattrs method from @bazeldnf//bazeldnf:defs.bzl") _xattrs(*args, **kwargs) def bazeldnf_dependencies(): """Download bazeldnf dependencies""" + + # buildifier: disable=print print("import bazeldnf_dependencies method from @bazeldnf//bazeldnf:deps.bzl") _bazeldnf_dependencies() From 8aa0338fa52ca30addfe6ef1e95aa9bffbdf3240 Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 08:57:03 +0200 Subject: [PATCH 4/7] buildifier: adopt best practices for method args For some reason the linter recomends having a name argument, and also making it explicit which are the required arguments to the methods --- internal/rpm.bzl | 1 - internal/rpmtree.bzl | 42 +++++++++++++++++++----------------------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/internal/rpm.bzl b/internal/rpm.bzl index 4a6d0fb..429d519 100644 --- a/internal/rpm.bzl +++ b/internal/rpm.bzl @@ -27,7 +27,6 @@ filegroup( def _rpm_impl(ctx): if ctx.attr.urls: downloaded_file_path = "downloaded" - download_path = ctx.path("rpm/" + downloaded_file_path) download_info = ctx.download( url = ctx.attr.urls, output = "rpm/" + downloaded_file_path, diff --git a/internal/rpmtree.bzl b/internal/rpmtree.bzl index bd4fdd2..d362341 100644 --- a/internal/rpmtree.bzl +++ b/internal/rpmtree.bzl @@ -104,19 +104,16 @@ _tar2files = rule( toolchains = [BAZELDNF_TOOLCHAIN], ) -def rpmtree(**kwargs): +def rpmtree(name, **kwargs): """Creates a tar file from a list of rpm files.""" - kwargs.pop("files", None) - basename = kwargs["name"] - kwargs.pop("name", None) - tarname = basename + ".tar" + tarname = name + ".tar" _rpm2tar( - name = basename, + name = name, out = tarname, **kwargs ) -def tar2files(**kwargs): +def tar2files(name, files = None, **kwargs): """Extracts files from a tar file. Args: @@ -125,19 +122,18 @@ def tar2files(**kwargs): If not provided, the function will fail. **kwargs: Additional keyword arguments to be passed to the _tar2files function. """ - files = kwargs["files"] - kwargs.pop("files", None) - basename = kwargs["name"] - kwargs.pop("name", None) - if files: - for k, v in files.items(): - name = basename + k - files = [] - for file in v: - files = files + [name + "/" + file] - _tar2files( - name = name, - prefix = k, - out = files, - **kwargs - ) + if not files: + fail("files is a required attribute") + + basename = name + for k, v in files.items(): + name = basename + k + files = [] + for file in v: + files.append(name + "/" + file) + _tar2files( + name = name, + prefix = k, + out = files, + **kwargs + ) From 704db8a0530d5c60137b012b643517978bdbb27e Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 08:58:23 +0200 Subject: [PATCH 5/7] toolchain: adding missing steps When adding toolchain we missed this --- internal/xattrs.bzl | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/xattrs.bzl b/internal/xattrs.bzl index c439a79..6666999 100644 --- a/internal/xattrs.bzl +++ b/internal/xattrs.bzl @@ -14,6 +14,8 @@ "Modify xattrs on tar file members" +load("//bazeldnf:toolchain.bzl", "BAZELDNF_TOOLCHAIN") + def _xattrs_impl(ctx): out = ctx.outputs.out args = ["xattr", "--input", ctx.files.tar[0].path, "--output", out.path] @@ -21,33 +23,29 @@ def _xattrs_impl(ctx): if ctx.attr.capabilities: capabilities = [] for k, v in ctx.attr.capabilities.items(): - capabilities += [k + "=" + ":".join(v)] + capabilities.append([k + "=" + ":".join(v)]) args += ["--capabilities", ",".join(capabilities)] if ctx.attr.selinux_labels: selinux_labels = [] for k, v in ctx.attr.selinux_labels.items(): - selinux_labels += [k + "=" + v] + selinux_labels.append([k + "=" + v]) args += ["--selinux-labels", ",".join(selinux_labels)] + bazeldnf = ctx.toolchains[BAZELDNF_TOOLCHAIN]._tool + ctx.actions.run( inputs = ctx.files.tar, outputs = [out], arguments = args, progress_message = "Enriching %s with xattrs" % ctx.label.name, - executable = ctx.executable._bazeldnf, + executable = bazeldnf, ) return [DefaultInfo(files = depset([ctx.outputs.out]))] _xattrs_attrs = { "tar": attr.label(allow_single_file = True), - "_bazeldnf": attr.label( - executable = True, - cfg = "exec", - allow_files = True, - default = Label("//cmd:prebuilt"), - ), "capabilities": attr.string_list_dict(), "selinux_labels": attr.string_dict(), "out": attr.output(mandatory = True), @@ -56,6 +54,7 @@ _xattrs_attrs = { _xattrs = rule( implementation = _xattrs_impl, attrs = _xattrs_attrs, + toolchains = [BAZELDNF_TOOLCHAIN], ) def xattrs(**kwargs): From 102a20ca0295f11e53ef546b33b02eafda8a77b2 Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 11:15:00 +0200 Subject: [PATCH 6/7] buildifier: move out of array addition When mutating arrays the linter prefers to use extend/append instead of += to the existing array. Also moving from arrays to ctx.actions.attr instead of arrays when passing values into ctx.actions, this reduces the memory preassure during the bazel analysis phase and delays it until the action graph gets executed. --- internal/bazeldnf.bzl | 13 ++++++------- internal/rpmtree.bzl | 37 +++++++++++++++++++------------------ 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/internal/bazeldnf.bzl b/internal/bazeldnf.bzl index ae49b2c..a5d5963 100644 --- a/internal/bazeldnf.bzl +++ b/internal/bazeldnf.bzl @@ -8,16 +8,15 @@ def _bazeldnf_impl(ctx): out_file = ctx.actions.declare_file(ctx.label.name + ".bash") args = [] if ctx.attr.command: - args += [ctx.attr.command] + args.append(ctx.attr.command) if ctx.attr.rulename: - args += ["--name", ctx.attr.rulename] + args.extend(["--name", ctx.attr.rulename]) if ctx.attr.rpmtree: - args += ["--rpmtree", ctx.attr.rpmtree] + args.extend(["--rpmtree", ctx.attr.rpmtree]) if ctx.file.tar: - args += ["--input", ctx.file.tar.path] - transitive_dependencies += [ctx.attr.tar.files] - for lib in ctx.attr.libs: - args += [lib] + args.extend(["--input", ctx.file.tar.path]) + transitive_dependencies.extend(ctx.attr.tar.files) + args.extend(ctx.attr.libs) toolchain = ctx.toolchains[BAZELDNF_TOOLCHAIN] diff --git a/internal/rpmtree.bzl b/internal/rpmtree.bzl index d362341..b3492f3 100644 --- a/internal/rpmtree.bzl +++ b/internal/rpmtree.bzl @@ -22,37 +22,36 @@ make available to bazel load("//bazeldnf:toolchain.bzl", "BAZELDNF_TOOLCHAIN") def _rpm2tar_impl(ctx): - rpms = [] - for rpm in ctx.files.rpms: - rpms += ["--input", rpm.path] + args = ctx.actions.args() out = ctx.outputs.out - args = ["rpm2tar", "--output", out.path] + args.add_all(["rpm2tar", "--output", out]) if ctx.attr.symlinks: symlinks = [] for k, v in ctx.attr.symlinks.items(): - symlinks += [k + "=" + v] - args += ["--symlinks", ",".join(symlinks)] + symlinks.append(k + "=" + v) + args.add_joined("--symlinks", symlinks, join_with = ",") if ctx.attr.capabilities: capabilities = [] for k, v in ctx.attr.capabilities.items(): - capabilities += [k + "=" + ":".join(v)] - args += ["--capabilities", ",".join(capabilities)] + capabilities.append(k + "=" + ":".join(v)) + args.add_joined("--capabilities", capabilities, join_with = ",") if ctx.attr.selinux_labels: selinux_labels = [] for k, v in ctx.attr.selinux_labels.items(): - selinux_labels += [k + "=" + v] - args += ["--selinux-labels", ",".join(selinux_labels)] + selinux_labels.append(k + "=" + v) + args.add_joined("--selinux-labels", selinux_labels, join_with = ",") - args += rpms + for rpm in ctx.files.rpms: + args.add_all(["--input", rpm.path]) ctx.actions.run( inputs = ctx.files.rpms, outputs = [out], - arguments = args, + arguments = [args], mnemonic = "Rpm2Tar", progress_message = "Converting %s to tar" % ctx.label.name, executable = ctx.toolchains[BAZELDNF_TOOLCHAIN]._tool, @@ -60,17 +59,19 @@ def _rpm2tar_impl(ctx): return [DefaultInfo(files = depset([ctx.outputs.out]))] +def _expand_path(files): + return [x.path for x in files] + def _tar2files_impl(ctx): - out = ctx.outputs.out - files = [] - for out in ctx.outputs.out: - files += [out.path] + args = ctx.actions.args() + + args.add_all(["tar2files", "--file-prefix", ctx.attr.prefix, "--input", ctx.files.tar[0]]) + args.add_all([ctx.outputs.out], map_each = _expand_path) - args = ["tar2files", "--file-prefix", ctx.attr.prefix, "--input", ctx.files.tar[0].path] + files ctx.actions.run( inputs = ctx.files.tar, outputs = ctx.outputs.out, - arguments = args, + arguments = [args], mnemonic = "Tar2Files", progress_message = "Extracting files", executable = ctx.toolchains[BAZELDNF_TOOLCHAIN]._tool, From b70cdac45e351c4bf0ed6b2f11c161cc54bc367d Mon Sep 17 00:00:00 2001 From: Manuel Naranjo Date: Wed, 17 Jul 2024 11:59:04 +0200 Subject: [PATCH 7/7] ci: adding a gazelle check Now we have a gazelle check in CI as well as we have buildifier --- .github/workflows/{buildifier.yaml => linter.yaml} | 4 +++- BUILD.bazel | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) rename .github/workflows/{buildifier.yaml => linter.yaml} (95%) diff --git a/.github/workflows/buildifier.yaml b/.github/workflows/linter.yaml similarity index 95% rename from .github/workflows/buildifier.yaml rename to .github/workflows/linter.yaml index 9d1cd99..bbc8125 100644 --- a/.github/workflows/buildifier.yaml +++ b/.github/workflows/linter.yaml @@ -1,4 +1,4 @@ -name: Buildifier +name: Linters # Controls when the action will run. on: @@ -37,3 +37,5 @@ jobs: external-cache: true - name: buildifier run: bazel run //:buildifier.check + - name: gazelle + run: bazel run //:gazelle.check diff --git a/BUILD.bazel b/BUILD.bazel index f78ec9d..90e6c1f 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -8,6 +8,11 @@ load("//bazeldnf:defs.bzl", "bazeldnf", "rpmtree", "tar2files") # gazelle:resolve go github.com/bazelbuild/buildtools/edit @com_github_bazelbuild_buildtools//edit:go_default_library gazelle(name = "gazelle") +gazelle( + name = "gazelle.check", + mode = "diff", +) + buildifier( name = "buildifier", )