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

Support Go 1.18 #2515

Closed
wants to merge 5 commits into from
Closed

Support Go 1.18 #2515

wants to merge 5 commits into from

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Jan 12, 2022

The number of CI configurations has shortened now, so I'm not sure if you want to add another, or bump one of them. I don't think 1.18 is available on Circle yet, but it should be available with the setup-go Action.

@QuLogic QuLogic added the enhancement New feature or request label Jan 12, 2022
@aykevl
Copy link
Member

aykevl commented Jan 21, 2022

IMHO we only really support something if we test it in CI (otherwise it could be silently broken any time), so yes I think we should run the tests for Go 1.18.

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 4, 2022

Okay, I've managed to run go test now that 0.22.0 is usable for me; looks like a few new os things:

    main_test.go:526: test error: could not compile: /usr/lib/golang/src/internal/fuzz/sys_posix.go:61:15: NewFile not declared by package os
    main_test.go:526: test error: could not compile: /usr/lib/golang/src/os/exec/exec.go:246:23: DevNull not declared by package os

I believe these all come from TestTest/*/Pass and TestTest/*/Fail, but strangely these are neither new in 1.18 nor removed. This seems to affect smoketest, wasmtest, and tinygo-test, so something is probably up.

I think I've figured out how to get this running on Actions at least, so let's see if it's broken in CI too.

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 4, 2022

The good news is it is failing on CI so it's not just on my side; the bad news is the errors appear to be different?

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 5, 2022

The major missing piece appears to be support bits for fuzzing. That requires a bit of not-quite-implemented stuff for signals, and files in src/testing/ (which are hidden since the GoRoot merger ignores Go's files when TinyGo has files in that directory, so src/testing/fuzz.go isn't symlinked into the root.) It also changes the signature of testing.MainStart.

I'm not sure what to do about fuzzing, but I've opened the signal parts in #2615.

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 6, 2022

main_test.go:526: test error: could not compile: /usr/lib/golang/src/internal/fuzz/sys_posix.go:61:15: NewFile not declared by package os

OK, this one was fixed by #2499 on dev already, so we don't see it on CI here. Instead, we see:

main_test.go:538: test error: could not compile: /opt/hostedtoolcache/go/1.18.0-beta2/x64/src/internal/fuzz/sys_posix.go:61:23: cannot use 3 (constant of type int) as os.FileHandle value in argument to os.NewFile: int does not implement os.FileHandle: missing method Close

because os.NewFile uses a uintptr in Go but in TinyGo it's an interface.

Locally I've added an empty fuzz shim with just the types and a couple functions that returns errors, plus modified testing.MainStart. This enables compiling and I can run all tests with #2615, but some of tinygo-test crashes in I/O. So not I can slowly work forwards to enabling the bits needed for fuzzing.

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 6, 2022

main_test.go:526: test error: could not compile: /usr/lib/golang/src/os/exec/exec.go:246:23: DevNull not declared by package os

This one is because DevNull is only defined on certain architectures; not sure what the equivalent is on e.g., WASM, but I guess should be /dev/null on the ARM and RISCV linux?

@QuLogic QuLogic force-pushed the go118 branch 3 times, most recently from 447ac25 to 4a05f70 Compare February 7, 2022 01:04
@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 7, 2022

With #2615, #2619 and some stub fuzzing bits, this can now build tests on Linux. Running the pass/fail host test fails though because of #2616, which I'm not sure about.

On Windows, the build fails because of:

Error:     main.go:1082: ...\src\internal\syscall\windows\registry\key.go:97:10: LockOSThread not declared by package runtime
Error:     main.go:1082: ...\src\internal\syscall\windows\registry\key.go:98:16: UnlockOSThread not declared by package runtime

On macOS, the build fails because of:

Error:     main_test.go:517: test error: could not compile: .../src/internal/fuzz/sys_posix.go:37:11: Munmap not declared by package syscall

(not sure why this isn't triggered on Linux as I don't see Munmap anywhere in TinyGo.)

@deadprogram
Copy link
Member

What is the status with this PR? Since Go 1.18 is going to be released very soon... 😺

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 12, 2022

This builds on Linux, but not the other platforms. It also can't run due to some missing LLVM implementation. See the previous comment.

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 15, 2022

