Skip to content
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

Fix: using powershell instead of cmd /C #708

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Conversation

n1vk
Copy link
Contributor

@n1vk n1vk commented Dec 3, 2024

No description provided.

@n1vk
Copy link
Contributor Author

n1vk commented Dec 3, 2024

An approach to resolve issue #707

@8LWXpg
Copy link
Contributor

8LWXpg commented Dec 21, 2024

Similar comment on #549.

@@ -21,7 +21,7 @@ func (e *Engine) startCmd(cmd string) (*exec.Cmd, io.ReadCloser, io.ReadCloser,
if !strings.Contains(cmd, ".exe") {
e.runnerLog("CMD will not recognize non .exe file for execution, path: %s", cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should replace CMD with powershell here.

@xiantang
Copy link
Collaborator

every windows os has powershell?
I'm afraid of it will break some users

@8LWXpg
Copy link
Contributor

8LWXpg commented Jan 21, 2025

As mentioned in #549, Windows PowerShell has been included by default since Windows 10 version 1607, which was released 2016, so the majority of the users should be fine.

@xiantang
Copy link
Collaborator

xiantang commented Jan 21, 2025

As mentioned in #549, Windows PowerShell has been included by default since Windows 10 version 1607, which was released 2016, so the majority of the users should be fine.

can we check first in the user host contains powershell, if doesn't have then fallback to old logic

@8LWXpg
Copy link
Contributor

8LWXpg commented Jan 22, 2025

can we check first in the user host contains powershell, if doesn't have then fallback to old logic

I check the Minimum Requirements for go compiler, and go 1.21 and later only supports Windows 10 and higher, which came with PowerShell 5.0 preinstalled.

Since we're using go 1.23, I think it should be acceptable to replace CMD with PowerShell.

@xiantang
Copy link
Collaborator

can we check first in the user host contains powershell, if doesn't have then fallback to old logic

I check the Minimum Requirements for go compiler, and go 1.21 and later only supports Windows 10 and higher, which came with PowerShell 5.0 preinstalled.

Since we're using go 1.23, I think it should be acceptable to replace CMD with PowerShell.

Make sense.

@xiantang xiantang merged commit 1068416 into air-verse:master Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants