[major][Rebase & FF] Consolidate alloc crate usage and opt out of alloc feature for patina_internal_cpu's patina sdk dependency - #1688
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ℹ️ QEMU Validation Skipped - Unsupported Target BranchThe Patina QEMU PR validation workflow did not run because the PR targets an unsupported branch. Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/32307466005
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
|
@vineelko, since this is 1/n, can you write the complete plan you have in mind in a single GitHub issue and link that to the PR? Then we can see where things are heading, agree that makes sense, and review the PR in that context. |
|
@makubacki Added #1689 issue with additional details to support the changes being made. Feel free to let me know if you see any edits. |
334eaaf to
5ed2eec
Compare
@makubacki, As I dig more into it, it feels like reliance on alloc feature/crate is very fragile and more intertwined than I originally thought within patina crates. This is more evident due to the fact that we built different combinations of the crates via Fully resolving these dependency issues is likely to take more time (not worth the effort), so I'm narrowing the scope of this PR to the changes needed to make the Rust MM Supervisor buildable without depending on the The first 3 commits are good to have in terms of code cleanup without any functional impact. The fourth commit removes opting into the |
It's unclear what the final state of Can you add an "Alloc Guidance" page to the "Developer Guides" section of the mdbook that explains (simply) how the feature should be used and how developers should use it when writing their code? You also have things in this PR like "use |
|
@makubacki, Added the documentation for the guidance of using alloc crate and feature in patina crates. |
e1d0bed to
4913ff1
Compare
Thanks. As noted, this is very tedious. I'll look through the rest of the file changes on Monday. Are the first three commits complete for their intended purpose? For example, does [major] Use std instead of alloc in tests completely update all cases where std should be used instead of alloc in tests? Does [major] Refer to types by name instead of fully qualified paths update all occurrences? |
No worries, Michael, take your time. I am pretty sure I did cover the entire code base multiple times and made sure any missing instances(discovered later) have been adjusted back into those 3 commits as appropriate. |
4913ff1 to
f267b06
Compare
|
Updated the PR with the commits we discussed offline. Left below commits |
….rs/main.rs) - This clean up will enable guarding alloc crate usage in a later PR Signed-off-by: Vineel Kovvuri[MSFT] <vineelko@microsoft.com>
f267b06 to
5677fcf
Compare
- Isolate alloc crate usage to non-test code - Test code is always compiled for the host target with std, so alloc imports should not be conditionally guarded with test. - Remove `extern crate std;` declaration from all tests.
…sdk crate - `alloc` feature on patina is not required to build patina_internal_cpu. - This eliminates the dependency on alloc for rust mm_supervisor(with its current dependencies) - explicitly validated by running below commands on `patina_internal_cpu` ``` cargo make all cargo clippy -p patina_internal_cpu --all-targets --all-features -- -D warnings cargo test -p patina_internal_cpu --no-run --no-default-features ```
5677fcf to
2ee3a89
Compare
|
Thanks for updating the scope to |
Description
This PR originally set out to consolidate and gate all
alloccrate usage across the Patina repo behind an appropriateallocfeature flag. That turned out to be trickier and more fragile than expected, due to implicit assumptions aboutalloccrate usage baked in throughout Patina. This effort actually started while building the Rust MM supervisor, where the Patina crates it depends on should not implicitly opt into thealloccrate. After a few iterations, we deliberately narrowed the scope of this change to only the crates the supervisor relies on(patina_internal_cpu), gating those explicitly. As a result, this change is now trimmed down to the 3 commits below. The first two are good general cleanup/consolidation on their own, the third commit stopspatina_internal_cpufrom enabling the alloc feature on its patina SDK dependency, removing the unnecessary alloc requirement for the Rust MM supervisor's current dependency set.[major] Consolidate extern crate alloc declarations to crate root(lib.rs/main.rs)
[major] Use
stdinstead ofallocin testsimports should not be conditionally guarded with test.
extern crate std;declaration from all tests.[major] patina_internal_cpu: Do not enable "alloc" feature on patina sdk crate
allocfeature on patina is not required to build patina_internal_cpu.its current dependencies)
patina_internal_cpuHow This Was Tested
cargo make allin patina and patina-dxe-core-qemuIntegration Instructions
This does not change the interface between external consumers and patina crates. Hence not marking as breaking change.