-
Notifications
You must be signed in to change notification settings - Fork 513
Enable Searching for Pwsh in the "Program Files (Arm)" directory on Windows Arm64 #5225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…windows arm64 - Add tests case documenting expected load order - Update test cases to refer to v7 instead of v6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the PowerShell executable search logic on Windows Arm64 by including the "Program Files (Arm)" directory. Key changes include:
- Adding an isArm64 flag in platform details.
- Iterating over multiple possible Program Files paths (including "Program Files (Arm)").
- Updating test cases to expect version 7 of PowerShell instead of version 6.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
test/core/platform.test.ts | Updated test cases to include "Program Files (Arm)" and version updates. |
src/platform.ts | Added isArm64 property and modified the logic to try multiple Program Files paths. |
Comments suppressed due to low confidence (1)
src/platform.ts:715
- Update the method's documentation to reflect the new return type, noting that it returns an array of possible ProgramFiles paths instead of a single string.
}: { useAlternateBitness?: boolean } = {}): (string | undefined)[] {
@@ -562,93 +564,96 @@ export class PowerShellExeFinder { | |||
}: { useAlternateBitness?: boolean; findPreview?: boolean } = {}): Promise< | |||
IPossiblePowerShellExe | undefined | |||
> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment explaining the iteration over multiple ProgramFiles paths to clarify the search order and rationale.
> { | |
> { | |
// Iterate over possible `ProgramFiles` paths to locate PowerShell installations. | |
// This accounts for different system configurations, such as 32-bit and 64-bit installations, | |
// and ensures compatibility with alternate bitness settings if specified. |
Copilot uses AI. Check for mistakes.
@microsoft-github-policy-service agree |
Thanks! Do we have a way to meaningfully test this? Do we need to spin up/add ARM to the testing matrix? |
Honestly I wasn't sure what the options were there. I would certainly recommend spinning up an arm CI pipeline if its possible as the related PSES changed I linked doesn't seem to impact my x64 builds but I'm really not sure why tbh. I did add the unit tests to describe the expected order as interim support. |
Bah, I thought I re-ran the formatter before committing. Sorry about that, I'll get that fixed later tonight |
PR Summary
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
x
between the square brackets.Please mark anything not applicable to this PR
NA
.WIP:
to the beginning of the title and remove the prefix when the PR is ready