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/dist: overwrites $GOROOT_BOOTSTRAP/pkg/xxx/math/big.a #22187

Closed
rsc opened this issue Oct 9, 2017 · 7 comments
Closed

cmd/dist: overwrites $GOROOT_BOOTSTRAP/pkg/xxx/math/big.a #22187

rsc opened this issue Oct 9, 2017 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Oct 9, 2017

cmd/dist does

$GOROOT_BOOTSTRAP/bin/go install -gcflags -l -tags math_big_pure_go bootstrap/cmd/...

The intended effect of the tags is to set them for building bootstrap/math/big. But they may also be present in $GOROOT_BOOTSTRAP/src/math/big, which will change the set of files used for the standard (that is, standard in $GOROOT_BOOTSTRAP) math/big. The change in the file set will - in Go 1.5 and later - make the standard math/big look out of date, which will cause it to be recompiled and reinstalled.

There are two problems here: (1) the user may not have write access to $GOROOT_BOOTSTRAP, and (2) installing the new math/big.a will make math/big.a look out of date to a regular $GOROOT_BOOTSTRAP/bin/go command not using those tags. One example of situation (1) is when using the default Ubuntu install of Go as GOROOT_BOOTSTRAP.

The fix here is probably to also specify -pkgdir as well pointing at a temporary location (wherever we wrote the bootstrap src would be fine). That will have the unfortunate effect of recompiling the entire $GOROOT_BOOTSTRAP standard library, adding maybe 10 seconds to the build, but it will ensure that nothing is ever written to $GOROOT_BOOTSTRAP/pkg during cmd/dist.

Prior to Go 1.8, the command was just "install -gcflags -l bootstrap/cmd/...", which would not recompile math/big, because the staleness computation doesn't know what compile flags were used. CL 31142 (/cc @mundaym @randall77), which was released in Go 1.8, added the -tags argument and caused these spurious rebuilds.

@ianlancetaylor ianlancetaylor added this to the Go1.9.2 milestone Oct 9, 2017
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 9, 2017
@rsc
Copy link
Contributor Author

rsc commented Oct 9, 2017

Unfortunately -pkgdir is only present in Go 1.5 and later, so the bootstrap toolchain will need to be queried. If "go tool" includes the string "compile", then it's Go 1.5 and later. (That check would work even when the more obvious "go version" reports an arbitrary Git hash.)

@rsc
Copy link
Contributor Author

rsc commented Oct 9, 2017

The go tool check might fail with gccgo; maybe better would be "go build -pkgdir" or "go help build" and look at the output to see if the flag is recognized.

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2017

This must be a recent regression? I've used a read-only $GOROOT_BOOTSTRAP on one of my machines for a long-time now, and I've only run into this problem this morning.

@rsc
Copy link
Contributor Author

rsc commented Oct 9, 2017

Are you sure it was read-only before this morning? One unanswered question in my mind is that what the path is from bootstrap/cmd/... to math/big. We know cmd/dist forks the latest math/big into bootstrap/math/big and updates all the bootstrap/cmd/... to refer to bootstrap/math/big instead of the real math/big. So the bootstrap commands must be importing some other package in the standard library - one that cmd/dist doesn't fork into bootstrap/... - that in turn imports math/big, or else why would this command be trying to build math/big at all?

If a change in Go 1.9.1 introduced that missing link to the old math/big, that would explain things, but I don't see such a change. The only changes are in net/smtp and cmd/go, neither of which should be relevant and neither of which mentions math/big.

@mdempsky
Copy link
Member

mdempsky commented Oct 9, 2017

I'm not confident that it was previously RO. I'm reasonably confident though that I would not have taken explicit action to change it from RO to RW though.

@rsc
Copy link
Contributor Author

rsc commented Oct 10, 2017

It looks like this might be due to local changes we have (inside Google) in our local Go 1.9 that introduces an unexpected math/big dependency where the proper Go 1.9 does not have them. However, the same problem will happen once the go command is more careful about gcflags changes (in Go 1.10, most likely) so this problem will recur on all systems (not just Google's) once Go 1.10 is deployed. Assuming that people are not building old copies of Go using newer GOROOT_BOOTSTRAPs, we only need to fix this by the time Go 1.10 is released. Changing the milestone.

@rsc rsc modified the milestones: Go1.9.2, Go1.10 Oct 10, 2017
@rsc
Copy link
Contributor Author

rsc commented Nov 29, 2017

The dist command does install cmd/.... If Go 1.10 is the bootstrap toolchain, then the recent change to install to install only the named targets will avoid reinstalling math/big, so this is only a problem with Go 1.9.x using our local changes inside Google. Not worth fixing.

@rsc rsc closed this as completed Nov 29, 2017
@golang golang locked and limited conversation to collaborators Nov 29, 2018
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.
Projects
None yet
Development

No branches or pull requests

4 participants