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

go/build: name_$(GOOS)_$(GOARCH)_test.syso is accepted, but included in non-test binaries #52921

Open
TBBle opened this issue May 16, 2022 · 0 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@TBBle
Copy link

TBBle commented May 16, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18.1 windows/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\paulh\AppData\Local\go-build
set GOENV=C:\Users\paulh\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\paulh\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\paulh\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.18.1
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\paulh\AppData\Local\Temp\go-build3103084805=/tmp/go-build -gno-record-gcc-switches

What did you do?

  • Given containerd, where cmd/containerd/... depends on pkg/os/....
  • Generated a file pkg/os/rsrc_windows_amd64_test.syso in order to add a manifest to the Windows AMD64 build of the test binary generated for that package, which did not include the "run as administrator" flag.
  • Generated a file cmd/containerd/rsrc_windows_amd64.syso in order to add a manifest for the containerd.exe generated for that package, which included the "run as administrator" flag.

containerd/containerd#6945 is where this work took place.

If this isn't clear, I could put together a minimal reproduction fairly easily.

What did you expect to see?

  • go test pkg/os/... would act as if it had a manifest, correctly identifying the Windows version on which it was running.
  • go install cmd/containerd/... would produce conatinerd.exe that correctly identifies the Windows version on which it is running, and is marked to require Administrator permissions.

What did you see instead?

  • go test pkg/os/... functioned as expected.
  • go install cmd/containerd/... did not have the 'run as administrator" flag.
  • go install cmd/containerd/... logged the following output:
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: .rsrc merge failure: multiple non-default manifests
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: .rsrc merge failure: multiple non-default manifests

Both of these issues went away if I deleted pkg/os/rsrc_windows_amd64_test.syso, demonstrating that it was including that file in the build of cmd/containerd/....

I had a quick look at src/go/build/build.go, and the problem seems to be that goodOSArchFile accepts name_$(GOOS)_$(GOARCH)_test.* and other _test.* variants, but the code that splits out the _test variants only processes _test.go.

I think perhaps goodOSArchFile should reject name_$(GOOS)_$(GOARCH)_test.* and accept explicitly name_$(GOOS)_$(GOARCH)_test.go so that I would have gotten much earlier feedback that what I had wasn't going to do the right thing. I'd prefer that the _test.* suffix work for all extensions, but that looks like a much larger change, as the Package struct doesn't have a way to represent non-Go Test files.

This problem is not syso-specific, so I imagine if someone was trying to use name_$(GOOS)_$(GOARCH)_test.s they'd have the same issue, but I guess this specific use-case is fairly specific to Windows binary resource tables integrated using syso files.

I plan to work around this using some variant of #42477 (comment), so this bug is mostly that the build didn't fail for the non-sensical input it was given.

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted labels May 16, 2022
@bcmills bcmills modified the milestones: Unplanned, Backlog May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants