-
Notifications
You must be signed in to change notification settings - Fork 39
build: setup msrv and edition consistently in all Cargo.toml #706
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
base: main
Are you sure you want to change the base?
Conversation
crates/superposition_sdk/Cargo.toml
Outdated
| description = "Rust SDK to work with Superposition" | ||
| edition = "2021" | ||
| edition = { workspace = true } | ||
| rust-version = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I am not wrong, this would get reverted in smithy generation
and I suppose we should use a different value just for the provider, so that we can have a higher inclusion percent
the server can be at a high MSRV like 1.82.0 but the sdk and the provider should not be dependent on this high of a number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was based on @SanchithHegde's comment using the cargo msrv command (I think uniffi forces it). Running it currently. Will add the output to the PR.
Can we configure smithy to generate this the way we want it? cc: @ShubhranshuSanjeev @ShreyBana
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knutties
smithy-build.json takes minimumSupportedRustVersion as config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➜ superposition git:(fix-msrv) ✗ cargo msrv verify --rust-version 1.81
[Meta] cargo-msrv 0.18.4
Compatibility Check #1: Rust 1.81.0
◝ Rust 1.81.0 Elapsed 2s ◞ Rust 1.81.0 Elapsed 3s ◡ Rust 1.81.0 Elapsed 3s ◜ Rust 1.81.0 Elapsed 4s [FAIL] Is incompatible
...
uniffi errorsOne option is to skip uniffi in rust builds via feature flag (which is actually a good idea by @SanchithHegde). I will do this as well in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knutties you would need to check the MSRV for the crate individually I guess
also, uniffi would be needed for the provider only (from the import point of view)
I was thinking something:
sdk and provider to have their respective MSRVs
and all other crates can use a common one from workspace (if at all a common MSRV is needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - that is the plan. For crates that wider people have to use - we can lower the msrv - but we will need to remove uniffi dependency in Rust SDKs as it is not needed. uniffi is not needed for the Rust provider - it is needed only in the other language bindings.
We might need some additional CI checks to ensure we compile with some lower msrv as well. I will do that in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the rust sdk does not include uniffi
|
MSRV and Edition Strategy Recommendations Current State Analysis Uses Rust 1.86.0 in CI and rust-toolchain.toml
[ Then update all crate Cargo.toml files to use: edition = { workspace = true } Rationale: Edition 2021 is stable and well-supported. No need to upgrade to 2024 yet unless you need specific features.
Tier 1: Public SDK/Provider Crates (Lower MSRV for broader compatibility) In each crate's Cargo.toml[
{
msrv-check:
Different crates have different MSRV requirements (uniffi constraint) Verify SDK builds with lower MSRVrustup install 1.75.0 Verify server builds with specified MSRVcargo +1.81.0 check -p context_aware_config Verify everything builds with current toolchaincargo check --all-features
Minimum Supported Rust Version (MSRV)
Summary SDK compatibility (lower MSRV for wider ecosystem adoption) |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughStandardizes workspace-wide Rust version and edition by adding Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @crates/superposition_types/Cargo.toml:
- Around line 3-5: The Cargo manifest sets edition = "2021" explicitly; change
it to inherit the workspace edition by replacing the hardcoded edition with
workspace inheritance (e.g., use edition.workspace = true or edition = {
workspace = true } to match other crates), so the 'edition' key follows the same
workspace-based configuration as 'rust-version'.
🧹 Nitpick comments (4)
crates/cac_client/Cargo.toml (1)
5-6: Same edition inconsistency asexperimentation_client.The
rust-version.workspace = trueaddition is correct. However,edition = "2021"remains hardcoded whilecac_tomlusesedition.workspace = true.For consistency, either:
- Set workspace
edition = "2021"and useedition.workspace = truein all crates, or- Explicitly document why some crates hardcode edition while others inherit it.
crates/superposition_core/Cargo.toml (1)
4-5: Inconsistent edition handling compared to other crates.This crate retains
edition = "2021"hardcoded while adoptingrust-version.workspace = true. Other crates in this PR (e.g.,service_utils) useedition.workspace = true. This creates an inconsistency where:
- If workspace edition is "2024" (as set in root Cargo.toml), this crate stays on "2021"
- If the intent is to keep all crates on "2021", then
edition.workspace = trueshouldn't be used in other cratesConsider aligning the approach—either all crates inherit edition from workspace or all specify it explicitly.
crates/superposition_provider/Cargo.toml (1)
4-5: Inconsistent edition handling; consider SDK MSRV requirements.Same inconsistency as
superposition_core: hardcodededition = "2021"while usingrust-version = { workspace = true }.Additionally, per the reviewer's tiered MSRV recommendation,
superposition_provideris a Tier 1 public SDK crate that should have a lower MSRV (1.75.0) for wider compatibility. Using the workspace's 1.85.0 may unnecessarily restrict consumers.If the intent is to keep this crate at edition 2021 with a lower MSRV, consider setting
rust-version = "1.75.0"explicitly rather than inheriting from workspace.Suggested approach for tiered MSRV
[package] name = "superposition_provider" version.workspace = true edition = "2021" -rust-version = { workspace = true } +rust-version = "1.75.0" license = { workspace = true }smithy/patches/rust.patch (1)
13-13: Consider usingedition.workspace = truefor consistency.Other crates in this PR adopt
edition.workspace = trueto centralize the edition setting. This patch keepsedition = "2021"hardcoded. If the Smithy codegen allows patching this field, consider aligning it with the workspace pattern:-edition = "2021" +edition.workspace = trueIf the Smithy generator enforces a specific edition and re-generates this line, the hardcoded value may be acceptable—but document this exception.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Cargo.tomlcrates/cac_client/Cargo.tomlcrates/cac_toml/Cargo.tomlcrates/context_aware_config/Cargo.tomlcrates/experimentation_client/Cargo.tomlcrates/experimentation_platform/Cargo.tomlcrates/frontend/Cargo.tomlcrates/service_utils/Cargo.tomlcrates/superposition/Cargo.tomlcrates/superposition_core/Cargo.tomlcrates/superposition_derives/Cargo.tomlcrates/superposition_macros/Cargo.tomlcrates/superposition_provider/Cargo.tomlcrates/superposition_sdk/Cargo.tomlcrates/superposition_types/Cargo.tomlrust-toolchain.tomlsmithy/patches/rust.patchsmithy/smithy-build.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-03T13:25:40.584Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 816
File: crates/frontend/src/pages/webhook.rs:136-137
Timestamp: 2026-01-03T13:25:40.584Z
Learning: In the superposition codebase (Rust frontend), the `Workspace` and `OrganisationId` newtype wrappers implement `Deref`, which allows `&Workspace` and `&OrganisationId` to be automatically coerced to `&str` when passed to functions expecting `&str` parameters. Manual `.0` dereferencing is not needed.
Applied to files:
crates/superposition_sdk/Cargo.tomlcrates/superposition_derives/Cargo.tomlcrates/superposition_types/Cargo.toml
📚 Learning: 2026-01-03T10:25:41.325Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 814
File: crates/superposition_types/src/api/workspace.rs:70-77
Timestamp: 2026-01-03T10:25:41.325Z
Learning: The `auto_populate_control` field in `CreateWorkspaceRequest` (`crates/superposition_types/src/api/workspace.rs`) is expected to default to `true`, not `false`. This is implemented using `#[serde(default = "default_true")]`.
Applied to files:
crates/experimentation_client/Cargo.tomlcrates/frontend/Cargo.tomlcrates/superposition_types/Cargo.tomlcrates/superposition_core/Cargo.tomlcrates/superposition_provider/Cargo.toml
📚 Learning: 2026-01-03T13:27:14.072Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 816
File: crates/frontend/src/pages/type_template.rs:82-87
Timestamp: 2026-01-03T13:27:14.072Z
Learning: In the frontend crate, both `Workspace` and `OrganisationId` types implement `Deref` trait (via `#[derive(Deref)]`), allowing automatic coercion from `&Workspace` to `&str` and `&OrganisationId` to `&str`. When passing these types to functions expecting `&str`, use `&workspace` or `&org` directly instead of `&workspace.0` or `&org.0`.
Applied to files:
crates/frontend/Cargo.toml
🔇 Additional comments (13)
crates/experimentation_client/Cargo.toml (1)
5-6: Inconsistent edition handling across crates.This file hardcodes
edition = "2021"whilecrates/cac_toml/Cargo.tomlusesedition.workspace = true. If the workspace definesedition = "2024"(per AI summary), this creates intentional divergence.Clarify the intent:
- If all crates should remain on edition 2021 (as per reviewer recommendation), consider setting
edition = "2021"at workspace level and usingedition.workspace = trueeverywhere.- If this crate intentionally stays on 2021 while others move to 2024, this is fine but should be documented.
crates/superposition_sdk/Cargo.toml (1)
11-11: LGTM for generated code.The hardcoded
rust-version = "1.85.0"is appropriate here since this file is auto-generated by Smithy. The value correctly corresponds to theminimumSupportedRustVersionsetting insmithy/smithy-build.json. Any MSRV adjustments for this SDK should be made in the Smithy configuration, not directly in this file.smithy/smithy-build.json (1)
37-37: The 1.85.0 MSRV is required by Rust's 2024 edition.The codebase uses Rust's 2024 edition (
edition = "2024"in the workspace Cargo.toml), which mandates Rust 1.85.0 as the minimum. This MSRV is set consistently across the workspace and all SDK crates (superposition_types,superposition_provider, andsuperposition_sdkall use 1.85.0), and is not a compatibility concern but rather a hard requirement of the edition selection. No change is possible without reconsidering the edition choice itself.Likely an incorrect or invalid review comment.
crates/service_utils/Cargo.toml (1)
5-6: Workspace inheritance is consistent here.The changes correctly inherit both edition and rust-version from the workspace. However, note that the workspace sets
edition = "2024", which inherently requires Rust 1.85.0+. This means the effective MSRV for this crate will be at least 1.85.0 regardless of any tiered MSRV strategy discussed in the PR comments.rust-toolchain.toml (1)
2-2: Rust 1.90.0 exists and is stable as of January 2026.The toolchain has been updated from 1.86.0 to 1.90.0. This is a valid, stable version (confirmed as of January 12, 2026, with 1.92.0 being the latest stable release). If there are concerns about the version bump magnitude, those should be addressed separately as an architectural decision rather than a toolchain availability issue.
crates/superposition_derives/Cargo.toml (1)
4-5: LGTM - Workspace inheritance for edition and rust-version.The changes correctly inherit
editionandrust-versionfrom the workspace, ensuring consistent configuration across the codebase.crates/superposition/Cargo.toml (1)
5-6: LGTM - Consistent workspace inheritance.The main server crate correctly inherits
editionandrust-versionfrom the workspace configuration.crates/experimentation_platform/Cargo.toml (1)
5-6: LGTM - Workspace inheritance applied consistently.The
editionandrust-versionsettings correctly inherit from the workspace.crates/superposition_macros/Cargo.toml (1)
5-6: LGTM - Workspace inheritance applied correctly.The workspace-scoped settings are properly configured for this macros crate.
crates/context_aware_config/Cargo.toml (1)
5-6: LGTM - Workspace inheritance applied consistently.The
editionandrust-versionsettings correctly inherit from the workspace configuration.crates/frontend/Cargo.toml (1)
5-6: LGTM - Workspace inheritance applied correctly.The workspace-scoped
editionandrust-versionsettings are correctly inherited. The workspace's Rust 1.85.0 is fully compatible with the WASM crate's dependencies, including wasm-bindgen.smithy/patches/rust.patch (2)
17-17: Verify SDK MSRV aligns with compatibility goals.The PR discussion recommends a tiered MSRV strategy with SDK crates (
superposition_sdk) at 1.75.0 for wider downstream compatibility. This setsrust-version = "1.85.0", which is significantly higher.If the Smithy-generated code genuinely requires 1.85.0 features, this is unavoidable—but if not, consider lowering the MSRV to expand the SDK's usability. Also ensure this value stays synchronized with
minimumSupportedRustVersioninsmithy/smithy-build.json.
10-18: Workspace metadata fields look good.The adoption of
version.workspace = true,license = { workspace = true }, andhomepage = { workspace = true }properly centralizes package metadata. Addingreadme = "README.md"is a sensible default.
| package.rust-version = "1.85.0" | ||
| package.edition = "2024" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation diverges from reviewer's tiered MSRV and edition recommendations.
The PR comments from knutties provide detailed recommendations that differ from this implementation:
| Aspect | Recommended | Implemented |
|---|---|---|
| Edition | Keep "2021" | "2024" |
| MSRV strategy | Tiered per crate (1.75.0–1.81.0) | Single workspace value (1.85.0) |
| SDK crates MSRV | 1.75.0 (for wider compatibility) | 1.85.0 |
Concerns:
-
Edition 2024 introduces breaking changes (e.g.,
genbecomes a keyword, changes to prelude,unsaferequirements inexternblocks). This requires code migration across all crates. -
Single MSRV means SDK consumers must use Rust 1.85.0+, which may limit adoption. The tiered approach allows
superposition_sdk,superposition_provider, andsuperposition_typesto support older Rust versions.
Please confirm whether the tiered MSRV strategy should be implemented, or if the decision is to proceed with a single workspace-level MSRV.
| edition.workspace = true | ||
| rust-version.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check workspace-level edition and rust-version settings
echo "=== Root Cargo.toml workspace.package settings ==="
grep -A 10 '\[workspace\.package\]' Cargo.toml || grep -A 10 '\[workspace\]' Cargo.toml | head -20
echo ""
echo "=== Edition settings across all crates ==="
fd -e toml -t f Cargo.toml --exec grep -l "edition" {} \; | xargs -I {} sh -c 'echo "--- {} ---" && grep "edition" {}'Repository: juspay/superposition
Length of output: 1940
The cac_toml/Cargo.toml correctly uses workspace inheritance, but the workspace has a critical edition mismatch issue.
The file correctly uses edition.workspace = true and rust-version.workspace = true. However, the workspace-level edition IS set to "2024" in the root Cargo.toml, and this creates a significant problem: 8 crates (including this one) will use edition 2024 via workspace inheritance, while 7+ other crates like cac_client and experimentation_client hardcode edition = "2021". This split causes real compatibility differences across the workspace (different prelude semantics, gen keyword reservation, etc.).
The workspace needs to either:
- Ensure all crates inherit from workspace (consistent 2024 edition), or
- Set workspace edition back to 2021 and explicitly configure edition 2024 only for crates that require it.
| version = { workspace = true } | ||
| edition = "2021" | ||
| rust-version = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent edition configuration - should use workspace inheritance.
Line 4 hardcodes edition = "2021" while other crates in this PR use edition.workspace = true. This creates inconsistency:
rust-versioncorrectly uses workspace inheritanceeditiondoes not, unlike the other crates modified in this PR
Suggested fix
[package]
name = "superposition_types"
version = { workspace = true }
-edition = "2021"
+edition = { workspace = true }
rust-version = { workspace = true }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| version = { workspace = true } | |
| edition = "2021" | |
| rust-version = { workspace = true } | |
| version = { workspace = true } | |
| edition = { workspace = true } | |
| rust-version = { workspace = true } |
🤖 Prompt for AI Agents
In @crates/superposition_types/Cargo.toml around lines 3 - 5, The Cargo manifest
sets edition = "2021" explicitly; change it to inherit the workspace edition by
replacing the hardcoded edition with workspace inheritance (e.g., use
edition.workspace = true or edition = { workspace = true } to match other
crates), so the 'edition' key follows the same workspace-based configuration as
'rust-version'.
0bb5204 to
7423bb1
Compare
| enum ResponseType { | ||
| UpdatePrecheck, | ||
| Response(ExperimentResponse), | ||
| Response(Box<ExperimentResponse>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This becomes a clippy failure with edition set, the enum variants have different sizes and therefore Box is needed for dynamic allocation
crates/cac_client/Cargo.toml
Outdated
| description = "CAC Client provides interfaces for applications to interact with the context-aware-config module in superposition. Note: this is deprecated, please use the provider" | ||
| version.workspace = true | ||
| edition = "2021" | ||
| rust-version.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For client also same version ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
| "service": "io.superposition#Superposition", | ||
| "module": "superposition_sdk", | ||
| "moduleVersion": "0.1.0", | ||
| "minimumSupportedRustVersion": "1.82.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure this is correct ?
I ran cargo msrv find and I found it to be 1.80.1
Problem
@SanchithHegde pointed out the missing
rust-versionandeditionvalues in our crates.Solution
Set the value in the top-level Cargo.toml and reference it in all the crates.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.