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

cmd/compile: new codegen test harness grabs wrong go binary to run tests #24217

Closed
ALTree opened this issue Mar 2, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@ALTree
Copy link
Member

commented Mar 2, 2018

The new code generation test harness introduced in CL 97355 does not work when working on tip with a codegen transformation only present on tip. For example, I recently taught the 386 backend to intrisify math.Sqrt and when I add the following test

func sqrt(x float64) float64 {
	// 386:"SQRTSD"
	return math.Sqrt(x)
}

to the tip repository and run it, it'll fail because the asmcheck driver in test/run.go uses this line to compile the codegen test files:

cmdline := []string{"go", "build", "-gcflags", "-S"}

and on my system go points to the stable installation in /usr/local/go, which is a 1.10 toolchain, where math.Sqrt is not intrisified on 386.

In the old test harness we use internal/testenv to ensure we're using the right go toolchain:

testenv.MustHaveGoRun(t)
gotool := testenv.GoToolPath(t)
cmd := exec.Command(gotool, "run",   [etc...])

but I believe we can't do this inside a test in the go/test.

@ALTree ALTree added the NeedsFix label Mar 2, 2018

@ALTree

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2018

I believe we just need to use testenv.GoTool() instead of "go".

@rasky

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

If I change that, I would change it for the whole testsuite (so not only asmcheck). Right now, it assumes that the just-build Go is first in the PATH.

@ALTree

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

But when you add a new test in the toplevel test folder it usually works fine even if the fix is only present in the tip repo and not in the go command in the PATH. Why does codegen fail?

@rasky

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

That’s surprising to me; you can check run.go and it always runs “go something”.

@rasky

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

... unless you usually run the other tests through all.bash, which tweaks the PATH

@ALTree

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

Ah, yes, I was confused. You are right, I checked my notes and they say that in order to directly run run.go and have it use tip I have to tweak my env. So yeah, this never actually worked out of the box for the test folder.

It's a little unfortunate, since it does work in the old test harness, but if we're fine with this I guess we can close.

@ALTree

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

Unavoidable I guess, sigh. Closing this.

@ALTree ALTree closed this Mar 2, 2018

@rasky

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

I don't think it's unavoidable: we can change run.go to use testenv.GoTool as Keith suggested.

@rasky rasky reopened this Mar 2, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Mar 3, 2018

Change https://golang.org/cl/98439 mentions this issue: test: use the version of Go used to run run.go

@gopherbot gopherbot closed this in ec0b8c0 Mar 3, 2018

@golang golang locked and limited conversation to collaborators Mar 3, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.