[Issue #33697] New standalone `info` command #36943

Markliniubility · 2023-11-16T07:22:30Z

#33697 @baronfel
Demo as below. Happy to discuss if there is any change needed:

baronfel · 2023-11-16T17:09:04Z

Hi @Markliniubility - thanks for your interest in this! I can't take a look in depth at this right now due to conference activities, but I hope to get to it early next week. Sorry for the delay there.

baronfel · 2023-11-29T16:20:19Z

src/Tests/dotnet-info.Tests/GivenThatIWantToShowInfoForDotnetCommand.cs

+            cmd.Should().Pass();
+            cmd.StdOut.Should().MatchRegex(InfoTextRegex);


consider using Verify for these tests so you can ensure that the entire format doesn't change without having to keep big literal strings in source code.

baronfel · 2023-11-29T16:20:31Z

src/Tests/dotnet-info.Tests/GivenThatIWantToShowInfoForDotnetCommand.cs

+            Assert.True(parsedJson[Sdk].Type == JTokenType.Object, "SDK should be an object.");
+            Assert.True(parsedJson[RuntimeEnv].Type == JTokenType.Object, "RuntimeEnvironment should be an object.");
+            Assert.True(parsedJson[Workloads].Type == JTokenType.Array, "Workloads should be an array.");
+        }


same here - let Verify handle JSON diffing for you

baronfel · 2023-11-29T16:22:52Z

src/Cli/dotnet/commands/dotnet-info/InfoCommand.cs

+            Reporter.Output.WriteLine($" OS Platform: {RuntimeEnvironment.OperatingSystemPlatform}");
+            Reporter.Output.WriteLine($" RID:         {GetDisplayRid(versionFile)}");
+            Reporter.Output.WriteLine($" Base Path:   {AppContext.BaseDirectory}");
+            PrintWorkloadsInfo();


This is a great start! I would love to see the information that the dotnet host provides here as well. In the current dotnet --info command that's the

Host,

.NET SDKs installed,

.NET runtimes installed,

Other architectures found,

Environment Variables, and

global.json file

sections. Especially the SDKs/Runtimes sections in a parseable format would be useful for folks.

Thank you for the feedback and apologies for the late response! I was exploring how to integrate the information into the new info command. I realized that the existing --info command have these information hard coded for printing in some C++ files in dotnet/runtime.

Though we have ways to acquire information about SDKs installed and runtimes installed from the dotnet/runtime C++ files: Interop.cs, as currently, I didn't find ways to acquire Host and Other architectures file information within dotnet/sdk. Therefore, A new standalone info command may need to make modifications to the dotnet/runtime project in this file, by adding get_environmental_variable, get_host_info, get_global_json functions.

If you happen to know any other way to get host information without making modifications to dotnet/runtime, or any existing methods within dotnet/sdk to get those information, it would be really helpful. Making changes to dotnet/runtime, which involves complex and low-level C++ code, is a significant undertaking and difficult to debug with.

Options:

Expose the missing pieces of information from corehost so SDK can utilize it via interop for this new command.

We are currently using RapidJSON in corehost to parse runtime json files. We can use it to emit info in JSON format when -f json arg (or any of its variants; --format json, --format=json) is set in a separate function like print_muxer_info_json, which will inject the host-specific parts (not very clean solution but doable..)

Without changing the corehost; spawn a process with unset unset DOTNET_ROOT; dotnet --info command, capture and parse result, convert to JSON and print. (worse than #2 because process spawning is expensive and not provided by all supported platforms)

I think the first option is preferred, but I will defer to @elinor-fung and @vitek-karas. Lets hear their thoughts. :)

I would prefer to do everything in managed code - we've discussed this in the past and the consensus was that the current design where part of the output is produced by the managed code and part of it is from native code is not what we want. Some of the downsides:

It produces partially localized output - the host parts are not localized (ever), while the SDK parts are.

It's complex to maintain (and confusing to work on).

As to what mechanism to use to expose the necessary information to managed code:

Introduce a new hostfxr API - or rather modify the existing. Effectively modify this struct to include the necessary info: https://github.com/dotnet/runtime/blob/c28bec4d3d63849c9e60dee1e7174b9a180a7e55/src/native/corehost/hostfxr.h#L311-L323.

Expose this information as runtime properties. This would have to use the new "on demand" runtime properties which are not yet wired to AppContext.GetData, so that would have to be added as well.

Personally - this feels more aligned with the hostfxr_dotnet_environment_info. But I don't feel strongly either way.

MiYanni · 2023-11-29T22:58:27Z

src/Cli/dotnet/commands/dotnet-info/InfoCommand.cs

+                versionFile.BuildRid;
+        }
+
+        public class RuntimeEnvironmentInfo


I'd put these classes that define the JSON model in a separate file.

MiYanni · 2023-11-29T22:59:55Z

src/Cli/dotnet/commands/dotnet-info/InfoCommand.cs

+
+        public class RuntimeEnvironmentInfo
+        {
+            public string Name { get; set; }


Please use JsonPropertyName to set the camelCase name style defined in baronfel's original issue.

Markliniubility · 2023-11-30T10:53:13Z

Thank you all for the comments! Working on it now and hopefully I will finish addressing them by early next week

vitek-karas · 2023-12-11T09:59:45Z

src/Cli/dotnet/commands/dotnet-info/InfoCommand.cs

+
+            public string ToJson()
+            {
+                return JsonSerializer.Serialize(this, new JsonSerializerOptions { WriteIndented = true });


For this relatively simple JSON content personally I would go with just Utf8JsonWriter. Maintaining the classes just for serialization feels like an overkill (and it will need lot of attributes to change the property names to camel case and so on).

There's also a performance consideration. Currently this uses reflection based serialization which brings in a lot of dependencies on the reflection stack which is pretty expensive. Since we're writing this as new code, we should try to make it better than that. If you decide to still go with object based serialization, then this should use the source generator. If you go with the direct JSON writer approach the problem goes away as well.

vitek-karas · 2023-12-11T10:22:15Z

Two general comments:

JSON Format
With this we're effectively signing up to maintaining the JSON format of the output backward compatible. As such we should do the necessary work to make that viable. This is mostly up to the SDK owners to decide, but personally I would prefer a short spec of the format, and an agreement on that before we go implement it. And then some good testing with the right setup to test for backward compatibility in the future. The spec should include discussion about future changes - let's say in .NET 10 we want to add something to the format - how would that look like? Do we need to version it somehow? and so on...

No-SDK dotnet --info behavior
It's possible to run dotnet --info on an installation which only has the runtime but no SDK. In that case it prints out a shorter version of the output. This output is fully produced by the native host, there's no managed code executed in that scenario.
If we were to add the JSON format to that variant as well, it would require full C++ implementation (in hostfxr).
Also goes back to the format above - in this scenario we don't have all the information, so the format would have to be able to accommodate that.

@baronfel, @elinor-fung for opinions and thoughts...

am11 · 2023-12-11T15:08:55Z

With this we're effectively signing up to maintaining the JSON format

This is a good point. In the (recent) past, there were changes made in the output of dontet --info; there is no contract.

This new command can provide a strong contract (spec, tests, versioning etc.) and maintain compatibility within the implementation, while dotnet --info can continue to freely evolve.

vitek-karas · 2023-12-11T15:13:05Z

Personally, I would like to keep the format of dotnet --info to be "free of backward compat burden". It's the thing users use to diagnose certain type of issues, and we ask them to send it to us for diagnostic purposes as well. As such I would like to keep the freedom to improve it as we see new types of problems to pop up, or as we learn about user scenarios where it can help.
It's also much easier to define a JSON based format which can evolve in backward compatible way - humanly readable text is next to impossible to evolve without breaking custom parsing logic.

baronfel · 2023-12-11T15:15:31Z

Agree in general with @vitek-karas - this new endpoint would become the documented, compatible-across-releases way to get structured information about your SDK environment. We should ask the host team to add any endpoints required to achieve data parity with dotnet --info today. We should IMO use classes to model the JSON structure to raise the bar for accidental changes to the data model.

MiYanni · 2023-12-11T19:28:45Z

@vitek-karas

It's possible to run dotnet --info on an installation which only has the runtime but no SDK.

Do you know how that interaction works with the CLI in this repo? I'm wondering... does the dotnet host ignore everything else when --info is provided? I'm fishing for a way to basically have another argument beyond --info that would be used by the SDK CLI. My current gripe is having both --info and info. Maybe we should have this new one be --info-detailed instead (or something similar)? I'm trying to make it slightly more intuitive for the user, because having --info and info do different things is really bizarre/obtuse.

baronfel · 2023-12-11T19:44:34Z

@MiYanni when --info is called with no SDK present the .NET Host can still output some information: https://github.com/dotnet/runtime/blob/main/src/native/corehost/fxr/fx_muxer.cpp#L1058

Otherwise the Host calls the --info command of the found SDK, then inserts its own details: https://github.com/dotnet/runtime/blob/main/src/native/corehost/fxr/fx_muxer.cpp#L1093-L1110

The host is only comparing via the first argument here, so theoretically new options/arguments could be added to --info. I'm not a fan of that, though, because --info is not a good command name. Options shouldn't masquerade as commands.

vitek-karas · 2023-12-11T19:54:07Z

I'm sorry - I missed that this is dotnet info and not dotnet --info.

Now that I know that... honestly I would prefer if we made these changes to dotnet --info without introducing a new syntax. Or maybe have both but one an alias of the other. I would find it really weird if the existing dotnet --info became "obsolete" and we would start suggesting people use dotnet info. I do agree that "options as commands" is not ideal, I just don't think the value is there in forcing a change.

Do you know how that interaction works with the CLI in this repo? I'm wondering... does the dotnet host ignore everything else when --info is provided?

The host recognizes --info as "Special". If it finds it, it will search for SDK (using the normal SDK lookup rules, just like if you run dotnet build). If it finds it, it runs dotnet --info as an SDK command (and prepends some text per Chet's note above). If it doesn't find any SDK, it will print out only info from the host.

am11 · 2023-12-11T21:06:23Z

If it finds it, it runs dotnet --info as an SDK command (and prepends some text per Chet's note above).

Yes, this 50-50 split in writing to console (first from C# then from C++) as part of executing dotnet --info is a bit tricky to extend. As a first step, could we change the corehost so that instead of prepending to stdout, corehost exposes that info through the interoperable API? (new API or an existing one as you suggested above?) This way the SDK would be in full control of the output. We can also teach the host to respect -f json when SDK is not available (corehost compiles with RapidJSON as is). We can then spec out the contract details for both managed and native to keep things in sync.

vitek-karas · 2023-12-11T21:45:31Z

As a first step, could we change the corehost so that instead of prepending to stdout, corehost exposes that info through the interoperable API?

I think we should do this - it would require some versioning magic but that doable (the host has to work against any SDK, so it would have to know to do the old thing for the old SDK and the new thing for the new SDK and unfortunately this can't be done through an API since the host literally runs the SDK as an app).

We can also teach the host to respect -f json when SDK is not available

Tentative yes - this is hostfxr, so size is not really a big concern (although self-contained apps would pay it without a need unfortunately). As you said we would have to spec it out and have some kind of validation to keep the host and SDK in sync on the format.

MiYanni · 2023-12-11T22:52:25Z

@baronfel

The host is only comparing via the first argument here, so theoretically new options/arguments could be added to --info. I'm not a fan of that, though, because --info is not a good command name. Options shouldn't masquerade as commands.

In the SDK CLI syntax, the command would be dotnet info. One thing I'm confused by, what is the user scenario in which they'd have the dotnet runtime but not the dotnet SDK? Meaning, how often is native dotnet --info used? We could just supersede the --info argument entirely and do a breaking change to make the command dotnet info on our side. In that situation, if the user did dotnet --info, we'd only output a warning that says the command has been deprecated and replaced with dotnet info, and a couple major SDKs down the line, remove the warning. If there is info we need from dotnet --info, there could be a JSON version created in the runtime side so we can read the information and output it within our dotnet info command. That's assuming you can call the dotnet host while running in the SDK. The user would never see that interaction, though.

Markliniubility · 2023-12-12T00:56:35Z

I think we should do this - it would require some versioning magic but that doable (the host has to work against any SDK, so it would have to know to do the old thing for the old SDK and the new thing for the new SDK and unfortunately this can't be done through an API since the host literally runs the SDK as an app).

Therefore, correct me if I am wrong, here are some steps we need to implement this new standalone info command.

Corehost exposes that info through the interoperable API (it is complicated as verisoning is a issue)
Use that information and make it into json/textual stdout in dotnet info within SDK
Deprecate message on the original --info command

As for now, shall I wait for the completion of 1 by corehost team, or shall I look into the corehost and potentially submit a PR (which I am not very certain how to make the versioning magic work..)

vitek-karas · 2023-12-15T11:18:56Z

We'd be happy to accept a PR for this in the corehost, but I agree that there's some design work to be done first. I think a good start would be to open an issue in the runtime repo to discuss how to expose the additional information (should be relatively simple API change), and then another one how to avoid producing the "host" part of dotnet --info when running with a new SDK which would do all of it from managed code (this will need the design work).

Please be patient with us for the next 2 weeks as lot of people are on holidays.

Markliniubility · 2024-02-21T06:05:33Z

Sorry for the delay (I was quite unsure about what to change in the runtime codebase and how to test my change): I just brought up the issues in dotnet runtime. dotnet/runtime#98735.

richlander · 2024-02-21T21:29:35Z

I really like how docker cli works with go format templates. This allows a lot of flexibility. In that experience, regular console text is the default. If you take a dependency on that layout, then caveat emptor. However, go format templates allow a ton of control over the underlying data, both inclusion/exclusion and format (text, table, JSON). The contract then transitions to the underlying data and not to its presentation in any form. It also offers a ton of flexibility. This is similar to ASP.NET Core auto serialization (XML, JSON, ...). Having something like that as an underlying CLI primitive would be amazing, generally, and would resolve a bunch of the concerns (I think).

marcpopMSFT · 2024-10-15T22:00:39Z

Old PR triage: @baronfel not sure where we landed with having a separate command here.

baronfel · 2024-10-15T22:02:03Z

We do want this separate command, but the linked runtime issue needs to be fleshed out to fully replace --info with this more full-featured command.

Initial Commit: functionality wise working

25e732a

dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Nov 16, 2023

Markliniubility added 5 commits November 16, 2023 13:57

Initial Test Writing

d9521c0

Update GivenThatIWantToShowHelpForDotnetCommand.cs

07dee66

Testing on Texting

fff2401

Json testing

c4820be

unit test fix

f2d7cce

baronfel requested a review from a team November 28, 2023 21:24

baronfel reviewed Nov 29, 2023

View reviewed changes

MiYanni reviewed Nov 29, 2023

View reviewed changes

vitek-karas reviewed Dec 11, 2023

View reviewed changes

nagilson requested a review from baronfel February 20, 2024 21:28

Markliniubility mentioned this pull request Feb 23, 2024

[API Proposal]: Expose dotnet info by introducing a new hostfxr API dotnet/runtime#98735

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #33697] New standalone `info` command #36943

[Issue #33697] New standalone `info` command #36943

Markliniubility commented Nov 16, 2023 •

edited

Loading

baronfel commented Nov 16, 2023

baronfel Nov 29, 2023

baronfel Nov 29, 2023

baronfel Nov 29, 2023

Markliniubility Dec 10, 2023 •

edited

Loading

am11 Dec 11, 2023 •

edited

Loading

Markliniubility Dec 11, 2023

vitek-karas Dec 11, 2023

MiYanni Nov 29, 2023

MiYanni Nov 29, 2023

Markliniubility commented Nov 30, 2023

vitek-karas Dec 11, 2023

vitek-karas commented Dec 11, 2023

am11 commented Dec 11, 2023

vitek-karas commented Dec 11, 2023

baronfel commented Dec 11, 2023

MiYanni commented Dec 11, 2023

baronfel commented Dec 11, 2023

vitek-karas commented Dec 11, 2023

am11 commented Dec 11, 2023

vitek-karas commented Dec 11, 2023 •

edited

Loading

MiYanni commented Dec 11, 2023

Markliniubility commented Dec 12, 2023

vitek-karas commented Dec 15, 2023

Markliniubility commented Feb 21, 2024

richlander commented Feb 21, 2024

marcpopMSFT commented Oct 15, 2024

baronfel commented Oct 15, 2024

		cmd.Should().Pass();
		cmd.StdOut.Should().MatchRegex(InfoTextRegex);

[Issue #33697] New standalone info command #36943

Are you sure you want to change the base?

[Issue #33697] New standalone info command #36943

Conversation

Markliniubility commented Nov 16, 2023 • edited Loading

baronfel commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Markliniubility Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

am11 Dec 11, 2023 • edited Loading

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

Markliniubility commented Nov 30, 2023

Choose a reason for hiding this comment

vitek-karas commented Dec 11, 2023

am11 commented Dec 11, 2023

vitek-karas commented Dec 11, 2023

baronfel commented Dec 11, 2023

MiYanni commented Dec 11, 2023

baronfel commented Dec 11, 2023

vitek-karas commented Dec 11, 2023

am11 commented Dec 11, 2023

vitek-karas commented Dec 11, 2023 • edited Loading

MiYanni commented Dec 11, 2023

Markliniubility commented Dec 12, 2023

vitek-karas commented Dec 15, 2023

Markliniubility commented Feb 21, 2024

richlander commented Feb 21, 2024

marcpopMSFT commented Oct 15, 2024

baronfel commented Oct 15, 2024

[Issue #33697] New standalone `info` command #36943

[Issue #33697] New standalone `info` command #36943

Markliniubility commented Nov 16, 2023 •

edited

Loading

Markliniubility Dec 10, 2023 •

edited

Loading

am11 Dec 11, 2023 •

edited

Loading

vitek-karas commented Dec 11, 2023 •

edited

Loading