-
Notifications
You must be signed in to change notification settings - Fork 482
regressive commit #1166
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
Comments
Hey @wt, I'm sorry for the trouble. I think your repro instructions are a little bit off (there is no package
That said, I now realize that our change was disruptive, since it added a package that in order to be analyzed correctly needs added external repositories... We ignored this problem in the past, and our handwavy solution was to have releases of rules_rust. AFAIK there is no progress on releases in the last months though. Maybe we should push for that? |
@hlopko Just to make sure I understand, those dependencies aren't actually used unless someone enables this new import feature with a flag, right? But the repositories (and I guess targets) now need to exist because otherwise bazel errors at loading time even though they're not actually used and it will prune them out at analysis time? If so, maybe we could work out a less intrusive way of doing this? In other places, I've created wrapper macros, either which internally delegate to the real rules: def import_supporting_rust_library(**kwargs):
proc_macro_deps = kwargs.get("proc_macro_deps", [])
if "my_lib" not in proc_macro_deps:
proc_macro_deps += ["my_lib"]
kwargs["proc_macro_deps = proc_macro_deps
rust_library(
**kwargs,
) or which decorate them like def add_import(rule, **kwargs):
proc_macro_deps = kwargs.get("proc_macro_deps", [])
if "my_lib" not in proc_macro_deps:
proc_macro_deps += ["my_lib"]
kwargs["proc_macro_deps = proc_macro_deps
rule(**kwargs)
...
add_import(
rust_library,
...
) And these can be loaded by people who want this feature instead of the standard ones. If that's not workable, could we perhaps have the default workspace setup script create some dummy empty repositories for them, and require that callers initialise these repos before the default workspace setup? It feels like requiring all users of rules_rust to pull in (currently) 18 crates into a workspace, and then ignore them, is a pretty heavyweight way of implementing an experimental optional feature currently only expected to be used at Google. |
The hello_world target is a target in my repo. My point was that there is nothing special about the target. You should be able to do the same with any rust target. Also, is there any feasible way to revert this until a fix is found. This is blocking an upgrade to 1.59.0 for us, and I just want to know what our options are. |
Potentially related, It does seem the new
A repro is just following the setup for https://bazelbuild.github.io/rules_rust/rust_analyzer.html within |
@illicitonion yup I agree, this is not handled well, I just didn't think of this scenario. @wt could you try #1177? I think it should fix your issue. @UebelAndre going to take a look now. |
#1177 does not fix this:
|
This should be reverted if no solution is on the horizon. |
@wt Can you provide your example workspace so someone could iterate on this locally? |
You don't need my workspace. Starting at your rules_rust root:
|
|
There is another command before the bazel query command. It changes to the right directory. |
Yes, I probably do mean that. |
Nope, you have to change into the directory. You can't just do It's another workspace. |
I'm afraid I cannot repro the precise scenario you are hitting. More specifically, #1177 fixes all the scenarios I tested.
produces the failure (as expected):
Now with #1177:
I get a successful run. The output is a bit long, so grepping for
There is no Could you try to isolate and minimize the repro case? For the time being, you can use the |
#1177 now seems to fix this issue. I had sync'd the branch before a merge with master. After the merge, we are all good. Please land with prejudice. |
I ran into this, since I use
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
name = "rules_rust",
sha256 = "4bc9124b7ac63ccca8e5eb1a09d57d00dc5e442a9c7fef16cc6f7958ae95c05a",
strip_prefix = "rules_rust-7c865ffeb1472c6ff2541c221169ba706ea0d839",
urls = [
"https://github.com/bazelbuild/rules_rust/archive/7c865ffeb1472c6ff2541c221169ba706ea0d839.tar.gz",
],
)
load("@rules_rust//rust:repositories.bzl", "rules_rust_dependencies", "rust_register_toolchains")
rules_rust_dependencies()
load("@rules_rust//rust:repositories.bzl", "rust_repositories")
rust_repositories()
# Fails with this commented out
# load("@rules_rust//util/import:deps.bzl", "import_deps")
#
# import_deps()
load("@rules_rust//rust:defs.bzl", "rust_library")
rust_library(
name = "lib",
srcs = ["lib.rs"],
)
pub fn test() {} When invoking a basic query: viz@vizblade:~/query_repro$ bazel query 'deps(//:lib)'
ERROR: /home/viz/.cache/bazel/_bazel_viz/2a819faec116417ac151d6e11b830676/external/rules_rust/util/import/raze/BUILD.bazel:60:6: no such package '@rules_rust_util_import__syn__1_0_82//': The repository '@rules_rust_util_import__syn__1_0_82' could not be resolved and referenced by '@rules_rust//util/import/raze:syn'
ERROR: /home/viz/.cache/bazel/_bazel_viz/2a819faec116417ac151d6e11b830676/external/rules_rust/util/import/raze/BUILD.bazel:33:6: no such package '@rules_rust_util_import__proc_macro2__1_0_33//': The repository '@rules_rust_util_import__proc_macro2__1_0_33' could not be resolved and referenced by '@rules_rust//util/import/raze:proc_macro2'
ERROR: /home/viz/.cache/bazel/_bazel_viz/2a819faec116417ac151d6e11b830676/external/rules_rust/util/import/raze/BUILD.bazel:51:6: no such package '@rules_rust_util_import__quote__1_0_10//': The repository '@rules_rust_util_import__quote__1_0_10' could not be resolved and referenced by '@rules_rust//util/import/raze:quote'
ERROR: /home/viz/.cache/bazel/_bazel_viz/2a819faec116417ac151d6e11b830676/external/rules_rust/util/import/raze/BUILD.bazel:60:6: no such package '@rules_rust_util_import__syn__1_0_82//': The repository '@rules_rust_util_import__syn__1_0_82' could not be resolved and referenced by '@rules_rust//util/import/raze:syn'
ERROR: /home/viz/.cache/bazel/_bazel_viz/2a819faec116417ac151d6e11b830676/external/rules_rust/util/import/raze/BUILD.bazel:15:6: no such package '@rules_rust_util_import__aho_corasick__0_7_15//': The repository '@rules_rust_util_import__aho_corasick__0_7_15' could not be resolved and referenced by '@rules_rust//util/import/raze:aho_corasick'
ERROR: /home/viz/.cache/bazel/_bazel_viz/2a819faec116417ac151d6e11b830676/external/rules_rust/util/import/raze/BUILD.bazel:24:6: no such package '@rules_rust_util_import__lazy_static__1_4_0//': The repository '@rules_rust_util_import__lazy_static__1_4_0' could not be resolved and referenced by '@rules_rust//util/import/raze:lazy_static'
ERROR: Evaluation of query "deps(//:lib)" failed: errors were encountered while computing transitive closure Uncommenting the
All of this feels pretty invasive for for something that's intended to be optional and probably won't be used outside of google. Reopening primarily because of 3) in that list. |
What version are you using? |
Apologies for the confusion, yes. |
@dfreese do you need to support Bazel <4.2? I was about to suggest to bump the min bazel version to 5.0 since we're only claiming in COMPATIBILITY that we support the latest Bazel LTS (and the overlap time with previous LTS is 3 months, and is going to pass soon). |
Sorry for the delay, missed the response. I was able to bump our team to 5.0, so bumping to 4.2 would be fine, though, unless it. It would be nice not to directly inject this unless it was really necessary. @hlopko is a wrapping macro usable internally? If not, do we expect this to be enough of a pattern that it's worth adding something to the toolchain to support injecting dependencies folks may want to add globally, such as in this case? |
Can this issue be closed at this point? If not, what would be needed to close it out? |
Yes I do anticipate that folks will need to inject dependencies, so adding a principled, supported feature for this sounds like a great idea. We'll explore the options in a separate issue/PR. Thanks! |
As for this issue, closing sounds good to me. |
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]>
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]>
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]>
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]>
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]>
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
Even though we're not using these, Bazel 7 hits errors if we try to run `bazel query deps($something)` on a Rust dependency. Fixed in `rules_rust>=0.38.0`. bazelbuild/rules_rust#1166 (comment) Signed-off-by: James Wainwright <[email protected]> (cherry picked from commit f9f257a)
3db49e8 is a regressive commit.
This commit breaks a very simple query for me.
I have a simple hello world rust target here:
//rust/example/hello_world:hello_world
.I run the following query:
bazel query 'deps(//rust/example/hello_world:hello_world)'
Before 3db49e8 (specifically 5591596), it works. After that commit, I get the following:
I also get this on the latest commit, which is 78d702b at the time of this issue's creation.
The text was updated successfully, but these errors were encountered: