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.

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

Shard rules_rust into multiple bzlmod modules #2882

Open
UebelAndre opened this issue Sep 18, 2024 · 5 comments · May be fixed by #3007
Open

Shard rules_rust into multiple bzlmod modules #2882

UebelAndre opened this issue Sep 18, 2024 · 5 comments · May be fixed by #3007
Labels

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.

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Nov 19, 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants