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/go: varying GOROOT_FINAL invalidates precompiled C dependencies of "net" #50183

Closed
lyderic opened this issue Dec 15, 2021 · 28 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@lyderic
Copy link

lyderic commented Dec 15, 2021

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

$ ~/go/bin/go1.18beta1 version
go version go1.18beta1 linux/amd64

Does this issue reproduce with the latest release?

It reproduces with the latest beta of 1.18. However, it is fine with go 1.17.5!

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

go env Output
$ ~/go/bin/go1.18beta1 version
go version go1.18beta1 linux/amd64
I [unix@go118 foo]$ ~/go/bin/go1.18beta1 env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/unix/.cache/go-build"
GOENV="/home/unix/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/unix/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/unix/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/unix/sdk/go1.18beta1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/unix/sdk/go1.18beta1/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18beta1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/unix/foo/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2623523050=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I tried to compile this program:

$ cat main.go 
package main

import (
        t "github.com/lyderic/tools"
)

func main() {
        t.Magentaln("FOO")
}

What did you expect to see?

I expected it to compile with go 1.18 as it compiles with go 1.17

What did you see instead?

I got the following error:

$ ~/go/bin/go1.18beta1 build
# runtime/cgo
cgo: C compiler "gcc" not found: exec: "gcc": executable file not found in $PATH

@lyderic lyderic changed the title affected/package: go 1.18 go 1.18 beta1: cgo: C compiler "gcc" not found Dec 15, 2021
@cherrymui
Copy link
Member

cherrymui commented Dec 15, 2021

Does you program use cgo anywhere? Is there any build tag that could cause different code to be built with Go 1.17 and Go 1.18? Thanks.

If you don't have a C compiler installed, maybe try setting CGO_ENABLED=0?

@cherrymui cherrymui added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 15, 2021
@lyderic
Copy link
Author

lyderic commented Dec 15, 2021

I confirm that I don't have a C compiler installed. I never needed one, don't need one for go 1.17. I confirm also that I don't use cgo anywhere.

I can work around the problem indeed as follows:

$ CGO_ENABLED=0 ~/go/bin/go1.18beta1 build

However, I would say this is a regression problem as setting CGO_ENABLED is not needed when using go 1.17.

@seankhliao
Copy link
Member

How was go1.18beta1 installed?

@lyderic
Copy link
Author

lyderic commented Dec 15, 2021

I installed go 1.17 on Arch with pacman and I ran the following commands:

$ sudo pacman -Syyu go go-tools
$ go install golang.org/dl/go1.18beta1@latest
$ ~/go/bin/go1.18beta1 download
$ ~/go/bin/go1.18beta1 version
go version go1.18beta1 linux/amd64

@seankhliao
Copy link
Member

Seems reproducible with just: (tried with both archlinux:base and debian container images)

package main

import _ "net/http"

func main() {}

@lyderic
Copy link
Author

lyderic commented Dec 15, 2021

Please note that we had the same regression issue when we moved from 1.16 to 1.17: #47215

It had been fixed then.

@cherrymui cherrymui added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Dec 15, 2021
@cherrymui cherrymui added this to the Go1.18 milestone Dec 15, 2021
@cherrymui
Copy link
Member

cc @bcmills as this is perhaps related to #47215

Also cc @golang/release for if there is anything related to building the release.

Marking as release blocker as it is a regression and was previously a release blocker.

@bcmills bcmills self-assigned this Dec 16, 2021
@aclements aclements changed the title go 1.18 beta1: cgo: C compiler "gcc" not found cmd/go: C compiler "gcc" not found building non-cgo code Jan 4, 2022
@heschi
Copy link
Contributor

heschi commented Jan 12, 2022

Ping: we think this is an RC blocker.

@bcmills
Copy link
Member

bcmills commented Jan 21, 2022

I can reproduce this without even using a container by using a go1.18beta1 installed via go install and paring back $PATH to not include a C compiler.

