WithEnv + Exec(ForEach) don't work togheter

[prochac]

The script.Exec contains this code

args, err := shell.Fields(cmdLine, nil)
if err != nil {
	return err
}

that already evaluates the env variables, but nil is passed in. So the variables are evaluated as empty string.

https://go.dev/play/p/P73IM0NLNBv

Or am I using it wrong?

[prochac]

Or it shouldn't be evaluated at all:
https://go.dev/play/p/XWxxo4eEla_t

[lazysegtree]

This is a valid issue. We don't respect the pipe's env while parsing shell command passed to Exec

fmt.Printf("Started\n")
	env := []string{`MY_ENV=foo`}
	out1, _ := script.NewPipe().WithEnv(env).Exec(`echo $MY_ENV`).String()

	cmd := exec.Command("sh", "-c", `echo $MY_ENV`)
	cmd.Env = env
	out2, _ := cmd.Output()

	out3, _ := exec.Command("sh", "-c", `MY_ENV=foo; echo $MY_ENV`).Output()
	fmt.Printf("out1 = '%s'\n", out1)
	fmt.Printf("out2 = '%s'\n", out2)
	fmt.Printf("out3 = '%s'\n", out3)

	os.Setenv("MY_ENV", "foo")
	out4, _ := script.NewPipe().WithEnv(env).Exec(`echo $MY_ENV`).String()
	fmt.Printf("(After setting env) out4 = '%s'\n", out4)

This prints

