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
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.
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)
On 2014/03/29 16:43:25, iant wrote: > Is this appropriate during a feature freeze? > > ...
9 years, 11 months ago
(2014-04-09 07:52:24 UTC)
#6
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.
On 2014/04/09 07:52:24, Peter Waller wrote: > > I have encountered a direct need for ...
9 years, 11 months ago
(2014-04-09 13:47:13 UTC)
#7
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
Well, the issue is flagged Go 1.3. This should either be finished (with passing to ...
9 years, 11 months ago
(2014-04-11 03:20:43 UTC)
#8
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?
On Apr 10, 2014 11:20 PM, <bradfitz@golang.org> wrote: > Well, the issue is flagged Go ...
9 years, 11 months ago
(2014-04-11 06:22:36 UTC)
#9
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.
On Thu, Apr 10, 2014 at 11:22 PM, minux <minux.ma@gmail.com> wrote: > > On Apr ...
9 years, 11 months ago
(2014-04-11 14:26:28 UTC)
#10
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
On Apr 11, 2014 10:26 AM, "Ian Lance Taylor" <iant@golang.org> wrote: > > On Thu, ...
9 years, 11 months ago
(2014-04-11 15:57:51 UTC)
#11
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.
I'd say just do it and see how invasive it looks. Worst case the work ...
9 years, 11 months ago
(2014-04-11 16:46:21 UTC)
#12
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.
>
On 2014/04/09 13:47:13, iant wrote: > I understand, but when you invoke the child process, ...
9 years, 11 months ago
(2014-04-11 19:53:00 UTC)
#13
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.
On Fri, Apr 11, 2014 at 12:53 PM, <peter.waller@gmail.com> wrote: > On 2014/04/09 13:47:13, iant ...
9 years, 11 months ago
(2014-04-11 20:00:49 UTC)
#14
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
PTAL. I implemented everything except for Plan 9. I need advice from Plan 9 experts ...
9 years, 11 months ago
(2014-04-13 18:09:24 UTC)
#15
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.
PTAL. syscall.Unsetenv is implemented on all supported platforms. On Sun, Apr 13, 2014 at 3:30 ...
9 years, 11 months ago
(2014-04-13 19:45:32 UTC)
#16
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!
On Sun, Apr 13, 2014 at 4:56 PM, David du Colombier <0intro@gmail.com>wrote: > > PTAL. ...
9 years, 11 months ago
(2014-04-13 21:28:40 UTC)
#17
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.
On Sun, Apr 13, 2014 at 5:28 PM, minux <minux.ma@gmail.com> wrote: > > On Sun, ...
9 years, 11 months ago
(2014-04-13 21:40:32 UTC)
#18
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?
I agree with Ian. Let's put this off until Go 1.4. Being marked Go 1.3 ...
9 years, 11 months ago
(2014-04-14 20:05:28 UTC)
#20
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.
Issue 82040044: code review 82040044: syscall: add Unsetenv(string)
Created 10 years ago by minux1
Modified 9 years, 10 months ago
Reviewers: golang-codereviews, brainman, dfc, iant, Peter Waller, bradfitz, rsc, lann
Base URL:
Comments: 0