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

os: Expand() and ExpandEnv() on Windows don't support Windows style environment variables. :/ #24848

Closed
jeffreydwalter opened this issue Apr 13, 2018 · 7 comments

Comments

@jeffreydwalter
Copy link

@jeffreydwalter jeffreydwalter commented Apr 13, 2018

Please answer these questions before submitting your issue. Thanks!

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

$ go version
go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jeff_walter/Code/kace/agent"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.9.2/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.9.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8w/05w3j5xj4q34m1z6w9ynmcf80000gp/T/go-build022427005=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

Go's os.Expand() and os.ExpandEnv() functions expect variables that start with $ on Windows and not the %WINDIR% style variables, which are native to that platform...

// Expand replaces ${var} or $var in the string based on the mapping function.

@jeffreydwalter

This comment has been minimized.

Copy link
Author

@jeffreydwalter jeffreydwalter commented Apr 13, 2018

For example, running the following on Windows:

package main

import (
    "fmt"
    "os"
)

func main() {
    fmt.Println(os.ExpandEnv("$WINDIR is %WINDIR% on Windows."))
}

produces:

C:\WINDOWS is %WINDIR% on Windows.

Which is completely stupid and unexpected for a native Windows person, imho.

@bontibon

This comment has been minimized.

Copy link
Contributor

@bontibon bontibon commented Apr 13, 2018

Previously discussed: #5177, #8469

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Apr 13, 2018

Yes, please see the previous discussions, in particular how the function is working exactly as documented and how there are alternatives like golang.org/x/sys/windows/registry.ExpandString.

I'm going to close this for now. If you have a specific proposal on a change to the os package that is backwards incompatible and reasonable, please feel free to file it. Otherwise, there isn't a bug here.

@mvdan mvdan closed this Apr 13, 2018
@jeffreydwalter

This comment has been minimized.

Copy link
Author

@jeffreydwalter jeffreydwalter commented Apr 13, 2018

Thanks, I did a search for those functions in the issues history and didn't find them.

@jeffreydwalter

This comment has been minimized.

Copy link
Author

@jeffreydwalter jeffreydwalter commented Apr 17, 2018

@bontibon, @mvdan the registry package does NOT solve my problem. I need to expand Windows style environment variable strings using a custom function, with https://golang.org/pkg/os/#Expand because I am launching processes as a specific user in Windows without a shell and need to expand the variables based on that user's environment.

So, my code basically grabs a token from the currently logged in user in Windows, uses that token to get the environment of that user, and then expands from there.

While I appreciate that Windows is clearly an after thought in the os package, and that the documentation covers the expected behavior, the current behavior is pretty idiotic on Windows.

Given the way these functions are written, I don't see a simple backwards compatible change to be made, but how about introducing a new function, ExpandWinEnv() or something like that, that strictly expands %SOMEVAR% style environment variables?

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented May 1, 2018

It seems to me like there's an easier workaround here - simply replacing %FOO% with ${FOO}, and then using os.Expand.

I know this isn't ideal, but this seems like a fairly niche use case and thus not very likely to be included into the os package. Though, you can of course submit a proposal suggesting adding an extra API to it, if you'd like.

@jeffreydwalter

This comment has been minimized.

Copy link
Author

@jeffreydwalter jeffreydwalter commented May 1, 2018

@golang golang locked and limited conversation to collaborators May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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