➜  examples git:(temp) ✗ go run env.go
Started
out1 = '
'
out2 = 'foo
'
out3 = 'foo
'
(After setting env) out4 = 'foo
'
➜  examples git:(temp) ✗```

[lazysegtree]

The way to fix it is passing a function to get env in shell.Fields as you pointed out.

Proof of concept

diff --git a/script.go b/script.go
index d7d1bc3..2efebac 100644
--- a/script.go
+++ b/script.go
@@ -428,7 +428,7 @@ func (p *Pipe) Error() error {
 // instead be redirected to a supplied writer, using [Pipe.WithStderr].
 func (p *Pipe) Exec(cmdLine string) *Pipe {
 	return p.Filter(func(r io.Reader, w io.Writer) error {
-		args, err := shell.Fields(cmdLine, nil)
+		args, err := shell.Fields(cmdLine, p.getShellEnvFunc())
 		if err != nil {
 			return err
 		}
@@ -453,6 +453,24 @@ func (p *Pipe) Exec(cmdLine string) *Pipe {
 	})
 }
 
+func (p *Pipe) getShellEnvFunc() func(string) string {
+	pipeEnv := p.environment()
+	if pipeEnv == nil {
+		return nil
+	}
+	pipEnvMap := map[string]string{}
+	for _, s := range pipeEnv {
+		vals := strings.Split(s, "=")
+		// Needs more error handling
+		if len(vals) == 2 {
+			pipEnvMap[vals[0]] = vals[1]
+		}
+	}
+	return func(key string) string {
+		return pipEnvMap[key]
+	}
+}
+
 // ExecForEach renders cmdLine as a Go template for each line of input, running
 // the resulting command, and produces the combined output of all these
 // commands in sequence. See [Pipe.Exec] for details on error handling and

New output of the same code that is in above message

➜  examples git:(temp) ✗ go run env.go
Started
out1 = 'foo
'
out2 = 'foo
'
out3 = 'foo
'
(After setting env) out4 = 'foo
'
➜  examples git:(temp) ✗

@bitfield Let me know if you think this is a valid bug, and the library should have a fix for it. The implementation can be refined more.

[lazysegtree]

For your information @prochac the below in your snippet (https://go.dev/play/p/XWxxo4eEla_t) doesn't works because what you are doing is passing literal string ${MY_ENV} to echo

cmd = exec.Command("echo", "${MY_ENV}")
	cmd.Env = []string{`MY_ENV="foo"`}
	out, err = cmd.Output()
	fmt.Println(string(out), err)

typing echo ${MY_ENV} works in shell, as shell first expands MY_ENV and then sends it to echo.

[bitfield]

I think the problem here is that echo doesn't expand variables; it's the shell that does that. Try https://go.dev/play/p/i-KO3tFxYcS

[lazysegtree]

@bitfield and this library is supposed to imitate shell, so the library should expand variables before passing the arguments to echo.

Do you agree or not that the below should return foo ? That what the whole issue is about.

script.NewPipe().WithEnv([]string{"MY_ENV=foo"}).Exec("echo $MY_ENV").String()

[lazysegtree]

Now if you say that bitfield/script should not expand variable, then $HOME shouldn't be expanded here. But it is expanded.

script.NewPipe().Exec(`echo $HOME`).String()

[bitfield]

That's a good point, I hadn't spotted that.

But let's go carefully and one step at a time. The purpose of WithEnv is to set environment variables that should be passed on to commands run with Exec, and it does that (not surprising, as there's a test for this, and it's passing). Try this program, for example:

https://go.dev/play/p/-w-SQASoGZ5

It prints, as you'd expect:

MY_ENV=foo

So there's nothing wrong with WithEnv, or the way we pass those environment settings to Exec.

There's a separate question about whether Exec should expand variables in the user's command line before it is passed to the spawned command. Currently it does not. There is an argument for opening an issue that it should (I personally am not convinced, but if you are, create the issue and we can discuss it there where it'll be on topic.)

[lazysegtree]

Yes, I agree that this issue is incorrectly titled. And yes, Exec works as expected. The question was only about the expansion of variables in the user's command line before it is passed to the spawned command.

After some thinking I also agree that it should not expand. The current WithEnv correctly imitates env command.

➜  ~/Workspace/kuknitin/cxxopts/example git:(lazysegtree) ✗ [11:57:43] env A=1 \
> echo $A

➜  ~/Workspace/kuknitin/cxxopts/example git:(lazysegtree) ✗ [11:57:49] env A=1 \
echo $HOME
/Users/kuknitin
➜  ~/Workspace/kuknitin/cxxopts/example git:(lazysegtree) ✗ [11:57:57]

➜  ~/Workspace/kuknitin/cxxopts/example git:(lazysegtree) ✗ [12:00:28] env A=1 \
> python3 -c 'import os; print(os.getenv("A"))'
1
➜  ~/Workspace/kuknitin/cxxopts/example git:(lazysegtree) ✗ [12:00:40]

[prochac]

My problem is, that the shell.Fields(cmdLine, nil) does expand the envs. It just expands them as an empty string.

I know why exec.Command("echo", "${MY_ENV}") doesn't expand. But it's preceded by the shell.Fields that does the expansion, but to empty strings.

https://go.dev/play/p/9dOvHriiZV3

[bitfield]

It just expands them as an empty string.

I think I finally get it! The point I was missing is that shell.Fields expands variables. (I actually wasn't aware of that, and now that I know it does, I wish it didn't.)

Unfortunately, what it takes is a func (string) string (because that's what os.Getenv is) and what we have is a []string of the form KEY=VALUE. That's a really awkward mismatch, as @lazysegtree demonstrated with the code that's necessary to resolve it.

While I'm sure that solution is correct, and I doubt if it can be made more elegant, I can't accept that code in this project. It offends me. And I don't think we should be in the business of writing it. We're going to more and more trouble over time just to (poorly) replicate the behaviour of the shell.

Instead, I'm inclined to say "Let's just use the shell". In other words, pass the user's command line directly and without modification to a sh -c command. What if they don't have sh, or they don't want to spawn an extra subprocess? I guess they can use exec.Command directly in that case.

[lazysegtree]

I am good with sh -c. It makes implementation so much simpler with any practical performance overhead that I can forsee. No need parsing or field splitting of user's command.

Although what about Windows? I don't know how much support we have for windows. Having to use cmd.exe or powershell.exe might have too much overhead.

[prochac]

Sorry :D yeah, I filled that ticket at quite late hour, not being native, and had focus on a different issue 😄

Yeah, what bothers me is the inconsistency. I solved my problem by doing the expansion using fmt.Sprintf.
But the inconsistency remains. TBH, I have no conservative solution.
It all starts with the decision, if the script should mimic Shell with the expansion, or not.

An alternative is to not use the script.Fields func from the expand.go file 😛 cut the expansion out, and use just the parser:
https://github.com/mvdan/sh/blob/master/shell/expand.go#L52

func cmdToFields(s string) ([]string, error) {
	p := syntax.NewParser()
	var words []*syntax.Word
	for w, err := range p.WordsSeq(strings.NewReader(s)) {
		if err != nil {
			return nil, err
		}
		words = append(words, w)
	}
	return words, nil
}