Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly override base env with supplied values for spawn #5907

Merged
merged 1 commit into from Nov 7, 2019

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

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:

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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process.spawn doesn't remove nil env variables
2 participants