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

misc/cgo/testplugin: TestIssue25756pie broken on darwin/arm64 on release-branch.go1.17 #52995

Closed
bcmills opened this issue May 19, 2022 · 11 comments
Closed

misc/cgo/testplugin: TestIssue25756pie broken on darwin/arm64 on release-branch.go1.17 #52995

bcmills opened this issue May 19, 2022 · 11 comments
Labels
arch-arm64 FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.17.11

Comments

@bcmills
Copy link
Contributor

bcmills commented May 19, 2022 

--- FAIL: TestIssue25756pie (1.25s)
    plugin_test.go:274: ./issue25756pie.exe: exit status 2
        panic: plugin.Open("life"): plugin was built with a different version of package sync/atomic

greplogs -l -e 'plugin was built with a different version of package sync/atomic' --since=2022-01-01
2022-05-18T16:46:28-0e7138a/darwin-arm64-11
2022-05-18T16:46:28-0e7138a/darwin-arm64-12

―

Curiously, this failed on the darwin-arm64-11 and darwin-arm64-12 builders but not darwin-arm64-11_0-toothrot. 🤔
I wonder if the new arm64 builders are misconfigured in some way?

(This is a release-blocker because darwin/arm64 is a first class port; attn @golang/release.)
@bcmills bcmills added OS-Darwin NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker arch-arm64 labels May 19, 2022
@bcmills bcmills added this to the Go1.17.11 milestone May 19, 2022
@bcmills
Copy link
Contributor Author

bcmills commented May 19, 2022

It looks like the test is using the go command from $PATH rather than the one from GOROOT/bin:
https://cs.opensource.google/go/go/+/master:misc/cgo/testplugin/plugin_test.go;l=116;drc=afd181cf0b69c3591d7e47ceca4fabf14434d77e

That could have something to do with it.

(That will be fixed automatically by CL 404134, but that isn't even merged at head, let alone at 1.17.)

@bcmills bcmills self-assigned this May 19, 2022
@gopherbot
Copy link

Change https://go.dev/cl/407294 mentions this issue: misc/cgo: invoke "go" from $GOROOT/bin instead of $PATH

@bcmills bcmills mentioned this issue May 19, 2022
Closed
gopherbot pushed a commit that referenced this issue May 19, 2022
@gopherbot
misc/cgo: invoke "go" from $GOROOT/bin instead of $PATH
9bc544a 
If PATH doesn't contain GOROOT/bin as the first element, this could
otherwise end up running entirely the wrong command (and from the
wrong GOROOT, even).

I pre-tested this change on release-branch.go1.17 using a gomote.
I believe that it will fix the test failure on that branch,
but will need to be backported.

For #52995.

Change-Id: Ib0c43289a1e0ccf9409f0f0ef8046501a955ce65
Reviewed-on: https://go-review.googlesource.com/c/go/+/407294
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/407315 mentions this issue: [release-branch.go1.17] misc/cgo: invoke "go" from $GOROOT/bin instead of $PATH

@gopherbot
Copy link

Change https://go.dev/cl/407316 mentions this issue: [release-branch.go1.18] misc/cgo: invoke "go" from $GOROOT/bin instead of $PATH

@bcmills
Copy link
Contributor Author

bcmills commented May 19, 2022

I can't reproduce this on a gomote instance. (I tried both go test ./cgo/testplugin and go tool dist test testplugin using a gomote push of the current release-branch.go1.17.)

I suspect that this may be a bad interaction with some bug in the buildlet binary on this builder, and possibly also a remaining bug in the test (https://golang.org/cl/407315 didn't fix it).

From the output of go version -m I determined that it is from some time at or after CL 356589 but before CL 381735 (Jan. 28). Notably, it does not include CL 354754.

As a next step I suggest that we bump the buildletVersion constant in cmd/buildlet and deploy a fresh build of it to these builders.

@cherrymui
Copy link
Member

See also #46239. It is not reproducible locally or on gomote. The skip is specific to the toothrot builders https://cs.opensource.google/go/go/+/release-branch.go1.17:misc/cgo/testplugin/plugin_test.go;l=268

@heschi
Copy link
Contributor

heschi commented May 23, 2022

I'll try a new version of the buildlet once someone is near the machines in case I break them again.

@gopherbot
Copy link

Change https://go.dev/cl/407881 mentions this issue: [release-branch.go1.17] cmd/dist: consistently set PWD when executing a command in a different directory

@bcmills
Copy link
Contributor Author

bcmills commented May 23, 2022

Just a hunch: https://go.dev/cl/353449 was included in Go 1.18, but not backported to 1.17. That might interact with dist test and the buildlet in some non-trivial way to produce what appears to be version skew due to a mismatched GOROOT.

I'll send a backport of that through the SlowBot to see if it fixes things. (https://go.dev/cl/407881)

@bcmills
Copy link
Contributor Author

bcmills commented May 23, 2022

Looks like that's the jackpot!
https://storage.googleapis.com/go-build-log/cefcdb71/darwin-arm64-12_6a7eca57.log

@gopherbot
Copy link

Closed by merging a900337 to release-branch.go1.17.

gopherbot pushed a commit that referenced this issue May 23, 2022
@heschi
[release-branch.go1.17] cmd/dist: consistently set PWD when executing…
a900337 
… a command in a different directory

Fixes #52995
Updates #33598

Change-Id: If0de906ffa2fcc83bb2a90f9e80a5b29d7667398
Reviewed-on: https://go-review.googlesource.com/c/go/+/353449
Trust: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit c035d82)
Reviewed-on: https://go-review.googlesource.com/c/go/+/407881
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 24, 2022
@dmitshur dmitshur added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jun 1, 2022
@rsc rsc unassigned bcmills Jun 22, 2022
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-arm64 FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.17.11
Development

No branches or pull requests
5 participants
@dmitshur @bcmills @gopherbot @cherrymui @heschi