search

ionide / proj-info

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

Executable using ProjInfo starts itself again with `--version` #116

Closed Booksbaum closed 3 years ago

Booksbaum commented 3 years ago

Describe the bug

A project using Ionide.ProjInfo and calling Ionide.ProjInfo.Init.init starts itself -- with just --version as argument.

If ....exe --version just returns its own version an exception is thrown:

Unhandled exception. System.Exception: Unable to parse sdk version from the string 'Ionide.ProjInfo.Tool, v1.0.0.0'. [...]

(Ionide.ProjInfo.Tool, v.... is what is returned by Ionide.ProjInfo.Tool.exe --version)

But worse when the program doesn't handle --version and instead calls again the init function. Now this process again starts itself with --version. And this child starts again another child, which starts another child, which starts another child, ...: windows linux

To Reproduce

NOTE: requires DOTNET_HOST_PATH to be NOT set! (-> is reason why this is happening. Explained further below.) (Remove-Item Env:\DOTNET_HOST_PATH in powershell)

Exception:
In current master branch of ionide/proj-info:
dotnet run --framework net5.0 --project .\src\Ionide.ProjInfo.Tool\ -- --project .\src\Ionide.ProjInfo.Tool\Ionide.ProjInfo.Tool.fsproj
(starts Ionide.ProjInfo.Tool with itself as project to analyze)

Output:

Unhandled exception. System.Exception: Unable to parse sdk version from the string 'Ionide.ProjInfo.Tool, v1.0.0.0'.  
This value came from running `[...]\proj-info\src\Ionide.ProjInfo.Tool\bin\Debug\net5.0\Ionide.ProjInfo.Tool.exe System.Collections.ObjectModel.Collection`1[System.String]` at path .\src\Ionide.ProjInfo.Tool
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1433.Invoke(String message) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\printf.fs:line 1433
   at Microsoft.FSharp.Core.PrintfModule.gprintf[a,TState,TResidue,TResult,TPrinter](FSharpFunc`2 envf, PrintfFormat`4 format) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\printf.fs:line 1385
   at Ionide.ProjInfo.Init.init(DirectoryInfo workingDirectory) in E:\prog\fsharp\editor\ionide\proj-info\src\Ionide.ProjInfo\Library.fs:line 138
   at Program.parseProject(FSharpFunc`2 loaderFunc, String path) in E:\prog\fsharp\editor\ionide\proj-info\src\Ionide.ProjInfo.Tool\Program.fs:line 27
   at Program.main(String[] argv) in E:\prog\fsharp\editor\ionide\proj-info\src\Ionide.ProjInfo.Tool\Program.fs:line 84

(Ionide.ProjInfo.Tool, v.... is what is returned by Ionide.ProjInfo.Tool.exe --version)

Child bomb:

Task Manager in Details tab shows lots of ProjInfoTest processes. Probably better to use Process Explorer (from sysinternals) which shows the processes as tree (see windows image above).

Expected behaviour

It should neither throw an error nor spawn children, but instead initialize ProjInfo correctly.

Reason

ProjInfo fails to get the current dotnet version.
Exception when there's a return value which it cannot parse, recursively children when the process reaches the same location and looks for the dotnet version again.

It wants to call dotnet --version. For this it needs the current dotnet path. Which it tries to get here: https://github.com/ionide/proj-info/blob/29950f00da845efe13a9163c296000cf28abbdcf/src/Ionide.ProjInfo/Utils.fs#L6-L16
If there's DOTNET_HOST_PATH it uses that value. But otherwise it returns the current process path. Which isn't the path to dotnet, but to the current exe.

The Tests handle this case:
https://github.com/ionide/proj-info/blob/29950f00da845efe13a9163c296000cf28abbdcf/test/Ionide.ProjInfo.Tests/Program.fs#L14-L23

Based on the comment I think old dotnet versions didn't spawn the executable. But that's not the case anymore (as seen in the screenshot above: dotnet.exe spawns ProjInfoTest.exe)

Like the tests Ionide.ProjInfo.Paths.dotnetRoot should look for DOTNET_ROOT (and DOTNET_ROOT (x64). This should be set when dotnet isn't installed in default location. (Though for me it's even set for default location)

Environment:

Additional context

Isolated from fsharp/FsAutoComplete#837
But at least now the new processes are proper children and get killed when the root process (the one without --version) is finished (or killed). I think because of newer ProjInfo version.

baronfel commented 3 years ago

I'm fine looking at these environment variables in this case, it would just need to be in conjunction with the current logic I think. I believe I copied at least most of the current implementation from the .net SDK binaries themselves and/or the MSBuild launcher in the SDK, so I'm a bit topsy-turvy trying to remember the logic for each choice I made.

Booksbaum commented 3 years ago

(copy&paste from #117 because more longterm/complex strategy)

I just looked at Omnisharp. It's looking at DOTNET_ROOT, and otherwise falls back to dotnet from PATH

Looking at usages of Paths.dotnetRoot, it's currently used in two ways:

I think these two can be split apart and made independent of dotnet dir and just rely on dotnet executable (which is most likely in PATH):

For sdks dotnet currently needs a full path because sdks need its directory -> just dotnet from PATH currently doesn't work. With dotnet --list-sdks that wouldn't matter any more

baronfel commented 3 years ago

I was hoping to not parse CLI output, but it's probably the correct way forward.

Booksbaum commented 3 years ago

That's already the case with --version. But the output of --version is just the version number, --list-sdks is a bit more complex.
One downside: old dotnet versions don't support --list-sdks (not sure when it was introduced, but the first dotnets definitely didn't)

I can take care of implementing it (probably tomorrow -- lots of football (played with a round ball and feet) and football (played with egg and hands) and I usually do something on the side while watching)

Booksbaum commented 3 years ago

Fixed in #117