dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.37k stars 378 forks source link

Integration with GenericHost #440

Closed haefele closed 5 years ago

haefele commented 5 years ago

Are there any plans on how to integrate this with the GenericHost from Microsoft.Extensions.Hosting?

I mean all the functionality the GenericHost provides like the hosting environment, logging, DI, configuration - would still be useful when creating a command-line API.

I saw that you can add your own registrations to the BindingContext.ServiceProvider - but I'm not quite sure if that's the right way to integrate the GenericHost with this library.

jonsequitur commented 5 years ago

It's been brought up a couple of times but not looked at closely. The service registration in BindingContext is still a work in progress. How might you imagine the integration working?

loic-sharma commented 5 years ago

@natemcmaster's fork of CommandLineUtils has integration with generic hosts: https://natemcmaster.github.io/CommandLineUtils/docs/advanced/generic-host.html

The highlights are:

  1. There's an extension method on IHostBuilder to run the command line app
  2. Dependency injection works with the command handlers
fredrikhr commented 5 years ago

I concluded in #533 that adding the generic can neatly be done as an Invocation Middleware. This means that command-handlers can accept parameters of type IHost and continue from there. E.g. a handler can get the Host DI-container from the IHost.Services property. This becomes especially nice when using the Host.CreateDefaultBuilder() factory method coming in Microsoft.Extensions.Hosting version 3.0 (currently in preview).

Actually there are some things to be said for separating the command-line parser services from the host. Take a look at #533 where I have done that, but provided the ability to bridge that separation if necessary.

The approach is similar to how the generic host separates host configuration and app configuration. I.e. the generic host actually first builds a Configuration for the host, and then injects that into the application DI-container. I create the Host and start it, and then add it to the invocation binding context.

If you do the Generic Host as a command-line invocation middleware, you still get the benefits of being able to do --help short-circuiting different behaviours depending on commands, etc, and it does so without having to jump through too many hoops.

alexdresko commented 5 years ago

@couven92 Now you just have to get your branch to pass the CI build. :)

haefele commented 5 years ago

@couven92 That is basically what I ended up doing as well - just not as well organized. Good job! :smile:

fredrikhr commented 5 years ago

533 is now merged, so now you can use UseHost to very easily and conveniently make use of the Generic Host. This will become even better if you use the Host.CreateDefaultBuilder method from the 3.0.0-preview5-* version of the Microsoft.Extensions.Hosting package on NuGet.

alexdresko commented 5 years ago

@couven92 Besides using .NET Core 3, and the latest command line nuget packages, what code changes need to be made to the generic host sample app for this integration to work?

I tried to figure it out on my own, but ran out of time. I'm a little confused about what to do.

fredrikhr commented 5 years ago

@alexdresko checkout https://github.com/couven92/dotnet-command-line-api/tree/hosting-sample/samples/HostedConsole for a small example using .NET Core 3.0 together with the new hosting API.

Or checkout using the following git command

git clone --depth 1 --branch hosting-sample https://github.com/couven92/dotnet-command-line-api.git -- couven92-dotnet-command-line-api

And navigate to the samples/HostedConsole subfolder, or simply open up the Solution.

fredrikhr commented 5 years ago

Note that System.CommandLine.Hosting also works with .NET Core 2.2. You'll just have to go through the pain of configuring the host yourself instead of being able to use the awfully convenient Host.CreateDefaultBuilder. Although, on the other hand, using Microsoft.Extensions.Hosting version 2.2.0 (which targets .NET Core 2.2) has a much smaller dependency graph exactly because it does not add anything to the HostBuilder.

EugeneKrapivin commented 5 months ago

After going down the rabbit hole, it seems that the UseHost method (which doesn't support the Host.CreateApplicationBuilder(args); host builder out of the box) still doesn't really use the DI container built by the host, from what I understood I have to inject the IHost into the command and use service provider to locally resolve dependencies.

Will you tackle this matter as part of #2341?

Back story: I'm playing around with an internal CLI tool which needs the Host (I'm using http factories, access token management, telemetry, multiple background services, channels) and I failed to find a community best practice to build commands which leverage the Host DI. I eventually resorted to building a custom pattern of command classes which get DI resolved in an IHostedService, with which I build up the RootCommand using closures to capture the command class which got all it needs injected from the Host DI. This whole approach seems a bit... yucky to be frank :D

Once you have real tight integration with Generic hosts System.CommandLine would be the first CLI toolkit to do so.

fredrikhr commented 5 months ago

I (the original author of System.CommandLine.Hosting) am currently looking into how to best support Host.CreateApplicationBuilder.

What you need to do is first acknowledge that command line argument parsing happens before the IHost is created. Therefore, CliCommand creation and parsing cannot be part of the Host DI system.

Instead you'll use the parsing result from System.CommandLine to feed into Options instances used by the IHost application like you're used to. But instead of binding from a json file, you bind from the ParseResult. For simple scenarios, there is an extension method BindCommandLine() on the OptionsBuilder.

Lastly, you'll want to use CommandHandler.Create in the assignment to your CliCommand.Action property. As an argument to create use a delegate with one argument of type IHost.

If you're using hosted services, the IHost should automatically start your service when your command handler is invoked. In that case simply call host.WaitForShutdown in your handler.

fredrikhr commented 5 months ago

@EugeneKrapivin so the Program.cs containg a rough outline of a hosted service with System.CommandLine would look like this:

using System.CommandLine;
using System.CommandLine.Hosting;
using System.CommandLine.NamingConventionBinder;

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

CliRootCommand cliRoot = new()
{
    Action = CommandHandler.Create(Run),
};
CliArgument<string> hostedServiceArg = new("myHostedServiceArg");
cliRoot.Arguments.Add(hostedServiceArg);

var cliConfig = new CliConfiguration(cliRoot);
cliConfig.UseHost(Host.CreateDefaultBuilder, host =>
{
    host.ConfigureServices((context, services) =>
    {
        //services.AddOptions<MyHostedServiceOptions>()
        //    .Configure<ParseResult>((opts, parseResult) =>
        //    {
        //        opts.Argument = parseResult.GetValue(hostedServiceArg);
        //    });
        //services.AddHostedService<MyHostedService>();
    });
});
return await cliConfig.InvokeAsync(args ?? Array.Empty<string>())
    .ConfigureAwait(continueOnCapturedContext: false);

static Task Run(IHost host) => host.WaitForShutdownAsync();
EugeneKrapivin commented 5 months ago

@fredrikhr thanks for your swift response :)

I saw this example in the Samples folder. I understand that this is the current design of the library. I was thinking along the lines of

// this will register all required components into the host DI container
builder.Services.AddCommandLineCli(root=> {
    root.AddCommand<MyCommand>(commandConfig => {...});
    // other configs
});
// ...
var host = builder.Build();

host.UseCommandCliService(args); // BackgroundService/HostedService with application lifecycle control in mind (i.e. shutdown host on help handler). On execute this host would parse the args, build the commands from factories feeding from the hosts IServiceProvider...

//...

await host.RunAsync(args);

it seems that mainly the restriction is stemming from the BindingContext being created as part of the InvocationContext, I think if it'd been a scope IServiceProvider things could have played a bit nicer.

btw I'm actually using the v2 beta nugets, I'm guessing those are part of the Powderhouse project?

fredrikhr commented 5 months ago

@EugeneKrapivin your proposal basically turns the model upside down. I.e. Making System.CommandLine a component of the application, instead of the current model where Hosting is a middleware of System.CommandLine. System.CommandLine does more when you call Invoke than what happens when you run Host.RunAsync. Therefore, the current model makes more sense and truly offers the best of both worlds. You strip away a lot of functionality if you reduce it the way you suggest. Also you loose some functionality in Ms.Ext.Hosting that way, while UseHost aims to preserve that all functionality in both Sys.Commandline and Ms.Ext.Hosting is usable without compromise.

EugeneKrapivin commented 5 months ago

@fredrikhr Well, I do not know all the functionality of the library, I assume I miss a lot of the capabilities =)

While having Sys.CommandLine the main host (or simply the only host) is a great way to go, it does feel wrong in my use-case to run a host with 2 BackgroundServices when you need HttpClientFactory with TokenManagement and some in process data processing... and essentially all I'm doing is a bunch of API calls analyze the responses and reduce them into a report. seems like a classic use-case

I mean, in my mind the CLI parser and command invoker should be the frontend of the command line process, not the main host. a bit like the minimal APIs/controllers are the frontend into the logic of my services.

What functionality you think would not fit well into the "GenericHost first" model?

fredrikhr commented 5 months ago

