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: pollutes environment variables on Plan 9 #34971

Closed
fhs opened this issue Oct 17, 2019 · 4 comments
Closed

cmd/go: pollutes environment variables on Plan 9 #34971

fhs opened this issue Oct 17, 2019 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9

Comments

@fhs
Copy link
Contributor

fhs commented Oct 17, 2019

What version of Go are you using (go version)?

$ go version
go version devel +master Wed Oct 16 20:56:59 EDT 2019 plan9/386

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE='on'
GOARCH='386'
GOBIN=''
GOCACHE='/usr/fhs/lib/cache/go-build'
GOENV='/usr/fhs/lib/go/env'
GOEXE=''
GOFLAGS=''
GOHOSTARCH='386'
GOHOSTOS='plan9'
GONOPROXY=''
GONOSUMDB=''
GOOS='plan9'
GOPATH='/home/fhs/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/big/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLDIR='/home/big/go/pkg/tool/plan9_386'
GCCGO='gccgo'
GO386='sse2'
AR='ar'
CC='6c'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
CGO_CFLAGS='-g -O2'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-g -O2'
CGO_FFLAGS='-g -O2'
CGO_LDFLAGS='-g -O2'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m32 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build631500855=/tmp/go-build -gno-record-gcc-switches'

What did you do?

cpu% cd $GOROOT/src/cmd/go
cpu% ls /env > /tmp/1
cpu% go test -c
cpu% ls /env > /tmp/2
cpu% diff -c /tmp/1 /tmp/2
/tmp/1:1,8 - /tmp/2:1,26
  '/env/*'
  /env/0
+ /env/AR
+ /env/CC
+ /env/CGO_ENABLED
+ /env/CXX
+ /env/GCCGO
+ /env/GIT_SSH_COMMAND
+ /env/GIT_TERMINAL_PROMPT
  /env/GO111MODULE
+ /env/GO386
+ /env/GOARCH
+ /env/GOCACHE
+ /env/GOENV
+ /env/GOHOSTARCH
+ /env/GOHOSTOS
+ /env/GOOS
  /env/GOPATH
+ /env/GOPROXY
+ /env/GOROOT
  /env/GOROOT_BOOTSTRAP
+ /env/GOSUMDB
+ /env/GOTOOLDIR
  /env/NPROC
  /env/apid
  /env/bootargs
cpu% ./go.test -test.run TestLocalImportsEasySub  # any test will do
PASS
cpu% ls /env > /tmp/3
cpu% diff -c /tmp/2 /tmp/3
/tmp/2:2,7 - /tmp/3:2,8
  /env/0
  /env/AR
  /env/CC
+ /env/CCACHE_DISABLE
  /env/CGO_ENABLED
  /env/CXX
  /env/GCCGO
/tmp/2:15,27 - /tmp/3:16,29
  /env/GOHOSTARCH
  /env/GOHOSTOS
  /env/GOOS
- /env/GOPATH
  /env/GOPROXY
  /env/GOROOT
  /env/GOROOT_BOOTSTRAP
  /env/GOSUMDB
  /env/GOTOOLDIR
+ /env/HOME
  /env/NPROC
+ /env/TMPDIR
  /env/apid
  /env/bootargs
  /env/bootdisk
cpu% go version
go: creating work dir: stat /tmp/cmd-go-test-744136408: '/tmp/cmd-go-test-744136408' file does not exist
cpu% cat /env/TMPDIR; echo
/tmp/cmd-go-test-744136408
cpu% 

What did you expect to see?

Working go command after testing cmd/go.

What did you see instead?

Environment polluted with $TMPDIR, which breaks cmd/go.

This is likely related to issue #25234.

@gopherbot please add labels OS-Plan9, NeedsFix

@gopherbot gopherbot added NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 labels Oct 17, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2019

The change here almost certainly belongs in the os package, not a special case in cmd/go.

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2019

Duplicate of #25234

@gopherbot
Copy link

Change https://golang.org/cl/202088 mentions this issue: runtime, os: don't share environment variables between processes on Plan 9

@gopherbot
Copy link

Change https://golang.org/cl/236520 mentions this issue: runtime, syscall: use local cache for Setenv/Getenv in Plan 9

gopherbot pushed a commit that referenced this issue Jun 19, 2020
In os.Getenv and os.Setenv, instead of directly reading and writing the
Plan 9 environment device (which may be shared with other processes),
use a local copy of environment variables cached at the start of
execution. This gives the same semantics for Getenv and Setenv as on
other operating systems which don't share the environment, making it
more likely that Go programs (for example the build tests) will be
portable to Plan 9.

This doesn't preclude writing non-portable Plan 9 Go programs which make
use of the shared environment semantics (for example to have a command
which exports variable definitions to the parent shell). To do this, use
  ioutil.ReadFile("/env/"+key) and
  ioutil.WriteFile("/env/"+key, value, 0666)
in place of os.Getenv(key) and os.Setenv(key, value) respectively.

Note that CL 5599054 previously added env cacheing, citing efficiency
as the reason. However it made the cache write-through, with Setenv
changing the shared environment as well as the cache (so not consistent
with Posix semantics), and Clearenv breaking the sharing of the
environment between the calling thread and other threads (leading to
unpredictable behaviour). Because of these inconsistencies (#8849),
CL 158970045 removed the cacheing again.

This CL restores cacheing but without write-through. The local cache is
initialised at start of execution, manipulated by the standard functions
in syscall/env_unix.go to ensure the same semantics, and exported only
when exec'ing a new program.

Fixes #34971
Fixes #25234
Fixes #19388
Updates #38772

Change-Id: I2dd15516d27414afaf99ea382f0e00be37a570c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/236520
Run-TryBot: David du Colombier <0intro@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Fazlul Shahriar <fshahriar@gmail.com>
Reviewed-by: David du Colombier <0intro@gmail.com>
@golang golang locked and limited conversation to collaborators Jun 5, 2021
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. OS-Plan9
Projects
None yet
Development

No branches or pull requests

3 participants