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 · 11 comments

Comments

Projects
None yet
6 participants
@bradfitz
Member

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.

@robpike

This comment has been minimized.

Contributor

robpike commented May 4, 2018

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

@bradfitz

This comment has been minimized.

Member

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

This comment has been minimized.

Member

alexbrainman commented May 4, 2018

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

@andig

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

alexbrainman commented Oct 7, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

Member

alexbrainman commented Oct 7, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Member

alexbrainman commented Oct 7, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment