Make rustfmt accessible (and enforceable?)

[mfarrugi]

rustfmt is currently shipped as part of the toolchain tarballs, but not accessible for interactive nor programmatic usage without some finagling.

Some usecases I've run into:

1. rustfmt against generated code
2. enforcing rustfmt as a pre-commit / build step

For enforcement, is it possible for rules_rust to take global configuration, and do this check as an extra-build step? Would that be a reasonable thing to do?

[acmcarther]

rustfmt is currently shipped as part of the toolchain tarballs, but not accessible for interactive nor programmatic usage without some finagling.

Do you know off hand if the packaged rustfmt matches https://github.com/rust-lang-nursery/rustfmt ? I know there are several rustfmt's hanging around and the nightly one is the one users are expected to be using. Either way, exposing the binary should be harmless, though I've historically included rustfmt as a workspace dependency and used the produced binary.

For enforcement, is it possible for rules_rust to take global configuration, and do this check as an extra-build step? Would that be a reasonable thing to do?

I'm not particularly keen on this idea. It's not conventional to fail a compilation due to formatting in the rust ecosystem, and to my knowledge it's not conventional in the Bazel ecosystem either.

My mental model of things is that it should be part of your (automated) CI process to enumerate changed files, apply rustfmt to them, and report in the review if there is a notable diff. Seems to me that incremental development (using ibazel or like) would be a world of pain if you had to have your formatting just right... unless your editor automatically formatted code. If it does, you still have to make sure that your editor uses the exact same version of rustfmt. I've griped about this aloud elsewhere

That said, I suspect you have some context that motivates the suggestion -- or at least describes why including the formatter as a crate is less-than-desirable.

[mfarrugi]

My thought was that its in the toolchain and should be available, which makes user's lives easier. I didn't realize the recommended version wasn't the same one in the stable tarball; at some point it will be and we can expose it then. I think the format check should be precommit, but I buy that bazel doesn't need to be responsible for this, especially when I think about changes to the formatting.

…

On Fri, May 25, 2018, 17:47 Alex McArther ***@***.***> wrote: rustfmt is currently shipped as part of the toolchain tarballs, but not accessible for interactive nor programmatic usage without some finagling. Do you know off hand if the packaged rustfmt matches https://github.com/rust-lang-nursery/rustfmt ? I know there are several rustfmt's hanging around and the nightly one is the one users are expected to be using. Either way, exposing the binary should be harmless, though I've historically included rustfmt as a workspace dependency and used the produced binary. For enforcement, is it possible for rules_rust to take global configuration, and do this check as an extra-build step? Would that be a reasonable thing to do? I'm not particularly keen on this idea. It's not conventional to fail a compilation due to formatting in the rust ecosystem, and to my knowledge it's not conventional in the Bazel ecosystem either. My mental model of things is that it should be part of your (automated) CI process to enumerate changed files, apply rustfmt to them, and report in the review if there is a notable diff. Seems to me that incremental development (using ibazel or like) would be a world of pain if you had to have your formatting just right... unless your editor automatically formatted code. If it does, you still have to make sure that your editor uses the exact same version of rustfmt. I've griped about this aloud elsewhere <google/cargo-raze#29 (comment)> That said, I suspect you have some context that motivates the suggestion -- or at least describes why including the formatter as a crate is less-than-desirable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLjeE_Ya-Q7RznEu4XbNRZnb4-77cq7ks5t2HvtgaJpZM4UOkcc> .

[acmcarther]

It sounds right to me as a default to expose whatever binaries are available in the tarball. I just fetched
https://static.rust-lang.org/dist/rust-1.24.1-x86_64-unknown-linux-gnu.tar.gz and I see./rls-preview/bin/rls, ./rustfmt-preview/bin/cargo-fmt, and ./rustfmt-preview/bin/rustfmt. We could expose these binaries in a way that fails gracefully if the binary locations change (e.g. "-preview" suffix is removed).

I didn't realize the recommended version wasn't
the same one in the stable tarball; at some point it will be and we can
expose it then.

Did you test this? I'm not sure what exactly is in the tarball, but it might be the right rustfmt (or perhaps a better rustfmt -- one that the whole team can consistently use). I suspect that rustup is using this binary in the tarball when you run rustup component add rustfmt-preview. I think this is demonstrated in https://github.com/rust-lang-nursery/rustup.rs/blob/166bb9f7fd99c7ee3cf61db39475669c7efae2ba/src/rustup-dist/src/component/package.rs#L75

[acmcarther]

Addendum: It is possible to discover the binaries to be exposed via several files in the blob. They are

./components: Lists (in plain text) the logical subdirectories that may have dependencies or binaries
/$component/manifest.in: Lists (in a particular format) files of interest to be exported.

For rust-1.24.1-x86_64-unknown-linux-gnu.tar.gz

./components

rustc
cargo
rls-preview
rustfmt-preview
rust-analysis-x86_64-unknown-linux-gnu
rust-std-x86_64-unknown-linux-gnu
rust-docs

./rustfmt-preview/manifest.im

file:share/doc/rustfmt/README.md
file:share/doc/rustfmt/LICENSE-APACHE
file:share/doc/rustfmt/LICENSE-MIT
file:bin/cargo-fmt
file:bin/rustfmt

[colatkinson]

Hey all, not sure if there's continued interest in this issue, but I did a bit of experimentation. From my understanding, it seems that the easiest way to make the rustfmt binary accessible is via the load_arbitrary_tool macro. A simple (and probably fairly ugly) patch is as follows:

diff --git a/rust/repositories.bzl b/rust/repositories.bzl
index 280aab2..f73baa9 100644
--- a/rust/repositories.bzl
+++ b/rust/repositories.bzl
@@ -97,6 +97,26 @@ filegroup(
         target_triple = target_triple,
     )
 
+def BUILD_for_rustfmt(target_triple):
+    """Emits a BUILD file the compiler .tar.gz."""
+
+    system = triple_to_system(target_triple)
+    return """
+load("@io_bazel_rules_rust//rust:toolchain.bzl", "rust_toolchain")
+
+# Is there an easier/better way of exposing an external binary?
+sh_binary(
+    name = "rustfmt",
+    srcs = ["bin/rustfmt{binary_ext}"],
+    visibility = ["//visibility:public"],
+)
+""".format(
+        binary_ext = system_to_binary_ext(system),
+        staticlib_ext = system_to_staticlib_ext(system),
+        dylib_ext = system_to_dylib_ext(system),
+        target_triple = target_triple,
+    )
+
 def BUILD_for_stdlib(target_triple):
     """Emits a BUILD file the stdlib .tar.gz."""
 
@@ -237,6 +257,20 @@ def load_arbitrary_tool(ctx, tool_name, param_prefix, tool_subdirectory, version
         stripPrefix = "{}/{}".format(tool_path, tool_subdirectory),
     )
 
+def _load_rustfmt(ctx):
+    target_triple = ctx.attr.exec_triple
+    load_arbitrary_tool(
+        ctx,
+        iso_date = ctx.attr.iso_date,
+        param_prefix = "rustfmt_",
+        target_triple = target_triple,
+        tool_name = "rust",
+        tool_subdirectory = "rustfmt-preview",
+        version = ctx.attr.version,
+    )
+
+    return BUILD_for_rustfmt(target_triple)
+
 def _load_rust_compiler(ctx):
     """Loads a rust compiler and yields corresponding BUILD for it
 
@@ -252,7 +286,7 @@ def _load_rust_compiler(ctx):
         iso_date = ctx.attr.iso_date,
         param_prefix = "rustc_",
         target_triple = target_triple,
-        tool_name = "rustc",
+        tool_name = "rust",
         tool_subdirectory = "rustc",
         version = ctx.attr.version,
     )
@@ -300,7 +334,7 @@ def _rust_toolchain_repository_impl(ctx):
 
     _check_version_valid(ctx.attr.version, ctx.attr.iso_date)
 
-    BUILD_components = [_load_rust_compiler(ctx)]
+    BUILD_components = [_load_rust_compiler(ctx), _load_rustfmt(ctx)]
     for target_triple in [ctx.attr.exec_triple] + ctx.attr.extra_target_triples:
         BUILD_components.append(_load_rust_stdlib(ctx, target_triple))

The tool can then be invoked as: bazel run @rust_linux_x86_64//:rustfmt -- --check ./hello_lib/src/lib.rs.

A couple of things to note:

• This requires downloading the rust dist--which is quite large (368 MB download, 1.2 GB uncompressed).
  • The patch above does reuse this download for retrieving rustc as well, so it's not as wasteful.
  • This can also open up other "standard" Rust tools, such as clippy and cargo. The latter especially could be interesting for integration with cargo-raze, though I'm not entirely sure how that would look.
• As shown in the sample invocation, there's the issue of platform-specific prefixes. Would it be desirable to add some sort of proxy for this?
• I'm not sure if there's a better way to wrap a vendored binary than sh_binary. Or the cleanest way may be to wrap it in an actual shell script that resides in the @io_bazel_rules_rust workspace.

Some final questions:

• Is this a feature in which there's still interest?
• Is it ok to depend on rust archives instead of rustc?
• Is the approach above the right way to go about things?

Depending on the answers to the above, I can open a PR for a non-hacky version of the above.

[acmcarther]

My level of commitment to rules_rust (and rust more generally) at the moment is low, but I know a lot about this, so my 2c.

What you have here looks broadly correct, though I think I'd still prefer avoiding depending on the whole rust bundle. I think there is or at least used to be a tools-only package which could be brought in separately using load_arbitrary_tool, though it's hard to verify this since someone broke the static fileserver index.html a while ago and never bothered to fix it (rust-lang/rust-forge#215).

sh_binary there is probably fine. This would probably be useful to active users, with the caveat of #87 (comment)

[dae]

Related: #388

[UebelAndre]

@damienmg is this not just a duplicate of #388? Should this be closed?