Navigation Menu

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: '$/' is an invalid environment variable name on plan 9 #53671

Closed
oridb opened this issue Jul 3, 2022 · 7 comments
Closed

cmd/go: '$/' is an invalid environment variable name on plan 9 #53671

oridb opened this issue Jul 3, 2022 · 7 comments
Assignees
@bcmills
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Plan9
Milestone
Go1.19

Comments

@oridb
Copy link
Contributor

oridb commented Jul 3, 2022 •
edited

When running the tests on plan 9, there are currently a number of failures when attempting to set
the environment, with the message:

can't open: '/env//' create without DMDIR

This is because src/cmd/go/script_test.go atempts to set '$/' to indicate the path separator.

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

commit e822b1e

Does this issue reproduce with the latest release?

The issue was added with commit 742dcba

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

Plan 9 (9front, amd64)

go env Output
% go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/usr/orib/lib/cache/go-build'
GOENV='/usr/orib/lib/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='plan9'
GOINSECURE=''
GOMODCACHE=''
GONOPROXY=''
GONOSUMDB=''
GOOS='plan9'
GOPATH=''
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/orib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLDIR='/usr/orib/go/pkg/tool/plan9_amd64'
GOVCS=''
GOVERSION='VERS'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='6c'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-g -O2'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-g -O2'
CGO_FFLAGS='-g -O2'
CGO_LDFLAGS='-g -O2'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build388427748=/tmp/go-build -gno-record-gcc-switches'

What did you do?

./run.rc

What did you expect to see?

More tests passing than I did

What did you see instead?

script_test.go failing with

can't open: '/env//' create without DMDIR
@oridb
Copy link
Contributor Author

oridb commented Jul 3, 2022

Removing setting the environment variable fixes some of the script tests, but others fail with a missing path separator in paths. Currently, I'm having trouble finding where $/ is getting used to create the paths.

@ianlancetaylor
Copy link
Contributor

CC @bcmills

I see $/ used, as ${/}, in a number of tests in cmd/go/testdata/script.

Perhaps we should just omit / and : when setting the background process environment in testScript.startBackground.

@ianlancetaylor ianlancetaylor added OS-Plan9 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 3, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jul 3, 2022
@ianlancetaylor ianlancetaylor changed the title go/cmd: '$/' is an invalid environment variable name on plan 9 cmd/go: '$/' is an invalid environment variable name on plan 9 Jul 3, 2022
@ianlancetaylor ianlancetaylor added the GoCommand cmd/go label Jul 3, 2022
@oridb
Copy link
Contributor Author

oridb commented Jul 3, 2022

Got it, I swapped the generated/golden output when I was reading it, and was looking for a place $/ was used in the go commands to generate the paths.

Need to run some tests, but patch on the way.

@gopherbot
Copy link

Change https://go.dev/cl/415681 mentions this issue: cmd/go: replace ${slash} with ${/} for plan9 compatibility

@mvdan
Copy link
Member

mvdan commented Jul 4, 2022

For some context, the testscript fork of script_test.go reached a similar conclusion: rogpeppe/go-internal#115

With the caveat that we want to keep ${/} working where possible, for the sake of backwards compatibility.

@bcmills
Copy link
Contributor

bcmills commented Jul 8, 2022

@oridb, I'm trying to better understand the nature of the problem. cmd/go uses os/exec under the hood, so if these environment variables are not valid on Plan 9, does that imply that some behavior in exec.Cmd also needs to change, or at least have its documentation clarified?

For example, should (*exec.Cmd).Start fail explicitly on Plan 9 if the Env slice contains an entry beginning with /? (Or should it ignore such entries entirely?)

@gopherbot
Copy link

Change https://go.dev/cl/416554 mentions this issue: cmd/go: avoid setting variables for '/' and ':' in TestScript subprocess environments

@bcmills bcmills modified the milestones: Backlog, Go1.19 Jul 8, 2022
@bcmills bcmills self-assigned this Jul 8, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@jproberts
cmd/go: avoid setting variables for '/' and ':' in TestScript subproc…
909025a 
…ess environments

Also simplify platform-dependent handling of the PATH variable,
to make it more like the existing platform-dependent handling for
HOME and TMPDIR.

Fixes golang#53671.

Change-Id: Ica2665d3f61988c66fb6982b9feb61ca48eced79
Reviewed-on: https://go-review.googlesource.com/c/go/+/416554
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jul 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bcmills bcmills

Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Plan9
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
5 participants
@ianlancetaylor @mvdan @bcmills @oridb @gopherbot