Repository navigation
Slai Agent Runtime: Support ARM architectures in package name normalization - #6585
Slai.life (huong8373tt-beep) wants to merge 4 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| Regex::Expression ArchitectureX32{ R"((?<=^|[^\p{L}\p{Nd}])(X32|X86)(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; | ||
| Regex::Expression ArchitectureX64{ R"((?<=^|[^\p{L}\p{Nd}])(X64|AMD64|X86([\p{Pd}\p{Pc}]64))(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; | ||
| Regex::Expression ArchitectureArm64{ R"((?<=^|[^\p{L}\p{Nd}])(ARM64|AARCH64)(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; | ||
| Regex::Expression ArchitectureArm{ R"((?<=^|[^\p{L}\p{Nd}])(ARM)(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; |
There was a problem hiding this comment.
As ARM does not end in a number, the trailing \P{Nd} ends up matching other letters (I think). Needs a negative test case like "Name armadillo" that does not replace anything or detect the architecture. Fixing it requires a bit more regex knowledge than is currently in my head. But regardless of the fix, a lot more test cases are likely needed.
Additionally, it makes sense to include ARM32 as a variant that might appear. That one should be able to use the existing pattern as it does end in a number.
There was a problem hiding this comment.
Implemented in 79514f1. ARM now requires a non-letter/non-digit boundary, so names such as Name armadillo remain unchanged and report Unknown. Added coverage for that negative case and for the ARM32 variant; ARM64 and AARCH64 continue to use the existing digit-terminated pattern.
I also checked the boundary cases (ARMfoo, ARM Edition, and ARM-foo) against the revised expression.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
Bot
added
Needs-Attention
JohnMcPMS
left a comment
JohnMcPMS
left a comment
There was a problem hiding this comment.
Note that there is a test that covers a lot of other inputs (NameNorm_Database_Initial) that might need to be updated.
| REQUIRE(normer.Normalize("Name arm64", {}).Architecture() == Architecture::Arm64); | ||
| REQUIRE(normer.Normalize("Name aarch64", {}).Architecture() == Architecture::Arm64); | ||
| REQUIRE(normer.Normalize("Name armadillo", {}).Architecture() == Architecture::Unknown); | ||
| REQUIRE(normer.Normalize("Name armadillo", {}).Name() == "Name armadillo"); |
There was a problem hiding this comment.
Normalization removes spaces. At some point the case is folded as well, but I don't remember if that is part of the normalization or the index storage.
| Regex::Expression ArchitectureX32{ R"((?<=^|[^\p{L}\p{Nd}])(X32|X86)(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; | ||
| Regex::Expression ArchitectureX64{ R"((?<=^|[^\p{L}\p{Nd}])(X64|AMD64|X86([\p{Pd}\p{Pc}]64))(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; | ||
| Regex::Expression ArchitectureArm64{ R"((?<=^|[^\p{L}\p{Nd}])(ARM64|AARCH64)(?=\P{Nd}|$)(?:\sEDITION)?)", reOptions }; | ||
| Regex::Expression ArchitectureArm{ R"((?<=^|[^\p{L}\p{Nd}])(ARM32(?=\P{Nd}|$)|ARM(?=[^\p{L}\p{Nd}]|$))(?:\sEDITION)?)", reOptions }; |
There was a problem hiding this comment.
There is still an issue with "ARM" being detected as a standalone word. Since it is itself both a word (🦾) and a company, we could be detecting one of those semantics rather than the architecture one. The name could be "ARM Compiler (x64)" from "ARM Corporation" and we would end up with "compiler" from "arm" (I don't think we apply the arch removal to publisher).
I think we might be far enough past ARM32 to completely ignore "ARM" due to its difficulties (we can keep the "ARM32" one as it matches the existing expression better).
There was a problem hiding this comment.
Updated in 211a8b4. The current ArchitectureArm pattern recognizes only ARM32; bare ARM is no longer matched or removed.
I also ran NameNorm_Database_Initial: the earlier bare-ARM variant changed the existing GNU Arm Embedded Toolchain ... fixture, while the current ARM32-only version preserves it. The ARM Compiler (x64) regression asserts the normalizer's canonical output (ARMCompiler) and confirms that x64, not the company name, is removed.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
Signed-off-by: wangluoye <294879785+huong8373tt-beep@users.noreply.github.com>
microsoft-github-policy-service
Bot
removed
the
Needs-Author-Feedback
|
Addressed in 211a8b4. I ran |
microsoft-github-policy-service
Bot
removed
the
Needs-Attention
Signed-off-by: wangluoye <294879785+huong8373tt-beep@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
2 participants
Description of the change
Adds ARM, ARM64, and AARCH64 architecture suffix recognition to package-name normalization. This lets installed ARP entries such as
PowerToys (Preview) arm64correlate with manifests whose names omit the architecture suffix.Closes #6490
Validation
AppInstallerCLITests(Release|x64) on Windows.AppInstallerCLITests.exe NameNorm_Architecture: 1 test case, 9 assertions passed.Attribution
This issue was discovered while Slai Agent Runtime was operating in a Windows environment. The developer authorized this contribution after the issue was reproduced and the fix was validated.
Microsoft Reviewers: Open in CodeFlow