ref: Replace x/devtools/internal/envutil with x/lib/envvar
A while ago I wrote envutil in the devtools repo, to make it
easier to deal with environment variables, and to make it easy
for the v23 tool to keep track of the mutations it had made.
Later on I noticed that there was similar code in x/ref in a few
places.
This CL removes envutil, and adds a new package envvar that may
be used by all other packages. It behaves similarly to envutil,
and also to the code scattered around x/ref.
In a subsequent CL I'll add cmdline support for environment
variables using envvar.
MultiPart: 2/2
Change-Id: Idd5c06ab79badf678cd5ce7e87493c04ea16204a
diff --git a/lib/exec/consts.go b/lib/exec/consts.go
index f275d78..54bb5ae 100644
--- a/lib/exec/consts.go
+++ b/lib/exec/consts.go
@@ -12,3 +12,21 @@
// of environment variables; exposing the name of this variable is intended to
// support such situations.
const ExecVersionVariable = "V23_EXEC_VERSION"
+
+const (
+ version1 = "1.0.0"
+ readyStatus = "ready::"
+ failedStatus = "failed::"
+ initStatus = "init"
+
+ // eofChar is written onto the status pipe to signal end-of-file. It
+ // should be the last byte written onto the pipe, before closing it.
+ // This signals to the reader that no more input is coming. This is
+ // needed since we cannot use the closing of the write end of the pipe
+ // to send io.EOF to the reader: since there are two write ends (one in
+ // the parent and one in the child), closing any one of the two is not
+ // going to send io.EOF to the reader.
+ // Since the data coming from the child should be valid utf-8, we pick
+ // one of the invalid utf-8 bytes for this.
+ eofChar = 0xFF
+)
diff --git a/lib/exec/parent.go b/lib/exec/parent.go
index e8747a1..230e5d0 100644
--- a/lib/exec/parent.go
+++ b/lib/exec/parent.go
@@ -18,9 +18,8 @@
"time"
"v.io/v23/verror"
-
+ "v.io/x/lib/envvar"
"v.io/x/lib/vlog"
-
"v.io/x/ref/lib/timekeeper"
)
@@ -126,22 +125,16 @@
// Start starts the child process, sharing a secret with it and
// setting up a communication channel over which to read its status.
func (p *ParentHandle) Start() error {
- // Make sure that there are no instances of the ExecVersionVariable
- // already in the environment (which can happen when a subprocess
- // creates a subprocess etc)
- nenv := make([]string, 0, len(p.c.Env)+1)
- for _, e := range p.c.Env {
- if strings.HasPrefix(e, ExecVersionVariable+"=") {
- continue
- }
- nenv = append(nenv, e)
- }
-
+ env := envvar.SliceToMap(p.c.Env)
if !p.protocol {
+ // Ensure ExecVersionVariable is not set if we're not using the protocol.
+ delete(env, ExecVersionVariable)
+ p.c.Env = envvar.MapToSlice(env)
return p.c.Start()
}
-
- p.c.Env = append(nenv, ExecVersionVariable+"="+version1)
+ // Ensure ExecVersionVariable is always set if we are using the protocol.
+ env[ExecVersionVariable] = version1
+ p.c.Env = envvar.MapToSlice(env)
// Create anonymous pipe for communicating data between the child
// and the parent.
diff --git a/lib/exec/shared.go b/lib/exec/shared.go
deleted file mode 100644
index 2baf026..0000000
--- a/lib/exec/shared.go
+++ /dev/null
@@ -1,23 +0,0 @@
-// Copyright 2015 The Vanadium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package exec
-
-const (
- version1 = "1.0.0"
- readyStatus = "ready::"
- failedStatus = "failed::"
- initStatus = "init"
-
- // eofChar is written onto the status pipe to signal end-of-file. It
- // should be the last byte written onto the pipe, before closing it.
- // This signals to the reader that no more input is coming. This is
- // needed since we cannot use the closing of the write end of the pipe
- // to send io.EOF to the reader: since there are two write ends (one in
- // the parent and one in the child), closing any one of the two is not
- // going to send io.EOF to the reader.
- // Since the data coming from the child should be valid utf-8, we pick
- // one of the invalid utf-8 bytes for this.
- eofChar = 0xFF
-)
diff --git a/lib/exec/util.go b/lib/exec/util.go
deleted file mode 100644
index 323097b..0000000
--- a/lib/exec/util.go
+++ /dev/null
@@ -1,66 +0,0 @@
-// Copyright 2015 The Vanadium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package exec
-
-import (
- "strings"
-
- "v.io/v23/verror"
-)
-
-var (
- errNotFound = verror.Register(pkgPath+".errNotFound", verror.NoRetry, "{1:}{2:} not found{:_}")
-)
-
-// Getenv retrieves the value of the given variable from the given
-// slice of environment variable assignments.
-func Getenv(env []string, name string) (string, error) {
- for _, v := range env {
- if strings.HasPrefix(v, name+"=") {
- return strings.TrimPrefix(v, name+"="), nil
- }
- }
- return "", verror.New(errNotFound, nil)
-}
-
-// Setenv updates / adds the value assignment for the given variable
-// in the given slice of environment variable assigments.
-func Setenv(env []string, name, value string) []string {
- newValue := name + "=" + value
- for i, v := range env {
- if strings.HasPrefix(v, name+"=") {
- env[i] = newValue
- return env
- }
- }
- return append(env, newValue)
-}
-
-// Mergeenv merges the values for the variables contained in 'other' with the
-// values contained in 'base'. If a variable exists in both, the value in
-// 'other' takes precedence.
-func Mergeenv(base, other []string) []string {
- otherValues := make(map[string]string)
- otherUsed := make(map[string]bool)
- for _, v := range other {
- if parts := strings.SplitN(v, "=", 2); len(parts) == 2 {
- otherValues[parts[0]] = parts[1]
- }
- }
- for i, v := range base {
- if parts := strings.SplitN(v, "=", 2); len(parts) == 2 {
- if otherValue, ok := otherValues[parts[0]]; ok {
- base[i] = parts[0] + "=" + otherValue
- otherUsed[parts[0]] = true
- }
- }
- }
- for k, v := range otherValues {
- if !otherUsed[k] {
- base = append(base, k+"="+v)
- }
- }
- return base
-}
diff --git a/lib/exec/util_test.go b/lib/exec/util_test.go
deleted file mode 100644
index dd97eb0..0000000
--- a/lib/exec/util_test.go
+++ /dev/null
@@ -1,57 +0,0 @@
-// Copyright 2015 The Vanadium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package exec
-
-import (
- "testing"
-)
-
-func TestEnv(t *testing.T) {
- env := make([]string, 0)
- env = Setenv(env, "NAME", "VALUE1")
- if expected, got := 1, len(env); expected != got {
- t.Fatalf("Unexpected length of environment variable slice: expected %d, got %d", expected, got)
- }
- if expected, got := "NAME=VALUE1", env[0]; expected != got {
- t.Fatalf("Unexpected element in the environment variable slice: expected %q, got %q", expected, got)
- }
- env = Setenv(env, "NAME", "VALUE2")
- if expected, got := 1, len(env); expected != got {
- t.Fatalf("Unexpected length of environment variable slice: expected %d, got %d", expected, got)
- }
- if expected, got := "NAME=VALUE2", env[0]; expected != got {
- t.Fatalf("Unexpected element in the environment variable slice: expected %q, got %q", expected, got)
- }
- value, err := Getenv(env, "NAME")
- if err != nil {
- t.Fatalf("Unexpected error when looking up environment variable value: %v", err)
- }
- if expected, got := "VALUE2", value; expected != got {
- t.Fatalf("Unexpected value of an environment variable: expected %q, got %q", expected, got)
- }
- value, err = Getenv(env, "NONAME")
- if err == nil {
- t.Fatalf("Expected error when looking up environment variable value, got none (value: %q)", value)
- }
-}
-
-func TestMerge(t *testing.T) {
- env := []string{"ANIMAL=GOPHER", "METAL=CHROMIUM"}
- env = Mergeenv(env, []string{"CAR=VEYRON", "METAL=VANADIUM"})
- if want, got := 3, len(env); want != got {
- t.Fatalf("Expected %d env vars, got %d instead", want, got)
- }
- for n, want := range map[string]string{
- "ANIMAL": "GOPHER",
- "CAR": "VEYRON",
- "METAL": "VANADIUM",
- } {
- if got, err := Getenv(env, n); err != nil {
- t.Fatalf("Expected a value when looking up %q, got none", n)
- } else if got != want {
- t.Fatalf("Unexpected value of environment variable %q: expected %q, got %q", n, want, got)
- }
- }
-}
diff --git a/test/modules/exec.go b/test/modules/exec.go
index 06d0e4f..ffbbbdd 100644
--- a/test/modules/exec.go
+++ b/test/modules/exec.go
@@ -9,11 +9,11 @@
"io"
"os"
"os/exec"
- "strings"
"sync"
"time"
"v.io/v23/verror"
+ "v.io/x/lib/envvar"
"v.io/x/lib/vlog"
vexec "v.io/x/ref/lib/exec"
"v.io/x/ref/lib/mgmt"
@@ -24,17 +24,17 @@
// execHandle implements both the command and Handle interfaces.
type execHandle struct {
*expect.Session
- mu sync.Mutex
- cmd *exec.Cmd
- name string
- entryPoint string
- handle *vexec.ParentHandle
- sh *Shell
- stderr *os.File
- stdout io.ReadCloser
- stdin io.WriteCloser
- procErrCh chan error
- opts *StartOpts
+ mu sync.Mutex
+ cmd *exec.Cmd
+ name string
+ handle *vexec.ParentHandle
+ sh *Shell
+ stderr *os.File
+ stdout io.ReadCloser
+ stdin io.WriteCloser
+ procErrCh chan error
+ opts *StartOpts
+ external bool
}
func testFlags() []string {
@@ -63,11 +63,11 @@
}
func newExecHandle(name string) command {
- return &execHandle{name: name, entryPoint: shellEntryPoint + "=" + name, procErrCh: make(chan error, 1)}
+ return &execHandle{name: name, procErrCh: make(chan error, 1)}
}
func newExecHandleForExternalCommand(name string) command {
- return &execHandle{name: name, procErrCh: make(chan error, 1)}
+ return &execHandle{name: name, procErrCh: make(chan error, 1), external: true}
}
func (eh *execHandle) Stdout() io.Reader {
@@ -95,19 +95,16 @@
}
func (eh *execHandle) envelope(sh *Shell, env []string, args ...string) ([]string, []string) {
+ // TODO(toddw): External commands probably shouldn't run any of this logic,
+ // and should just return args, env directly.
newargs := []string{os.Args[0]}
newargs = append(newargs, testFlags()...)
newargs = append(newargs, args...)
- // Be careful to remove any existing shellEntryPoint env vars. This
- // can happen when subprocesses run other subprocesses etc.
- cleaned := make([]string, 0, len(env)+1)
- for _, e := range env {
- if strings.HasPrefix(e, shellEntryPoint+"=") {
- continue
- }
- cleaned = append(cleaned, e)
- }
- return newargs, append(cleaned, eh.entryPoint)
+ // Be careful to overwrite shellEntryPoint if it already exists - e.g. when a
+ // subprocess runs another subprocess.
+ newenv := envvar.SliceToMap(env)
+ newenv[shellEntryPoint] = eh.name
+ return newargs, envvar.MapToSlice(newenv)
}
func (eh *execHandle) start(sh *Shell, agentfd *os.File, opts *StartOpts, env []string, args ...string) (Handle, error) {
@@ -118,8 +115,8 @@
cmdPath := args[0]
newargs, newenv := args, env
- // If an entry point is specified, use the envelope execution environment.
- if len(eh.entryPoint) > 0 {
+ // If this isn't an external command, use the envelope execution environment.
+ if !eh.external {
cmdPath = os.Args[0]
newargs, newenv = eh.envelope(sh, env, args[1:]...)
}
diff --git a/test/modules/func.go b/test/modules/func.go
index 7445eab..9331f7f 100644
--- a/test/modules/func.go
+++ b/test/modules/func.go
@@ -11,6 +11,7 @@
"sync"
"time"
+ "v.io/x/lib/envvar"
"v.io/x/lib/vlog"
"v.io/x/ref/test/expect"
)
@@ -111,7 +112,7 @@
main := fh.main
fh.mu.Unlock()
- cenv := envSliceToMap(env)
+ cenv := envvar.SliceToMap(env)
vlog.VI(1).Infof("Start: %q args: %v", fh.name, args)
vlog.VI(2).Infof("Start: %q env: %v", fh.name, cenv)
err := main(stdin, stdout, stderr, cenv, args[1:]...)
diff --git a/test/modules/modules_test.go b/test/modules/modules_test.go
index d63e5c6..67b5b9a 100644
--- a/test/modules/modules_test.go
+++ b/test/modules/modules_test.go
@@ -644,22 +644,6 @@
}
}
-func TestSetEntryPoint(t *testing.T) {
- env := map[string]string{"a": "a", "b": "b"}
- nenv := modules.SetEntryPoint(env, "eg1")
- if got, want := len(nenv), 3; got != want {
- t.Errorf("got %d, want %d", got, want)
- }
- nenv = modules.SetEntryPoint(env, "eg2")
- if got, want := len(nenv), 3; got != want {
- t.Errorf("got %d, want %d", got, want)
- }
- sort.Strings(nenv)
- if got, want := nenv, []string{"V23_SHELL_HELPER_PROCESS_ENTRY_POINT=eg2", "a=a", "b=b"}; !reflect.DeepEqual(got, want) {
- t.Errorf("got %v, want %v", got, want)
- }
-}
-
func TestNoExec(t *testing.T) {
ctx, shutdown := test.InitForTest()
defer shutdown()
diff --git a/test/modules/registry.go b/test/modules/registry.go
index 22970ee..d33b283 100644
--- a/test/modules/registry.go
+++ b/test/modules/registry.go
@@ -13,6 +13,7 @@
"sync"
"time"
+ "v.io/x/lib/envvar"
"v.io/x/lib/vlog"
vexec "v.io/x/ref/lib/exec"
@@ -96,20 +97,6 @@
return os.Getenv(shellEntryPoint) != ""
}
-// SetEntryPoint returns a slice of strings that is guaranteed to contain
-// one and only instance of the entry point environment variable set to the
-// supplied name value.
-func SetEntryPoint(env map[string]string, name string) []string {
- newEnv := make([]string, 0, len(env)+1)
- for k, v := range env {
- if k == shellEntryPoint {
- continue
- }
- newEnv = append(newEnv, k+"="+v)
- }
- return append(newEnv, shellEntryPoint+"="+name)
-}
-
// Dispatch will execute the requested subprocess command from a within a
// a subprocess. It will return without an error if it is executed by a
// process that does not specify an entry point in its environment.
@@ -193,7 +180,7 @@
}(os.Getppid())
flag.Parse()
- return m.main(os.Stdin, os.Stdout, os.Stderr, envSliceToMap(os.Environ()), flag.Args()...)
+ return m.main(os.Stdin, os.Stdout, os.Stderr, envvar.SliceToMap(os.Environ()), flag.Args()...)
}
// WaitForEOF returns when a read on its io.Reader parameter returns io.EOF
diff --git a/test/modules/shell.go b/test/modules/shell.go
index 8858ebe..ae9b409 100644
--- a/test/modules/shell.go
+++ b/test/modules/shell.go
@@ -155,6 +155,7 @@
"v.io/v23"
"v.io/v23/context"
"v.io/v23/security"
+ "v.io/x/lib/envvar"
"v.io/x/ref"
"v.io/x/ref/lib/exec"
"v.io/x/ref/services/agent/agentlib"
@@ -510,6 +511,7 @@
// t.Fatal(err)
// }
func (sh *Shell) StartWithOpts(opts StartOpts, env []string, name string, args ...string) (Handle, error) {
+ var err error
if opts.Error != nil {
return nil, opts.Error
}
@@ -526,7 +528,6 @@
}
if sh.ctx != nil && opts.ExecProtocol && opts.Credentials == nil {
- var err error
opts.Credentials, err = sh.NewChildCredentials("child")
if err != nil {
return nil, err
@@ -534,10 +535,7 @@
}
cmd := desc.factory()
expanded := append([]string{name}, sh.expand(args...)...)
- cenv, err := sh.setupCommandEnv(env)
- if err != nil {
- return nil, err
- }
+ cenv := sh.setupCommandEnv(env)
var p *os.File
if opts.Credentials != nil {
@@ -566,10 +564,7 @@
if cmd == nil {
return []string{}, []string{}
}
- menv, err := sh.setupCommandEnv(env)
- if err != nil {
- return []string{}, []string{}
- }
+ menv := sh.setupCommandEnv(env)
return cmd.factory().envelope(sh, menv, args...)
}
@@ -600,24 +595,24 @@
// and an indication of whether it is defined or not.
func (sh *Shell) GetVar(key string) (string, bool) {
sh.mu.Lock()
- defer sh.mu.Unlock()
v, present := sh.env[key]
+ sh.mu.Unlock()
return v, present
}
// SetVar sets the value to be associated with key.
func (sh *Shell) SetVar(key, value string) {
sh.mu.Lock()
- defer sh.mu.Unlock()
// TODO(cnicolaou): expand value
sh.env[key] = value
+ sh.mu.Unlock()
}
// ClearVar removes the speficied variable from the Shell's environment
func (sh *Shell) ClearVar(key string) {
sh.mu.Lock()
- defer sh.mu.Unlock()
delete(sh.env, key)
+ sh.mu.Unlock()
}
// GetConfigKey returns the value associated with the specified key in
@@ -642,12 +637,9 @@
// Env returns the entire set of environment variables associated with this
// Shell as a string slice.
func (sh *Shell) Env() []string {
- vars := []string{}
sh.mu.Lock()
- defer sh.mu.Unlock()
- for k, v := range sh.env {
- vars = append(vars, k+"="+v)
- }
+ vars := envvar.MapToSlice(sh.env)
+ sh.mu.Unlock()
return vars
}
@@ -723,25 +715,21 @@
return err
}
-func (sh *Shell) setupCommandEnv(env []string) ([]string, error) {
- osmap := envSliceToMap(os.Environ())
- evmap := envSliceToMap(env)
+func (sh *Shell) setupCommandEnv(env []string) []string {
+ osmap := envvar.SliceToMap(os.Environ())
+ evmap := envvar.SliceToMap(env)
sh.mu.Lock()
defer sh.mu.Unlock()
- m1 := mergeMaps(osmap, sh.env)
+ m1 := envvar.MergeMaps(osmap, sh.env)
// Clear any VeyronCredentials directory in m1 as we never
// want the child to directly use the directory specified
// by the shell's VeyronCredentials.
delete(m1, ref.EnvCredentials)
delete(m1, ref.EnvAgentEndpoint)
- m2 := mergeMaps(m1, evmap)
- r := []string{}
- for k, v := range m2 {
- r = append(r, k+"="+v)
- }
- return r, nil
+ m2 := envvar.MergeMaps(m1, evmap)
+ return envvar.MapToSlice(m2)
}
// ExpectSession is a subset of v.io/x/ref/tests/expect.Session's methods
diff --git a/test/modules/util.go b/test/modules/util.go
index a19e8b9..db463a1 100644
--- a/test/modules/util.go
+++ b/test/modules/util.go
@@ -43,43 +43,6 @@
f.Close()
}
-// envSliceToMap returns a map representation of a string slive
-// of environment variables.
-func envSliceToMap(env []string) map[string]string {
- m := make(map[string]string)
- if env == nil {
- return m
- }
- for _, osv := range env {
- if len(osv) == 0 {
- continue
- }
- parts := strings.SplitN(osv, "=", 2)
- key := parts[0]
- if len(parts) == 2 {
- m[key] = parts[1]
- } else {
- m[key] = ""
- }
- }
- return m
-}
-
-// mergeMaps merges two maps, a & b, with b taking preference over a.
-func mergeMaps(a, b map[string]string) map[string]string {
- if len(b) == 0 {
- return a
- }
- merged := make(map[string]string)
- for k, v := range a {
- merged[k] = v
- }
- for k, v := range b {
- merged[k] = v
- }
- return merged
-}
-
func principalFromDir(dir string) (security.Principal, error) {
p, err := vsecurity.LoadPersistentPrincipal(dir, nil)
if err == nil {
