From 7e319a3670a284e1a2ece8ce79c6b13c688cb119 Mon Sep 17 00:00:00 2001 From: Oliver Lee Date: Fri, 28 Jun 2024 12:33:25 +0200 Subject: [PATCH] fix formatting of files in place (#11) This commit renames and redefines the `clang_format_aspect` to take `clang-format` options and execution requirements. It also defines a fix aspect which modifies files in place with `clang-format`, removing the need for an update script and subsequent need to determine the `bazel` process. resolves #5 Change-Id: I90d6c95e921d6ab9565bfe5318635901d3de8ad1 --- BUILD.bazel | 21 ---- README.md | 176 +++++++++++----------------- defs.bzl | 207 +++++++++++++-------------------- example/default/.bazelrc | 2 +- example/format-binary/.bazelrc | 2 +- example/format-config/.bazelrc | 2 +- example/format-ignore/.bazelrc | 2 +- update.template.sh | 103 ---------------- wrapper.sh | 12 -- 9 files changed, 153 insertions(+), 374 deletions(-) delete mode 100644 update.template.sh delete mode 100755 wrapper.sh diff --git a/BUILD.bazel b/BUILD.bazel index c4e7ddd..5eee7dd 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,12 +1,5 @@ -load("//:defs.bzl", "bool_flag", "clang_format_update") - package(default_visibility = ["//visibility:public"]) -bool_flag( - name = "dry_run", - build_setting_default = True, -) - # Your binary should be a `*_binary`, *NOT* filegroup filegroup( name = "default_binary", @@ -37,17 +30,3 @@ label_flag( name = "ignore", build_setting_default = ":default_ignore", ) - -filegroup( - name = "wrapper", - srcs = ["wrapper.sh"], -) - -filegroup( - name = "template", - srcs = ["update.template.sh"], -) - -clang_format_update( - name = "update", -) diff --git a/README.md b/README.md index 7cfffc0..3ec367c 100644 --- a/README.md +++ b/README.md @@ -1,21 +1,22 @@ # bazel_clang_format -Run clang-format on Bazel C++ targets directly. It's like +Run `clang-format` on Bazel C++ targets directly. It's like [bazel_clang_tidy](https://github.com/erenon/bazel_clang_tidy) but for -clang-format. +`clang-format`. ## usage -```py +Update your project with + +```Starlark # //:WORKSPACE.bazel load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_repository") BAZEL_CLANG_FORMAT_COMMIT = ... -BAZEL_CLANG_FORMAT_SHA = ... http_repository( name = "bazel_clang_format", - sha256 = BAZEL_CLANG_FORMAT_SHA, + integrity = ..., strip_prefix = "bazel_clang_format-{commit}".format( commit = BAZEL_CLANG_FORMAT_COMMIT, ), @@ -25,155 +26,116 @@ http_repository( ) ``` -You can now compile using the default clang format configuration provided using -the following command: - -``` -bazel build //... \ - --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect \ - --output_groups=report -``` +```Starlark +# //:.bazelrc -By default, `.clang-format` from this repo is applied. If you wish to override -the config, define a filegroup: +build:clang-format --aspects @bazel_clang_format//:defs.bzl%check_aspect +build:clang-format --output_groups=report -```py -# //:BUILD.bazel -filegroup( - name = "clang_format_config", - srcs = [".clang-format"], - visibility = ["//visibility:public"], -) +build:clang-format-fix --aspects @bazel_clang_format//:defs.bzl%fix_aspect +build:clang-format-fix --output_groups=report +build:clang-format-fix --use_action_cache=false ``` -and override the default config file using the config setting: +Check formatting with ```sh -bazel build //... \ - --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect \ - --@bazel_clang_format//:config=//:clang_format_config \ # <----------- - --output_groups=report +bazel build //... --config=clang-format ``` -To specify a specific binary (e.g. `clang-format` is specified by a hermetic -toolchain like [this](https://github.com/grailbio/bazel-toolchain), with the -binary setting: +Fix formatting with ```sh -bazel build //... \ - --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect \ - --@bazel_clang_format//:binary=@llvm15//:clang-format \ # <----------- - --output_groups=report +bazel build //... --config=clang-format-fix ``` -Now if you don't want to type this out every time, it is recommended that you -add a config in your .bazelrc that matches this command line; +This will use `clang-format` in your `PATH` and `.clang-format` defined in this +repo. -Config shorthand: -``` -# //.bazelrc -build:clang-format --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect -build:clang-format --output_groups=report -``` -or with configuration: +### using a hermetic toolchain -``` -# //.bazelrc -build:clang-format --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect -build:clang-format --@bazel_clang_format//:binary=@llvm15//:clang-format -build:clang-format --@bazel_clang_format//:config=//:clang_format_config -build:clang-format --output_groups=report -``` +
-then run: +To specify a specific binary (e.g. `clang-format` is specified by a hermetic +toolchain like [this](https://github.com/grailbio/bazel-toolchain)), update the +build setting in `.bazelrc`. -```sh -bazel build //... --config clang-format -``` +```Starlark +# //:.bazelrc -To format all source files: +build:clang-format-base --output_groups=report +build:clang-format-base --@bazel_clang_format//:binary=@llvm18//:clang-format -```sh -bazel run @bazel_clang_format//:update \ - --@bazel_clang_format//:binary=@llvm15//:clang_format \ - --@bazel_clang_format//:config=//:clang_format_config -``` +build:clang-format --aspects @bazel_clang_format//:defs.bzl%check_aspect -with a specific binary/config if desired. +build:clang-format-fix --aspects @bazel_clang_format//:defs.bzl%fix_aspect +build:clang-format-fix --use_action_cache=false +``` -Or to format specific targets: +
-```sh -bazel run @bazel_clang_format//:update -- //src/... -``` +### specifying `.clang-format` -## ignoring formatting of specific targets +
-Formatting can be skipped for specific targets by specifying a filegroup +To override the default `.clang-format`, define a `filegroup` containing the +replacement config and update build setting in `.bazelrc`. -```python +```Starlark # //:BUILD.bazel +load("@bazel_clang_format//:defs.bzl") + filegroup( - name = "clang_format_ignore", - srcs = [ - "//third_party/lib1", - "//third_party/lib2", - ], + name = "clang-format-config", + srcs = [".clang-format"], + visibility = ["//visibility:public"], ) ``` -```sh -# //.bazelrc -... -build:clang-format --@bazel_clang_format//:ignore=//:clang_format_ignore +```Starlark +# //:.bazelrc + +build:clang-format-base --output_groups=report +build:clang-format-base --@bazel_clang_format//:config=//:clang-format-config # <----- ... ``` -## defaults without .bazelrc +
-Both the aspect and update rule can be defined locally to bake in a default -binary or config. +### ignoring certain targets -```python -# //:BUILD.bazel -load("@bazel_clang_format//:defs.bzl", "clang_format_update") +
-alias( - name = "default_clang_format_binary", - actual = "@llvm_toolchain//:clang-format", -) +Formatting can be skipped for certain targets by specifying a filegroup -filegroup( - name = "default_clang_format_config", - srcs = [".clang-format"] - visibility = ["//visibility:public"], -) +```Starlark +# //:BUILD.bazel -clang_format_update( - name = "clang_format", - binary = ":default_clang_format_binary", - config = ":default_clang_format_config", +filegroup( + name = "clang-format-ignore", + srcs = [ + "//third_party/lib1", + "//third_party/lib2", + ], ) ``` -```python -# //:aspects.bzl -load("@bazel_clang_format//:defs.bzl", "make_clang_format_aspect") +```Starlark +# //:.bazelrc -clang_format = make_clang_format_aspect( - binary = "//:default_clang_format_binary", - config = "//:default_clang_format_config", -) +build:clang-format-base --output_groups=report +build:clang-format-base --@bazel_clang_format//:ignore=//:clang-format-ignore# <----- +... ``` -```sh -bazel run //:clang_format -bazel build //... --aspects //:aspects.bzl%clang_format --output_groups=report -``` +
## Requirements - Bazel ??? - clang-format ??? + +I'm not sure what the minimum versions are but please let me know if you are +using a version that doesn't work. diff --git a/defs.bzl b/defs.bzl index 68fa994..059cc8b 100644 --- a/defs.bzl +++ b/defs.bzl @@ -2,26 +2,6 @@ clang-format aspect """ -# Avoid the need to bring in bazel-skylib as a dependency -# https://github.com/bazelbuild/bazel-skylib/blob/main/docs/common_settings_doc.md -BuildSettingInfo = provider( - doc = "Contains the value of a build setting.", - fields = { - "value": "The value of the build setting in the current configuration. " + - "This value may come from the command line or an upstream transition, " + - "or else it will be the build setting's default.", - }, -) - -def _impl(ctx): - return BuildSettingInfo(value = ctx.build_setting_value) - -bool_flag = rule( - implementation = _impl, - build_setting = config.bool(flag = True), - doc = "A bool-typed build setting that can be set on the command line", -) - def _source_files_in(ctx, attr): if not hasattr(ctx.rule.attr, attr): return [] @@ -32,59 +12,73 @@ def _source_files_in(ctx, attr): return [f for f in files if f.is_source] -def _check_format(ctx, package, f): +def _do_format(ctx, f, format_options, execution_reqs): binary = ctx.attr._binary.files_to_run.executable - dry_run = ctx.attr._dry_run[BuildSettingInfo].value - - out = ctx.actions.declare_file( - "{name}.clang_format".format( - # don't duplicate package in the out file - name = f.short_path.removeprefix(package + "/"), - ), - ) + out = ctx.actions.declare_file(f.short_path + ".clang_format") - ctx.actions.run( - inputs = [ctx.file._config] + ([binary] if binary else []) + [f], + ctx.actions.run_shell( + inputs = [ + ctx.file._config, + f, + ] + ([binary] if binary else []), outputs = [out], - executable = ctx.executable._wrapper, - arguments = [ - binary.path if binary else "clang-format", - ctx.file._config.path, - f.path, - out.path, - ] + (["--dry-run"] if dry_run else [""]), + command = """ +set -euo pipefail + +test -e .clang-format || ln -s -f {config} .clang-format +{binary} {format_options} {infile} +touch {outfile} +""".format( + config = ctx.file._config.path, + binary = binary.path if binary else "clang-format", + format_options = " ".join(format_options), + infile = f.path, + outfile = out.path, +), mnemonic = "ClangFormat", + progress_message = "Formatting {}".format(f.short_path), + execution_requirements = execution_reqs, ) return out -def _clang_format_aspect_impl(target, ctx): - ignored = {f.owner: "" for f in ctx.attr._ignore.files.to_list()} - - if target.label in ignored.keys(): - return [OutputGroupInfo(report = depset([]))] - - outputs = [ - _check_format(ctx, target.label.package, f) - for f in ( - _source_files_in(ctx, "srcs") + - _source_files_in(ctx, "hdrs") - ) - ] - - return [OutputGroupInfo(report = depset(outputs))] - -def make_clang_format_aspect(binary = None, config = None, ignore = None): +def _clang_format_aspect_impl(format_options, execution_requirements): + def impl(target, ctx): + ignored = {f.owner: "" for f in ctx.attr._ignore.files.to_list()} + + if target.label in ignored.keys(): + return [OutputGroupInfo(report = depset([]))] + + outputs = [ + _do_format( + ctx, + f, + format_options, + execution_requirements, + ) + for f in ( + _source_files_in(ctx, "srcs") + + _source_files_in(ctx, "hdrs") + ) + ] + + return [OutputGroupInfo(report = depset(outputs))] + + return impl + +def make_clang_format_aspect( + binary = None, + config = None, + ignore = None, + options = None, + execution_requirements = None): return aspect( - implementation = _clang_format_aspect_impl, + implementation = _clang_format_aspect_impl( + options or [], + execution_requirements or {}, + ), fragments = ["cpp"], attrs = { - "_wrapper": attr.label( - executable = True, - cfg = "exec", - allow_files = True, - default = Label("//:wrapper"), - ), "_binary": attr.label( default = Label(binary or "//:binary"), ), @@ -95,76 +89,35 @@ def make_clang_format_aspect(binary = None, config = None, ignore = None): "_ignore": attr.label( default = Label(ignore or "//:ignore"), ), - "_dry_run": attr.label(default = Label("//:dry_run")), }, required_providers = [CcInfo], toolchains = ["@bazel_tools//tools/cpp:toolchain_type"], ) -clang_format_aspect = make_clang_format_aspect() - -def _clang_format_update_impl(ctx): - update_format = ctx.actions.declare_file( - "bazel_clang_format.{}.sh".format(ctx.attr.name), - ) - - bindir = update_format.path[:update_format.path.find("bin/")] + "bin/" - - binary = ctx.attr.binary or ctx.attr._binary - config = ctx.attr.config or ctx.attr._config - ignore = ctx.attr.ignore or ctx.attr._ignore - - # get the workspace of bazel_clang_format, not where this update rule is - # defined - workspace = ctx.attr._template.label.workspace_name - - ctx.actions.expand_template( - template = ctx.attr._template.files.to_list()[0], - output = update_format, - substitutions = { - "@BINARY@": str(binary.label), - "@CONFIG@": str(config.label), - "@IGNORE@": str(ignore.label), - "@WORKSPACE@": workspace, - "@BINDIR@": bindir, - }, - ) - - format_bin = binary.files_to_run.executable - runfiles = ctx.runfiles( - ([format_bin] if format_bin else []) + - config.files.to_list(), - ) - - return [DefaultInfo( - executable = update_format, - runfiles = runfiles, - )] +check_aspect = make_clang_format_aspect( + options = ["--color=true", "--Werror", "--dry-run"], + execution_requirements = { + "no-remote": "1", + "local": "1", + }, +) -clang_format_update = rule( - implementation = _clang_format_update_impl, - fragments = ["cpp"], - attrs = { - "_template": attr.label(default = Label("//:template")), - "_binary": attr.label(default = Label("//:binary")), - "_config": attr.label( - allow_single_file = True, - default = Label("//:config"), - ), - "_ignore": attr.label( - default = Label("//:ignore"), - ), - "binary": attr.label( - doc = "Set clang-format binary to use. Overrides //:binary", - ), - "config": attr.label( - allow_single_file = True, - doc = "Set clang-format config to use. Overrides //:config", - ), - "ignore": attr.label( - doc = "Set clang-format ignore targets to use. Overrides //:ignore", - ), +fix_aspect = make_clang_format_aspect( + options = ["-i"], + # https://stackoverflow.com/questions/50025990/disable-sandbox-in-custom-rule + # + # https://bazel.build/reference/be/common-definitions#common.tags + # + # however, due to + # https://github.com/bazelbuild/bazel/issues/15516 + # https://github.com/bazelbuild/bazel/issues/21587 + # + # fixing formatting will require use of --use_action_cache=false + # https://bazel.build/versions/6.5.0/docs/user-manual#use-action-cache + execution_requirements = { + "no-sandbox": "1", + "no-cache": "1", + "no-remote": "1", + "local": "1", }, - toolchains = ["@bazel_tools//tools/cpp:toolchain_type"], - executable = True, ) diff --git a/example/default/.bazelrc b/example/default/.bazelrc index d1e3a46..dd82396 100644 --- a/example/default/.bazelrc +++ b/example/default/.bazelrc @@ -1,2 +1,2 @@ - build:clang-format --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect + build:clang-format --aspects @bazel_clang_format//:defs.bzl%check_aspect build:clang-format --output_groups=report diff --git a/example/format-binary/.bazelrc b/example/format-binary/.bazelrc index ce52358..62311bf 100644 --- a/example/format-binary/.bazelrc +++ b/example/format-binary/.bazelrc @@ -1,3 +1,3 @@ - build:clang-format --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect + build:clang-format --aspects @bazel_clang_format//:defs.bzl%check_aspect build:clang-format --@bazel_clang_format//:binary=@llvm14//:clang-format build:clang-format --output_groups=report diff --git a/example/format-config/.bazelrc b/example/format-config/.bazelrc index 493922b..28818f1 100644 --- a/example/format-config/.bazelrc +++ b/example/format-config/.bazelrc @@ -1,3 +1,3 @@ - build:clang-format --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect + build:clang-format --aspects @bazel_clang_format//:defs.bzl%check_aspect build:clang-format --@bazel_clang_format//:config=//:clang_format_config build:clang-format --output_groups=report diff --git a/example/format-ignore/.bazelrc b/example/format-ignore/.bazelrc index 2d1cca9..b40a639 100644 --- a/example/format-ignore/.bazelrc +++ b/example/format-ignore/.bazelrc @@ -1,4 +1,4 @@ - build:clang-format --aspects @bazel_clang_format//:defs.bzl%clang_format_aspect + build:clang-format --aspects @bazel_clang_format//:defs.bzl%check_aspect build:clang-format --@bazel_clang_format//:binary=@llvm14//:clang-format build:clang-format --@bazel_clang_format//:ignore=//:clang_format_ignore build:clang-format --output_groups=report diff --git a/update.template.sh b/update.template.sh deleted file mode 100644 index 8320cf3..0000000 --- a/update.template.sh +++ /dev/null @@ -1,103 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -function bazel_bin -{ -pid=$PPID -bin=$(readlink -f /proc/$pid/exe) - -while $bin --version | grep -q -v '^bazel'; do - pid=$(ps -o ppid= $pid | xargs) - bin=$(readlink -f /proc/$pid/exe) -done - -echo "$bin" -} - -bazel=$(bazel_bin) - -function stale -{ -echo "$1" \ - | grep "ERROR: action 'ClangFormat" \ - | sed "s/^ERROR: action 'ClangFormat \(.*\)\.clang_format' is not up-to-date$/\1/" || true -} - -bazel_query=("$bazel" query \ - --color=yes \ - --noshow_progress \ - --ui_event_filters=-info) - -bazel_format=("$bazel" build \ - --noshow_progress \ - --ui_event_filters=-info,-stdout \ - --color=no \ - --aspects=@@WORKSPACE@//:defs.bzl%clang_format_aspect \ - --@@WORKSPACE@//:binary=@BINARY@ \ - --@@WORKSPACE@//:config=@CONFIG@ \ - --@@WORKSPACE@//:ignore=@IGNORE@ \ - --output_groups=report) - -bazel_format_file=("${bazel_format[@]}" --compile_one_dependency) - -cd $BUILD_WORKSPACE_DIRECTORY - -args=$(printf " union %s" "${@}" | sed "s/^ union \(.*\)/\1/") - -source_files=$("${bazel_query[@]}" \ - "let t = kind(\"cc_.* rule\", ${args:-//...} except deps(@IGNORE@, 1)) in labels(srcs, \$t) union labels(hdrs, \$t)") - -"$bazel" build @BINARY@ - -result=$("${bazel_format_file[@]}" \ - --keep_going \ - --check_up_to_date \ - $source_files 2>&1 || true) - -files=$(stale "$result") - -if [[ -z $files ]] && [[ $(echo "$result" | grep "ERROR:" | wc -l) -gt 0 ]]; then - echo "$result" - exit 1 -fi - -# libraries without `srcs` are not handled correctly with `--compile_one_dependency` -header_libs=$("${bazel_query[@]}" \ - "attr(\"srcs\", \"\[\]\", kind(\"cc_library rule\", ${args:-//...}))") - -result=$("${bazel_format[@]}" \ - --keep_going \ - --check_up_to_date \ - $header_libs 2>&1 || true) - -header_files=$(stale "$result") - -file_count=$(echo "$files" | sed '/^\s*$/d' | wc -l) -header_file_count=$(echo "$header_files" | sed '/^\s*$/d' | wc -l) - -[[ $file_count -ne 0 ]] || [[ $header_file_count -ne 0 ]] || exit 0 - -# use bazel to generate the formatted files in a separate -# directory in case the user is overriding .clang-format -[[ $file_count -eq 0 ]] || "${bazel_format_file[@]}" --@@WORKSPACE@//:dry_run=False $files - -# format all header only libs -[[ $header_file_count -eq 0 ]] || "${bazel_format[@]}" --@@WORKSPACE@//:dry_run=False $header_libs - -for arg in $(echo "$files" "$header_files"); do - generated="@BINDIR@${arg}.clang_format" - if [[ ! -f "$generated" ]]; then - continue - fi - - # fix file mode bits - # https://github.com/bazelbuild/bazel/issues/2888 - chmod $(stat -c "%a" "$arg") "$generated" - - # replace source with formatted version - mv "$generated" "$arg" -done - -# run format check to cache success -[[ $file_count -eq 0 ]] || "${bazel_format_file[@]}" $files -[[ $header_file_count -eq 0 ]] || "${bazel_format[@]}" $header_libs diff --git a/wrapper.sh b/wrapper.sh deleted file mode 100755 index 4b7d7e1..0000000 --- a/wrapper.sh +++ /dev/null @@ -1,12 +0,0 @@ -#!/usr/bin/env bash -set -eu - -binary=$1 -config=$2 -infile=$3 -outfile=$4 -dry_run=$5 - -test -e .clang-format || ln -s -f $config .clang-format - -$binary --color=true --Werror $dry_run $infile > $outfile