HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.17k stars 654 forks source link

No consistent way to unset environment variable #10395

Closed tobil4sk closed 3 years ago

tobil4sk commented 3 years ago

Currently, some targets have ways of unsetting environment variables, while others do not. So, after doing:

Sys.putEnv("VARIABLE", "1");

There is no consistent way to remove the environment variable. Different targets have different behaviours, listed below.

1) No way to completely unset environment variable at all (at least on Linux):

2) Environment variable unset with Sys.putEnv("VARIABLE", null);:

3) Environment variable unset with Sys.putEnv("VARIABLE", ""); on Windows:

It might be best to have a separate function, for example Sys.unsetEnv("VARIABLE");, that handles this.

It would also be good if Sys.putEnv("VARIABLE", null); and Sys.putEnv("VARIABLE", ""); worked consistently between platforms, however, this is more difficult because some platforms may have these behaviours because of their APIs. It also requires an agreement on the behaviour of the function in these two cases.

Simn commented 3 years ago

The first thing to investigate here is if platforms generally allow the distinction between the variable not being set (equivalent to null) and the variable being set but having no value ("").

tobil4sk commented 3 years ago

The targets in the first and second category do make this distinction.

Neko and Eval do not, however they give an error when trying to do Sys.putEnv("VARIABLE", null);. C# does not distinguish between the two either but it does allow setting it to null as well.

tobil4sk commented 3 years ago

I did a bit more investigating, turns out I was experiencing an issue with the Haxe server and Eval is part of the first group as well. (ie it does indeed distinguish between the two).

tobil4sk commented 3 years ago

Correction, because I think I slightly misunderstood the question: Both C# and Neko on Windows can tell whether a variable doesn't exist or whether it exists and is set to "". However, for both these targets setting the variable to "" deletes it. (This is because of the APIs: C# and VC)

The rest of the targets also can tell the difference.

Simn commented 3 years ago

Then "" should carry no special semantics here, it should just set the variable to the empty string. Using null however should unset the variable. In that case we don't need additional API because that would just be a convenience function anyway.

tobil4sk commented 3 years ago

I would argue that it makes more sense to create a separate function for unsetting the variable. On most targets, as setting to "" or null does different things, there will have to be a check to see whether or not the value passed in is null, and then a different function will be called depending on whether a value was passed or null. Something like this:


function putEnv(name:String, value:Null<String>) {
    if (value == null)
        someApi.unsetEenv(name);
    else
        someApi.putEnv(name, value);
}

In this case, I would argue that it wouldn't really be a convenience function, as the two functions would be for separate API calls and really do different things. Also, putting them together as one function introduces an unnecessary branch in the code, as the user knows when they call the function whether they are doing it to set a value or delete the variable, so there is no reason for the function to have to figure that out again.

I hope my opinion makes sense, it might be nitpicking but also it would require specifically changing the type of the value to be Null<String> which in my opinion complicates things more.

I did testing on Windows, and all targets apart from Python and php (I haven't tested lua though) delete the variable if "" is inputted. This is a bit annoying as it means that Sys.putEnv("VARIABLE", ""); does something different on Windows and Linux. That part is kinda out of Haxe's control though.

I'm happy to work on this once it's decided whether there should be a separate function or not.

tobil4sk commented 3 years ago

After more consideration I saw why it's better to not change the api, because it would come with a lot of required changes in a bunch of other repositories. Also if it's already been implemented on a couple targets (namely hl and php) then might as well make sure it works everywhere.

So anyway, I made a PR for this: #10402