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

Jump to bottom

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

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

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

ALTree opened this issue Mar 2, 2018 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@ALTree
Copy link
Member

ALTree commented Mar 2, 2018 •
edited

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 The path to resolution is known, but the work has not been done. label Mar 2, 2018
@ALTree
Copy link
Member Author

ALTree commented Mar 2, 2018

cc @rasky @randall77

@randall77
Copy link
Contributor

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

@rasky
Copy link
Member

rasky 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
Copy link
Member Author

ALTree 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
Copy link
Member

rasky commented Mar 2, 2018

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

@rasky
Copy link
Member

rasky commented Mar 2, 2018

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

@ALTree
Copy link
Member Author

ALTree commented Mar 2, 2018 •
edited

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
Copy link
Member Author

ALTree commented Mar 2, 2018

Unavoidable I guess, sigh. Closing this.

@ALTree ALTree closed this as completed Mar 2, 2018
@rasky
Copy link
Member

rasky 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
Copy link

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

@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rasky @ALTree @randall77 @gopherbot