I think this is specific to the way that golang.org/dl/go1.18beta1 distributes itself. Notably, it does not occur when using gotip in an identical environment:

$ go1.18beta1 build -o /dev/null .
# runtime/cgo
cgo: C compiler "gcc" not found: exec: "gcc": executable file not found in $PATH

$ gotip version
go version devel go1.18-d15481b8c Fri Jan 21 01:14:28 2022 +0000 linux/amd64

Still investigating the root cause.

@bcmills
Copy link
Member

bcmills commented Jan 21, 2022

This notably does not reproduce with a go1.18beta1 built from source using make.bash:

$ go1.18beta1 build -o /dev/null .
# runtime/cgo
cgo: C compiler "gcc" not found: exec: "gcc": executable file not found in $PATH

$ ~/go1.18beta1/bin/go build -o /dev/null .

@bcmills
Copy link
Member

bcmills commented Jan 21, 2022

Ah. This is probably from https://go.dev/cl/353352 (which fixed #48319).

When -trimpath is not set, the value of GOROOT_FINAL ends up in the compiled C dependencies. Because of that, we now include GOROOT_FINAL (which defaults to GOROOT) in the cache key for those dependencies.

x/build/cmd/release explicitly sets GOROOT_FINAL to /usr/local/go, but go1.18beta1 doesn't actually end up installing itself there: instead, it installs to $HOME/sdk/go1.18beta1. When you run go1.18beta1 env GOROOT it notices the mismatch and reports the real GOROOT instead, but the built C artifacts are stale because they have embedded the path /usr/local/go instead of $HOME/sdk/go1.18beta1.

So when you run go build, it tries to rebuild the C dependencies of the net package (see #47257 (comment)), and because CGO_ENABLED=1 it requires gcc to do so.

Indeed, setting GOROOT_FINAL to match the value set by x/build/cmd/release causes the precompiled .a file to no longer be stale and allows the build to succeed:

$ GOROOT_FINAL=/usr/local/go go1.18beta1 build -o /dev/null .

@bcmills
Copy link
Member

bcmills commented Jan 21, 2022

One possible resolution might be to build the release with -trimpath, so that users could themselves build with -trimpath to avoid staleness. (That change would be up to @golang/release to decide.)

Another possible resolution is to declare that you need a C compiler in order to build with CGO_ENABLED=1. (That seems eminently reasonable to me, but it would be a change in policy.)

A third possible resolution is to declare that if you want to use a precompiled Go distribution without a C compiler, you must either set GOROOT_FINAL explicitly to the path used when building that distribution, or install that precompiled distribution to exactly its preordained GOROOT_FINAL location.

@bcmills
Copy link
Member

bcmills commented Jan 21, 2022

As a more immediate workaround (but IMO clearly a hack), I suppose we could change golang.org/dl/internal/version to explicitly set GOROOT_FINAL itself before invoking the go command. 😅

(@golang/release: how do y'all feel about that?)

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 21, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 21, 2022
@bcmills bcmills changed the title cmd/go: C compiler "gcc" not found building non-cgo code cmd/go: varying GOROOT_FINAL invalidates precompiled C dependencies of "net" Jan 21, 2022
@ianlancetaylor
Copy link
Contributor

When -trimpath is not set, the value of GOROOT_FINAL ends up in the compiled C dependencies. Because of that, we now include GOROOT_FINAL (which defaults to GOROOT) in the cache key for those dependencies.

Looking at #48319 the issues to be the DW_AT_comp_dir debugging entry which describes the compilation directory. But I don't understand why we can't use -fdebug-prefix-map to avoid this.

@bcmills
Copy link
Member

bcmills commented Jan 22, 2022

I think that would be more-or-less equivalent to always using the -trimpath behavior for C dependencies in the standard library. That may be an option , although it would lose some debugging information that one might expect to be preserved when -trimpath isn't set.

@ianlancetaylor
Copy link
Contributor

Seems that we already do the equivalent for the Go code in the standard library, at least in the release.

@bcmills
Copy link
Member

bcmills commented Jan 24, 2022

My understanding is that the Go linker expands the elided GOROOT at link time to produce absolute paths in the resulting binaries:
https://cs.opensource.google/go/go/+/master:src/cmd/link/internal/ld/dwarf.go;l=1138-1168;drc=587b3c1192397393afb0ec5acd608e3dfe9f2116

I don't know whether it does that only for Go libraries, or for C dependencies of Go libraries as well. (Or do we always use the C linker if the binary includes C dependencies?)

@cherrymui
Copy link
Member

I think that expansion only applies to Go code, as it applies to files from the FileTable from the CompilationUnit, which seems specific to Go object files.

@gopherbot
Copy link

Change https://golang.org/cl/380503 mentions this issue: internal/version: set GOROOT_FINAL and PWD in environments

@bcmills
Copy link
Member

bcmills commented Jan 24, 2022

Absent a clear alternative, I'm going to go ahead and mail the GOROOT_FINAL change in golang.org/dl/internal/version.
It's at least a strict improvement over the status quo.

@dmitshur
Copy link
Contributor

If I understand correctly, once the accepted proposal #47257 is implemented, this issue (and others like it) will stop being relevant. So that is the long-term fix for this problem, and we just need a short-term one here.

One possible resolution might be to build the release with -trimpath, so that users could themselves build with -trimpath to avoid staleness. (That change would be up to @golang/release to decide.)

It seems that it's not tenable to release with -trimpath unless that becomes the default in cmd/go as well, since we want to avoid staleness with the most common configuration, which is using defaults. Either way, this isn't relevant post-#47257, and there are better short-term alternatives below.

Another possible resolution is to declare that you need a C compiler in order to build with CGO_ENABLED=1. (That seems eminently reasonable to me, but it would be a change in policy.)

Needing a C compiler to build with CGO_ENABLED=1 seems reasonable to me too, and I don't see how things can work otherwise. This seems inevitable post-#47257, and before then, there are better short-term alternatives below.

A third possible resolution is to declare that if you want to use a precompiled Go distribution without a C compiler, you must either set GOROOT_FINAL explicitly to the path used when building that distribution, or install that precompiled distribution to exactly its preordained GOROOT_FINAL location.

As a more immediate workaround (but IMO clearly a hack), I suppose we could change golang.org/dl/internal/version to explicitly set GOROOT_FINAL itself before invoking the go command. 😅

(@golang/release: how do y'all feel about that?)

This seems like the best option to improve things before #47257 is resolved. Its only downside is it increases complexity in golang.org/dl/internal/version, but oh well.

Now that os.Executable() exists and cmd/go finds itself and infers its GOROOT from that, I wonder if GOROOT_FINAL has outlived its usefulness and can be removed, which would help remove the complexity/issues it adds. If so, this workaround in golang.org/dl/internal/version can eventually go away.

@dmitshur
Copy link
Contributor

Based on analysis in #50183 (comment) and following discussion, it sounds like it's determined that cmd/go itself is working as intended for 1.18, and there are no changes planned to it.
CL 380503 will improve the behavior of the golang.org/dl/... commands.
Is that all that's needed here for 1.18, or is there anything more?

@bcmills
Copy link
Member

bcmills commented Jan 25, 2022

Is that all that's needed here for 1.18, or is there anything more?

I'm going to run it by @rsc and @matloob later today, but my opinion at the moment is that CL 380503 is sufficient to address this issue for 1.18.

@bcmills
Copy link
Member

bcmills commented Jan 25, 2022

@rsc pointed out that setting GOROOT_FINAL will also affect the GOROOT stamped into any user-built executables, which would be unfortunate.

We're going to see if the -fdebug-prefix-map approach is simple enough for 1.18, since that's likely equivalent to what we'll do for #47257 anyway. (Then we could remove GOROOT_FINAL from the cache key for libraries.)

@gopherbot
Copy link

Change https://golang.org/cl/380915 mentions this issue: cmd/go: avoid recording GOROOT_FINAL in precompiled C archives

@gopherbot
Copy link

Change https://golang.org/cl/380914 mentions this issue: cmd/go: refactor TestScript/build_issue48319 to check a more general property

@gopherbot
Copy link

Change https://golang.org/cl/380916 mentions this issue: cmd/go: add mv and support "! cmp" in script tests

gopherbot pushed a commit that referenced this issue Jan 26, 2022
For #50183

Change-Id: Ie384333fb7a69d0d2cfaba0cfc4eb7afba2fd745
Reviewed-on: https://go-review.googlesource.com/c/go/+/380916
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2022
…property

The test previously checked that the DWARF DW_AT_comp_dir attribute
matched GOROOT_FINAL. However, on further consideration, we believe
that DW_AT_comp_dir should not actually match GOROOT_FINAL: the DWARF
spec says that DW_AT_comp_dir records “the current working directory
of the compilation command that produced this compilation unit”, but
the actual working directory of the compilation command proper is a
throwaway directory in the build cache — it is neither stable nor
meaningful.

However, the test was getting at a real issue that we do care about:
namely, that the binary produced by a 'go build' command with cgo
enabled should not reuse a dependency that embeds a stale
GOROOT_FINAL.

This change refactors the test to verify the latter property instead
of checking DW_AT_comp_dir specifically.

For #50183
Updates #48319

Change-Id: I0b1151d9ba3d0ff903f72e27850306406e5cb518
Reviewed-on: https://go-review.googlesource.com/c/go/+/380914
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 26, 2022
@dmitshur dmitshur removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 26, 2022
@lyderic
Copy link
Author

lyderic commented Jan 31, 2022

The issue is fixed for me when using version 1.18beta2 instead of 1.18beta1.

Many thanks!

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
For golang#50183

Change-Id: Ie384333fb7a69d0d2cfaba0cfc4eb7afba2fd745
Reviewed-on: https://go-review.googlesource.com/c/go/+/380916
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
…property

The test previously checked that the DWARF DW_AT_comp_dir attribute
matched GOROOT_FINAL. However, on further consideration, we believe
that DW_AT_comp_dir should not actually match GOROOT_FINAL: the DWARF
spec says that DW_AT_comp_dir records “the current working directory
of the compilation command that produced this compilation unit”, but
the actual working directory of the compilation command proper is a
throwaway directory in the build cache — it is neither stable nor
meaningful.

However, the test was getting at a real issue that we do care about:
namely, that the binary produced by a 'go build' command with cgo
enabled should not reuse a dependency that embeds a stale
GOROOT_FINAL.

This change refactors the test to verify the latter property instead
of checking DW_AT_comp_dir specifically.

For golang#50183
Updates golang#48319

Change-Id: I0b1151d9ba3d0ff903f72e27850306406e5cb518
Reviewed-on: https://go-review.googlesource.com/c/go/+/380914
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
C archives for packages in GOROOT are shipped along with binary
releases of the Go toolchain. Although we build the toolchain with
GOROOT_FINAL set, we don't know actually know where the release will
be installed: the user's real GOROOT can differ arbitrarily from our
GOROOT_FINAL.

(In the specific case of toolchains installed through golang.org/dl
wrappers, the release's GOROOT_FINAL is /usr/local/go but the actual
GOROOT to which the release is installed is
$HOME/sdk/$(go env GOVERSION).)

Fixes golang#50183
Updates golang#48319

Change-Id: If10a42f90c725300bbcb89c3b5b01a2d93ab6ef7
Reviewed-on: https://go-review.googlesource.com/c/go/+/380915
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@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.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants