-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
internal/cfg: GOROOT_FINAL isn't included in KnownEnv, hard to tell if that's a bug or intentional #39819
Comments
It might be indirect enough to be considered no effect. Perhaps the scope of these env vars are vars that can modify cmd/go behavior only after the Go tree has been installed, and cmd/go tests are out of scope. I wasn't sure from the documentation, which is in large part what this issue is about. |
Hmm, that's an interesting question. The main purpose of go/src/cmd/go/internal/cfg/cfg.go Lines 336 to 344 in daf70d6
On the other hand, it really does affect the behavior of |
Note that there are a few other environment variables that affect compiler behavior that are also not supported in
However, |
GOROOT_FINAL has been removed following #62047. |
@dmitshur I noticed this issue while searching on github, now that GOROOT_FINAL has been removed, please close this issue. |
It’s true that GOROOT_FINAL is removed in Go 1.23, so it’s no longer relevant. As #39819 (comment) points out, there are more env vars that might need consideration. But perhaps that can be deferred until there’s a concrete need. Closing for now. |
I've noticed the current
cfg.KnownEnv
list does not include theGOROOT_FINAL
environment variable.We know from #39385 that
GOROOT_FINAL
has some (albeit indirect) effect on the behavior of the Go command (fortunately,GOROOT_FINAL_OLD
no longer does).Should
GOROOT_FINAL
be included incfg.KnownEnv
, or is it intentional that it's not listed?The documentation for
KnownEnv
is somewhat brief:It'd be nice to clarify if it's meant to be an exhaustive list or not, and whether some env vars should be intentionally left out or not.
/cc @bcmills @matloob @jayconrod
The text was updated successfully, but these errors were encountered: