[email protected] #1199
Conversation
![publish-to-bcr-bot[bot]](https://avatars.githubusercontent.com/in/234555?s=60&v=4){kind=small}
fmeum
added
the
presubmit-auto-run
| version = "0.34.1", | ||
| ) | ||
|
|
||
| print("WARNING: The rules_rust Bazel module is still highly experimental and subject to change at any time. Only use it to try out bzlmod for now.") # buildifier: disable=print |
There was a problem hiding this comment.
prints in module files are displayed when the module file is evaluated, which happens during version resolution and thus even if this module doesn't end up being selected. For this reason, getting rid of this warning is extremely difficult as you need to make sure its never mentioned by any module file in the full transitive closure. The only way is usually to yank these versions and even that required changes to Bazel (bazelbuild/bazel@2a2a474). We tried this in rules_go and now consider it a mistake.
Instead, if you really want to show this warning (we learned that almost nobody cared), I would recommend doing so from a module extension. That way, the warning is only shown if that version of the module is selected and actually used.
There was a problem hiding this comment.
I added this warning, but only did so because rules_go did so and so I thought it was best practice. Happy to remove it.
There was a problem hiding this comment.
Fix is in bazelbuild/rules_rust#2345.
| rust = use_extension("//rust:extensions.bzl", "rust") | ||
|
|
||
| # Allow us to run, for example, "bazel build //tools/runfiles" with bzlmod. | ||
| # Register it as a dev dependency so that we don't force this toolchain on | ||
| # downstream users. | ||
| rust.toolchain(edition = "2021") | ||
| use_repo(rust, "rust_toolchains") | ||
|
|
||
| register_toolchains( | ||
| "@rust_toolchains//:all", | ||
| dev_dependency = True, | ||
| ) | ||
|
|
||
| use_repo(rust, "rust_host_tools") |
There was a problem hiding this comment.
Do you really want to declare the toolchain tag as a non-dev dependency, but then only register the toolchain as a dev dependency? Based on the comment, I would have expected the rust extension usage to also come with dev_dependency = True.
What is the story around default toolchain registrations for rules_rust? If a ruleset needs to compile a rust tool from source, can it rely on some "reasonable" Rust toolchain being registered even if the root module itself doesn't use rules_rust?
There was a problem hiding this comment.
Ah yeah, it should probably either be use_extension as a dev dependency or register toolchains without a dev dependency. My intention was to ensure that it was explicit and that you didn't accidentally use the default toolchain when you'd intended to use a custom one, so I'd intended for use_extension to be a dev dependency.
However, I'd like to hear your opinion on best practices. Is one option preferred over the other?
There was a problem hiding this comment.
As Xudong likes to put it, all of us are just "winging it" when it comes to Bzlmod - it's too early proven best practices. What I found to work well so far is to have rulesets with relatively canonical SDKs (mostly not C++) promise to always register some reasonably recent version of a hermetic SDK so that transitive deps of a project can use rust_binary without placing the toolchain resolution burden on the root module. That does seem to work for Go, but whether it makes sense for Rust depends a lot on the ecosystem.
There was a problem hiding this comment.
I believe this is fixed by bazelbuild/rules_rust#2351.
There was a problem hiding this comment.
Does this still allow the root module to override the toolchain used?
There was a problem hiding this comment.
Yes. According to the bazel docs, the toolchain resolution is:
- Toolchains registered using --extra_toolchains are added first. (Within these, the last toolchain has highest priority.)
- Toolchains registered using register_toolchains in the transitive external dependency graph, in the following order: (Within these, the first mentioned toolchain has highest priority.)
- Toolchains registered by the root module (as in, the MODULE.bazel at the workspace root);
- Toolchains registered in the user's WORKSPACE file, including in any macros invoked from there;
- Toolchains registered by non-root modules (as in, dependencies specified by the root module, and their dependencies, and so forth);
- Toolchains registered in the "WORKSPACE suffix"; this is only used by certain native rules bundled with the Bazel installation.
All you need to do is to call register_toolchains from the root module, and you should be able to override the toolchains.
| @@ -0,0 +1,647 @@ | |||
| --- | |||
| aspects_flags: &aspects_flags | |||
There was a problem hiding this comment.
This collection of checks looks very heavy. Most Bzlmod-related failures can already be verified by running just the analysis phase. Given the already heavy load on the bazelbuild CI, would it be possible to cut this down and/or sprinkle in uses of --nobuild?
CC @meteorcloudy who knows the state of the CI system better
There was a problem hiding this comment.
Thanks! Yes, I totally agree with @fmeum. The presubmit.yml file in the BCR is not meant to run all the integration/unit tests in your normal CI pipeline. It should be a few integration tests that cover the most common usages of this module. Ideally, by including a test module
There was a problem hiding this comment.
I will propose a simpler pre-submit check as the full set of checks will have already run for the PRs.
There was a problem hiding this comment.
I put up a PR which introduces a rules_bazel_integration_test that builds and runs a hello world using rules_rust.
There was a problem hiding this comment.
Fixed by bazelbuild/rules_rust#2347.
See discussion in github.com/bazelbuild/bazel-central-registry/pull/1199
See discussion in github.com/bazelbuild/bazel-central-registry/pull/1199
…2347) - Create `bzlmod/hello_world` package based upon `examples/bzlmod/hello_world`. - Add `rules_bazel_integration_test` so that `//bzlmod:hello_world_test` can be executed as a test from the parent workspace. - Add `filegroup` targets named `all_files` to collect files for all of the packages that are necessary to use `rules_rust`. These are an input to the `rules_bazel_integration_test` test. - Replace `.bcr/presubmit.yml` to test `//bzlmod:hello_world_test`. - Update CI to test `//bzlmod:hello_world_test`. Related to bazelbuild/bazel-central-registry#1199 (comment).
Release: https://github.com/bazelbuild/rules_rust/releases/tag/0.34.1
Author: @scentini
fyi: @UebelAndre, @illicitonion
Automated by Publish to BCR