search

ionide / proj-info

Parse and evaluate MsBuild project files
MIT License
64 stars 37 forks source link

ProjInfo sets environment variables for the current process. #191

Open safesparrow opened 1 year ago

safesparrow commented 1 year ago

Here environment variables are set for the current process.

This means that spawning an unrelated child process after using ProjInfo is affected by those. In my case, I have to reset them after, otherwise child dotnet processes that I run myself don't work as expected.

Ideally ProjInfo would not change current process's environment, and if it does, it should at least revert any changes it makes (which doesn't help if other things are using the Environment in parallel, but is better than the current state).

If the environment is only set for the benefit of child processes, could ProcessStartInfo be utilised to set them for child processes only?

baronfel commented 1 year ago

Instead of environment variables, we could return a collection of global properties, but then callers would be forced to use those global properties at all times or else something might be incorrect. The SDK and MSBuild tooling rely on the values of those properties being set.

safesparrow commented 1 year ago

It's fine to use environment variables, but instead of changing the current environment, they should only be applied to child processes. Otherwise the user is forced to figure out what they need to undo manually, which is not user-friendly.

One way to do this would be for functions like Init.init to return toolsPath * EnvironmentChanges, and things like WorkspaceLoader.Create accept such a tuple.

A backwards-compatible way would be for these functions to have overloads that do that on top of current versions.

Personally I don't depend on ProjInfo setting those variables - I don't need them as long as ProjInfo can set them wherever it needs - and in my case the only top-level API I use is WorkspaceLoader.Create.