feat(provider): per-provider Cargo feature flags (RFC, KeePass as first migration)#502
feat(provider): per-provider Cargo feature flags (RFC, KeePass as first migration)#502pofallon wants to merge 10 commits into
Conversation
Adds an optional `cargo_feature: Option<String>` field to provider descriptor TOMLs (`crates/fnox-core/providers/*.toml`). When set, the build script emits `#[cfg(feature = "<name>")]` around every piece of generated code that references that provider: the ProviderConfig + ResolvedProviderConfig enum variants, every match arm in the methods / resolver / instantiator generators, the `use super::super::<module>` statements, and the wizard entry. To support cfg-gating individual wizard entries (Rust doesn't allow `#[cfg]` attributes inside array literals), `ALL_WIZARD_INFO` becomes a `LazyLock<Vec<WizardInfo>>` populated by cfg-gated `v.push(...)` statements. Consumers calling `ALL_WIZARD_INFO.iter()` continue to work unchanged via `LazyLock`'s `Deref` to `Vec<WizardInfo>`; lifetime behavior is the same because the `LazyLock` itself is `static`. This commit alone is a no-op for the build: no provider sets `cargo_feature` yet, so every cfg attribute the codegen emits is the empty token stream and the generated code is byte-equivalent. The follow-up commits use this infrastructure to make individual providers optional.
Adds a [features] block to both `crates/fnox-core/Cargo.toml` and the top-level `fnox` binary crate's Cargo.toml. Per-provider features are declared but empty for now (no deps are moved to optional yet) — this commit's purpose is to put the feature surface up for review in isolation before any provider migration changes behavior. - fnox-core: default = ["all-providers"]; all-providers includes every per-provider feature. - fnox binary: passes through to fnox-core, plus sets `default-features = false` on its `fnox-core` dep so the feature selection actually flows. Behavior is unchanged today: every provider's dep is still non-optional, so every feature flag is a no-op and the artifact produced by `cargo build` and `cargo build --no-default-features` is identical. The size savings land as each provider is migrated to `optional = true` deps in follow-up commits. The block is heavily commented with the three-step recipe for adding a new provider feature, so future contributors don't have to reverse-engineer the build-script + Cargo.toml + mod.rs interaction.
KeePass is the first provider migrated to the per-provider feature
flag infrastructure introduced in the previous two commits. With
default features the build is byte-equivalent to before (the keepass
crate is still compiled in). With `--no-default-features` it drops
out, taking the `keepass` dependency and its transitives (the kdbx
parser, blake2b_simd, etc.) with it.
Changes per the three-step recipe in fnox-core/Cargo.toml's [features]
block:
1. `crates/fnox-core/providers/keepass.toml` — set
`cargo_feature = "keepass"` so the build script wraps every piece
of generated provider-registration code in
`#[cfg(feature = "keepass")]`.
2. `crates/fnox-core/Cargo.toml` — mark the `keepass` dep
`optional = true` and wire `keepass = ["dep:keepass"]` under
`[features]`.
3. `crates/fnox-core/src/providers/mod.rs` — gate the `pub mod keepass;`
declaration and split the explicit `use super::super::{...}` list
so `keepass` lives in its own cfg block (so adding the next gated
provider doesn't reshape this list).
Binary-side mirror, gated by the binary's own `keepass` feature (which
itself enables `fnox-core/keepass`):
- `src/commands/provider/mod.rs`: cfg-gate the `ProviderType::KeePass`
clap variant + its strum attribute.
- `src/commands/provider/add.rs`: cfg-gate the
`ProviderType::KeePass => …` arm that builds a `ProviderConfig::KeePass`
default scaffold. Both have to be gated together because the
ProviderConfig variant itself is now gated.
Test update:
- `provider_add_types_match_provider_definitions` (the drift test that
compares clap's `ProviderType` variants to the on-disk TOML
descriptors) now reads the descriptor's `cargo_feature` field and
skips entries whose feature isn't enabled in the current build.
Without this, the test fails under `--no-default-features` because
the TOML side still lists keepass but clap correctly doesn't.
Verified:
- `cargo build` and `cargo build --no-default-features` both succeed.
- `cargo test` and `cargo test --no-default-features` both pass.
- The slim build's compile log confirms `keepass` crate is not built.
(The pre-existing `providers::keychain::tests::test_keychain_set_and_get`
fails in environments without a Secret Service / D-Bus keyring — same
failure on `main`. Unrelated to this change.)
CI: new `feature-combos` job builds + tests with three Cargo flag sets in parallel: - `--no-default-features` (no providers from the new feature set) - `--no-default-features --features keepass` - `--all-features` Catches cfg-gating regressions in either direction. Skips the keychain test that needs an OS keyring (already covered by ci-bats). The new job is added to `final`'s needs list. Add a matrix entry whenever a new per-provider feature lands. README: new `## Cargo features` section between `Installation` and `Development`. Documents how to opt out of providers, the default-on convention, an example `default-features = false` snippet for library embedders, and a forward-looking note that more providers will be feature-gated in follow-up work (the binary-size win for embedders is several MiB once google/azure/keepass/yubikey/dbus all move).
There was a problem hiding this comment.
Code Review
This pull request introduces per-provider Cargo features to enable slimmer binary builds by allowing users to opt out of specific providers, starting with KeePass. The changes include updating the build script to conditionally generate provider-related code, modifying fnox-core and the CLI to respect these feature flags, and adding documentation. Feedback includes a suggestion to maintain backward compatibility for the ALL_WIZARD_INFO public API by using Box::leak to keep it as a slice, and a recommendation to use toml_edit instead of manual string parsing for more robust testing of provider descriptors.
| pub static ALL_WIZARD_INFO: LazyLock<Vec<WizardInfo>> = LazyLock::new(|| { | ||
| let mut v: Vec<WizardInfo> = Vec::new(); | ||
| #(#wizard_info_entries)* | ||
| v | ||
| }); |
There was a problem hiding this comment.
Changing ALL_WIZARD_INFO from a slice to LazyLock<Vec<WizardInfo>> is a breaking change for the public API of fnox-core. While LazyLock is necessary to support #[cfg] attributes on individual entries, you can maintain the original &'static [WizardInfo] type by using Box::leak on the initialized Vec. This preserves backward compatibility for consumers expecting a slice and better reflects the read-only nature of this static data.
| pub static ALL_WIZARD_INFO: LazyLock<Vec<WizardInfo>> = LazyLock::new(|| { | |
| let mut v: Vec<WizardInfo> = Vec::new(); | |
| #(#wizard_info_entries)* | |
| v | |
| }); | |
| pub static ALL_WIZARD_INFO: LazyLock<&'static [WizardInfo]> = LazyLock::new(|| { | |
| let mut v: Vec<WizardInfo> = Vec::new(); | |
| #(#wizard_info_entries)* | |
| Box::leak(v.into_boxed_slice()) | |
| }); |
| let cargo_feature = content | ||
| .lines() | ||
| .find_map(|line| { | ||
| let line = line.trim(); | ||
| let rest = line.strip_prefix("cargo_feature")?; | ||
| let rest = rest.trim_start().strip_prefix('=')?.trim(); | ||
| rest.strip_prefix('"') | ||
| .and_then(|s| s.strip_suffix('"')) | ||
| .map(str::to_owned) | ||
| }); |
There was a problem hiding this comment.
The manual string-based parsing of the provider TOML files is fragile and may fail if the file contains comments on the same line as cargo_feature or uses single quotes. Since toml_edit is already a dependency in the workspace and used in the fnox binary, it should be used here for more robust and idiomatic parsing.
let cargo_feature = content.parse::<toml_edit::DocumentMut>().ok()
.and_then(|doc| doc.get("cargo_feature").and_then(|v| v.as_str()).map(|s| s.to_owned()));
Greptile SummaryThis PR introduces per-provider Cargo feature flags to
Confidence Score: 5/5Safe to merge — the change is additive and backward-compatible; cargo build with defaults compiles and tests identically to before. All three layers (codegen, Cargo features, CI) are internally consistent. The binary's drift test explicitly accounts for feature-gated providers and runs across all three CI feature combos. No logic regressions were found across the changed paths. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "ci(provider): drop --lib so the drift te..." | Re-trigger Greptile |
The previous example claimed to "skip the KeePass provider" but passed `--features all-providers`, which (per fnox-core's Cargo.toml) expands to `["keepass"]` — so the command actually enabled KeePass and was equivalent to the default build. Following the doc gave zero binary-size benefit. Also drops the parenthetical "`all-providers` minus any feature you don't pass": Cargo doesn't support feature subtraction. The new wording explains the actual model — `--no-default-features` turns everything off, then each `--features <name>` opts a single provider back in — and shows both the all-off and pick-one-back patterns.
Per PR review (gemini-code-assist): the original change to `LazyLock<Vec<WizardInfo>>` was a larger API break than necessary because it changed the inner element type from `[WizardInfo]` (slice) to `Vec<WizardInfo>`. Downstream consumers that took `&[WizardInfo]` arguments or stored `&'static [WizardInfo]` bindings would break. Switch to `LazyLock<&'static [WizardInfo]>` with `Box::leak(v.into_boxed_slice())` inside the initializer. The Vec is allocated once at first access, leaked into a `'static` slice, and the LazyLock thereafter hands out `&'static [WizardInfo]` — restoring the original element type. The only observable change vs the pre-feature-gate API is the outer `LazyLock` wrapper; `ALL_WIZARD_INFO.iter()` works unchanged via `Deref`, and bindings like `let x: &'static [WizardInfo] = …` need one explicit `*` deref to compile. That's the minimum break consistent with letting individual entries be cfg-gated.
Per PR review (gemini-code-assist): the previous string-based
`strip_prefix("cargo_feature")` / `strip_suffix('"')` parser was
fragile against inline comments, single-quoted strings, multi-line
values, and `cargo_feature` keys nested inside other tables. Replace
with the canonical `toml_edit::DocumentMut::parse()` + `.get(...)`
walk — `toml_edit` is already a workspace dep and the binary
already pulls it as a runtime dep, so no new dependency is added.
Behavior preserved: returns the parsed string if `cargo_feature` is
declared at the top level, returns `None` otherwise. The `match`
that maps the parsed name to `cfg!(feature = "…")` is unchanged.
Also expands the doc comment to explain why the match has to be
updated by hand for every new gated provider — `cfg!()` requires a
string literal so the test can't go fully generic the way the build
script can.
Per PR review (greptile-apps): the previous matrix interpolated raw
flag strings — including spaces — into the rust-cache `shared-key`.
GH Actions tolerates spaces in cache keys but some cache backends
behave oddly with them. Switch the matrix from a bare `flags` list
to `combo.{name, flags}` pairs, where `name` is a lowercase
dash-separated slug used in the cache key and the job name. `flags`
keeps the actual cargo arguments.
Adds a comment documenting the slug convention so future contributors
keep the pattern when extending the matrix.
Per PR review (greptile-apps): the drift test and the build script both have to be updated when a new gated provider lands, but the requirement isn't called out anywhere. Add cross-references so future contributors (human or agent) catch it: - `crates/fnox-core/build/generate_providers.rs`: the `cargo_feature` field's doc comment now points at the drift test and explains why `cfg!()`'s string-literal constraint means the test can't be generic the way the build script is. - `AGENTS.md`: new "Cargo features" section documents the full three-place recipe (provider TOML, fnox-core Cargo.toml, binary side + drift test + CI matrix slug). Slotted before "GitHub Interactions" to keep that section's prominence. No code changes; tests and build unaffected.
Per PR review (greptile-apps P1): the previous CI step ran `cargo test --lib ...`, which restricts test execution to library crates only. That excluded the binary crate's `provider_add_types_match_provider_definitions` drift test — the very test updated in this PR to skip feature-gated providers via `provider_feature_enabled`. Without it running across feature combos, the matrix only verified "things compile under each combo"; it didn't catch the regression it was added to guard against (forgetting to update the test's match arm when adding a new gated provider). `--lib` was originally there to avoid the keychain integration test that needs an OS keyring service. That's already handled by the existing `--skip providers::keychain::tests::test_keychain_set_and_get` filter, so `--lib` was redundant in addition to being harmful. Removing `--lib` adds 8 more tests to each matrix entry (the binary's unit + drift tests). All four combos verified locally: default : 8 binary tests pass --no-default-features : 8 binary tests pass --no-default-features --features keepass : 8 binary tests pass --all-features : 8 binary tests pass Includes an inline comment in the workflow explaining why `--lib` is deliberately absent so a future contributor doesn't put it back.
Summary
Introduces opt-in Cargo feature flags for individual providers in
fnox-core. Downstream embedders that don't ship every provider candrop the unused ones to shrink their binary. Defaults are unchanged —
cargo buildcontinues to compile every provider, so this isbackward-compatible for current users and packagers.
This PR lands the infrastructure plus KeePass as the first
migration as a proof of pattern. If the design is acceptable, the
other heavy providers (Google Cloud, Azure, YubiKey/CTAP, D-Bus
keyring backend) are mechanical follow-ups using the same recipe;
I've sized that work at ~6–8h and would be happy to send the full set
as a follow-up PR.
Marking draft to invite design feedback before the rest of the
migration lands.
Motivation
Working on a downstream daemon (remo-broker) that embeds
fnox-coreand only needs a couple of providers. The current binaryends up at ~32 MiB largely because every provider compiles in
unconditionally, dragging in (per
cargo bloat):google-cloud-secretmanager-v1(610 KiB),google-cloud-auth(396 KiB),tonic(132 KiB),google_cloud_gax*(~200 KiB)openssl-sys(2.0 MiB, pulled by the Google + Azure paths viareqwest@0.12→native-tls)azure_core+azure_identity+typespec_client_core(~130 KiB),quick_xml(~100 KiB, mostly Azure)keepass(235 KiB) +blake2b_simdctap-hid-fido2(62 KiB) + thehidapi/libudevchain on Linuxdbus+libdbus_sys+dbus_secret_service*(~340 KiB) for theLinux keyring backend
jsonwebtoken+rsa+num_bigint_dig(~180 KiB)Conservative attribution across the top-100 crates (per-crate
cargo tree -iwalks confirming each is reachable only throughunused-provider chains) puts the savings at ~5.4 MiB of
.textsection / ~19–22% of the stripped binary for the minimal AWS+age
profile — see remo-broker's binary-size analysis for the
full breakdown.
The same win is available to anyone who builds a slim CLI, packages
fnox for a constrained distro, or vendors
fnox-coreinto adifferent binary.
Design
Three pieces, one commit each, each commit's build a no-op except
where its predecessor's infrastructure is exercised:
1. Build-script plumbing (
4700298)New optional
cargo_feature: Option<String>field in the providerTOML descriptor schema. The codegen reads it and, for providers that
set it, wraps every piece of generated code that references them in
#[cfg(feature = \"<name>\")]: theProviderConfig/ResolvedProviderConfigenum variants, every match arm in themethods / resolver / instantiator generators, the
use super::super::<module>statements, and the wizard entry.ALL_WIZARD_INFOchanges shape from&[WizardInfo]toLazyLock<Vec<WizardInfo>>so the wizard entries can beindividually cfg-gated —
#[cfg]attributes aren't permitted insidean array literal, but they are on statements. Consumers using
ALL_WIZARD_INFO.iter()keep working unchanged thanks toLazyLock'sDeref<Target = Vec<...>>.This commit alone is a build no-op: no provider sets
cargo_featureyet, so every emitted cfg-attr is the empty token stream.
2. Cargo features scaffolding (
ad23715)Adds
[features]blocks to bothcrates/fnox-core/Cargo.tomland thebinary's
Cargo.toml:fnox-core:default = [\"all-providers\"]withall-providersexpanding to the per-provider list.
fnoxbinary: mirror passthrough so binary code that referencescfg-gated provider types (
ProviderType::KeePassetc.) staysconsistent.
fnox-coreis pulled withdefault-features = falseso the feature selection actually flows; the binary's
defaultfeature is
[\"all-providers\"], which chains through to[\"keepass\"]and from there to[\"fnox-core/keepass\"].Per-provider features are declared but empty (no deps moved to
optional = trueyet). This commit is also a no-op for the buildartifact — its purpose is to put the feature surface up for review
in isolation before the first migration.
3. KeePass migration (
3435dae)The proof of pattern. KeePass first because it's the simplest gated
provider (no cross-provider references, all kept inside fnox-core).
The recipe documented in
Cargo.toml's[features]block:cargo_feature = \"keepass\"inkeepass.toml.keepassdepoptional = trueand wirekeepass = [\"dep:keepass\"]in[features].pub mod keepass;incrates/fnox-core/src/providers/mod.rsplus the corresponding line in the explicit
use super::super::{...}block.ProviderType::KeePass(clap variant) and theProviderType::KeePass => ProviderConfig::KeePass { ... }arm insrc/commands/provider/add.rs.provider_add_types_match_provider_definitions)updated to read the descriptor's
cargo_featureand skip entrieswhose feature isn't enabled, so it stays meaningful across feature
combos.
Verified:
cargo buildandcargo build --no-default-featuresboth succeed.cargo testandcargo test --no-default-featuresboth pass.keepasscrate is not built.4. CI feature-combos matrix (
fd9c44a)New
feature-combosjob builds + tests with three flag sets inparallel:
--no-default-features,--no-default-features --features keepass,--all-features. Catches cfg regressions in either direction. Addedto
final'sneedslist. Add a matrix entry whenever a newper-provider feature lands.
README gains a
## Cargo featuressection with the slim-buildrecipe and a forward-looking note about what migrates next.
What's not in this PR (proposed follow-ups)
To keep this PR reviewable I deliberately stopped at one migrated
provider. If you're happy with the shape, the rest is mechanical:
google—gcp_auth,google-cloud-kms,google-cloud-secretmanager-v1(plus their transitive
openssl-sys,tonic,reqwest@0.12chain— the single biggest binary-size win)
azure—azure_core,azure_identity,azure_security_keyvault_*yubikey—ctap-hid-fido2(already target-gated for non-musl)linux-keyring—dbus,dbus-secret-service-keyring-store,the Linux-target
openssl-sysOpen to either: I send each as a separate follow-up PR, or I send all
of them in a single sweep PR after this one merges. Your call.
Questions for review
keepassto matchserde_rename; happy touse any convention you prefer (e.g.,
provider-keepass).default = [\"all-providers\"]preserves current behavior butmeans
cargo build --no-default-featuresbecomes the explicitslim path. Alternative: leave
default = []and require everyone(including the current build) to opt in via
all-providers. Ichose the backward-compatible shape; flag if you'd rather break
compat in this change.
LazyLock<Vec<WizardInfo>>— the cleanest workaround I foundfor cfg-gated array elements. Open to alternatives if you have one.
provider. Some providers naturally group (Azure has two,
yubikey/fido2 share deps). Should I keep one-feature-per-provider
or introduce group features (
azure,google) up front?matching feature in the
fnoxbinary. Acceptable maintenanceburden, or would you prefer a macro / build-script approach?
Test plan
cargo build(default) compilescargo build --no-default-featurescompilescargo testpasses (8 passed)cargo test --no-default-featurespasses (8 passed)keepasscrate is absentopened)
mise run ci(I don't havemise/hkset up locally in thisenvironment; relying on upstream CI to catch any lint issues)
This PR was authored by Claude Code per the AGENTS.md instruction;
the design and motivation are based on downstream
remo-broker's binary-size analysis.