Conversation
149b2ce to
2a1cd4a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the feature definitions in src/libkrun/Cargo.toml and src/vmm/Cargo.toml to ensure proper propagation to sub-crates such as vmm, devices, and arch. A review comment suggests extending this propagation to the aws-nitro feature for consistency with the other updated features.
c468229 to
ae8c30c
Compare
Signed-off-by: Jan Noha <nohajc@gmail.com>
ae8c30c to
1a6912f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates feature propagation across the workspace's Cargo.toml files, ensuring that top-level features correctly enable corresponding functionality in sub-crates like vmm and devices. A critical issue was identified in src/vmm/Cargo.toml where referencing the target-specific cpuid dependency in the global features table will cause build failures on non-x86_64 platforms.
| input = ["krun_input"] | ||
| tee = ["devices/tee", "arch/tee"] | ||
| amd-sev = ["blk", "bitfield", "bitflags", "iocuddle", "tee", "kbs-types", "serde", "serde_json", "devices/amd-sev", "arch/amd-sev"] | ||
| tdx = ["blk", "tee", "kbs-types", "serde", "serde_json", "dep:tdx", "devices/tdx", "arch/tdx", "cpuid/tdx"] |
There was a problem hiding this comment.
Referencing cpuid/tdx in the global features table will cause build failures on non-x86_64 platforms (such as aarch64 or macOS). This is because the cpuid dependency is target-specific (defined under [target.'cfg(target_arch = "x86_64")'.dependencies] at line 49), and Cargo requires that any dependency referenced in a feature list using the pkg/feat syntax must be present for all targets.
To resolve this while maintaining feature propagation, you should consider making cpuid an optional dependency and using the weak dependency syntax cpuid?/tdx. Note that if cpuid is required for other non-TDX functionality on x86_64, you will need to ensure it remains enabled for those cases.
There was a problem hiding this comment.
Isn't TDX only available on Intel anyway?
There was a problem hiding this comment.
If needed, we can move cpuid to unconditional dependencies because the crate code is gated to x86_64 already.
I tried to actually import libkrun as a Rust dependency (based on 44312fd) and it revealed feature propagation to sub-crates is broken.
So, if I have this in
Cargo.toml, it won't enable the corresponding features insrc/{devices,vmm}for example.