report nice error message when dotnet is not installed or not valid #5477

chunyu3 · 2025-01-03T05:01:46Z

Fix #5364

add invalid-runtime-dependency diagnostic
When dotnet generation fail, check dotnet to filter out the failure because of runtime dependency is not valid
- dotnet sdk is not installed
- the installed dotnet sdk is not meet with the version requirement.

azure-sdk · 2025-01-03T05:11:22Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

@typespec/http-client-csharp

packages/http-client-csharp/emitter/src/emitter.ts

…5364

packages/http-client-csharp/emitter/src/emitter.ts

JoshLove-msft

Added a few questions

packages/http-client-csharp/emitter/src/emitter.ts

…5364

JoshLove-msft · 2025-01-14T23:50:19Z

packages/http-client-csharp/emitter/src/emitter.ts

+ * @param program The typespec compiler program
+ * @param minVersionRequisite The minimum required version
+ */
+async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {


Can we add a unit test of this function?

JoshLove-msft · 2025-01-14T23:50:27Z

packages/http-client-csharp/emitter/src/emitter.ts

+ * @param program The typespec compiler program
+ * @param minVersionRequisite The minimum required version
+ */
+async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {


Suggested change

async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {

async function validateDotNet(program: Program, minVersionRequisite: string): Promise<boolean> {

JoshLove-msft · 2025-01-14T23:51:57Z

packages/http-client-csharp/emitter/src/emitter.ts

@@ -7,6 +7,7 @@ import {
  getDirectoryPath,
  joinPaths,
  logDiagnostics,
+  NoTarget,


Where is this used?

JoshLove-msft · 2025-01-14T23:52:21Z

packages/http-client-csharp/emitter/src/emitter.ts

+  configurationFileName,
+  minVersionRequisiteForDotnet,
+  tspOutputFileName,
+} from "./constants.js";


Should this be importing from the ts file?

JoshLove-msft · 2025-01-14T23:56:17Z

packages/http-client-csharp/emitter/src/emitter.ts

+    const dotnetVersions = findDonetVersion(result.stdout) ?? findDonetVersion(result.stderr);
+    if (dotnetVersions) {
+      for (const version of dotnetVersions) {
+        if (semver.gt(version, minVersionRequisite)) return true;


Not sure that it is worth using the semver dependency for this simple check. We can even make the check simpler if we don't accept an arbitrary minVersion parameter and just check that the major version is 8.

JoshLove-msft

Added a few additional comments, but overall looks like a nice improvement to the UX.

jsquire · 2025-01-15T00:34:59Z

packages/http-client-csharp/emitter/src/constants.ts

@@ -7,3 +7,4 @@ export const projectedNameClientKey = "client";
 export const mockApiVersion = "0000-00-00";
 export const tspOutputFileName = "tspCodeModel.json";
 export const configurationFileName = "Configuration.json";
+export const minVersionRequisiteForDotnet = "8.0.0";


Suggested change

export const minVersionRequisiteForDotnet = "8.0.0";

export const minSupportedDotNetSdkVersion = "8.0.0";

The name of this reads oddly and makes it hard to understand the purpose.

jsquire · 2025-01-15T00:38:36Z

packages/http-client-csharp/emitter/src/emitter.ts

+            if (isValid) {
+              if (result.stderr) Logger.getInstance().error(result.stderr);
+              if (result.stdout) Logger.getInstance().verbose(result.stdout);
+              throw new Error(`Failed to generate SDK. Exit code: ${result.exitCode}`);


Suggested change

throw new Error(`Failed to generate SDK. Exit code: ${result.exitCode}`);

throw new Error(`Failed to generate the library. Exit code: ${result.exitCode}`);

We generate .NET libraries. The "Azure SDK" is a branding for the entire collection of libraries. Even in the unbranded 3rd party space, we should stick to this terminology.

jsquire · 2025-01-15T00:40:50Z

packages/http-client-csharp/emitter/src/emitter.ts

+ * @param program The typespec compiler program
+ * @param minVersionRequisite The minimum required version
+ */
+async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {


Suggested change

async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {

async function validateDotNetSdk(program: Program, minVersion: string): Promise<boolean> {

We should be clear that we're validating the .NET SDK installed.

jsquire · 2025-01-15T00:42:11Z

packages/http-client-csharp/emitter/src/emitter.ts

+        if (semver.gt(version, minVersionRequisite)) return true;
+      }
+      reportDiagnostic(program, {
+        code: "invalid-runtime-dependency",


This code is misleading, as we're validating the .NET SDK, not one of the runtimes.

jsquire · 2025-01-15T00:44:17Z

packages/http-client-csharp/emitter/src/emitter.ts

+    return false;
+  }
+  try {
+    const result = await execAsync("dotnet", ["--list-sdks"]);


I don't think this is the right check. We could see .NET 8 as installed but have a global.json in the filesystem forcing us to use an earlier version. This check would pass, but we wouldn't be using v8+.

We should be running dotnet --version and evaluating the version used. If not otherwise constrained, this will be the latest on the system.

jsquire · 2025-01-15T00:52:34Z

packages/http-client-csharp/emitter/src/emitter.ts

+          );
+          if (result.exitCode !== 0) {
+            const isValid = await validateDotnet(sdkContext.program, minVersionRequisiteForDotnet);
+            // if the dotnet runtime is valid, the error is not runtime issue, log it as normal


This comment is misleading. We're working with the .NET SDK, which is very different than a .NET runtime.

jsquire · 2025-01-15T00:53:53Z

packages/http-client-csharp/emitter/src/lib/lib.ts

@@ -37,6 +37,14 @@ const $lib = createTypeSpecLibrary({
        default: paramMessage`${"message"}`,
      },
    },
+    "invalid-runtime-dependency": {


Suggested change

"invalid-runtime-dependency": {

"invalid-dotnet-sdk-dependency": {

jsquire · 2025-01-15T00:54:14Z

packages/http-client-csharp/emitter/src/lib/lib.ts

+    "invalid-runtime-dependency": {
+      severity: "error",
+      messages: {
+        default: paramMessage`invalid runtime dependency installed.`,


This is incorrect. We're talking about the .NET SDK, not a runtime. Those are different things.

jsquire · 2025-01-15T00:54:55Z

packages/http-client-csharp/emitter/src/lib/lib.ts

+      severity: "error",
+      messages: {
+        default: paramMessage`invalid runtime dependency installed.`,
+        missing: paramMessage`Dotnet is not found in PATH. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,


Suggested change

missing: paramMessage`Dotnet is not found in PATH. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,

missing: paramMessage`The dotnet command was not found in the PATH. Please install the .NET SDK version ${"dotnetMajorVersion"} or above. Guidance for installing the .NET SDK can be found at ${"downloadUrl"}"`,

We should not be linking to the download, but to the .NET SDK product page, which has instructions for installing in different environments and covers standard mechanisms like package managers, which are preferable to direct download.

jsquire · 2025-01-15T00:56:28Z

packages/http-client-csharp/emitter/src/lib/lib.ts

+      messages: {
+        default: paramMessage`invalid runtime dependency installed.`,
+        missing: paramMessage`Dotnet is not found in PATH. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,
+        invalidVersion: paramMessage`Dotnet SDK in PATH is version ${"installedVersion"}. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,


Suggested change

invalidVersion: paramMessage`Dotnet SDK in PATH is version ${"installedVersion"}. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,

invalidVersion: paramMessage`The .NET SDK found is version ${"installedVersion"}. Please install the .NET SDK ${"dotnetMajorVersion"} or above and ensure there is no global.json in the file system requesting a lower version. Guidance for installing the .NET SDK can be found at ${"downloadUrl"}"`,

We should not be linking to the download, but to the .NET SDK product page, which has instructions for installing in different environments and covers standard mechanisms like package managers, which are preferable to direct download.

report nice error message when dotnet is not installed

81589b3

chunyu3 requested review from m-nash and JoshLove-msft as code owners January 3, 2025 05:01

microsoft-github-policy-service bot added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Jan 3, 2025

ArcturusZhang reviewed Jan 3, 2025

View reviewed changes

packages/http-client-csharp/emitter/src/emitter.ts Outdated Show resolved Hide resolved

change the diagonosic code

57ca9c0

archerzz approved these changes Jan 3, 2025

View reviewed changes

Merge branch 'main' of https://github.com/microsoft/typespec into Fix…

e68c4d9

…5364

chunyu3 requested a review from ArcturusZhang January 3, 2025 06:50

Merge branch 'main' into Fix5364

176c45f

JoshLove-msft reviewed Jan 7, 2025

View reviewed changes

packages/http-client-csharp/emitter/src/emitter.ts Outdated Show resolved Hide resolved

JoshLove-msft requested changes Jan 7, 2025

View reviewed changes

JoshLove-msft reviewed Jan 7, 2025

View reviewed changes

packages/http-client-csharp/emitter/src/emitter.ts Outdated Show resolved Hide resolved

chunyu3 added 3 commits January 8, 2025 20:03

Merge branch 'main' of https://github.com/microsoft/typespec into Fix…

3adef56

…5364

check dotnet runtime when generate fail

a6d54ca

use semver to parse and compare version

4dee9a5

chunyu3 changed the title ~~report nice error message when dotnet is not installed~~ report nice error message when dotnet is not installed or not valid Jan 13, 2025

Merge branch 'main' into Fix5364

80bc6c7

chunyu3 requested a review from JoshLove-msft January 14, 2025 01:55

JoshLove-msft reviewed Jan 14, 2025

View reviewed changes

JoshLove-msft requested changes Jan 14, 2025

View reviewed changes

jsquire requested changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report nice error message when dotnet is not installed or not valid #5477

report nice error message when dotnet is not installed or not valid #5477

chunyu3 commented Jan 3, 2025 •

edited

Loading

azure-sdk commented Jan 3, 2025

JoshLove-msft left a comment

JoshLove-msft Jan 14, 2025

JoshLove-msft Jan 14, 2025

JoshLove-msft Jan 14, 2025

JoshLove-msft Jan 14, 2025

JoshLove-msft Jan 14, 2025

JoshLove-msft left a comment

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

jsquire Jan 15, 2025

	async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {
	async function validateDotNet(program: Program, minVersionRequisite: string): Promise<boolean> {

	export const minVersionRequisiteForDotnet = "8.0.0";
	export const minSupportedDotNetSdkVersion = "8.0.0";

	throw new Error(`Failed to generate SDK. Exit code: ${result.exitCode}`);
	throw new Error(`Failed to generate the library. Exit code: ${result.exitCode}`);

	async function validateDotnet(program: Program, minVersionRequisite: string): Promise<boolean> {
	async function validateDotNetSdk(program: Program, minVersion: string): Promise<boolean> {

	"invalid-runtime-dependency": {
	"invalid-dotnet-sdk-dependency": {

	missing: paramMessage`Dotnet is not found in PATH. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,
	missing: paramMessage`The dotnet command was not found in the PATH. Please install the .NET SDK version ${"dotnetMajorVersion"} or above. Guidance for installing the .NET SDK can be found at ${"downloadUrl"}"`,

	invalidVersion: paramMessage`Dotnet SDK in PATH is version ${"installedVersion"}. Please install DotNet ${"dotnetMajorVersion"} or above. Dotnet can be downloaded from ${"downloadUrl"}"`,
	invalidVersion: paramMessage`The .NET SDK found is version ${"installedVersion"}. Please install the .NET SDK ${"dotnetMajorVersion"} or above and ensure there is no global.json in the file system requesting a lower version. Guidance for installing the .NET SDK can be found at ${"downloadUrl"}"`,

report nice error message when dotnet is not installed or not valid #5477

Are you sure you want to change the base?

report nice error message when dotnet is not installed or not valid #5477

Conversation

chunyu3 commented Jan 3, 2025 • edited Loading

azure-sdk commented Jan 3, 2025

JoshLove-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshLove-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunyu3 commented Jan 3, 2025 •

edited

Loading