Skip to content

runtime: GOROOT() docs are confusing #22302

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.

Sign up for GitHub

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
Closed
natefinch opened this issue Oct 17, 2017 · 5 comments
Closed

runtime: GOROOT() docs are confusing #22302

natefinch opened this issue Oct 17, 2017 · 5 comments
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@natefinch
Copy link
Contributor

natefinch commented Oct 17, 2017 •
edited
Loading

The docs say:

GOROOT returns the root of the Go tree. It uses the GOROOT environment variable, if set, or else the root used during the Go build.

Thus I expect this would print "Hi!":

package main

import (
    "fmt"
    "log"
    "os"
    "runtime"
)

func main() {
    err := os.Setenv("GOROOT", "Hi!")
    if err != nil {
        log.Fatal(err)
    }
    fmt.Println(runtime.GOROOT())
}

However, this prints my real goroot, not "Hi!".

Either GOROOT() should actually check the environment variable when called, or the docs should be clarified to specify that it returns the GOROOT environment variable set at init() time.
@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Oct 17, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Oct 17, 2017
@dmitshur
Copy link
Contributor

dmitshur commented Oct 17, 2017 •
edited
Loading

@gopherbot added the Documentation label automatically due to "docs" in issue title, but I think it needs to be decided/confirmed whether it's a bug in implementation or documentation.

If the current behavior is deemed correct, here's a take at fixing the documentation:

GOROOT returns the root of the Go tree. It uses the GOROOT environment variable, if set at process start, or else the root used during the Go build.

Also, https://godoc.org/runtime#NumCPU has some precedent:

The set of available CPUs is checked by querying the operating system at process startup. Changes to operating system CPU allocation after process startup are not reflected.

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed Documentation Issues describing a change to documentation. labels Oct 18, 2017
@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Oct 18, 2017
@mvdan
Copy link
Member

mvdan commented Oct 18, 2017

Oh well, I tried.

@dmitshur dmitshur removed the Documentation Issues describing a change to documentation. label Oct 18, 2017
@rsc
Copy link
Contributor

rsc commented Oct 30, 2017

Decision: document current behavior. runtime.GOROOT is already too subtle without changing this detail.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 30, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 30, 2017
@dmitshur
Copy link
Contributor

In that case, it'll be a good idea to add a test for this behavior (if one doesn't already exist).

@dmitshur dmitshur added the Documentation Issues describing a change to documentation. label Oct 31, 2017
@dmitshur dmitshur mentioned this issue Nov 2, 2017
Open
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/75751 mentions this issue: runtime: clarify GOROOT return value in documentation

@dmitshur dmitshur mentioned this issue Sep 17, 2018
Open
@golang golang locked and limited conversation to collaborators Nov 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@rsc @dmitshur @natefinch @ianlancetaylor @mvdan @gopherbot