@EugeneKrapivin non exhaustive list:

  1. HostBuilder currently takes a string array in its ctor and uses these to configure the Host and assigns a source for IConfiguration. This will become unsable since it only supports a tiny subset of what is parseable by Sys.CommandLine and throws else. Therefore, you must pass an empty array to HostBuilder and parse the actual commandline with Sys.CommandLine. In comparison, the UseHost method takes the actual commandline arguments and parses these and then passes the unrecognised arguments further to the HostBuilder ctor (thus also allowing to configure the host by commandline arguments, e.g. setting Environment, Content Path, and some internals of the Hosting library)
  2. All other middleware in the Invocation pipeline of System.Commandline will not work properly. I.e. you must not call the Invoke methods on ony of the Sys.Commandline types.
  3. It makes the separation between the Sys.Commandline service provider and the Hosting service provider even less obvious. Any model binding used in Sys.Commandline will not work with the DI of Hosting and vice versa. That is true already, but makes more sense in the current model as CliBindingContext model binder happens before the Hosting DI even is created or initialized.
  4. Short circuiting the DI setup when --help --version or suggestion is specified on the command line (or anything else that would not use DI) will not work, you'll always build up the DI container, even if not needed.
  5. IConfiguration using the [config] commandline directive as source will not work.
EugeneKrapivin commented 5 months ago

I see... seems like making the library work as part of the Generic Host pattern of .NET would be a huge undertaking. Thanks for your time :)

fredrikhr commented 5 months ago

When it comes to class design, I usually create MyAppCommandDefinition classes (either just one, or one per subcommand) containing the CliCommand, Options and Arguments as fields. Then I create ConfigureHost, ConfigureServies and ConfigureOptions methods. Then I create MyAppCommandRunner classes that are added as services to host DI (and use ctor injection from Host DI). Then, back in the Definition class I add a CommandHandler method, with one IHost argument and then use host.Services.GetRequiredService() to get an instance of the runner for the command and then finally call a Run method on the runner.

The definition class is simply a container to store the command and its options and arguments, this is purely useful for the ConfigureOptions method (or if Options are not used to get the option values inside the runner).

Then the runner class is purely a Hosting DI service class and its Run method is what actually runs a command. It works as a lightweight inline IHostedService. A hosted service will run in the background until something triggers the app to shut down. The Runner will run and after the run method returns, the host will shutdown. Pick whatever works for you. If using only hosted services you'll use host.WaitForShutdown in the CommandHandler method. If using a Runner class, you'll wait for the runners run method to complete.

EugeneKrapivin commented 5 months ago

Sounds like what I came up with eventually, I think. :) I have multiple commands implementing

public interface ICommand
{
    string Description { get; }
    string Name { get; }
    Command CreateCommand();
}

The constructors have all the dependencies required to execute the command

    public AnalyzeCommand(Channel<ComplianceTestRequest> channel, IPackageFactory packageFactory)
    {
        _channel = channel;
        _packageFactory = packageFactory;
    }

The create method actually creates the Command itself, the arguments, the options, sets the handler as one of the methods of the class, using the closure to keep reference on the required dependencies.

In my Program.cs I'm using the Host.CreateApplicationBuilder(args); and register the command classes

builder.Services.AddTransient<ICommand, ListCommand>();
builder.Services.AddTransient<ICommand, AnalyzeCommand>();

and of course all the other dependencies. Note that everything is in the Host DI container.

I registered a BackgroundService with dependency on the commands and the IHostApplicationLifetime

    public CliService(IEnumerable<ICommand> commands, IHostApplicationLifetime applicationLifetime)
    {
        _commands = commands;
        _applicationLifetime = applicationLifetime;
    }

the Execute method of the BackgroundService builds the RootCommand and runs it.

protected override async Task ExecuteAsync(CancellationToken stoppingToken)
{
    var root = new RootCommand("compliance analyzer");

    foreach (var command in _commands)
    {
        root.AddCommand(command.CreateCommand());
    }
    var builder =
        new CommandLineBuilder(root)
            .UseVersionOption()
            .UseHelpBuilder((bindingContext) =>
            {
              // must call application stop, otherwise it hangs
                _applicationLifetime.StopApplication();

                return new HelpBuilder(LocalizationResources.Instance);
            })
            .UseEnvironmentVariableDirective()
            .UseParseDirective()
            .UseSuggestDirective()
            .RegisterWithDotnetSuggest()
            .UseTypoCorrections()
            .UseParseErrorReporting()
            .CancelOnProcessTermination()
            .UseExceptionHandler((ex, context) =>
            {
                AnsiConsole.WriteException(ex);
            }, 1);

    var app = builder.Build();

    await app.InvokeAsync(Environment.GetCommandLineArgs()[1..]);
}

yes, I'm using Spectre Console for my output, I tried to hack the HelpBuilder to use some Spectre markup, but gave up.

My setup is not lightweight as I have multiple http clients, a couple of background hosts that process data, some channels, configs, I gather telemetry and write file logs... so I really need to be Host first and CLI later...