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

net: powershell invocation in tests broken in sysarm32 mode #42858

Open
zx2c4 opened this issue Nov 27, 2020 · 6 comments
Open

net: powershell invocation in tests broken in sysarm32 mode #42858

zx2c4 opened this issue Nov 27, 2020 · 6 comments
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 27, 2020

Windows 10 for ARM64 will run ARM32 executables just fine, which is why we're using it for Go builders.

There's currently a bug in which invoking powershell from an arm32 process will fail:
image

I've worked around that by grabbing a zip of powershell 7.1, adding the unzipped directory to the builder's PATH, and then copying pwsh.exe to powershell.exe. I'm waiting for some builds to finish, but I think that should work alright.

The reason this matters for Go is that some of the tests use powershell just to invoke a simple command:

func runCmd(args ...string) ([]byte, error) {
        removeUTF8BOM := func(b []byte) []byte {
                if len(b) >= 3 && b[0] == 0xEF && b[1] == 0xBB && b[2] == 0xBF {
                        return b[3:]
                }
                return b
        }
        f, err := ioutil.TempFile("", "netcmd")
        if err != nil {
                return nil, err
        }
        f.Close()
        defer os.Remove(f.Name())
        cmd := fmt.Sprintf(`%s | Out-File "%s" -encoding UTF8`, strings.Join(args, " "), f.Name())
        out, err := exec.Command("powershell", "-Command", cmd).CombinedOutput()

I'm pretty sure we can come up with something better than powershell -Command for the tests, which would make this problem go away. Or sooner or later Microsoft will probably fix it. Or my workaround will suffice.

Either way, I thought I should at least document this.

CC @alexbrainman @jstarks @cagedmantis

@alexbrainman
Copy link
Member

I am happy to get rid of powershell dependency, if possible.

From what I remember we mainly use powershell to be able to read non-ASCII output of Windows commands on non-English versions of Windows.

Including @mattn because he uses non-English Windows.

Alex

@ALTree ALTree added OS-Windows arch-arm Issues solely affecting the 32-bit arm architecture. labels Nov 28, 2020
@mattn
Copy link
Member

mattn commented Nov 28, 2020

One another way to avoid the issue is call chcp 65001 before any commands. it output english. Or call API GetConsoleCP and SetConsoleOutputCP.

cp := windows.GetConsoleCP()
defer windows.SetConsoleOutputCP(cp)
windows.SetConsoleOutputCP(windows.CP_UTF8)

// do something

(But currently internal/syscall/windows does not have the APIs)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/273673 mentions this issue: net: set and restore console codepage in runCmd

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 28, 2020

windows.SetConsoleOutputCP(windows.CP_UTF8)

I wonder if Go should call that in the runtime init anyway...

@mattn
Copy link
Member

mattn commented Nov 28, 2020

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 28, 2020

No.

https://dev.to/mattn/please-stop-hack-chcp-65001-27db

Good to know. Thanks for the info.

@FiloSottile FiloSottile changed the title windows/arm: powershell broken in sysarm32 mode net: powershell invocation in tests broken in sysarm32 mode Nov 29, 2020
@FiloSottile FiloSottile added the Testing An issue that has been verified to require only test changes, not just a test failure. label Nov 29, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 7, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

7 participants