Use parent environment to allow variable substitution#165
Conversation
…ed environment variables.
ashald
left a comment
ashald
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I left few comments, could you take a look please?
| Map<String, String> currentEnv = generalCommandLine.getEnvironment(); | ||
| Map<String, String> newEnv = EnvFileConfigurationEditor.collectEnv(goRunConfigurationBase, new HashMap<>(currentEnv)); | ||
| Map<String, String> newEnv = EnvFileConfigurationEditor.collectEnv(goRunConfigurationBase, EnvUtil.getInitialEnv(generalCommandLine)); | ||
| Map<String, String> currentEnv = generalCommandLine.getEffectiveEnvironment(); |
There was a problem hiding this comment.
Calling getEffectiveEnvironment() was wrong (unintentional change, missed in the code cleanup).
cmdline.getEnvironment() returns the reference to the environment, not a copy. As with the old code, clear and putAll on it modifies the data used by the command line. I added comments to clarify, because it's confusing at first sight.
| protected void patchCommandLine(@NotNull AbstractPythonRunConfiguration configuration, @Nullable RunnerSettings runnerSettings, @NotNull GeneralCommandLine cmdLine, @NotNull String runnerId) throws ExecutionException { | ||
| Map<String, String> newEnv = EnvFileConfigurationEditor.collectEnv(configuration, EnvUtil.getInitialEnv(cmdLine)); | ||
| Map<String, String> currentEnv = cmdLine.getEnvironment(); | ||
| Map<String, String> newEnv = EnvFileConfigurationEditor.collectEnv(configuration, new HashMap<>(currentEnv)); |
There was a problem hiding this comment.
After this change, currentEnv defined above becomes unused - should we drop it altogether?
There was a problem hiding this comment.
As above. Keeping the references to currentEnv together seemed clearer to me.
| protected void patchCommandLine(@NotNull AbstractRubyRunConfiguration<?> configuration, @Nullable RunnerSettings runnerSettings, @NotNull GeneralCommandLine cmdLine, @NotNull String runnerId) throws ExecutionException { | ||
| Map<String, String> newEnv = EnvFileConfigurationEditor.collectEnv(configuration, EnvUtil.getInitialEnv(cmdLine)); | ||
| Map<String, String> currentEnv = cmdLine.getEnvironment(); | ||
| Map<String, String> newEnv = EnvFileConfigurationEditor.collectEnv(configuration, new HashMap<>(currentEnv)); |
There was a problem hiding this comment.
After this change, currentEnv defined above becomes unused - should we drop it altogether?
|
Can I help with anything to get this merged? |
2 participants
I'm experimenting to support EnvFile with [BashSupport Pro}(https://www.bashsupport.com).
I noticed that the variable substitution isn't working as it should.
Currently EnvFile only provides the variables configured directly in the run configuration to the var env providers.
Now, if a variable substitution references a variable, which is inherited from the parent environment, then it has an empty value. That's happening because
eneralCommandLine.getEnvironment()is not including the variables from the parent environment.The SDK's process setup adds the parent environment variables to the new process, if the setting for this is enabled: https://github.com/JetBrains/intellij-community/blob/8e89a51b18059a34081c84439c6c19483f64ad10/platform/platform-util-io/src/com/intellij/execution/configurations/GeneralCommandLine.java#L469
EnvFile must follow the same logic to provide the same set of variables to the patched command line.
I successfully tested this with Python, the other modified providers follow the same logic.
I don't know much about
EnvFile, but hope that this is helpful.