Executable target for rustfmt

[c0nk]

We would like to run rustfmt directly through bazel to check formatting from our CI system. For example: bazel run @io_bazel_rules_rust//:rustfmt --. Is this somehow possible?

[mfarrugi]

It should be somewhere under the toolchain repository (@rust_linux_x86_64//...).

[dae]

Here are some simple cross-platform rules you can use. Put the following in a .bzl file:

def _rustfmt_impl(ctx):
    toolchain = ctx.toolchains["@io_bazel_rules_rust//rust:toolchain"]
    script_name = ctx.label.name + "_script"
    rustfmt = toolchain.rustfmt.path
    if ctx.attr.is_windows:
        script_name += ".bat"
        rustfmt = "@" + rustfmt.replace("/", "\\")
    script = ctx.actions.declare_file(script_name)

    args = [f.path for f in ctx.files.srcs]

    if ctx.attr.fix:
        mode = "--emit files"
    else:
        mode = "--check"

    ctx.actions.write(
        output = script,
        content = "{rustfmt} {mode} --edition {edition} {files}".format(
            rustfmt = rustfmt,
            edition = toolchain.default_edition,
            files = " ".join(args),
            mode = mode,
        ),
    )

    runfiles = ctx.runfiles(files = ctx.files.srcs + [toolchain.rustfmt])
    return [DefaultInfo(runfiles = runfiles, executable = script)]

_ATTRS = {
    "srcs": attr.label_list(allow_files = True),
    "is_windows": attr.bool(mandatory = True),
    "fix": attr.bool(mandatory = True),
}

_rustfmt_test = rule(
    implementation = _rustfmt_impl,
    test = True,
    toolchains = [
        "@io_bazel_rules_rust//rust:toolchain",
    ],
    attrs = _ATTRS,
)

_rustfmt_fix = rule(
    implementation = _rustfmt_impl,
    executable = True,
    toolchains = [
        "@io_bazel_rules_rust//rust:toolchain",
    ],
    attrs = _ATTRS,
)

def rustfmt_test(name, srcs, **kwargs):
    _rustfmt_test(
        name = name,
        srcs = srcs,
        testonly = True,
        fix = False,
        is_windows = select({
            "@bazel_tools//src/conditions:host_windows": True,
            "//conditions:default": False,
        }),
        **kwargs
    )

def rustfmt_fix(name, srcs, **kwargs):
    # don't match //package/...
    tags = kwargs.get("tags", [])
    tags.append("manual")

    _rustfmt_fix(
        name = name,
        srcs = srcs,
        tags = tags,
        fix = True,
        is_windows = select({
            "@bazel_tools//src/conditions:host_windows": True,
            "//conditions:default": False,
        }),
        **kwargs
    )

Then you can add the following to your BUILD:

load(":defs.bzl", "rustfmt_fix", "rustfmt_test")

rustfmt_test(
    name = "format",
    srcs = glob([
        "src/**/*.rs",
    ]),
)

rustfmt_fix(
    name = "format_fix",
    srcs = glob([
        "src/**/*.rs",
    ]),
)

To check if your files are formatted:

$ bazel test :format

To fix any files that are formatted incorrectly:

$ bazel run :format_fix

@mfarrugi If there is interest in integrating this into this repo, please let me know. It probably won't work if there are thousands of files matched by the glob.

[dfreese]

@dae I think that'd be useful to incorporate, but I imagine it probably working as an aspect, similar to the clippy integration. That way you could run:

bazel build --aspects=@io_bazel_rules_rust//rust:rust.bzl%rust_rustfmt_aspect //...

[dfreese]

Otherwise I see it being difficult to manage the editions, and the number of files.

[wchargin]

Another reason for this: tonic_build runs rustfmt on its generated
code. You can turn this off with Builder::format, but this makes
the generated code mega-ugly (5000-character lines with whole submodules
in them). Not the end of the world since it’s gencode, but it’d be much
nicer to just be able to write

my_rule(  # genrule, cargo_build_script, etc.
    ...,
    data = ["@io_bazel_rules_rust//:rustfmt"],
)

Without any data dependency, it appears to be implicitly nonhermetic—it
works on my machine but fails on our CI.

[illicitonion]

Another reason for this: tonic_build runs rustfmt on its generated
code. You can turn this off with Builder::format, but this makes
the generated code mega-ugly (5000-character lines with whole submodules
in them). Not the end of the world since it’s gencode, but it’d be much
nicer to just be able to write

my_rule(  # genrule, cargo_build_script, etc.
    ...,
    data = ["@io_bazel_rules_rust//:rustfmt"],
)

You can already do this in the platform-specific toolchains, we make executable targets like @rust_darwin_x86_64//:rustfmt in

rules_rust/rust/repositories.bzl

Lines 248 to 254 in f33e3b3

	def BUILD_for_rustfmt(target_triple):
	"""Emits a BUILD file the rustfmt .tar.gz."""
	system = triple_to_system(target_triple)
	return _build_file_for_rustfmt_template.format(
	binary_ext = system_to_binary_ext(system),
	)

does that help at all?

[wchargin]

Yep! I saw that, indeed. But directly depending on a platform-specific
target like @rust_linux_x86_64//:rustfmt would of course make the
build itself non-portable, and I’d like to avoid that.

[illicitonion]

We could maybe add to io_bazel_rules_rust something along the lines of:

alias(
    name = "rustfmt",
    actual = select({
        "@io_bazel_rules_rust//rust/platform:osx": "@rust_darwin_x86_64//:rustfmt",
        "@io_bazel_rules_rust//rust/platform:linux": "@rust_linux_x86_64//:rustfmt",
        ...
    }),
)

so that there's a single target you can point at which acts differently depending on your platform?

[djmarcin]

@rust_linux_x86_64//:rustfmt does not seem to work currently, because it resolves to multiple paths.

cargo_build_script(
    name = "build_script",
    srcs = ["build.rs"],
    build_script_env = {
        "PROTOC": "$(execpath @com_google_protobuf//:protoc)",
        "RUSTFMT": "$(execpath @rust_linux_x86_64//:rustfmt)",
    },
    data = [
        "@com_google_protobuf//:protoc",
        "@rust_linux_x86_64//:rustfmt",
    ],
)

ERROR: .../BUILD:6:19: in _build_script_run rule //common:build_script: label '@rust_linux_x86_64//:rustfmt' in $(location) expression expands to more than one file, please use $(locations @rust_linux_x86_64//:rustfmt) instead.  Files (at most 5 shown) are: [bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/rustfmt, external/rust_linux_x86_64/bin/rustfmt]

However using locations as suggested would result in passing an env var looking like RUSTFMT="bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/rustfmt external/rust_linux_x86_64/bin/rustfmt" which doesn't work when the env var is expected to be a single path to the executable. Have I missed something about how to set this up?

It seems worth noting as well that the protoc execpath is a full path, not just a partial relative path, e.g /home/vagrant/.cache/bazel/_bazel_vagrant/71ed060a30795ab0736b520a305b19cd/sandbox/linux-sandbox/897/execroot/__main__/bazel-out/k8-fastbuild/bin/external/com_google_protobuf/protoc

[illicitonion]

@rust_linux_x86_64//:rustfmt does not seem to work currently, because it resolves to multiple paths.

cargo_build_script(
    name = "build_script",
    srcs = ["build.rs"],
    build_script_env = {
        "PROTOC": "$(execpath @com_google_protobuf//:protoc)",
        "RUSTFMT": "$(execpath @rust_linux_x86_64//:rustfmt)",
    },
    data = [
        "@com_google_protobuf//:protoc",
        "@rust_linux_x86_64//:rustfmt",
    ],
)

ERROR: .../BUILD:6:19: in _build_script_run rule //common:build_script: label '@rust_linux_x86_64//:rustfmt' in $(location) expression expands to more than one file, please use $(locations @rust_linux_x86_64//:rustfmt) instead.  Files (at most 5 shown) are: [bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/rustfmt, external/rust_linux_x86_64/bin/rustfmt]

However using locations as suggested would result in passing an env var looking like RUSTFMT="bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/rustfmt external/rust_linux_x86_64/bin/rustfmt" which doesn't work when the env var is expected to be a single path to the executable. Have I missed something about how to set this up?

Interesting - can you try either @rust_linux_x86_64//:rustfmt_bin or @rust_linux_x86_64//:bin/rustfmt instead?

It seems worth noting as well that the protoc execpath is a full path, not just a partial relative path, e.g /home/vagrant/.cache/bazel/_bazel_vagrant/71ed060a30795ab0736b520a305b19cd/sandbox/linux-sandbox/897/execroot/__main__/bazel-out/k8-fastbuild/bin/external/com_google_protobuf/protoc

Yeah, this is expected - build scripts run in $CARGO_MANIFEST_DIR instead of the exec root, to maximise compatibility with cargo, so we need to use absolute paths because execpaths would be relative to the wrong directory. The alternative is to run build scripts in the exec root, but then most build scripts which try to read files will be broken - both options are bad :)

[djmarcin]

Thanks -- @rust_linux_x86_64//:bin/rustfmt is not visible, but @rust_linux_x86_64//:rustfmt_bin works 👍

[UebelAndre]

I love the idea of a rustfmt target. I'd expect it to work similar to buildifier if possible though 😅

[damienmg]

This is a dupe of #87. Closing in favor of the one that has the most history.