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: go generate sets more variables than its documentation specifies #41943

Open
dmitshur opened this issue Oct 12, 2020 · 4 comments
Open

cmd/go: go generate sets more variables than its documentation specifies #41943

dmitshur opened this issue Oct 12, 2020 · 4 comments

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 12, 2020

go generate documentation currently says:

Go generate sets several variables when it runs the generator:

$GOARCH
	The execution architecture (arm, amd64, etc.)
$GOOS
	The execution operating system (linux, windows, etc.)
$GOFILE
	The base name of the file.
$GOLINE
	The line number of the directive in the source file.
$GOPACKAGE
	The name of the package of the file containing the directive.
$DOLLAR
	A dollar sign.

Other than variable substitution and quoted-string evaluation, no special processing such as "globbing" is performed on the command line.

As a last step before running the command, any invocations of any environment variables with alphanumeric names, such as $GOFILE or $HOME, are expanded throughout the command line. The syntax for variable expansion is $NAME on all operating systems. Due to the order of evaluation, variables are expanded even inside quoted strings. If the variable NAME is not set, $NAME expands to the empty string.

The last sentence is relevant. It leads the user to believe that if a NAME is not one of the aforementioned variables and is not set in the environment, $NAME would expand to the empty string.

This is not the case for some Go-specific variables (beyond those mentioned in the list above) because expandVar uses os.Getenv(word) to look up environment variables that aren't in g.env. To achieve the behavior as documented above, it should instead be looking up environment variables from cfg.OrigEnv, since cmd/go's main modifies the environment at startup (see here).

To reproduce, consider this program:

package main

import (
	"fmt"
	"os"
)

//go:generate echo ${DOLLAR}GOCACHE expands to '$GOCACHE'

func main() {
	v, ok := os.LookupEnv("GOCACHE")
	if !ok {
		fmt.Println("env var GOCACHE is unset")
		return
	}
	fmt.Printf("$GOCACHE = %q\n", v)
}

What did you do?

$ unset GOCACHE
$ go run .
$ go generate

What did you expect to see?

$ go run .
env var GOCACHE is unset
$ go generate
$GOCACHE expands to ''

What did you see instead?

$ go run .
env var GOCACHE is unset
$ go generate
$GOCACHE expands to '/Users/gopher/Library/Caches/go-build'

/cc @bcmills @jayconrod @matloob

@dmitshur
Copy link
Member Author

@dmitshur dmitshur commented Oct 12, 2020

NeedsDecision is to decide whether to fix the implementation or the documentation.

@dmitshur dmitshur removed the Documentation label Oct 12, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented Oct 12, 2020

I think the current documentation is generally correct: it says it sets some variables, it doesn't say anything about starting from an empty environment. So, one would expect that environment variables from the parent process would be set, like HOME.

Perhaps we can clarify the documentation to say which of the go env variables are always present too - presumably all of them.

@dmitshur
Copy link
Member Author

@dmitshur dmitshur commented Oct 12, 2020

it doesn't say anything about starting from an empty environment

I agree with this. But this issue isn't expecting an empty environment plus the 6 documented variables. It's expecting the caller's environment plus the 6 documented variables.

The current implementation produces the caller's environment plus a subset of go env (undocumented) plus 6 documented variables.

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 13, 2020

Right, apologies for misreading. Ignore the first part of my previous comment. I still think that this just needs clearer documentation, and I think that we should expose all of go env for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.