Now that my PRs were merged, I split out a few things that are not specific to Go 1.18 itself into separate PRs, rebased this on top of them / latest dev, and bumped to rc1. I also left out the stubs for fuzzing, just to see if that would compile.

This update seems to have caused a new issue at places like:

Error:     main.go:1093: /Users/runner/hostedtoolcache/go/1.18.0-rc1/x64/src/fmt/format.go:72:17: todo: cap: unknown type

This line is calling cap on buffer, which is type buffer []byte. It looks like cap allows slices, but maybe it doesn't drop to the underlying type for evaluating that?

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 15, 2022

I also left out the stubs for fuzzing, just to see if that would compile.

OK, I restored the stubs for now, since things are still broken without them.

@QuLogic QuLogic force-pushed the go118 branch 3 times, most recently from 9dd8f1e to c6ea0cb Compare March 20, 2022 23:25
@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 21, 2022

Based on #2616 (comment), I changed the internal/fuzz stub to hide the entirety of the package. This should be enough to run the smoketest at least.

@QuLogic QuLogic force-pushed the go118 branch 2 times, most recently from 527ced5 to 9e59e98 Compare March 21, 2022 09:43
This simply shadows the real code temporarily to see what else is
broken. It only defines a single type to fix testing/internal/testdeps.
@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 21, 2022

Windows is still troubled by LockOSThread/UnlockOSThread, but macOS and Linux now pass make test. They are crashing in make tinygo-test with:

#0  __restore_sigs (set=0x7fffb80089f8) at lib/musl/src/signal/block.c:44
#1  0x0000000000215bd2 in raise (sig=<optimized out>) at lib/musl/src/signal/raise.c:11
#2  0x00000000002159b6 in abort () at lib/musl/src/exit/abort.c:13
#3  0x000000000021982d in runtime.runtimePanic (msg=...) at .../tinygo/src/runtime/panic.go:20
#4  0x0000000000219b4b in runtime.nilPanic () at .../tinygo/src/runtime/panic.go:32
#5  0x00000000002216ca in (Go interface method) () at /usr/lib/golang/src/os/signal/signal.go:333
#6  0x0000000000233951 in os.removeAll (path=...) at .../tinygo/src/os/removeall_noat.go:44
#7  os.RemoveAll (path=...) at .../tinygo/src/os/path.go:67
#8  (*testing.common).TempDir$2 () at .../tinygo/src/testing/testing.go:313
#9  0x0000000000233de6 in (*testing.common).runCleanup (c=0x7fffb8010120) at .../tinygo/src/testing/testing.go:344
#10 testing.tRunner$1 () at .../tinygo/src/testing/testing.go:361
#11 0x0000000000233de6 in testing.tRunner (t=<optimized out>, fn=...)
#12 0x00000000002375d6 in testing_test.TestTempDirInCleanup (t=0x7fffb800fd40) at .../tinygo/src/testing/testing_test.go:34
#13 0x0000000000233b22 in testing.tRunner (t=0x2, fn=...) at .../tinygo/src/testing/testing.go:366
#14 0x0000000000233ef2 in testing.runTests$1 (t=0x7fffb8009ec0) at .../tinygo/src/testing/testing.go:469
#15 0x0000000000233b22 in testing.tRunner (t=0x2, fn=...) at .../tinygo/src/testing/testing.go:366
#16 0x000000000023727c in testing.runTests (matchString=..., tests=...) at .../tinygo/src/testing/testing.go:467
#17 (*testing.M).Run (m=<optimized out>) at .../tinygo/src/testing/testing.go:443
#18 testing_test.TestMain (m=<optimized out>) at .../tinygo/src/testing/testing_test.go:23
#19 0x0000000000237c21 in main.main () at ~/.cache/go-build/2b/2b4feb4a2b2d25afe5cd27aa5b910f0bd3aea48bbf65fe9841e079a47c561bd3-d:79

This is very strange though; it seems that the Lstat call in os.RemoveAll returned both a nil result and a nil error.

@dkegel-fastly
Copy link
Contributor

#2725 adds stubs for LockOSThread / UnlockOSThread.

Only hitch appears to be compress/bzip2 on windows.

Thank you for working so hard on this, it's very nearly there!

@dkegel-fastly
Copy link
Contributor

minimal go 1.18 support has landed. Thanks to QuLogic for getting the ball rolling. Closing.

@QuLogic QuLogic deleted the go118 branch August 6, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants