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: GOROOT override using linker -X flag is probably not right #22155

Closed
rsc opened this issue Oct 5, 2017 · 2 comments
Closed

cmd/go: GOROOT override using linker -X flag is probably not right #22155

rsc opened this issue Oct 5, 2017 · 2 comments

Comments

@rsc
Copy link
Contributor

rsc commented Oct 5, 2017

CL 61310 fixed #21313.

The original report was that if you unpack a distribution expecting to go into /usr/local/go into, say, /tmp/go, and then you build a binary, the result of runtime.GOROOT() from that binary says /usr/local/go instead of /tmp/go (unless you've explicitly set $GOROOT to /tmp/go or something else). The reporter wanted to get /tmp/go instead.

CL 61310 added this logic:

if cfg.GOROOT != runtime.GOROOT() {
	ldflags = append(ldflags, "-X=runtime/internal/sys.DefaultGoroot="+cfg.GOROOT)
}

cfg.GOROOT is the GOROOT deduced from the location of the go command (so in this case /tmp/go/bin/go yields /tmp/go), and assuming $GOROOT isn't set runtime.GOROOT() still returns /usr/local/go, so the condition is true, and the -X flag overrides the setting of sys.DefaultGoroot.

But this is probably not right, for at least two reasons.

First, if you do have $GOROOT set to /tmp/go, because you think you need to do that to make anything work properly (as you did before cmd/go sniffed out GOROOT as well as it does today), then runtime.GOROOT() will return /tmp/go, the -X will not be added, and and the resulting binary will have a DefaultGoroot of /usr/local/go, just like before the change.

Second, it doesn't seem to be stable. If you run /tmp/go install -a std cmd, hoping to improve the situation, that actually does install a new /tmp/go/bin/go with DefaultGoroot=/tmp/go due to the -X flag, but the original runtime source code has not changed. Then if you run a build with the newly installed /tmp/go/bin/go, it will think it's in the right place, because inside that binary DefaultGoroot=/tmp/go, even though in the runtime sources it is not. So all the binaries /tmp/go builds at that point will not use the -X flag and will end up with the /usr/local/go default. If you run /tmp/go install -a std cmd again you'll get a toolchain with /usr/local/go. It will flip back and forth.

I might be wrong - I haven't tried this - but assuming I am right these both seem problematic to me.

I understand the motivation but I think the fix is probably something different. Filing this bug so we don't forget, and I'll return to this once other cmd/go work is done.

/cc @crawshaw

@rsc rsc added this to the Go1.10 milestone Oct 5, 2017
@crawshaw
Copy link
Member

crawshaw commented Oct 5, 2017

Those are both good points.

My first instinct is to remove the condition and always pass the -X flag.

@gopherbot
Copy link

Change https://golang.org/cl/86835 mentions this issue: cmd/link: set runtime.GOROOT default during link

@golang golang locked and limited conversation to collaborators Jan 9, 2019
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants