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.

Sign up for GitHub

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

Jump to bottom

Shard rules_rust into multiple bzlmod modules #2882

Closed
UebelAndre opened this issue Sep 18, 2024 · 7 comments · Fixed by #3007
Closed

Shard rules_rust into multiple bzlmod modules #2882

UebelAndre opened this issue Sep 18, 2024 · 7 comments · Fixed by #3007
Labels
bzlmod

Comments

@UebelAndre
Copy link
Collaborator

It seems to me that the MODULE.bazel file is getting a bit unwieldy and rust is having to declare dependencies for things not all users want or need.

rules_rust/MODULE.bazel

Lines 37 to 45 in 350d249

bazel_dep(
name = "protobuf",
version = "21.7",
repo_name = "com_google_protobuf",
)
bazel_dep(
name = "aspect_rules_js",
version = "1.39.0",
)

rules_rust/MODULE.bazel

Lines 63 to 160 in 350d249

internal_deps = use_extension("//rust/private:extensions.bzl", "i")
use_repo(
internal_deps,
"cargo_bazel.buildifier-darwin-amd64",
"cargo_bazel.buildifier-darwin-arm64",
"cargo_bazel.buildifier-linux-amd64",
"cargo_bazel.buildifier-linux-arm64",
"cargo_bazel.buildifier-windows-amd64.exe",
"cui",
"cui__anyhow-1.0.75",
"cui__camino-1.1.6",
"cui__cargo-lock-9.0.0",
"cui__cargo-platform-0.1.4",
"cui__cargo_metadata-0.18.1",
"cui__cargo_toml-0.19.2",
"cui__cfg-expr-0.15.5",
"cui__clap-4.3.11",
"cui__crates-index-2.2.0",
"cui__hex-0.4.3",
"cui__indoc-2.0.4",
"cui__itertools-0.12.0",
"cui__maplit-1.0.2",
"cui__normpath-1.1.1",
"cui__once_cell-1.19.0",
"cui__pathdiff-0.2.1",
"cui__regex-1.10.2",
"cui__semver-1.0.20",
"cui__serde-1.0.190",
"cui__serde_json-1.0.108",
"cui__serde_starlark-0.1.14",
"cui__sha2-0.10.8",
"cui__spdx-0.10.3",
"cui__spectral-0.6.0",
"cui__tempfile-3.8.1",
"cui__tera-1.19.1",
"cui__textwrap-0.16.0",
"cui__toml-0.8.10",
"cui__tracing-0.1.40",
"cui__tracing-subscriber-0.3.17",
"cui__url-2.5.2",
"generated_inputs_in_external_repo",
"libc",
"llvm-raw",
"rrra__anyhow-1.0.71",
"rrra__clap-4.3.11",
"rrra__env_logger-0.10.0",
"rrra__itertools-0.11.0",
"rrra__log-0.4.19",
"rrra__serde-1.0.171",
"rrra__serde_json-1.0.102",
"rules_rust_bindgen__bindgen-0.69.1",
"rules_rust_bindgen__bindgen-cli-0.69.1",
"rules_rust_bindgen__clang-sys-1.6.1",
"rules_rust_bindgen__clap-4.3.3",
"rules_rust_bindgen__clap_complete-4.3.1",
"rules_rust_bindgen__env_logger-0.10.0",
"rules_rust_prost",
"rules_rust_prost__h2-0.4.6",
"rules_rust_prost__heck",
"rules_rust_prost__prost-0.13.1",
"rules_rust_prost__prost-types-0.13.1",
"rules_rust_prost__protoc-gen-prost-0.4.0",
"rules_rust_prost__protoc-gen-tonic-0.4.1",
"rules_rust_prost__tokio-1.39.3",
"rules_rust_prost__tokio-stream-0.1.15",
"rules_rust_prost__tonic-0.12.1",
"rules_rust_proto__grpc-0.6.2",
"rules_rust_proto__grpc-compiler-0.6.2",
"rules_rust_proto__log-0.4.17",
"rules_rust_proto__protobuf-2.8.2",
"rules_rust_proto__protobuf-codegen-2.8.2",
"rules_rust_proto__tls-api-0.1.22",
"rules_rust_proto__tls-api-stub-0.1.22",
"rules_rust_test_load_arbitrary_tool",
"rules_rust_tinyjson",
"rules_rust_toolchain_test_target_json",
"rules_rust_wasm_bindgen__anyhow-1.0.71",
"rules_rust_wasm_bindgen__assert_cmd-1.0.8",
"rules_rust_wasm_bindgen__diff-0.1.13",
"rules_rust_wasm_bindgen__docopt-1.1.1",
"rules_rust_wasm_bindgen__env_logger-0.8.4",
"rules_rust_wasm_bindgen__log-0.4.19",
"rules_rust_wasm_bindgen__predicates-1.0.8",
"rules_rust_wasm_bindgen__rayon-1.7.0",
"rules_rust_wasm_bindgen__rouille-3.6.2",
"rules_rust_wasm_bindgen__serde-1.0.171",
"rules_rust_wasm_bindgen__serde_derive-1.0.171",
"rules_rust_wasm_bindgen__serde_json-1.0.102",
"rules_rust_wasm_bindgen__tempfile-3.6.0",
"rules_rust_wasm_bindgen__ureq-2.8.0",
"rules_rust_wasm_bindgen__walrus-0.20.3",
"rules_rust_wasm_bindgen__wasm-bindgen-0.2.92",
"rules_rust_wasm_bindgen__wasm-bindgen-cli-support-0.2.92",
"rules_rust_wasm_bindgen__wasm-bindgen-shared-0.2.92",
"rules_rust_wasm_bindgen__wasmparser-0.102.0",
"rules_rust_wasm_bindgen__wasmprinter-0.2.60",
"rules_rust_wasm_bindgen_cli",
)

rules_rust/MODULE.bazel

Lines 170 to 180 in 350d249

register_toolchains(
"//proto/protobuf:default-proto-toolchain",
)
register_toolchains(
"//proto/prost:default_prost_toolchain",
)
register_toolchains(
"//bindgen:default_bindgen_toolchain",
)

To improve maintainability and to keep the dependency trees tight, I'm wondering if it'd be better to have different bzlmod extensions for the various packages within rules_rust. Concretely, I'm proposing that @rules_rust//bindgen become @rules_rust_bindgen and be an independent module developers can choose to include but it's dependencies would not be a part of @rules_rust.

The scope of the proposed changed:

before load after load
@rules_rust//bindgen @rules_rust_bindgen
@rules_rust//proto/protobuf @rules_rust_protobuf
@rules_rust//proto/prost @rules_rust_prost
@rules_rust//wasm_bindgen @rules_rust_wasm_bindgen

The @rules_rust//crate_universe, @rules_rust//cargo, @rules_rust//tools, and @rules_rust//util and any dependencies they may have would remain the same (in core @rules_rust).

I don't have strong opinions on changes to repo structure but I'm thinking it would be good to create an extensions directory where bindgen, proto, and wasm_bindgen could be moved into which I think would make it easier to add additional integrations down the line.
@illicitonion
Copy link
Collaborator

Sounds good to me!

I guess we would still release all of these at the same time (i.e. there would always be matching versions)? Or would we move to more granular releases of each?

I think my only concern is that we'll need to be a little careful around compatibility (e.g. if we add/remove attrs to crate_universe, we introduce compatibility hazards if people start using older rules_rust with newer bindgen or similar).

But I'm happy to do the splitting up and work out the exact details about compatibility in the future - the bzlmod ecosystem as a whole is still working out compatibility somewhat...

@UebelAndre
Copy link
Collaborator Author

I guess we would still release all of these at the same time (i.e. there would always be matching versions)? Or would we move to more granular releases of each?

I would keep everything at the same version and recommend folks keep them all in sync.

I think my only concern is that we'll need to be a little careful around compatibility (e.g. if we add/remove attrs to crate_universe, we introduce compatibility hazards if people start using older rules_rust with newer bindgen or similar).

Yeah, I figured this would be unlikely and we could advise folks to keep versions in sync.

But I'm happy to do the splitting up and work out the exact details about compatibility in the future - the bzlmod ecosystem as a whole is still working out compatibility somewhat...

If there are no major objections then I think it'd be good. Do you have thoughts on moving any code around? I could do some moves and add the commit to a .git-blame-ignore-revs file to keep history looking better.

@illicitonion
Copy link
Collaborator

Happy to move code around however makes sense :) Probably easier to discuss in a proposed PR?

@konkers
Copy link
Contributor

konkers commented Sep 18, 2024

Looks reasonable to me.

This was referenced Nov 19, 2024
Merged
Closed
@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Nov 19, 2024 •
edited
Loading

Instead of have a workspace for each extension, I've opted to create rules_rust_ext that contains everything at #3007. I think this is a happy medium and will improve our ability to maintain core and 3rd party Rust rules.

The design still not yet final so any suggestions are welcome!

@UebelAndre
Copy link
Collaborator Author

Instead of have a workspace for each extension, I've opted to create rules_rust_ext that contains everything at #3007. I think this is a happy medium and will improve our ability to maintain core and 3rd party Rust rules.

The design still not yet final so any suggestions are welcome!

With the support of the BazelCI team we're able to move forward with the original proposal! There will no longer be a rules_rust_ext repository and instead each package will be it's own workspace.

@konkers
Copy link
Contributor

konkers commented Nov 26, 2024

Instead of have a workspace for each extension, I've opted to create rules_rust_ext that contains everything at #3007. I think this is a happy medium and will improve our ability to maintain core and 3rd party Rust rules.
The design still not yet final so any suggestions are welcome!

With the support of the BazelCI team we're able to move forward with the original proposal! There will no longer be a rules_rust_ext repository and instead each package will be it's own workspace.

Awesome!

github-merge-queue bot pushed a commit that referenced this issue Nov 26, 2024
@UebelAndre
Moved rust extension rules into a separate rules_rust_ext workspace. (
2bad96d 
#3007)

This change moves the `bindgen`, `proto`, and `wasm_bindgen`
sub-packages into individual workspaces within the `extensions`
directory. The intent is improve ease of maintenance of both core and
extension Rust rules by ensuring changes to extensions have no impact on
the core rules. Core rules should *never* depend on extensions.

Load statements should be updated according to the following table:

| before | after |
| --- | --- |
| `@rules_rust//bindgen` | `@rules_rust_bindgen//` |
| `@rules_rust//proto/prost` | `@rules_rust_prost//` |
| `@rules_rust//proto/protobuf` | `@rules_rust_protobuf//` |
| `@rules_rust//wasm_bindgen` | `@rules_rust_wasm_bindgen//` |

closes #2882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bzlmod
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Moved rust extension rules into a separate workspaces. UebelAndre/rules_rust
3 participants
@konkers @illicitonion @UebelAndre