Repository navigation
refactor(cases): separate CPU profiles from base architectures - #1084
Conversation
Keep the enabled base-ISA matrices while resolving profile-specific case suites and rejecting contradictory profile declarations Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
…ntity Move profile settings out of architecture matrices and document inheritance of base case suites Add manifest rejection cases and protect the existing thirty enabled CI matrix entries Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
Exercise a supported profile with an explicit compiler triple and reject inconsistent or unknown target profile names Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed exact head 23afce6.
All four new CPU-profile integration tests passed locally with Rust 1.89.0, including base/profile suite inheritance, invalid combinations, execution-table validation and the existing 30 case-matrix entries. I found no additional functional blocker in the disabled-profile migration.
Required correction — pinned rustfmt fails
Reproduce:
rustfmt +1.89.0 --edition 2021 --check tools/src/cases.rs tools/tests/case_profiles.rs
cargo +1.89.0 fmt --all -- --checkObserved diffs include Target::value, profiles validation, the Manifest filters and multiple new test assertions/manifest replacements. Format the two touched Rust files, retain the disabled profile status and existing matrix contract, then rerun:
cargo +1.89.0 test --locked --jobs 2 -p wave-tools --test case_profilesObtain successful required checks on that updated head. No new CPU target implementation, ISA support claim or additional source split is requested.
LunaStev
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 784009d.
All four case_profiles tests still pass, including disabled-profile identity, base/profile suite inheritance, validation and the existing 30 case-matrix entries. The follow-up did not introduce a new functional finding in those checks.
The formatting request is not complete. Full pinned formatting exits 1 locally, and the exact-head CI 41823126579 fails the same check.
cargo +1.89.0 fmt --all -- --check
rustfmt +1.89.0 --edition 2021 --check tools/src/cases.rs tools/tests/case_profiles.rsRemaining differences include cases.rs:218,264,312 and case_profiles.rs:35,112,125. For example, the formatter combines the fields.as_table error expression around cases.rs:218 and changes the Manifest root-field predicate layout around line264.
Generate these changes with the pinned Rust 1.89.0 formatter and repository rustfmt.toml, then verify the complete check. Keep profiles disabled and the enabled target/matrix contract unchanged; no extra target support or file split is requested.
|
Remaining changes before merge — reviewed head The four profile tests pass and the enabled case matrix remains at 30 entries. Please complete the formatting correction. The pinned check still differs in tools/src/cases.rs around 218, 264 and 312 and tools/tests/case_profiles.rs around 35, 112 and 125. Generate the touched-file formatting with the repository configuration, then validate: rustfmt +1.89.0 --edition 2021 --config-path rustfmt.toml tools/src/cases.rs tools/tests/case_profiles.rs
cargo +1.89.0 fmt --all -- --check
cargo +1.89.0 test --locked --jobs 2 -p wave-tools --test case_profilesKeep all four planned profiles disabled and preserve the existing enabled targets/matrix. No additional architecture implementation or file split is requested. Detailed findings and reproduction: review. |
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed head 11b028b3eff5c3e3b2d8e4b2a5a70d31cee467f7.
The remaining formatting request is addressed. The changes since the tested head are formatter output only; profile/base-architecture separation, the 30-entry enabled matrix and all four disabled planned profiles are preserved. The pinned full Cargo formatting check now passes locally. The previous four focused profile tests passed, and exact-head CI 42057855005 is completed/success with all 50 distinct required jobs successful.
No remaining actionable finding in this patch. Approved for normal squash merge at this exact head.
Fixes #458
The case manifest treated Rhea, SHAKTI, XiangShan and T-Head as additional architectures even though they are CPU profiles of AArch64 and RISC-V
I moved the planned profiles into a separate manifest table, kept the base architecture for target identity, and made profile targets inherit the base and profile case suites
Added validation for unknown profiles, mismatched base architectures, unsupported combinations and missing execution entries
Added tests for planned and enabled profile resolution, target metadata, case-suite order and the existing thirty CI matrix entries
All four profiles remain disabled and the existing enabled matrices are unchanged
Haven't run the Rust test suite locally, CI still needs to verify the manifest migration