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/exec: add Cmd.SetEnv method #12868

Closed
bradfitz opened this issue Oct 7, 2015 · 32 comments
Closed

os/exec: add Cmd.SetEnv method #12868

bradfitz opened this issue Oct 7, 2015 · 32 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 7, 2015

I'd like to add a method to os/exec.Cmd to ease setting environment variables on commands before they're run.

Manipulating Env []string lists is tedious and error-prone, especially considering potential duplicates. (since programs would others choose either the first or last occurrence, unpredictably)

The proposed Cmd.SetEnv(key, value string) method would be documented as removing duplicates.

On Windows, environment variables are case-insensitive, so the method on Windows should de-dup case insensitively.

This method would replace a handful of copies of func mergeEnvLists that exist in various forms around the standard library, tests, and x/* repos.

/cc @ianlancetaylor @adg @rsc @griesemer @robpike

@bradfitz bradfitz added the Proposal label Oct 7, 2015
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 7, 2015

I'm not actually dead set on the exact signature. Maybe taking ...string of "key=value" strings would be better, to avoid quadratic behavior when setting a bunch of values.

We would also need to define whether the slice is mutated in-place or replaced. And the semantics when cmd.Env is nil (probably initialize it to os.Environ())

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 7, 2015

SGTM. In cmd/go/go_test.go I also found it useful to have an Unsetenv.

@adg
Copy link
Contributor

@adg adg commented Oct 8, 2015

I'd be happy with cmd.SetEnv("KEY=val", "KEY=val", ...)

And the semantics when cmd.Env is nil (probably initialize it to os.Environ())

Makes sense to me.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 8, 2015

I started to implement cmd.SetEnv("KEY=val", "KEY=val", ...) but found two gross things about it:

  1. the os package spells it os.Setenv with a lowercase 'e'.
  2. the os package signature is Setenv(key, value string), so people would assume that, and then it would silently compile with a SetEnv(...string) signature. So then what? Runtime panic? Magically work if even number of arguments and odd ones don't have = bytes in them? Gross.

So then I was back to thinking about just Setenv(key, value string) and Unsetenv(key string) (for @ianlancetaylor). But then we're back to them being quadratic if there's a bunch of them.

Maybe we shouldn't care. N is small, etc (few environment variables).

But if we did care, I was toying with ideas like:

// EnvMod represents a modification to environment variables.                                                                               
type EnvMod struct {
        key string
        val string
        set bool
}

func Unsetenv(key string) EnvMod      { return EnvMod{key: key} }
func Setenv(key, value string) EnvMod { return EnvMod{key: key, val: value, set: true} }

func (c *Cmd) ModifyEnv(mods ...EnvMod) { ... }

But call sites are then kinda ugly:

        c.ModifyEnv(exec.Setenv("GOROOT", v), exec.Unsetenv("GOBIN"))

(and calling exec.Setenv directly and ignoring its return value looks like it does something but doesn't)

So then I thought: maybe we just keep the functions simple:

    func(c *Cmd) Setenv(key, value string) {...}
    func(c *Cmd) Unsetenv(key string) {...}

... but for people who care about the quadratic behavior, they can optionally use:

    // ModifyEnv runs fn, which should contain a series of Setenv and/or Unsetenv calls.
    // Use of ModifyEnv is optional but avoids quadratic behavior eliminating duplicates
    // when modifying many environment variables.
    func (c *Cmd) ModifyEnv(fn func()) {
              // build, cache data structure on c
              fn()
              // clear cache data structure on c
    }

There's not much (any?) precedent for such a function, though. The closest is App Engine's RunInTransaction perhaps: https://godoc.org/google.golang.org/appengine/datastore#RunInTransaction

Maybe we can introduce ModifyEnv later if needed, and just accept the poor behavior of (*Cmd).Setenv and (*Cmd).Unsetenv in the meantime?

@extemporalgenome
Copy link
Contributor

@extemporalgenome extemporalgenome commented Oct 8, 2015

Perhaps we could have Setenv just append to the Env slice with custom logic: if the append would allocate, then do a pruning first (later values take precedence). Start (and Run) would also need to do a pruning as well.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 8, 2015

We can't dedup in Start or Run. That would be a change of behavior and it's valid (even if weird) to have multi-valued keys. And if we did dedup: first or last? Different programs pick a different one.

@adg
Copy link
Contributor

@adg adg commented Oct 9, 2015

Why don't we just let people who really care about the quadratic behavior futz with cmd.Env manually?

@adg
Copy link
Contributor

@adg adg commented Oct 9, 2015

Although I also like the variadic approach panicking on cmd.Setenv("KEY", "value").

To unset you could cmd.Setenv("KEY=value", "UNSETKEY=").

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 9, 2015

UNSETKEY hack is gross. Then it's a magic environment variable you can't set.

@adg
Copy link
Contributor

@adg adg commented Oct 9, 2015

No no I meant that UNSETKEY is literally the key name that you want to unset. It's unset because the value component is empty. It could be "FOO=". Sorry for the confusion.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 9, 2015

Oh. I also considered that but there's a difference between empty and unset and we wouldn't want to differ from os.Setenv semantics. That distinction and complaints is why we finally added os.Unsetenv.

@masiulaniec
Copy link

@masiulaniec masiulaniec commented Oct 10, 2015

I'm not actually dead set on the exact signature. Maybe taking ...string of "key=value" strings would be better

Since it doesn't look like it's been considered: don't forget about the approach taken by https://golang.org/pkg/strings/#NewReplacer

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 10, 2015

@masiulaniec, good point. That's worth consideration. It doesn't quite work for the Unsetenv case, though. Unless we were to declare some magical value for the unset value (like "\x00", since "" is a valid value, and a NULL byte can't exist in environment variables). Little inconsistent with os.Setenv though.

I'm increasingly thinking we should just call it (*Cmd).MergeEnv(...) in any case, so we can get the case right on Env and be allowed to set our own semantics instead of conforming to expectations of os.Setenv.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 12, 2015

I'm glad this task is being addressed and made easier, because I too had to write little internal helpers to help manage env vars for exec.Cmds. The way I've done it was:

// environ is a slice of strings representing the environment, in the form "key=value".
type environ []string

// Unset a single environment variable.
func (e *environ) Unset(key string) {
    for i := range *e {
        if strings.HasPrefix((*e)[i], key+"=") {
            (*e)[i] = (*e)[len(*e)-1]
            *e = (*e)[:len(*e)-1]
            break
        }
    }
}

And usage is:

env := environ(os.Environ())
env.Unset("FOO")
cmd.Env = env
@adg
Copy link
Contributor

@adg adg commented Oct 12, 2015

I'm increasingly thinking we should just call it (*Cmd).MergeEnv(...)

SGTM

@rsc rsc added this to the Proposal milestone Oct 24, 2015
@rsc
Copy link
Contributor

@rsc rsc commented Oct 24, 2015

A few points.

  1. I don't see what this has to do with exec.Cmd. You're talking about writing general helper functions for manipulating []string representing environment lists and then hard-coding that they only operate on cmd.Env.

  2. I don't see why methods on exec.Cmd must necessarily store their state in the Env field. The current semantics are that cmd.Env == nil means "use the current environment", while cmd.Env != nil means "use this exact environment". You could reasonably make the methods operate on behind-the-scenes state and define that cmd.Env == nil means "use the current environment as modified by calls to Setenv/Unsetenv", while if cmd.Env != nil then the calls are ignored.

  3. Along that line of thought, the Setenv and Unsetenv methods could just record the operations in an unexported field and then they could be replayed onto the effective environment at (*Cmd).Start time. Then they could apply to both nil and non-nil cmd.Env.

  4. Or maybe this really should be a separate type (like exec.Env) that helps you prepare an environment list. Internally the Env implementation would be something like a slice+map.

    var env exec.Env
    env = exec.Environ()
    env.Setenv("XXX", "YYY")
    env.Unsetenv("ZZZ")
    cmd.Env = env.List()
    
@minux
Copy link
Member

@minux minux commented Oct 24, 2015

@rsc rsc changed the title proposal: add os/exec.Cmd.SetEnv method proposal: os/exec: add Cmd.SetEnv method Oct 24, 2015
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Aug 15, 2016

I would still like to do this. Ideally in Go 1.8.

@robpike
Copy link
Contributor

@robpike robpike commented Aug 15, 2016

Accepted pending final details about the API.

@adg adg modified the milestones: Go1.8, Proposal Aug 15, 2016
@cbednarski
Copy link

@cbednarski cbednarski commented Aug 16, 2016

Slightly OT but as a history lesson, is this a slice to match os.Environ()'s type or is there some other reason? Is order important? Does some system allow dupes? Seems like a map would fit the data better (I'm aware there is a BC issue there; just curious on the original intent).

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 17, 2016

@cbednarski The environment is represented as a slice mainly because in C it is a char**, both in how it is passed to the program (as the third argument to main or in the environ global variable) and in how it is passed to execve system call.

All Unix systems do support duplicate environment variables, though their effect on specific programs depends on how those programs handle the environment array.

@cbednarski
Copy link

@cbednarski cbednarski commented Aug 17, 2016

@ianlancetaylor Great explanation, thanks very much!

@rsc
Copy link
Contributor

@rsc rsc commented Oct 18, 2016

Not sure whether this is still about Cmd.SetEnv or about the more general exec.Env I suggested. Either way, looks like it will wait for Go 1.9.

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 18, 2016
@rsc rsc changed the title proposal: os/exec: add Cmd.SetEnv method os/exec: add Cmd.SetEnv method Oct 18, 2016
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Feb 28, 2017

How about this instead:

  • No new API.
  • Do the merge-env-lists work in os/exec/#Cmd.Start, always, unconditionally. Later dups in the []string list take precedence. On Windows, it's case insensitive.
  • If you want to really really send duplicate environment keys to your child process, use os.StartProcess instead, which is the low-level interface.

/cc @josharian

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Feb 28, 2017

To be clear, that means all code like this is now correct and does what people expect:

cmd := exec.Command("prog")
cmd.Env = append(os.Environ(), "FOO=bar")

... without worrying about whether os.Environ() contains $FOO or not. And without worrying about how prog would handle dups (first or last FOO wins?).

@rsc
Copy link
Contributor

@rsc rsc commented Feb 28, 2017

OK.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 28, 2017

And note that that matches bash's behavior:

$ A=1 A=2 env | head -n 1
A=2
@bradfitz bradfitz self-assigned this Feb 28, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2017

CL https://golang.org/cl/37586 mentions this issue.

@gopherbot gopherbot closed this in e73f489 Feb 28, 2017
gopherbot pushed a commit that referenced this issue Mar 2, 2017
This is now handled by os/exec.

Updates #12868

Change-Id: Ic21a6ff76a9b9517437ff1acf3a9195f9604bb45
Reviewed-on: https://go-review.googlesource.com/37698
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
iversond added a commit to psadmin-io/ps-tuxbeat that referenced this issue May 18, 2017
@alexellis
Copy link

@alexellis alexellis commented Aug 29, 2017

@bradfitz I know this is an old issue but I wanted to ask if you can pull back environmental variables set by an executing process? I've been testing this and while I see them reflected in the executing process - I only get the original set from the Golang code.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 29, 2017

@alexellis This issue is closed, and in any case we don't use the issue tracker for questions. Please see https://golang.org/wiki/Questions. Thanks.

(I'm not sure what you are asking, but to modify the current process environment, use os.Setenv, os.Unsetenv, os.Clearenv. Please take any follow up to a forum, not this issue. Thanks.)

@alexellis
Copy link

@alexellis alexellis commented Aug 29, 2017

No that wasn't the question - it seemed like this is where the design decision was made - so was hoping for a quick answer without having to sign up to anything.

  • I use "exec" to create a new process i.e. Node.js then set some environment.
  • In the child process I can see the new environment set and working
  • From the Golang code which created the process via this call - I cannot see the environment as updated by the child process

Is this even possible?

openfaas/faas#157

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 29, 2017

Please use a forum. You don't have to sign up. Thanks.

@golang golang locked and limited conversation to collaborators Aug 29, 2018
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
You can’t perform that action at this time.