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

cmd/go: detect corrupt (overwritten) installation #51760

Open
prattmic opened this issue Mar 17, 2022 · 6 comments
Open

cmd/go: detect corrupt (overwritten) installation #51760

prattmic opened this issue Mar 17, 2022 · 6 comments
Labels
GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@prattmic
Copy link
Member

Every major release, a few users seem to trip over accidentally creating a corrupt installation by extracting the new Go release over an old Go release, without deleting the old release. This leaves around files from the old release that no longer exist in the new release, often leading to build failures.

Some examples of this:

1.18: https://www.reddit.com/r/golang/comments/tfvb6i/go_118_redeclared_in_this_block_error/ StackGuardMultiplier redeclared in this block
1.17: #47773 #48714 PtrSize redeclared in this block
1.16: #43320 #46936 memstats.gc_sys undefined (type mstats has no field or method gc_sys) (plus 4 linked issues in other repos with folks hitting the same problem).

I'm sure there are more, this is just from 5 minutes of searching.

I think we should make the Go command attempt to detect such corrupt installations and point users in the right direction towards fixing it.

One way to achieve this would be to autogenerate a unique file per release (e.g., cmd/go/internal/version/go1.19.1.txt or cmd/go/internal/version/c379c3d58d5.txt (tip build) [1]). The Go command would then check this directory in GOROOT (if it exists, I think source-less installations are technically possible?), and if it finds multiple files [2] then it exits with an error, informing the user that the installation is corrupt and to delete and reinstall.

[1] I propose a different file even for minor and tip releases because, though less common, even 1.19 -> 1.19.1 may remove files and could result in a corrupt installation if overwritten.

[2] Initially I was thinking the Go command should be a bit more strict and look for a file matching the version of the Go command, but I suspect this could trip up folks working on Go itself. If you modify code in GOROOT and rebuild the Go command, but don't regenerate the version file then we don't want that to report an error.

cc @bcmills @matloob
@prattmic prattmic added the GoCommand cmd/go label Mar 17, 2022
@prattmic prattmic added this to the Backlog milestone Mar 17, 2022
@prattmic
Copy link
Member Author

prattmic commented Mar 17, 2022 •
edited

I suppose I didn't quite spell out what users are doing wrong above. Specifically, to get these errors, users:

  1. Have an existing Go installation, at e.g., /usr/local/go.
  2. They download a new Go release (tar, likely) and extract it over the old release. e.g., tar -C /usr/local -xzf go1.18.linux-amd64.tar.gz.

Because they did not remove /usr/local/go first, any files that don't exist in the tar (because they were removed upstream) will not overwrite that file in /usr/local/go, effectively leaving packages that are 99% at the new version, but with one extra file from the old version.

My suggestion above detects this because each version has a unique filename. Installing one release over another will leave you with two version files, e.g., go1.19.txt and go1.20.txt.

@ALTree
Copy link
Member

ALTree commented Mar 17, 2022

Not a fan of having the go command check whether the installation is corrupted every time it's invoked.

We could stress this in the installation instructions. I think it's mostly a Linux issue, and for Linux we say (https://go.dev/doc/install):

1. Extract the archive you downloaded into /usr/local, creating a Go tree in /usr/local/go.

Important: This step will remove a previous installation at /usr/local/go, if any, prior to extracting. Please back up any data before proceeding.

For example, run the following as root or through sudo:

$ rm -rf /usr/local/go && tar -C /usr/local -xzf go1.18.linux-amd64.tar.gz

Notice how the point 1. just says "Extract the archive you downloaded into /usr/local". Many people probably skim over the paragraph, stop reading after this sentence, and immediately try to untar. Only at the bottom we mention rm -rf in the command, but it's even called "an example".

But we we change the 1. sentence to:

  1. Remove the current go tree under /usr/local/go, then extract the new archive you downloaded into /usr/local/go.

it'd probably be clearer.

@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 17, 2022
@prattmic
Copy link
Member Author

Not a fan of having the go command check whether the installation is corrupted every time it's invoked.

I'm generally not a fan of running something like this every time, but the go command is generally accessing tons of files anyways, so it seems like this would not matter much. That said, we may want to limit it to the most relevant commands (go build), or we could even limit this check to run only if standard library packages fail to build, though IMO that seems a bit overly targetted.

But we we change the 1. sentence to:

  1. Remove the current go tree under /usr/local/go, then extract the new archive you downloaded into /usr/local/go.

it'd probably be clearer.

I like this, I think this wording is more clear, and it feels like a change worth making regardless of whether the go command does anything. It is hard to tell exactly how users are ending up making an incorrect installation, so it hard to tell if this would be enough on its own, but perhaps it would?

@bcmills
Copy link
Contributor

bcmills commented Mar 17, 2022

FWIW, you could achieve something like this within a package (perhaps runtime or internal/buildcfg?) with carefully-crafted declarations:

runtime/zgo1.19.go

const redeclaredIfInstallDirectoryIsDirty = "go1.19"

I suspect that cmd/dist could even be modified to produce such a file as part of the release process.

@gopherbot
Copy link

Change https://go.dev/cl/393755 mentions this issue: _content/doc: emphasize need to remove existing installations

@ALTree
Copy link
Member

ALTree commented Mar 18, 2022

CL 393755 proposal:
install

gopherbot pushed a commit to golang/website that referenced this issue Mar 24, 2022
@ALTree
_content/doc: emphasize need to remove existing installations
37dcb7b 
This change updates the Linux installation instructions to emphasize
the need to delete an existing Go tree in /usr/local/go before
untarring the downloaded archive.

This is a common mistake people seem to make on Linux, and it's known
to produce broken Go installations.

Updates golang/go#51760

Change-Id: Ia7375957a6034defc708add74cf3c2cf4dbee5d4
Reviewed-on: https://go-review.googlesource.com/c/website/+/393755
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jamal Carvalho <jamalcarvalho@google.com>
Trust: Ian Lance Taylor <iant@google.com>
passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
@passionSeven
_content/doc: emphasize need to remove existing installations
c1d6328 
This change updates the Linux installation instructions to emphasize
the need to delete an existing Go tree in /usr/local/go before
untarring the downloaded archive.

This is a common mistake people seem to make on Linux, and it's known
to produce broken Go installations.

Updates golang/go#51760

Change-Id: Ia7375957a6034defc708add74cf3c2cf4dbee5d4
Reviewed-on: https://go-review.googlesource.com/c/website/+/393755
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jamal Carvalho <jamalcarvalho@google.com>
Trust: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@prattmic @ALTree @bcmills @gopherbot @heschi