rules_rust icon indicating copy to clipboard operation
rules_rust copied to clipboard

Genquery: cycle in dependency graph

Open UebelAndre opened this issue 2 years ago • 5 comments

It seems using genquery on any Rust targets leads to failures due to a cycle in the dependency graph. The diff below can be used to repro the issue and get the output that follows.

diff --git a/crate_universe/BUILD.bazel b/crate_universe/BUILD.bazel
index 370dc5c3..ffb18c11 100644
--- a/crate_universe/BUILD.bazel
+++ b/crate_universe/BUILD.bazel
@@ -76,6 +76,12 @@ rust_binary(
     deps = [":cargo_bazel"],
 )

+genquery(
+    name = "genquery",
+    scope = [":cargo_bazel_bin"],
+    expression = "deps(:cargo_bazel_bin)",
+)
+
 alias(
     name = "bin",
     actual = ":cargo_bazel_bin",
~/Code/rules_rust (main ✗) bazel build //crate_universe:genquery
ERROR: /Users/user/Code/rules_rust/util/process_wrapper/BUILD.bazel:31:36: in rust_binary_without_process_wrapper rule //util/process_wrapper:process_wrapper: cycle in dependency graph:
    //crate_universe:genquery (e5528793e6772dcf5ff2b9774ea78fbb21b115ab96d65cadb8cb5b68206d2fe6)
    //crate_universe:genquery (577c459edef5fa899d167957b8c740aeb5edc881970dd81d94adeef309381026)
    //crate_universe:cargo_bazel_bin
.-> //util/process_wrapper:process_wrapper
|   //util/import:import
|   //util/import:import_macro_label
|   //util/import:import_macro
|   //util/import:import_macro_impl
`-- //util/process_wrapper:process_wrapper
ERROR: Analysis of target '//crate_universe:genquery' failed; build aborted

UebelAndre avatar Feb 27 '24 16:02 UebelAndre

cc @scentini @cfredric

UebelAndre avatar Feb 27 '24 16:02 UebelAndre

I believe the issue is caused by https://github.com/bazelbuild/rules_rust/blob/0.39.0/rust/private/rust.bzl#L531-L534

UebelAndre avatar Feb 27 '24 16:02 UebelAndre

Could you the fix simply be removing _import_macro_dep from rust_binary_without_process_wrapper?

tidefield avatar Mar 18 '24 14:03 tidefield

I opened https://github.com/bazelbuild/rules_rust/pull/2559 since it doesn't seem like there's a simple fix here. The import stuff likely needs it's own bootstrapping rule. Since that code is dead in the repo though, I think it's fair to remove this integration. Though I'd be happy to review a better fix. As long as the new integration test I added passes I'd be a happy camper.

UebelAndre avatar Mar 18 '24 16:03 UebelAndre

@gregce @katre it seems like the issue here is genquery's sensitivity to loading phase dependency cycles. Looking through internal channels, --experimental_skip_ttvs_for_genquery should be the fix, however it is not enabled by default for Bazel; is there a reason for it?

scentini avatar Mar 26 '24 17:03 scentini

@scentini I'm gonna close this issue out since the issue is fixed and there's now regression testing for it. If there's a new flag being flipped in Bazel that would make this better than we can get better integration for the import_macro in at that time.

That said, feel free to re-open if you feel it was closed in error!

UebelAndre avatar Jun 25 '24 04:06 UebelAndre