Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[not resolved, just worked around]@rules_rust//tools/rustfmt only runs on built targets #749

Closed
UebelAndre opened this issue May 19, 2021 · 8 comments
Assignees
Labels

Comments

@UebelAndre
Copy link
Collaborator

I noticed if I run bazel clean, then intentionally ruin formatting in a source file for one of my rust targets, then run bazel run @rules_rust//tools/rustfmt, that my target isn't formatted. In fact, no target is formatted. Under the hood, that binary is running a bazel build command with the following arguments:

"build",
"--aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect",
"--output_groups=rustfmt_manifest",

My thought was --output_groups=rustfmt_manifest would tell bazel that this is the only target that needs to be produced and only do work to generate that. But alas, no manifest is generated. Is there something I'm missing about the use of output_groups? I thought this would work

@UebelAndre UebelAndre added the bug label May 19, 2021
@UebelAndre
Copy link
Collaborator Author

@hlopko do you have any thoughts on why this might be the case? 😅

@hlopko
Copy link
Member

hlopko commented May 20, 2021

  1. Can you share a reproduction repository if you have one, and share the exact bazel invocations that lead to the bug?
  2. My guess would be that query_rustfmt_targets returns empty list or something in that direction.
  3. Am I understanding you correctly that if you do bazel build //:some_rust_binary && bazel run @rules_rust//tools/rustfmt, files are reformatted, but when you stick bazel clean between those (bazel build //:some_rust_binary && bazel clean && bazel run @rules_rust//tools/rustfmt) files are no longer reformatted?

@hlopko hlopko self-assigned this May 20, 2021
@UebelAndre
Copy link
Collaborator Author

I think this was a false alarm. I'm able to format sources from a clean build without building anything by applying the following diff:

diff --git a/tools/rustfmt/srcs/main.rs b/tools/rustfmt/srcs/main.rs
index 5f17316..0d302a6 100644
--- a/tools/rustfmt/srcs/main.rs
+++ b/tools/rustfmt/srcs/main.rs
@@ -76,7 +76,7 @@ fn query_rustfmt_targets(options: &Config) -> Vec<String> {
 fn generate_rustfmt_target_manifests(options: &Config, targets: &[String]) {
     let build_args = vec![
         "build",
-        "--aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect",
+        "--aspects=//rust:defs.bzl%rustfmt_aspect",
         "--output_groups=rustfmt_manifest",
     ];

It turns out when the following is evaluated:

if rust_common.crate_info not in target:
return []

There is a difference between CrateInfo coming from @rules_rust and //. I don't understand why but I'd say for users of rules_rust they should be experiencing correct behavior.

@hlopko
Copy link
Member

hlopko commented May 25, 2021

There is a difference between CrateInfo coming from @rules_rust and //

What kind of difference? Targets coming from @rules_rust don't have CrateInfo and targets coming from // do? Or something else? Both are surprising to me.

@UebelAndre
Copy link
Collaborator Author

Sorry for the delay here.

With this diff, if you bazel build --config=rustfmt //... from the root of the repo, you'll run into an error

diff --git a/.bazelrc b/.bazelrc
index db9695b..a61cbe3 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -2,5 +2,5 @@
 # https://docs.bazel.build/versions/master/best-practices.html#using-the-bazelrc-file

 # Enable rustfmt for all targets in the workspace
-build:rustfmt --aspects=//rust:defs.bzl%rustfmt_aspect
+build:rustfmt --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect
 build:rustfmt --output_groups=+rustfmt_checks
diff --git a/rust/private/rustfmt.bzl b/rust/private/rustfmt.bzl
index 9c59f48..789791a 100644
--- a/rust/private/rustfmt.bzl
+++ b/rust/private/rustfmt.bzl
@@ -13,6 +13,11 @@ def _find_rustfmtable_srcs(target, aspect_ctx = None):
     Returns:
         list: A list of formattable sources (`File`).
     """
+
+    if aspect_ctx and aspect_ctx.rule.kind in ["rust_library", "rust_binary", "rust_test"]:
+        print(target)
+        crate_info = target[rust_common.crate_info]
+
     if rust_common.crate_info not in target:
         return []

This is that error

DEBUG: /Users/user/Code/rules_rust/rust/private/rustfmt.bzl:18:14: <target //tools/runfiles:runfiles, keys:[CrateInfo, DepInfo, CcInfo, OutputGroupInfo]>
ERROR: /Users/user/Code/rules_rust/tools/runfiles/BUILD.bazel:8:13: in @rules_rust//rust/private:rustfmt.bzl%rustfmt_aspect aspect on rust_library rule //tools/runfiles:runfiles:
Traceback (most recent call last):
	File "/private/var/tmp/_bazel_user/76282c66b0dfe3c5cb9a230bdc913a52/external/rules_rust/rust/private/rustfmt.bzl", line 83, column 34, in _rustfmt_aspect_impl
		srcs = _find_rustfmtable_srcs(target, ctx)
	File "/private/var/tmp/_bazel_user/76282c66b0dfe3c5cb9a230bdc913a52/external/rules_rust/rust/private/rustfmt.bzl", line 19, column 28, in _find_rustfmtable_srcs
		crate_info = target[rust_common.crate_info]
Error: <target //tools/runfiles:runfiles> (rule 'rust_library') doesn't contain declared provider 'CrateInfo'
ERROR: Analysis of aspect '@rules_rust//rust:defs.bzl%rustfmt_aspect of //tools/runfiles:runfiles' failed; build aborted: Analysis of target '//tools/runfiles:runfiles' failed

I forget what the failure mode was that lead me to an error message that lead me to find this works from the repo root if @rules_rust is omitted but I think that's a straight forward enough reproduction of the issue. I think I should be able to use the following diff and have everything continue to work.

diff --git a/.bazelrc b/.bazelrc
index db9695b..a61cbe3 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -2,5 +2,5 @@
 # https://docs.bazel.build/versions/master/best-practices.html#using-the-bazelrc-file

 # Enable rustfmt for all targets in the workspace
-build:rustfmt --aspects=//rust:defs.bzl%rustfmt_aspect
+build:rustfmt --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect
 build:rustfmt --output_groups=+rustfmt_checks

Unfortunately, it doesn't and @rules_rust must be dropped to get the aspect to function in the root of rules_rust.

@hlopko
Copy link
Member

hlopko commented Jun 9, 2021

Oh that's a Bazel bug. I just found bazelbuild/bazel#11734.

@UebelAndre
Copy link
Collaborator Author

Ah, neat. Should I close this issue then?

@hlopko
Copy link
Member

hlopko commented Jun 9, 2021

Yeah let's do that.

Bazel team, if you're reading this, this is not resolved, we are just using a workaround and we don't want to keep the issue open only to wait for the fix in Bazel :)

@hlopko hlopko closed this as completed Jun 9, 2021
@hlopko hlopko changed the title @rules_rust//tools/rustfmt only runs on built targets [not resolved, just worked around]@rules_rust//tools/rustfmt only runs on built targets Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants