code review 82040044: syscall: add Unsetenv(string)

syscall: add Unsetenv(string) also fix the TODO (pass through to C) in Clearenv. Update issue 6423

[minux1, 2014-03-29 08:29:23]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[brainman, 2014-03-29 08:41:12]

LGTM Alex

[dave_cheney.net, 2014-03-29 08:48:27]

not lgtm, issue 6423 doesn't feel like it warrants adding a new syscall for 1.3. I would prefer to see this deferred til 1.4 and some discussion of the merits of the issue. > On 29 Mar 2014, at 19:29, minux.ma@gmail.com wrote: > > 6423 

[iant, 2014-03-29 16:43:25]

Is this appropriate during a feature freeze? I don't understand the point of the issue. Removing an environment variable from the current environment doesn't seem very useful.

[minux1, 2014-03-29 17:27:48]

On Mar 29, 2014 12:43 PM, <iant@golang.org> wrote: > Is this appropriate during a feature freeze? that issue is labeled Go1.3 (not Go1.3Maybe) for a long time, so I think it's appropriate to fix it even after freeze. > I don't understand the point of the issue. Removing an environment > variable from the current environment doesn't seem very useful. the problem is setting an env var to empty isn't equivalent to unsetting it and unset a variable helps when os/exec a program (otherwise each time you exec a program, you will have to filter the cmd.Env yourself) 

[Peter Waller, 2014-04-09 07:52:24]

On 2014/03/29 16:43:25, iant wrote: > Is this appropriate during a feature freeze? > > I don't understand the point of the issue. Removing an environment variable > from the current environment doesn't seem very useful. I have encountered a direct need for this building a continuous integration system with go. Our server invokes a child process, but needs to clear auth credentials. Setting it to empty string is not sufficient since the behaviour of the child process may be different depending on the presence of the very same auth credentials in the environment. Without minux's patch, any solution one might implement is racy, since it can't perform `envLock.lock()`. This is bad. So, LGTM.

[iant, 2014-04-09 13:47:13]

On 2014/04/09 07:52:24, Peter Waller wrote: > > I have encountered a direct need for this building a continuous integration > system with go. > > Our server invokes a child process, but needs to clear auth credentials. > > Setting it to empty string is not sufficient since the behaviour of the > child process may be different depending on the presence of the very same > auth credentials in the environment. > > Without minux's patch, any solution one might implement is racy, since > it can't perform `envLock.lock()`. This is bad. > > So, LGTM. I understand, but when you invoke the child process, you can set the environment that should be provided to it. At that time you can remove the environment variable. That is a non-racy solution. Ian

[bradfitz, 2014-04-11 03:20:43]

Well, the issue is flagged Go 1.3. This should either be finished (with passing to C, and tests), or the Go 1.3 label should be removed from the issue. This seems simple enough to be worth finishing if somebody has time. Minux?

[minux1, 2014-04-11 06:22:36]

On Apr 10, 2014 11:20 PM, <bradfitz@golang.org> wrote: > Well, the issue is flagged Go 1.3. > > This should either be finished (with passing to C, and tests), or the Go > 1.3 label should be removed from the issue. > > This seems simple enough to be worth finishing if somebody has time. > Minux? I'm very happy to finish it (with proper interaction with cgo) if we've reached consensus on the issue priority. 

[iant, 2014-04-11 14:26:28]

On Thu, Apr 10, 2014 at 11:22 PM, minux <minux.ma@gmail.com> wrote: > > On Apr 10, 2014 11:20 PM, <bradfitz@golang.org> wrote: >> Well, the issue is flagged Go 1.3. >> >> This should either be finished (with passing to C, and tests), or the Go >> 1.3 label should be removed from the issue. >> >> This seems simple enough to be worth finishing if somebody has time. >> Minux? > I'm very happy to finish it (with proper interaction with cgo) if we've > reached consensus on the issue priority. My vote is to change the issue to go1.4 and deal with it then. It seems to me to be a very marginal feature, and I see now that this CL is quite incomplete. It will need changes to the runtime and runtime/cgo packages as well. Ian 

[minux1, 2014-04-11 15:57:51]

On Apr 11, 2014 10:26 AM, "Ian Lance Taylor" <iant@golang.org> wrote: > > On Thu, Apr 10, 2014 at 11:22 PM, minux <minux.ma@gmail.com> wrote: > > > > On Apr 10, 2014 11:20 PM, <bradfitz@golang.org> wrote: > >> Well, the issue is flagged Go 1.3. > >> > >> This should either be finished (with passing to C, and tests), or the Go > >> 1.3 label should be removed from the issue. > >> > >> This seems simple enough to be worth finishing if somebody has time. > >> Minux? > > I'm very happy to finish it (with proper interaction with cgo) if we've > > reached consensus on the issue priority. > > My vote is to change the issue to go1.4 and deal with it then. It > seems to me to be a very marginal feature, and I see now that this CL > is quite incomplete. It will need changes to the runtime and > runtime/cgo packages as well. this CL is incomplete because I want to get feedback on the new api first. I don't think it requires big changes in runtime and runtime/cgo. also, the Clearenv doesn't interoperate with cgo yet, and there is a TODO in the code, fixing this issue will also fix that TODO. 

[bradfitz, 2014-04-11 16:46:21]

I'd say just do it and see how invasive it looks. Worst case the work is then done and we can submit it Jun 1st-ish. On Fri, Apr 11, 2014 at 8:57 AM, minux <minux.ma@gmail.com> wrote: > > On Apr 11, 2014 10:26 AM, "Ian Lance Taylor" <iant@golang.org> wrote: > > > > On Thu, Apr 10, 2014 at 11:22 PM, minux <minux.ma@gmail.com> wrote: > > > > > > On Apr 10, 2014 11:20 PM, <bradfitz@golang.org> wrote: > > >> Well, the issue is flagged Go 1.3. > > >> > > >> This should either be finished (with passing to C, and tests), or the > Go > > >> 1.3 label should be removed from the issue. > > >> > > >> This seems simple enough to be worth finishing if somebody has time. > > >> Minux? > > > I'm very happy to finish it (with proper interaction with cgo) if we've > > > reached consensus on the issue priority. > > > > My vote is to change the issue to go1.4 and deal with it then. It > > seems to me to be a very marginal feature, and I see now that this CL > > is quite incomplete. It will need changes to the runtime and > > runtime/cgo packages as well. > this CL is incomplete because I want to get feedback on the new api first. > I don't think it requires big changes in runtime and runtime/cgo. > > also, the Clearenv doesn't interoperate with cgo yet, and there is a TODO > in the code, fixing this issue will also fix that TODO. > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > 

[Peter Waller, 2014-04-11 19:53:00]

On 2014/04/09 13:47:13, iant wrote: > I understand, but when you invoke the child process, you can set the environment > that should be provided to it. At that time you can remove the environment > variable. That is a non-racy solution. The problem is that you may want the child to inherit some environment and not others, and you may be putting together a couple of libraries which are both trying to fix up the environment on start up. I appreciate the application could do it all, but it's not great to force cooperation between separate parts of the program where none should be needed.

[iant, 2014-04-11 20:00:49]

On Fri, Apr 11, 2014 at 12:53 PM, <peter.waller@gmail.com> wrote: > On 2014/04/09 13:47:13, iant wrote: >> >> I understand, but when you invoke the child process, you can set the > > environment >> >> that should be provided to it. At that time you can remove the > > environment >> >> variable. That is a non-racy solution. > > > The problem is that you may want the child to inherit some environment > and not others, and you may be putting together a couple of libraries > which are both trying to fix up the environment on start up. > > I appreciate the application could do it all, but it's not great to > force cooperation between separate parts of the program where none > should be needed. I don't deny that one can construct cases where this functionality is useful. If it were never useful, nobody would want it. I just don't think it's very important. But I'm willing to go along with Brad's suggestion. If somebody wants to write a complete CL soon, we can review it. Ian 

[minux1, 2014-04-13 18:09:24]

PTAL. I implemented everything except for Plan 9. I need advice from Plan 9 experts on how to implement it on Plan 9, should I just remove /env/name? Its test (in misc/cgo/test) will be in CL 87430043 together with os.Unsetenv.

[minux1, 2014-04-13 19:45:32]

PTAL. syscall.Unsetenv is implemented on all supported platforms. On Sun, Apr 13, 2014 at 3:30 PM, David du Colombier <0intro@gmail.com>wrote: > > PTAL. I implemented everything except for Plan 9. I need > > advice from Plan 9 experts on how to implement it on Plan 9, > > should I just remove /env/name? > > Yes, removing /env/name should be fine. Let me know when the > CLs will be ready, so I could try on my side before submitting. > Thank you! 

[minux1, 2014-04-13 21:28:40]

On Sun, Apr 13, 2014 at 4:56 PM, David du Colombier <0intro@gmail.com>wrote: > > PTAL. syscall.Unsetenv is implemented on all supported platforms. > > Thanks. I think you also may want to remove the > environment variable from the cache. > You're right. Updated. 

[minux1, 2014-04-13 21:40:32]

On Sun, Apr 13, 2014 at 5:28 PM, minux <minux.ma@gmail.com> wrote: > > On Sun, Apr 13, 2014 at 4:56 PM, David du Colombier <0intro@gmail.com>wrote: > >> > PTAL. syscall.Unsetenv is implemented on all supported platforms. >> >> Thanks. I think you also may want to remove the >> environment variable from the cache. >> > You're right. Updated. > After a 2nd thought, I think Plan 9 shouldn't cache environment variables in, e.g., syscall.Environ(), because the environment variable could change at any time without the Go program knowing (by parent, child process that share environment with the Go process.) Am I wrong? 

[bradfitz, 2014-04-14 19:59:06]

R=rsc,iant

[rsc, 2014-04-14 20:05:28]

I agree with Ian. Let's put this off until Go 1.4. Being marked Go 1.3 does not mean "must be fixed before Go 1.3". It means that we need to make a decision about whether to fix it. At this point, if something requires new API, we should put it off. It's too late to be designing API we have to keep forever.

[rsc, 2014-04-17 02:46:12]

R=close Please 'hg mail' after 1.3 to reopen this CL.

[lann, 2014-04-27 07:05:57]

In case the merits of this request are still under debate, here's a bit of code we all execute that treats unset and empty env vars differently: https://github.com/github/git-msysgit/blob/0fce85db5bf812147941e10c489253286b... Specifically, most git commands will fail with `GIT_DIR=` vs GIT_DIR being unset.