[bazel] Swap to rules_rs for Rust build rules#17398
[bazel] Swap to rules_rs for Rust build rules#17398dzbarsky wants to merge 6 commits intoSeleniumHQ:trunkfrom
Conversation
Review Summary by QodoMigrate Rust build rules from rules_rust to rules_rs
WalkthroughsDescription• Migrate Rust build system from rules_rust to rules_rs • Simplify Cargo dependency management without bazel lockfiles • Remove manual cargo repinning task and workflow • Add platform-specific constraints for Linux and Windows • Update Bazel module dependencies and toolchain configuration File Changes1. MODULE.bazel
|
Code Review by Qodo
1. Missing rust:update task
|
| Bazel.execute('build', args, '//rust:selenium-manager') | ||
| end | ||
|
|
||
| desc 'Update the rust lock files' |
There was a problem hiding this comment.
i'm not familiar with rake, is there anything additional that needs to be done here?
| ], | ||
| use_experimental_platforms = True, | ||
| ) | ||
| crate.annotation( |
There was a problem hiding this comment.
rules_rs has recommended annotations for these to swap to rules_cc based builds, but I kept things as they were for this change
| Bazel.execute('build', args, '//rust:selenium-manager') | ||
| end | ||
|
|
||
| desc 'Update the rust lock files' |
There was a problem hiding this comment.
1. Missing rust:update task 🐞 Bug ≡ Correctness
The PR deletes the rust:update rake task, but Rakefile still invokes it in release_updates, which will fail at runtime with an unknown task error. This breaks the release preparation workflow when running rake release_updates.
Agent Prompt
### Issue description
`rake release_updates` invokes `rust:update`, but this PR removed the `rust:update` task definition from `rake_tasks/rust.rake`, so the release workflow will now fail with an unknown-task error.
### Issue Context
With the switch to `rules_rs`, there is no longer a Bazel-specific Rust lockfile repin step, so the old `rust:update` task was deleted. The top-level release task still expects it to exist.
### Fix Focus Areas
- Rakefile[112-122]
- rake_tasks/rust.rake[9-25]
### Suggested fix
Remove `Rake::Task['rust:update'].invoke` from the `release_updates` task (or reintroduce a `rust:update` task as a backward-compatible no-op if you still want the task name to exist).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Removing the rust:update call, do we need to keep rust:update as a no-op to avoid breaking workflows or is it fine to remove?
There was a problem hiding this comment.
rust:update is still called in release.yml and pre-release.yml workflows. It would be better to update those instead of keeping it as a no-op.
|
@dzbarsky could you sign the CLA before we can look to merge it? |
19bc642 to
6a0166d
Compare
yep, all good! |
shs96c
left a comment
There was a problem hiding this comment.
I like the idea. Just a question.... Also, was this tested on macOS?
| Bazel.execute('build', args, '//rust:selenium-manager') | ||
| end | ||
|
|
||
| desc 'Update the rust lock files' |
| # For build stamping | ||
| build --enable_platform_specific_config | ||
| common --enable_platform_specific_config | ||
| common:linux --host_platform=//:local_linux_gnu |
There was a problem hiding this comment.
Why do we need these changes? The local cc_toolchain setup should already have set these to the right values. I also notice that macos is skipped.
There was a problem hiding this comment.
it's because rules_rs, unlike rules_rust, supports multiple target triples per OS (*-linux-gnu and *-linux-musl, *-windows-gnullvm, *-windows-gnu, *-windows-msvc). It was not possible to target musl and non-msvc windows in rules_rust. However, this means there's an extra constraint needed to distinguish these targets, and since we need to also build targeting the host platform (for proc macros and cfg=exec targets like build scripts), we need to specify the constraint on the host as well.
Macos doesn't have these variants, hence it doesn't need the extra configuration! (and yes this was tested on macos)
| Bazel.execute('build', args, '//rust:selenium-manager') | ||
| end | ||
|
|
||
| desc 'Update the rust lock files' |
There was a problem hiding this comment.
rust:update is still called in release.yml and pre-release.yml workflows. It would be better to update those instead of keeping it as a no-op.
|
@cgoldberg thanks for pointing out the |
|
@dzbarsky yea, I think your workflow changes look correct. thanks. |
|
@cgoldberg I had a CI failure with the remote build using a platform without the constraint and thus failing toolchain resolution. Updated and repushed, could you help trigger CI? Would love a bit of help getting this in! |
It's running now |
hmm i'm not sure how target selection is implemented but Rust job was skipped, that seems dangerous :) |
I'm not sure how the target selection works either, and I can't figure out how to trigger the Rust job manually on your branch :/ |
|
CI is failing all over the place because GitHub Actions is having issues right now. I'll try to figure it out once they get things stabilized. |
💥 What does this PR do?
This PR migrates to using rules_rs instead of rules_rust for our Cargo lockfile integration and toolchains. rules_rs is a redesign ruleset that can operate directly on Cargo.toml/Cargo.lock without the need for additional bazel lockfiles or a bazel-specific repin operation. The toolchains are also optimized to minimize downloads of duplicative bytes compared to the default rules_rust ones.
🔧 Implementation Notes
This was straightforward.
🤖 AI assistance
Codex 0-shot :) It followed the instructions in rules_rs README perfectly.
💡 Additional Considerations
N/a
🔄 Types of changes
Cleanup / build ergonomics improvement