feat(toolkit-macros): compile-time gear dependency linking via re-exports#4146
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR changes ChangesGear dependency wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GearCrate
participant ToolkitMacro
participant DependencyCrate
GearCrate->>ToolkitMacro: Declare deps with crate identifiers
ToolkitMacro->>DependencyCrate: Resolve and re-export dependencies
ToolkitMacro->>GearCrate: Emit gear metadata and generated wiring
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
libs/toolkit-macros/src/lib.rs (1)
771-787: 🩺 Stability & Availability | 🔵 TrivialAnchor the re-export path with a leading
::to avoid module-scope shadowing.The generated
pub use#crate_identas#alias_ident;expands at the call site of the#[gear]macro. If the macro is invoked within a submodule that defines a local item (e.g.,mod crate_nameorconst crate_name), the unqualifieduseresolution may resolve to that local item instead of the intended extern crate. Using::#crate_ident`` forces resolution against the extern prelude, ensuring the re-export remains stable regardless of local scope.♻️ Proposed change
quote! { #[cfg(not(test))] #[doc(hidden)] - pub use `#crate_ident` as `#alias_ident`; + pub use ::`#crate_ident` as `#alias_ident`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/toolkit-macros/src/lib.rs` around lines 771 - 787, The generated re-export in the macro expansion is currently using an unqualified path that can be shadowed by local module items. Update the re-export inside the dep reexports generation in lib.rs so the pub use in the quote! block anchors the crate path with a leading ::, using the existing crate_ident and alias_ident symbols, to ensure the extern crate is always resolved from the extern prelude even when #[gear] is invoked inside a submodule with a conflicting local name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/toolkit-macros/src/lib.rs`:
- Around line 771-787: The generated re-export in the macro expansion is
currently using an unqualified path that can be shadowed by local module items.
Update the re-export inside the dep reexports generation in lib.rs so the pub
use in the quote! block anchors the crate path with a leading ::, using the
existing crate_ident and alias_ident symbols, to ensure the extern crate is
always resolved from the extern prelude even when #[gear] is invoked inside a
submodule with a conflicting local name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea1176a7-fe6b-4137-a78c-d7bf844fa564
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomlexamples/oop-gears/calculator-gateway/calculator-gateway/Cargo.tomlexamples/toolkit/users-info/users-info/Cargo.tomlgears/credstore/credstore/Cargo.tomlgears/credstore/plugins/static-credstore-plugin/Cargo.tomlgears/mini-chat/mini-chat/Cargo.tomlgears/simple-user-settings/simple-user-settings/Cargo.tomlgears/system/account-management/account-management/Cargo.tomlgears/system/account-management/plugins/static-idp-plugin/Cargo.tomlgears/system/api-gateway/Cargo.tomlgears/system/authn-resolver/authn-resolver/Cargo.tomlgears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.tomlgears/system/authn-resolver/plugins/static-authn-plugin/Cargo.tomlgears/system/authz-resolver/authz-resolver/Cargo.tomlgears/system/authz-resolver/plugins/static-authz-plugin/Cargo.tomlgears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.tomlgears/system/oagw/oagw/Cargo.tomlgears/system/resource-group/resource-group/Cargo.tomlgears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.tomlgears/system/tenant-resolver/tenant-resolver/Cargo.tomlgears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.tomlgears/system/usage-collector/usage-collector/Cargo.tomllibs/toolkit-macros/src/lib.rs
3a8c413 to
bb56a4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/oop-gears/calculator-gateway/calculator-gateway/Cargo.toml`:
- Around line 39-42: Add the required cf-studio-path configuration with the
value ".cf-studio" to the Cargo manifest’s package metadata, alongside the
existing cargo-shear metadata. Preserve the current ignored dependency
configuration.
In `@gears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.toml`:
- Around line 27-28: Add the Cargo manifest metadata key cf-studio-path with
value ".cf-studio" to
gears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.toml (lines 27-28),
both affected dependency sections in gears/bss/ledger/ledger/Cargo.toml (lines
71 and 83-87), and gears/simple-user-settings/simple-user-settings/Cargo.toml
(line 25).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f858e232-6f3b-4ebd-a24b-6ba34ff29729
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomlexamples/oop-gears/calculator-gateway/calculator-gateway/Cargo.tomlexamples/toolkit/users-info/users-info/Cargo.tomlgears/bss/ledger/ledger/Cargo.tomlgears/credstore/credstore/Cargo.tomlgears/credstore/plugins/static-credstore-plugin/Cargo.tomlgears/file-storage/file-storage/Cargo.tomlgears/mini-chat/mini-chat/Cargo.tomlgears/simple-user-settings/simple-user-settings/Cargo.tomlgears/system/account-management/account-management/Cargo.tomlgears/system/account-management/plugins/static-idp-plugin/Cargo.tomlgears/system/api-gateway/Cargo.tomlgears/system/authn-resolver/authn-resolver/Cargo.tomlgears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.tomlgears/system/authn-resolver/plugins/static-authn-plugin/Cargo.tomlgears/system/authz-resolver/authz-resolver/Cargo.tomlgears/system/authz-resolver/plugins/static-authz-plugin/Cargo.tomlgears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.tomlgears/system/oagw/oagw/Cargo.tomlgears/system/resource-group/resource-group/Cargo.tomlgears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.tomlgears/system/tenant-resolver/tenant-resolver/Cargo.tomlgears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.tomlgears/system/usage-collector/usage-collector/Cargo.tomllibs/toolkit-macros/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (23)
- gears/credstore/plugins/static-credstore-plugin/Cargo.toml
- gears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.toml
- gears/system/authn-resolver/authn-resolver/Cargo.toml
- gears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.toml
- gears/mini-chat/mini-chat/Cargo.toml
- gears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.toml
- gears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.toml
- gears/system/account-management/account-management/Cargo.toml
- gears/system/authn-resolver/plugins/static-authn-plugin/Cargo.toml
- gears/system/authz-resolver/authz-resolver/Cargo.toml
- gears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.toml
- gears/system/tenant-resolver/tenant-resolver/Cargo.toml
- gears/credstore/credstore/Cargo.toml
- gears/system/api-gateway/Cargo.toml
- examples/toolkit/users-info/users-info/Cargo.toml
- gears/system/authz-resolver/plugins/static-authz-plugin/Cargo.toml
- Cargo.toml
- gears/file-storage/file-storage/Cargo.toml
- gears/system/resource-group/resource-group/Cargo.toml
- gears/system/oagw/oagw/Cargo.toml
- libs/toolkit-macros/src/lib.rs
- gears/system/usage-collector/usage-collector/Cargo.toml
- gears/system/account-management/plugins/static-idp-plugin/Cargo.toml
bb56a4b to
8a225e9
Compare
|
Could we avoid deriving the Rust crate identifier from the dependency’s runtime name ( For example: #[toolkit::gear(
name = "api-gateway",
deps = [grpc_hub, authn_resolver],
)]Each gear macro expansion could expose its canonical runtime name: #[doc(hidden)]
pub const __TOOLKIT_GEAR_NAME: &str = "authn-resolver";Then the #[doc(hidden)]
pub use ::authn_resolver as _toolkit_gear_dep_authn_resolver;
builder.register_core_with_meta(
"api-gateway",
&[::authn_resolver::__TOOLKIT_GEAR_NAME],
// ...
);This would keep the link-time behavior from this PR, while removing the implicit @maurolacy, what do you think about this direction? |
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
I think this is a great idea, since in that way the compiler can validate those identifiers directly. Now, introducing a new Since the naming convention currently holds, and for ~30 crates, why introduce a mapping at all? In any case, that mapping can be introduced later, if / when needed (i.e. when somebody breaks the naming convention for some (sound) reason). So, my proposal would be a hybrid of the current changes and your proposal:
I'll go ahead and implement this variant, and we can discuss it further in any case. MF: sorry, I edited this comment by mistake. (reverted to original) |
8a225e9 to
fa65cbe
Compare
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
fa65cbe to
cec1f7f
Compare
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
e89f43c to
0cd439e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gears/system/authn-resolver/authn-resolver/Cargo.toml`:
- Line 26: Add the required cf-studio-path metadata with value ".cf-studio" to
gears/system/authn-resolver/authn-resolver/Cargo.toml at lines 26-26,
gears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.toml at lines 27-28,
and gears/system/authz-resolver/plugins/static-authz-plugin/Cargo.toml at lines
24-24, preserving the existing manifest entries.
In `@gears/system/resource-group/resource-group/Cargo.toml`:
- Around line 63-64: Add cf-studio-path = ".cf-studio" to both manifest
dependency sections: gears/system/resource-group/resource-group/Cargo.toml at
lines 63-64 and gears/system/usage-collector/usage-collector/Cargo.toml at lines
23-25. Ensure both Cargo.toml files define the required configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d41a92f-9fd0-4972-8230-82d2cd5efd7d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (58)
.github/workflows/ci.ymlCargo.tomlexamples/oop-gears/calculator-gateway/calculator-gateway/Cargo.tomlexamples/oop-gears/calculator-gateway/calculator-gateway/src/gear.rsexamples/toolkit/users-info/users-info/Cargo.tomlexamples/toolkit/users-info/users-info/src/gear.rsgears/bss/ledger/ledger/Cargo.tomlgears/bss/ledger/ledger/src/module.rsgears/credstore/credstore/Cargo.tomlgears/credstore/credstore/src/gear.rsgears/credstore/plugins/static-credstore-plugin/Cargo.tomlgears/credstore/plugins/static-credstore-plugin/src/gear.rsgears/file-storage/file-storage/Cargo.tomlgears/file-storage/file-storage/src/gear.rsgears/mini-chat/mini-chat/Cargo.tomlgears/mini-chat/mini-chat/src/gear.rsgears/mini-chat/mini-chat/src/infra/plugins/static_audit/gear.rsgears/mini-chat/mini-chat/src/infra/plugins/static_model_policy/gear.rsgears/simple-user-settings/simple-user-settings/Cargo.tomlgears/simple-user-settings/simple-user-settings/src/gear.rsgears/system/account-management/account-management/Cargo.tomlgears/system/account-management/account-management/src/gear.rsgears/system/account-management/plugins/static-idp-plugin/Cargo.tomlgears/system/account-management/plugins/static-idp-plugin/src/gear.rsgears/system/api-gateway/Cargo.tomlgears/system/api-gateway/src/gear.rsgears/system/authn-resolver/authn-resolver/Cargo.tomlgears/system/authn-resolver/authn-resolver/src/gear.rsgears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.tomlgears/system/authn-resolver/plugins/oidc-authn-plugin/src/gear.rsgears/system/authn-resolver/plugins/static-authn-plugin/Cargo.tomlgears/system/authn-resolver/plugins/static-authn-plugin/src/gear.rsgears/system/authz-resolver/authz-resolver/Cargo.tomlgears/system/authz-resolver/authz-resolver/src/gear.rsgears/system/authz-resolver/plugins/static-authz-plugin/Cargo.tomlgears/system/authz-resolver/plugins/static-authz-plugin/src/gear.rsgears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.tomlgears/system/authz-resolver/plugins/tr-authz-plugin/src/gear.rsgears/system/oagw/oagw/Cargo.tomlgears/system/oagw/oagw/src/gear.rsgears/system/resource-group/resource-group/Cargo.tomlgears/system/resource-group/resource-group/src/gear.rsgears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/rg-tr-plugin/src/gear.rsgears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/single-tenant-tr-plugin/src/gear.rsgears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/static-tr-plugin/src/gear.rsgears/system/tenant-resolver/tenant-resolver/Cargo.tomlgears/system/tenant-resolver/tenant-resolver/src/gear.rsgears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.tomlgears/system/usage-collector/plugins/noop-usage-collector-plugin/src/module.rsgears/system/usage-collector/usage-collector/Cargo.tomlgears/system/usage-collector/usage-collector/src/module.rslibs/toolkit-macros-tests/tests/ui/fail/deps_not_array.stderrlibs/toolkit-macros/src/lib.rslibs/toolkit/src/lib.rslibs/toolkit/tests/macro_tests.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- gears/system/authn-resolver/plugins/static-authn-plugin/Cargo.toml
- gears/system/account-management/account-management/Cargo.toml
- gears/system/tenant-resolver/tenant-resolver/Cargo.toml
- gears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.toml
- gears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.toml
- gears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.toml
- gears/system/api-gateway/Cargo.toml
- gears/system/authz-resolver/authz-resolver/Cargo.toml
- gears/credstore/plugins/static-credstore-plugin/Cargo.toml
- gears/simple-user-settings/simple-user-settings/Cargo.toml
- gears/file-storage/file-storage/Cargo.toml
- gears/system/account-management/plugins/static-idp-plugin/Cargo.toml
- gears/bss/ledger/ledger/Cargo.toml
- gears/system/oagw/oagw/Cargo.toml
- gears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.toml
- gears/credstore/credstore/Cargo.toml
- gears/mini-chat/mini-chat/Cargo.toml
- examples/oop-gears/calculator-gateway/calculator-gateway/Cargo.toml
- gears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.toml
- Cargo.toml
|
Agreeing with the hybrid @maurolacy described. The One thing not yet raised: the |
|
Yeah, I was thinking and analysing this a bit more with AI, and the fact remains: This is a real limitation of proc macros in Rust; they can only emit code at their invocation site. No way to "reach up" to the crate root. So the options remain:
Regarding |
8c8a53d to
3006fc8
Compare
3006fc8 to
8065c59
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 282-289: Add the required cf-studio-path configuration set to
.cf-studio in Cargo.toml and each affected manifest:
gears/credstore/plugins/static-credstore-plugin/Cargo.toml,
gears/simple-user-settings/simple-user-settings/Cargo.toml,
gears/system/authz-resolver/plugins/static-authz-plugin/Cargo.toml, and
gears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.toml.
In `@CONTRIBUTING.md`:
- Line 94: Update the dependency-attribute example in CONTRIBUTING.md to use the
fully qualified toolkit::gear syntax, including the namespace in the attribute
declaration. Preserve the existing deps example and surrounding documentation.
In `@gears/credstore/plugins/static-credstore-plugin/Cargo.toml`:
- Line 22: Add the required cf-studio-path configuration to the
static-credstore-plugin Cargo.toml, setting it to .cf-studio alongside the
existing types-registry workspace dependency.
In `@gears/simple-user-settings/simple-user-settings/Cargo.toml`:
- Line 25: Add the required cf-studio-path configuration to the
simple-user-settings package's Cargo.toml, setting it explicitly to .cf-studio
alongside the existing authz-resolver workspace dependency.
In `@gears/system/authz-resolver/plugins/static-authz-plugin/Cargo.toml`:
- Line 24: Update the static-authz-plugin Cargo.toml configuration to add the
required cf-studio-path setting with the value .cf-studio, alongside the
existing types-registry workspace dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 638a3e43-2caf-4044-9e6d-2e8b21e52507
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.github/workflows/ci.ymlCONTRIBUTING.mdCargo.tomlexamples/oop-gears/calculator-gateway/calculator-gateway/Cargo.tomlexamples/oop-gears/calculator-gateway/calculator-gateway/src/gear.rsexamples/toolkit/users-info/users-info/Cargo.tomlexamples/toolkit/users-info/users-info/src/gear.rsgears/bss/ledger/ledger/Cargo.tomlgears/bss/ledger/ledger/src/module.rsgears/credstore/credstore/Cargo.tomlgears/credstore/credstore/src/gear.rsgears/credstore/plugins/static-credstore-plugin/Cargo.tomlgears/credstore/plugins/static-credstore-plugin/src/gear.rsgears/file-storage/file-storage/Cargo.tomlgears/file-storage/file-storage/src/gear.rsgears/mini-chat/mini-chat/Cargo.tomlgears/mini-chat/mini-chat/src/gear.rsgears/mini-chat/mini-chat/src/infra/plugins/static_audit/gear.rsgears/mini-chat/mini-chat/src/infra/plugins/static_model_policy/gear.rsgears/simple-user-settings/simple-user-settings/Cargo.tomlgears/simple-user-settings/simple-user-settings/src/gear.rsgears/system/account-management/account-management/Cargo.tomlgears/system/account-management/account-management/src/gear.rsgears/system/account-management/plugins/static-idp-plugin/Cargo.tomlgears/system/account-management/plugins/static-idp-plugin/src/gear.rsgears/system/api-gateway/Cargo.tomlgears/system/api-gateway/src/gear.rsgears/system/authn-resolver/authn-resolver/Cargo.tomlgears/system/authn-resolver/authn-resolver/src/gear.rsgears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.tomlgears/system/authn-resolver/plugins/oidc-authn-plugin/src/gear.rsgears/system/authn-resolver/plugins/static-authn-plugin/Cargo.tomlgears/system/authn-resolver/plugins/static-authn-plugin/src/gear.rsgears/system/authz-resolver/authz-resolver/Cargo.tomlgears/system/authz-resolver/authz-resolver/src/gear.rsgears/system/authz-resolver/plugins/static-authz-plugin/Cargo.tomlgears/system/authz-resolver/plugins/static-authz-plugin/src/gear.rsgears/system/authz-resolver/plugins/tr-authz-plugin/Cargo.tomlgears/system/authz-resolver/plugins/tr-authz-plugin/src/gear.rsgears/system/oagw/oagw/Cargo.tomlgears/system/oagw/oagw/src/gear.rsgears/system/resource-group/resource-group/Cargo.tomlgears/system/resource-group/resource-group/src/gear.rsgears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/rg-tr-plugin/src/gear.rsgears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/single-tenant-tr-plugin/src/gear.rsgears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.tomlgears/system/tenant-resolver/plugins/static-tr-plugin/src/gear.rsgears/system/tenant-resolver/tenant-resolver/Cargo.tomlgears/system/tenant-resolver/tenant-resolver/src/gear.rsgears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.tomlgears/system/usage-collector/plugins/noop-usage-collector-plugin/src/module.rsgears/system/usage-collector/usage-collector/Cargo.tomlgears/system/usage-collector/usage-collector/src/module.rslibs/toolkit-macros-tests/tests/ui/fail/deps_not_array.stderrlibs/toolkit-macros/src/lib.rslibs/toolkit/src/lib.rslibs/toolkit/tests/macro_tests.rs
🚧 Files skipped from review as they are similar to previous changes (46)
- gears/system/authn-resolver/authn-resolver/Cargo.toml
- libs/toolkit-macros-tests/tests/ui/fail/deps_not_array.stderr
- gears/credstore/credstore/src/gear.rs
- gears/system/authz-resolver/authz-resolver/Cargo.toml
- gears/system/tenant-resolver/tenant-resolver/Cargo.toml
- gears/system/authn-resolver/plugins/oidc-authn-plugin/Cargo.toml
- gears/system/authn-resolver/authn-resolver/src/gear.rs
- gears/system/tenant-resolver/plugins/static-tr-plugin/Cargo.toml
- gears/system/usage-collector/usage-collector/src/module.rs
- gears/system/tenant-resolver/tenant-resolver/src/gear.rs
- libs/toolkit/tests/macro_tests.rs
- gears/credstore/credstore/Cargo.toml
- gears/system/usage-collector/plugins/noop-usage-collector-plugin/src/module.rs
- .github/workflows/ci.yml
- gears/system/usage-collector/plugins/noop-usage-collector-plugin/Cargo.toml
- gears/system/tenant-resolver/plugins/rg-tr-plugin/src/gear.rs
- gears/credstore/plugins/static-credstore-plugin/src/gear.rs
- libs/toolkit/src/lib.rs
- gears/system/oagw/oagw/src/gear.rs
- gears/system/tenant-resolver/plugins/single-tenant-tr-plugin/Cargo.toml
- gears/system/resource-group/resource-group/src/gear.rs
- gears/bss/ledger/ledger/src/module.rs
- examples/toolkit/users-info/users-info/Cargo.toml
- gears/file-storage/file-storage/Cargo.toml
- gears/mini-chat/mini-chat/src/gear.rs
- examples/oop-gears/calculator-gateway/calculator-gateway/src/gear.rs
- gears/system/tenant-resolver/plugins/single-tenant-tr-plugin/src/gear.rs
- examples/oop-gears/calculator-gateway/calculator-gateway/Cargo.toml
- gears/mini-chat/mini-chat/src/infra/plugins/static_model_policy/gear.rs
- gears/bss/ledger/ledger/Cargo.toml
- gears/system/authz-resolver/authz-resolver/src/gear.rs
- gears/mini-chat/mini-chat/Cargo.toml
- gears/system/usage-collector/usage-collector/Cargo.toml
- gears/system/tenant-resolver/plugins/rg-tr-plugin/Cargo.toml
- gears/system/account-management/account-management/Cargo.toml
- gears/system/account-management/plugins/static-idp-plugin/src/gear.rs
- gears/system/account-management/plugins/static-idp-plugin/Cargo.toml
- gears/system/oagw/oagw/Cargo.toml
- gears/system/authn-resolver/plugins/static-authn-plugin/Cargo.toml
- gears/system/api-gateway/Cargo.toml
- gears/system/authn-resolver/plugins/oidc-authn-plugin/src/gear.rs
- examples/toolkit/users-info/users-info/src/gear.rs
- gears/system/authz-resolver/plugins/static-authz-plugin/src/gear.rs
- gears/system/resource-group/resource-group/Cargo.toml
- libs/toolkit-macros/src/lib.rs
- gears/file-storage/file-storage/src/gear.rs
|
|
||
| #### Gear dependencies and `cargo-shear` | ||
|
|
||
| When a gear declares dependencies via `#[gear(deps = [...])]`, the dep entries are |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the actual toolkit::gear attribute in the example.
The documented syntax omits the toolkit:: namespace, while this feature is exposed as #[toolkit::gear(deps = [...])]. As written, contributors may copy an attribute that does not resolve.
Proposed fix
-When a gear declares dependencies via `#[gear(deps = [...])]`, the dep entries are
+When a gear declares dependencies via `#[toolkit::gear(deps = [...])]`, the dep entries are📝 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.
| When a gear declares dependencies via `#[gear(deps = [...])]`, the dep entries are | |
| When a gear declares dependencies via `#[toolkit::gear(deps = [...])]`, the dep entries are |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` at line 94, Update the dependency-attribute example in
CONTRIBUTING.md to use the fully qualified toolkit::gear syntax, including the
namespace in the attribute declaration. Preserve the existing deps example and
surrounding documentation.
The #[gear(deps = [...])] macro generates hidden pub use re-exports that cargo-shear cannot trace. Document this in the macro's rustdoc and in CONTRIBUTING.md so new gear dependencies don't cause unexpected CI failures in the shear job. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Mauro Lacy <mauro.lacy@acronis.com>
8065c59 to
ea5f581
Compare
Problem
The
depsfield in#[toolkit::gear(deps = [...])]declares runtime lifecycledependencies between gears. At startup,
build_dependency_graph()validates thatevery declared dep exists in the
inventory-based registry. However,inventoryrelies on linker-section tricks: only crates explicitly referenced with
use X as _;in the generated
main.rssurvive the linker. Transitive Cargo dependencies gettheir
inventory::submit!registrations silently stripped.This forces users to flatten the entire gear dependency tree in
Gears.toml. Forexample, listing
api-gatewayis not enough. You must also manually listgrpc-hub,authn-resolver,types-registry, and every other transitive dep.Forgetting one causes a runtime
UnknownDependencyerror at startup, with nocompile-time signal that anything is wrong.
Solution
The
#[toolkit::gear]macro now generatespub usere-exports for each declareddependency:
#[toolkit::gear(name = "api-gateway", deps = ["grpc-hub", "authn-resolver"])]expands to (among other things):
This forces the linker to keep the dependency gear crates (and their
inventory::submit!registrations) alive when a gear is pulled in transitively.The re-exports chain: if
api-gatewayre-exportsauthn-resolver, andauthn-resolverre-exportstypes-registry, then pulling inapi-gatewayaloneis sufficient to register the entire transitive dependency tree.
The re-exports are gated behind
#[cfg(not(test))], so that test builds (wheregears are often defined in the same compilation unit) don't require phantom
external crate dependencies.
Trade-off: Cargo.toml dependencies
For the
pub useto compile, each gear'sCargo.tomlmust list its dep gearcrates as Cargo dependencies (not just SDKs). This is a new requirement, but
forgetting a dependency is now a compile-time error (
no external crate),not a silent runtime failure. The SDK dependencies remain necessary for the
actual API types used in gear implementations.
Naming convention
This change relies on a strict, bidirectional naming convention:
api-gatewaycf-gears-api-gatewayapi_gatewaytypes-registrycf-gears-types-registrytypes_registryThe macro derives the lib name from the dep's gear name by replacing
-with_.This convention is already followed by all gear crates in the repo (24/27 gears;
the 3 exceptions are internal plugins in
mini-chatwhich are same-crate andunaffected).
Changes
libs/toolkit-macros/src/lib.rs: Generatepub usere-exports for eachentry in
deps = [...], gated behind#[cfg(not(test))].Cargo.toml(root): Added 8 workspace-level dependency entries for gearcrates that were previously only available as SDKs.
Cargo.tomlfiles: Added the full gear crate as a dependency(via
{ workspace = true }) for each declared dep.Verification
cargo build: Entire workspace compiles cleanly.cargo test --workspace: All test suites pass.What this enables
With this change,
cargo gearsand future tooling no longer needs to resolveand flatten the transitive gear dependency tree. Users can list only the gears
they directly use in
Gears.toml, and the macro-generated re-exports ensure thefull dependency chain is linked.
Summary by CodeRabbit
New Features
Documentation
CI Improvements