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: make a Go 2 type for environment variables? #25252

Open
bradfitz opened this issue May 4, 2018 · 12 comments
Open

os: make a Go 2 type for environment variables? #25252

bradfitz opened this issue May 4, 2018 · 12 comments
Labels
Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. v2 A language change or incompatible library change
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz commented May 4, 2018

Should we change the type of environment variables in Go 2?

Currently it's []string which is slightly odd. We at least solved the duplicated problems in Go 1.9 with https://golang.org/doc/go1.9#os/exec (#12868).

In #25210 (comment), @alexbrainman suggested map[string]string but that doesn't work well for case-insensitive Windows, and doesn't permit intentional(ly odd) duplicates or specific ordering on Unix, if such control is ever desired. (whether we wish to permit that is an additional question)

It might be nice to have some new opaque type (not exposing the underlying representation) and have various constructors and accessors.

@bradfitz bradfitz added the v2 A language change or incompatible library change label May 4, 2018
@bradfitz bradfitz added this to the Go2 milestone May 4, 2018
@robpike
Copy link
Contributor

robpike commented May 4, 2018

What's the problem you want to solve? []string seems perfectly fine to me.

@bradfitz
Copy link
Contributor Author

bradfitz commented May 4, 2018

They're tedious to manipulate.

For example, the common case of filtering an environment variable from reaching a child is tedious, especially if done portably for Windows.

Even adding an environment to a child's process was very difficult prior to Go 1.9.

Third, our API makes it too easy to create child processes with environments that are non-functional on Windows (#25210).

@alexbrainman
Copy link
Member

What's the problem you want to solve? []string seems perfectly fine to me.

Lets say I have []string returned by os.Environ(). And I want to add a directory to the PATH environment variable before I pass it into a child process? For extra complexity, assume your program has to work on Windows too, so your PATH variable can be spelled out as Path or patH or anything in between.

Alex

@ianlancetaylor ianlancetaylor added the Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. label Sep 12, 2018
@andig
Copy link
Contributor

andig commented Oct 4, 2018

It might be nice to have some new opaque type (not exposing the underlying representation) and have various constructors and accessors.

Sounds like overkill to me.

In #25210 (comment), @alexbrainman suggested map[string]string but that doesn't work well for case-insensitive Windows, and doesn't permit intentional(ly odd) duplicates or specific ordering on Unix, if such control is ever desired. (whether we wish to permit that is an additional question)

Would it be possible to normalize these? I'm not sure how wide-spread case sensitive (and overlapping!) environment variables really are?

@robpike
Copy link
Contributor

robpike commented Oct 6, 2018

The problems you mention seem easy to fix with a new function or two. It doesn't strike me that a type is necessary.

@mewmew
Copy link
Contributor

mewmew commented Oct 6, 2018

It might be nice to have some new opaque type (not exposing the underlying representation) and have various constructors and accessors.

Do you simply mean to update the os package e.g.

package os

func Environ() Env {}

type Env struct {
   // implementation of Env can be different on different architectures.
   vars []envVar
}

type envVar struct {
   key string
   data string
}

// Get is case sensitive.
func (e Env) Get(key string) (val string, ok bool) {}
// Find is case insensive.
func (e Env) Find(key string) (val string, ok bool) {}
func (e Env) Set(key, val string) {}

@bradfitz Or do you mean to add something that is not already in the language?

@alexbrainman
Copy link
Member

@robpike

The problems you mention seem easy to fix with a new function or two. It doesn't strike me that a type is necessary.

SGTM. Please, share your ideas.


@mewmew

func Environ() Env {}

Environ function returns []string at this moment

https://golang.org/pkg/os/#Environ

so, your suggestion cannot be implemented today, until we allow os package to change API.

Other than that, your suggestion is, probably, reasonable. I am not sure why you say "Get is case sensitive" - all Get, Find and Set has to be case insensitive on Windows.

Alex

@mewmew
Copy link
Contributor

mewmew commented Oct 7, 2018

Other than that, your suggestion is, probably, reasonable. I am not sure why you say "Get is case sensitive" - all Get, Find and Set has to be case insensitive on Windows.

@alexbrainman I wanted to give an example with two functions for locating environment variables (Get and Find), so that they can be located other case-sensitively or case-insensitively. The name Get would then perhaps be GetExact or something along those lines.

My primary concern was rather that this would lead to an unnecessary change of the Go language rather than a Go standard library package. I fully understand that the change of os.Environ would have to happen for Go 2 (as per Go 1 contract).

@alexbrainman
Copy link
Member

I wanted to give an example with two functions for locating environment variables (Get and Find), so that they can be located other case-sensitively or case-insensitively.

I do not see why we need 2 different functions (case-sensitive and non-case-sensitive) on Windows?

My primary concern was rather that this would lead to an unnecessary change of the Go language rather than a Go standard library package.

No argument here. I do not think we need changes to the language to fix this problem.

Alex

@mewmew
Copy link
Contributor

mewmew commented Oct 7, 2018

I do not see why we need 2 different functions (case-sensitive and non-case-sensitive) on Windows?

There probably only needs to be one function for Windows. But besides the syscall package, I thought that the exposed API for each Go package was same across all OSes? But I might be wrong. If they are the same however, then for Unix there may still need to be methods to handle both case-sensitive and case-insensitive.

However, I think that those more intimately involved in the OS and environment code knows this far better than I. Both you and Bradfitz. So to not increase further noise into the discussion, I'll follow the discussion, but probably can't add too much of value regarding the exact API needed. My main wish was to establish that this would be a library and not a language change.

Cheers,
Robin

@alexbrainman
Copy link
Member

I thought that the exposed API for each Go package was same across all OSes?

Yes, if we add any new environment handling function to os package, if should do the same thing regardless of OS it runs on.

for Unix there may still need to be methods to handle both case-sensitive and case-insensitive.

I think all Unixes use case-sensitive comparisons when searching for environment variable by name. But I could be wrong.

Alex

@kfsone
Copy link

kfsone commented Jun 12, 2019

Would it be possible to normalize these? I'm not sure how wide-spread case sensitive (and overlapping!) environment variables really are?

Pervasive. Almost all POSIX shells use environment variables as variables, PowerShell being one notable exception. But, basically, shell, bash, dash, ksh, csh, etc, all use environment variables as variables.

function add_path() {
    local new_path=$1; shift    # 'new_path' is an environment variable
    if [ -n "${PATH}" ]; then   # 'PATH' is the well-known environment variable
      PATH+=":"
    fi
    PATH+="${new_path}"
}

or

bash$ foo=bar python -c 'import os; print os.environ["foo"]'
bar

People also routinely use different-cased versions of names to denote function or script locality - as we do in go.

FWIW: Environment access in Powershell Core on Linux and Mac is case sensitive.

06/12/2019 14:22:54> $psversiontable

Name                           Value
----                           -----
PSVersion                      6.2.1
PSEdition                      Core
GitCommitId                    6.2.1
OS                             Linux 4.15.0-36-generic #39~16.04.1-Ubuntu SMP Tue Sep 25 08:59:23 UTC 2018
Platform                       Unix
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
06/12/2019 14:22:33> $env:PATH 
/opt/microsoft/powershell/6:/home/...
06/12/2019 14:22:51> $env:path

6/12/2019 14:10:12> [Environment]::GetEnvironmentVariable('PATH')
/opt/microsoft/powershell/6:/home/...
06/12/2019 14:22:30> [Environment]::GetEnvironmentVariable('Path')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

7 participants