Skip to content

Properly override base env with supplied values for spawn#5907

Merged
headius merged 1 commit intojruby:masterfrom
matthewd:spawn-env-override
Nov 7, 2019
Merged

Properly override base env with supplied values for spawn#5907
headius merged 1 commit intojruby:masterfrom
matthewd:spawn-env-override

Conversation

@matthewd
Copy link
Contributor

@matthewd matthewd commented Oct 7, 2019

Hi!

I have no idea what I'm doing (and don't have a local dev environment, so I'm making things up without even a typecheck to back me up 😔), so this is probably disastrously wrong in detail, but...

I think something along these lines fixes #3428, as well as a wider issue:

% env | grep EDITOR
EDITOR=vim

% ruby -e 'system({ "EDITOR" => "emacs" }, "env")' | grep EDITOR
EDITOR=emacs

% rbenv shell jruby-9.2.8.0
% ruby -e 'system({ "EDITOR" => "emacs" }, "env")' | grep EDITOR
EDITOR=vim
EDITOR=emacs

POSIXly speaking (without having checked the relevant docs), that seems Bad. Even without the not-removing-nils issue, it appears to be a bit hit-or-miss which entry a process will choose to honour... I'm struggling to show it in isolation, but I got here because I was seeing the "old" value win in my real usage.

@headius headius added this to the JRuby 9.2.10.0 milestone Oct 28, 2019
@headius
Copy link
Member

headius commented Oct 28, 2019

This slipped through the cracks but can probably be merged for 9.2.10. Note that ShellLauncher is only used when we can't (or don't) use true native process lauching via PopenExecutor, so this code will only affect a subset of process logic.

@headius
Copy link
Member

headius commented Oct 28, 2019

Remarkably, I didn't even know it was possible to have the same entry twice in env.

@matthewd
Copy link
Contributor Author

matthewd commented Oct 29, 2019

Note that ShellLauncher is only used when we can't (or don't) use true native process lauching via PopenExecutor

Ah, this had me confused for a moment -- I'm on macOS, so would expect popen to be doing the thing. But it turns out:

jruby/core/src/main/java/org/jruby/util/io/PopenExecutor.java

Line 1016 in 439aceb

eargp.envp_str = ShellLauncher.getModifiedEnv(runtime, env, clearEnv);

Remarkably, I didn't even know it was possible to have the same entry twice in env.

Me neither! In retrospect it follows from the "series of contiguous \0-separated key-value pairs" definition that it's possible, but I suspect it's not valid, and very much doubt most applications have given conscious thought to the eventuality.

@headius headius merged commit 836ecee into jruby:master Nov 7, 2019
@headius
Copy link
Member

headius commented Nov 7, 2019

@matthewd Can you come up with a spec for this behavior?

matthewd added a commit to gel-rb/gel that referenced this pull request Mar 21, 2020
@matthewd
jruby/jruby#5907 has been released
8b2cf42 
I think it seems fine to require a current JRuby.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

JRuby 9.2.10.0

Development

Successfully merging this pull request may close these issues.

Process.spawn doesn't remove nil env variables

2 participants

@matthewd @headius