Environment variables are persisted in Ruby 3.0

[DannyBen]

Describe the problem

The environment variables options (in its two forms) behave differently, where one form seems to persist the variables even for subsequent commands.

Steps to reproduce the problem

require 'tty/command'

shell = TTY::Command.new

env1 = { a: "a", b: "b" }
env2 = { c: "d", d: "d" }

# PASS
shell.run :echo, env: env1
# => Running ( export A="a" B="b" ; echo )

# FAIL - env is expected not to be used here
shell.run :echo
# => Running ( export A="a" B="b" ; echo )

# FAIL - env1 was merged with env2 here, unexpectedly
shell.run env2, :echo
# => Running ( export C="d" D="d" A="a" B="b" ; echo )

# 1 x PASS: env2 was NOT persisted from the previous call
# 1 x FAIL: env1 is still here
shell.run :echo
# => Running ( export A="a" B="b" ; echo )

Actual behaviour

Environment variables were sometimes persisted and used in subsequent commands, even if they did not have env setting.

Expected behaviour

Expected env vars to only influence the running command, and not contaminate subsequent commands.

Side note: In case there is a desire to set a global environment, I expected it to be defined in the form of TTY::Command.new env: { ... }

Additional notes

I was also wondering why the environment is prepended to the command in this form:

export VAR="val" VAR2="val" ; command

instead of this form (no export, no semicolon):

VAR="val" VAR2="val" command

which, to my understanding, it the common way of setting environment variables for a single command.

Describe your environment

• OS version: Ubuntu 18
• Ruby version: ruby 3.0.0p0
• TTY::Command version: 0.10.0

[piotrmurach]

Thank you for this report. You're right, the env vars shouldn't be persisted for different commands. I also agree that only the variables specified at initialisation should be persisted for all the command runs. Do you have time to submit a patch?

The reason I went with export was to ensure that the executable run inherits env vars, as it, for example, may run in a subshell or execute another program that requires these variables. As for the syntax, I wanted to combine the export with the command in a single line for printing. Do you see downsides to this approach? I'm open to considering using your approach. Or alternatively, provide an option to pick one or the other.

[DannyBen]

I might have time to take a look.

About the export env vars variants - changing to a non export version might break something for someone (although it shouldn't in my view), and the primary reason I thought it would be better is that it is a) more elegant, b) more common and c) probably works for all use cases. I might be wrong, so let's leave it as is.

[piotrmurach]

I had a little more time to look into this issue. Turns out that the test you have provided in the PR passes without the change to the update call. The update method is to do with passing along timeout, binmode etc but not env variables.

On further investigation, I cannot replicate or confirm any environment variables 'persisting' over to the next command. Here is a slightly modified version of your example run against the current gem release:

# frozen_string_literal: true

require_relative "lib/tty-command"

shell = TTY::Command.new

env1 = { a: "a", b: "b" }
env2 = { c: "d", d: "d" }

shell.run :echo, "env1#{env1}", env: env1
shell.run :echo, "empty"
shell.run :echo, "env2#{env2}", env: env2
shell.run env1, :echo, "env1#{env1}"

And the output is as expected:

Are you sure you're not merging some env vars before passing them to the run method?

[DannyBen]

This is weird.

1. The standalone code I provided at the beginning of the issue fails exactly as I ran it. There is no hidden step I took other than what I pasted.
2. The tests that I have designed for this case most definitely failed with the second run call showing the environment of the first call, just as it did in the reproduction code.
3. The use of Hash#update seems very suspicious there, as it is an alias to Hash#merge!, and I believe you did not intend to change the original argument.

Here is a screenshot:

In addition, here is the area in the code that calls Cmd#update

https://github.com/piotrmurach/tty-command/blob/e7384949c4d029995ecdd7f0a8f39ee517c3c967/lib/tty/command.rb#L177

and after the first call @cmd_options becomes contaminated with an env hash.

I added a byebug interrupt just after this call and recorded this terminal cast.

And for good measure, here is the recording of the failing focused spec.

[DannyBen]

Hey - I think I may have found the source for discrepancy....

It seems like Hash#update has changed in Ruby 3.0

I have conducted my own CI test on a code that

a) has the test and b) does NOT have the patch (still uses Hash#update)

The result can be seen here

• Here are the Hash#update docs for Ruby 2.7
• Here are the Hash#update docs for Ruby 3.0

[piotrmurach]

Great debugging session! Thank you for digging into this. I tested against 2.7.2 but I should've known better given I asked for the Ruby version in the template.😅

It seems like Hash#update has changed in Ruby 3.0

I checked the Hash#update against Ruby 2.7 and 3.0 and it is backwards compatible. However, what has changed is the ** operator behaviour. Prior to Ruby 3.0 this operator creates a copy of the parameter so even though update modifies receiver it doesn't mutate the original options hash.

def magic(options)
  options.update({c: "c", d: "d"})
end

env1 = {a: "a", b: "b"}
magic(**env1) 
# => {:a=>"a", :b=>"b", :c=>"c", :d=>"d"}
env1
# => {:a=>"a", :b=>"b"}

In Ruby 3.0 the original env1 gets updated:

env1 = {a: "a", b: "b"}
magic(**env1) 
# => {:a=>"a", :b=>"b", :c=>"c", :d=>"d"}
env1
# => {:a=>"a", :b=>"b", :c=>"c", :d=>"d"}  BINGO!

The ** operator doesn't seem to be necessary anyway as the hash argument shouldn't be coerced at all. It was submitted as a fix for Ruby 2.7 warnings.

You're right that the intention behind Cmd#update is to update the current instance options without modifying the original TTY::Command instance options.

Therefore, your patch is still valid and needs to be merged. Thanks for your time resolving this issue.❤️

[DannyBen]

Therefore, your patch is still valid and needs to be merged. Thanks for your time resolving this issue.❤️

Sure thing, I enjoyed this session as well. Happy to contribute.

[piotrmurach]

@DannyBen Released v0.10.1 with your patch. Thank you.

[DannyBen]

Excellent, thanks.