richterger / Perl-LanguageServer

Language Server for Perl
Other
220 stars 53 forks source link

Add an option to merge existing env instead of re-initializing it #132

Closed alexgarel closed 1 year ago

alexgarel commented 2 years ago

I work with a docker container where we set a lot of configuration through environment.

It would be very cumbersome to replicate all those envs in the launch.json (also because they are tweak in a number of ways to represent different scenarios).

This issue is already discussed in #39

Currently PerlLanguageServer reset the ENV but I would like it to provide a boolean "mergeEnv" option, that when true would merge existing env with provided "env" key.

Something that in DebuggerProcess.pm, would be

--- a/lib/Perl/LanguageServer/DebuggerProcess.pm
+++ b/lib/Perl/LanguageServer/DebuggerProcess.pm
@@ -122,8 +122,16 @@ sub launch

     my $fn = $workspace -> file_client2server ($self -> program) ;
     my $pid ;
+    my $env_ref = \%ENV;
     {
     local %ENV ;
+    if ($self -> debug_adapter -> merge_env) {
+        # keep existing ENV
+        foreach (keys %{$env_ref})
+            {
+            $ENV{$_} = $env_ref -> {$_} ;    
+            }
+    }
+
     foreach (keys %{$self -> env})
         {
         $ENV{$_} = $self -> env -> {$_} ; 
richterger commented 2 years ago

Since you already most of the work in the above patch. If you create a pull request for this, I will integrate it in Perl::LanguageServer. Otherwise I will take a look when I have some spare time. P.S. If you have any questions about how to add the option for launch.json, I am happy to assist you

dseynhae commented 1 year ago

I am not sure this should be an option, I think it is normal, expected behavior that the code application inherits all the environment variables from the calling shell/environment...

richterger commented 1 year ago

Environment is now passed per default. This can be disabled by disablesPassEnv, see 29ad712a55348a1ba06a375bbf5e00fb62abbf05

alexgarel commented 1 year ago

Thanks so much @richterger, I should really look into it anew !