report nice error message when dotnet is not installed #5477
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| } catch (error: any) { | ||
| if (error && "code" in (error as {}) && error["code"] === "ENOENT") { | ||
| reportDiagnostic(sdkContext.program, { | ||
| code: "dependency-runtime-missing", |
There was a problem hiding this comment.
i think runtime-dependency-missing makes more sense.
Has this code been reviewed?
There was a problem hiding this comment.
update to runtime-dependency-missing. The code is for csharp-emitter only. if needed, we can review by .net arch.
| throw new Error(`Failed to generate SDK. Exit code: ${result.exitCode}`); | ||
| } | ||
| } catch (error: any) { | ||
| if (error && "code" in (error as {}) && error["code"] === "ENOENT") { |
There was a problem hiding this comment.
Is this too general of a condition? Is there a way to make it specific to dotnet being missing?
There was a problem hiding this comment.
Also, don't we need to ensure that .NET 8 is installed? What would happen with the current approach if .NET 6 is installed.
dotnet --list-sdks will output the list of installed SDKs. We should check that they have greater than or equal to the version we use in the global.json.
There was a problem hiding this comment.
hello @JoshLove-msft as discussed before, we will not do runtime time check every time when we generate sdk, we just check the error if it is failed because of missing .NET and give out a nice error message.
And we ask .NET 8 in the message to consistent with the requirement we write in the Readme.md of the library.
There was a problem hiding this comment.
Discussed in the meeting, we will do runtime check when generation fail, and identify if is because of dotnet missing. I will update it. Thanks
| code: "runtime-dependency-missing", | ||
| format: { | ||
| dotnetMajorVersion: "8", | ||
| downloadUrl: "https://dotnet.microsoft.com/en-us/download", |
There was a problem hiding this comment.
| downloadUrl: "https://dotnet.microsoft.com/en-us/download", | |
| downloadUrl: "https://dotnet.microsoft.com/download/dotnet/8.0", |
There was a problem hiding this comment.
Need to confirm with .NET folks that latest patch from major version is fine (as opposed to matching the version we use in global.json exactly).
There was a problem hiding this comment.
Hello @JoshLove-msft We just need user to install .NET 8 or above, (consistent with the requirement written in readme.md), so we provide a url to download .NET , not a dotnet 8 specific.
There was a problem hiding this comment.
After discussion, we decide to suggest user to use the exact version in global.json, and provide the dotnet 8 download uri. Thanks.
Fix #5364
dependency-runtime-missingdiagnosticdependency-runtime-missingdiagnostic error