dotnet --info should indicate invalid global.json

[matkoch]

Describe the bug

dotnet --version completes fine even when global.json is malformed JSON.

To Reproduce

Make global.json malformed and execute dotnet --version.

Exceptions (if any)

Should exit with non-zero exit code. Instead:

$ dotnet --version
A JSON parsing exception occurred in [/Users/matt/code/nuke/external/functions/global.json], offset 36 (line 3, column 24): Invalid encoding in string.
6.0.100

$ echo $?
0

Further technical details

MacOS

[nohwnd]

This is not related to dotnet test.

Other commands return 1 as the exit code, which seems appropriate:

C:\t\mstest31> dotnet build --foo; $LASTEXITCODE
Microsoft (R) Build Engine version 17.2.0-preview-22062-10+32e260b12 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

MSBUILD : error MSB1001: Unknown switch.
Switch: --foo

For switch syntax, type "MSBuild -help"
1 # <----
C:\t\mstest31> dotnet test --foo; $LASTEXITCODE
MSBUILD : error MSB1001: Unknown switch.
Switch: --foo

For switch syntax, type "MSBuild -help"
1 # <----

@marcpopMSFT Is returning 1 as exit code correct, or is 0 returned on purpose?

[nohwnd]

The same thing happens when other commands are run (e.g. dotnet run, dotnet build, dotnet new, dotnet test), this seems to be a bigger issue, and should exit the command prematurely, because it ends up running using a different SDK than intended.

# file global.json
{
  "sdk": {
    "version": "6.0.100"
  # } is missing here
}

C:\t\mstest40> dotnet build
A JSON parsing exception occurred in [C:\t\mstest40\global.json], offset 39 (line 4, column 2): Missing a comma or '}' after an object member.
Microsoft (R) Build Engine version 17.2.0-preview-22062-10+32e260b12 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  mstest40 -> C:\t\mstest40\bin\Debug\net7.0\mstest40.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.03
C:\t\mstest40> dotnet --version
A JSON parsing exception occurred in [C:\t\mstest40\global.json], offset 39 (line 4, column 2): Missing a comma or '}' after an object member.
7.0.100-preview.2.22074.5

[vrushalivartak]

Hi @livarcocc,
Below is my analysis looking at the source code of dotnet sdk and runtime. I am no expert in this area. Let me know if I am going in right direction or missing any point.
As I understand, it is not dotnet sdk but the dotnet runtime that checks for global.json at startup and throws error - "A JSON parsing exception occurred...". Due to this error, not being able to retrieve the SDK settings from global.json, dotnet runtime falls back to default i.e. it uses latest installed .Net SDK.
'dotnet --version' simply returns the product version (from .version file if present or the assembly's fileversioninfo) which it does successfully and exits with zero. And the behavior is the same for other dotnet commands as well. In case of malformed global.json, all the commands are executed successfully using latest installed .Net SDK.

What is expected here? to handle this situation in dotnet.exe code? If so, a multi-level lookup of global.json is needed. hostfxr_resolve_sdk2 function of runtime does exactly that. It can be invoked via ResolveNETCoreSdkDirectory function of a wrapper class NETCoreSdkResolver. However it returns no information of 'global.json found but malformed'. It treats it as if global.json not found. Hence a change would be needed in hostfxr_resolve_sdk2 runtime function.

hostfxr_resolve_sdk2 runtime function currently invokes callback with a key-value pair, following 3 possible keys,
resolved_sdk_dir
global_json_path
requested_version
We will need a fourth one, or somehow be able to convey the malformed_json information in the value.

The question is,
should this situation be handled in dotnet.exe code? Or should it be handled in runtime itself?
Afterall, the purpose of global.json is for user to be able to convey to runtime the specific SDK settings. If global.json is present which means user expected global.json settings to take into effect. And hence if global.json itself is malformed, should runtime simply throw error and exit instead of print warning and continue.

Runtime also invokes the same function hostfxr_resolve_sdk2 at startup. Hence if my analysis is correct, then in any case, the code change will be needed in hostfxr_resolve_sdk2 function of runtime.

[nohwnd]

I don't think livarcocc will help here anymore :)

The code responsible for the resolving is here:

https://github.com/dotnet/runtime/blob/main/src/native/corehost/fxr/sdk_resolver.cpp#L164-L172

and tests are here: https://github.com/dotnet/runtime/blob/main/src/installer/tests/HostActivation.Tests/SDKLookup.cs

Accommodating for multiple other cases of how the file can be broken.

@vitek-karas is this continuing by design, or should be closed because we cannot change it now? Or the behavior should be changed to exit with exit code?

[vitek-karas]

This behavior has been in .NET since forever - the inital commit which introduced the support for global.json already had that behavior: https://github.com/dotnet/core-setup/blob/3a8102572c134462b3e44fef7ac0446c27183fe7/cli/fxr/fx_muxer.cpp#L94

I do agree that it's not an ideal behavior - for one we're printing to stderr without actually failing (which we try to avoid wherever possible). On the other hand, this would be a breaking change - and in the global component hostfxr which is not "side by side" (as in, the latest version is used for all invocations of dotnet.exe on the machine) - so installing a new version of .NET with such a change would change it for all invocations of dotnet.exe on the machine - with no way to workaround (other than avoid installing new versions).

I don't think it's worth the impact of the change to fix this - this is a first issue on the topic I've seen in the last 4 years (at least, probably more), so it doesn't seem to be a common pain point.

Personally I would be in favor going even further with the current behavior - not write into stderr in this case, to follow our guidance everywhere else, which is that the host must not write to stderr unless it actually terminates immediately afterwards (the reason for this is that the host should not pollute the stdout/stderr of the app it's going to run - even if the app is the SDK, as the caller may rely on ability to parse the output).

@elinor-fung what do you think?

[elinor-fung]

I agree that the impact of changing the behaviour to actually fail here doesn't seem worth it.

I think removing the write to stderr (switching it to tracing that has to be turned on) is reasonable. But that does lose the clear indication that a global.json was found and parsing failed. Maybe as part of that, the output of dotnet --info can indicate invalid global.json. Right now, it is reported as no global.json (which matches how we behave in that case):

global.json file:
  Not found

Maybe something like:

global.json file:
  Invalid [C:\repos\global.json]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Describe the bug

dotnet --version completes fine even when global.json is malformed JSON.

To Reproduce

Make global.json malformed and execute dotnet --version.

Exceptions (if any)

Should exit with non-zero exit code. Instead:

$ dotnet --version
A JSON parsing exception occurred in [/Users/matt/code/nuke/external/functions/global.json], offset 36 (line 3, column 24): Invalid encoding in string.
6.0.100

$ echo $?
0

Further technical details

MacOS

Author:	matkoch
Assignees:	-
Labels:	area-System.Text.Json, good first issue
Milestone:	-