Add GHA workflow to run test_crosscompile and go test #141
Conversation
.github/workflows/test.yml
Outdated
| - macos-latest | ||
|
|
||
| steps: | ||
| - name: Set up Go 1.17.x |
There was a problem hiding this comment.
Actually I notice that the go.mod targets 1.13 so maybe we want to multi-compile to check 1.13, 1.17 and 1.18 all work?
There was a problem hiding this comment.
Thank you for this! Awesome idea!
We actually need to support all the way to 1.6 to make sure we are compatible, this is because of some of the more exotic os/arch combinations which use very old versions.
Ideally, we would also test on Windows. We have a PR for the actual support, but even before then, we should ensure we don't create compilation error.
Not sure what is available or practical here, but as we are an ancient lib, deeply embedded in many repo dependency tree, the more checks we have the better. It is easy to introduce something like stirngs.Builder, fmt.Errorf %w or something like that without realizing it was not available back then and would break compatibility.
There was a problem hiding this comment.
So I split this into crosscompile.yml and test.yml. I'm just using your test_crosscompile script for the former but maybe it makes sense to trim that script to just the complicated cases and have a large matrix of GOOS * GOARCH * GOVERSION to run go build across a load of github jobs. Might even be able to get the riscv one working in that matrix and just remove the script all together as long as act can handle it.
That would give you a high build coverage of different versions.
I'm not sure there's as much value in running tests against every go version. We're limited in GHA to some fairly standard runner boxes (ubuntu, macos, windows) and they're going to be running recent OS images so probably just want to run actual tests (when they get added) on latest stables (1.17, 1.18) and maybe the oldest (1.6)?
|
Thank you for digging into this! I am a bit skeptical that the cross compile script will work, but I guess there is only one way to find out :) |
|
Indeed it failed :(. But as it is merged, you now have the ability to run the actions from your fork if you want to open a new PR. |
|
Yup will look into it! I think I can rewrite this to just use GHA matrix rather than relying on a local script to do the matrix. |
creack/pty doesn't currently have any go tests, but running it means as soon as any are added they'll get picked up on PR.
The build comment changes are because "go test" complained about the format without a space.