Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generic host builder pattern for System.CommandLine.Hosting #918

Open
NikiforovAll opened this issue Jun 5, 2020 · 16 comments · May be fixed by #919
Open

Add generic host builder pattern for System.CommandLine.Hosting #918

NikiforovAll opened this issue Jun 5, 2020 · 16 comments · May be fixed by #919
Labels
Area-App Models enhancement New feature or request

Comments

@NikiforovAll
Copy link
Contributor

Align System.CommadLine.Hosting model with the way ASP.NET Core uses Generic Host.

It should be possible to configure CommandLineBuilder like this:

public static IHostBuilder CreateHostBuilder(string[] args) =>
    CommandLineHost.CreateDefaultBuilder(args)
        .ConfigureAppConfiguration((hostingContext, config) =>
        {
        })
        .ConfigureCommandLineDefaults(cmdHost =>
        {
            cmdHost.ConfigureServices(services =>
            {
            })
        })
        .ConfigureCommandLineBuilder(cmdBuilder => // CommandLineBuilder
        {
           cmdBuilder.UseReflectionAppModel(); // e.g. https://github.com/KathleenDollard/command-line-api-starfruit 
        });
    });
@NikiforovAll NikiforovAll linked a pull request Jun 6, 2020 that will close this issue
@shaggygi
Copy link
Contributor

shaggygi commented Jun 6, 2020

Would be nice to make it easier to use the Worker Service, as well. Referencing #556.

@NikiforovAll
Copy link
Contributor Author

@shaggygi I think this solution allows it, because ParseResult is added as Singleton, so you can consume it from Worker.

Currently, it is not documented though.

@KathleenDollard
Copy link
Contributor

You all will teach me a good bit on this, so let me start with what might be a simple question.

When generic host is in place, does it make sense to have the CommandLine Host on the stack, or (at least as an option) can we put the results in an instance (from StarFruit) available via DI ad then step out of the way.

(Question is based on current assumptions about how GenericHost works)

@jonsequitur jonsequitur added Area-App Models enhancement New feature or request labels Jun 10, 2020
@NikiforovAll
Copy link
Contributor Author

@KathleenDollard
It is definitely useful to be able to build DI container based on parsing results.
Also, there is already a method to use System.CommandLine.Binding.ModelBinder<T> binder:

services.AddOptions<MyAppOptions>()
    .BindCommandLine();
// source: src\System.CommandLine.Hosting\HostingExtensions.cs:70

Although, it introduces a cyclic dependency between CommandLineHostBuilder and CommandLineBuilder because:

  1. Presumably, we want the next services InvocationContext, IConsole, IInvocationResult, ParseResult to be available during host configuration. (It is up to debate what services should be provided, but I mentioned services used in the current implementation.)
  2. Currently, it is only possible to add IHost service to invocationContext.BindingContext outside of UseMiddleware method. In order to trigger middleware, we need to trigger commandLineBuilder.Build().InvokeAsync.

The current implementation gets around this by making IHost host = hostBuilder.Build() a part of the middleware, so InvocationContext is available from the middleware context.

@jonsequitur
Copy link
Contributor

Can the patterns we use here be generalized to include IWebHostBuilder? The goals are the same it seems: #916.

@NikiforovAll
Copy link
Contributor Author

NikiforovAll commented Jun 10, 2020

Unfortunatelly, IWebHostBuilder doesn't implement IHostBuilder directly. It should be possible to migrate to generic host.

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureCommandLineBuilder(cmdBuilder => // CommandLineBuilder
        {
           cmdBuilder.UseReflectionAppModel(); // e.g. https://github.com/KathleenDollard/command-line-api-starfruit 
        });
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        });

@jonsequitur
Copy link
Contributor

To the extent that the patterns are similar, it would be good to have approaches and matching code for both, even if it means we need another project.

@KathleenDollard
Copy link
Contributor

Aarrgh. Seems like an understanding of why IWebHostBuilder and IHostBuilder differ would be a starting point.

I don't yet see why multiple projects help over other approaches.

@jonsequitur
Copy link
Contributor

They'll have different dependencies and you won't want the union of the two dependency graphs.

@jonsequitur
Copy link
Contributor

I think a more inverted model, and doing away with the concept of a CommandLineHost, might align well to the existing host models, which would give us something like:

// Generic host:
new HostBuilder()
    .ConfigureCommandLine((CommandLineBuilder cmdLineBuilder) =>
    {
        cmdBuilder.DoStuff();
    })
// Web host:
new WebHostBuilder()
    .ConfigureCommandLine((CommandLineBuilder cmdLineBuilder) =>
    {
        cmdBuilder.DoStuff();
    })

@NikiforovAll
Copy link
Contributor Author

NikiforovAll commented Jun 10, 2020

@jonsequitur

  1. Could you please describe the use case of ConfigureCommandLine in the case of WebHostBuilder:IWebHostBuilder. How is it going to be consumed and what services should be added to DI?

For configuration purposes next methods are used:

new HostBuilder()
    .ConfigureHostConfiguration(config => config .AddCommandLine(args))
    .ConfigureAppConfiguration((hostingContext, config) => config.AddCommandLine(args));
// Microsoft.Extensions.Hosting.IHostBuilder ConfigureHostConfiguration
// Microsoft.Extensions.Hosting.IHostBuilder ConfigureAppConfiguration

Please see: comment above

  1. There is another aspect of ConfigureCommandLine that could be host-dependent. Mainly, at what point of time parseResult.InvokeAsync() is executed. What do you think, how should it be organized?
    In Add generic host builder pattern [WIP] #919, I've added CommandLineExecutorService that shuts down IHostBuilder after command execution.
services.AddHostedService<CommandLineExecutorService>()
  1. I agree that CommandLineHost might be a dubious concept to follow.
    It is just something similar to WebHost.CreateDefaultBuilder. A factory to create IHostBuilder = Host.CreateDefaultBuilder(args) + cmdBuilder.UseDefaults().

I apologize for so many questions. It is quite an interesting bit of functionality to me.

@jonsequitur
Copy link
Contributor

re: 1. I think the use cases are the same for both generic host and web host, which is I want to use DI, logging, etc. in an app that also has a command line experience that provides help, completions, and so on. The application type is orthogonal to the desire to have a good CLI. As examples, dotnet-interactive and dotnet-try are both .NET tools that have some modes that spin up a web server and others that do not, specified using command-line arguments.

@KathleenDollard
Copy link
Contributor

Update after some exploration.

You can find the code I wrote on this at https://github.com/KathleenDollard/command-line-api-starfruit. It's the hosting projects in the main branch. Here are my updated thoughts:

  • I started out pretty "naïve"/ignorant on this. Anything I said earlier in this thread should be taken with a huge grain of salt. I am on a curve to learn this, so don't take this too seriously either.
  • The StarFruit spike has a GeneralAppModel (the space I'm currently working in) that uses Generic Host. It doesn't yet support invocation features of System.CommandLine
  • I think we want an IHostedService for the invocation (what I drafted) an an IHostedService wrapper for all of the invocation pipeline elements. This will result in a single way to configure System.CommandLIne, and that being GenericHost configuration.
  • I think these wrappers should exist so System.CommandLine does not take a dependency on GenericHost and IHostedService. This is a loosely held opinion because the interfaces are in a separate package with enums, constants and at least one base class.
  • For samples that aren't StarFruit/GeneralAppModel, different IHostedService for configuring the parser and executing OR a single IHostedService that itself is configurable, like with a lambda/delegate that builds the parser.
  • For StarFruit/GeneralAppModel I am challenged figuring out how "switch" style usage. Probably because I'm being a bit stubborn about the obvious of configuring with a lambda/delegate that contains the "switch"
  • I have not reviewed the Hosting work in System.CommandLine.Hosting with my updated thinking, so I don't really have an opinion. But it is possible that it needs to be entirely replaced due to changes around us.
  • This is my current thinking, it may change again.
  • I plan to stay focused on the GeneralAppModel work for a while, so if anyone wants to pick this up, I'm happy to help.

Sorry for the long post.

@niemyjski
Copy link

niemyjski commented Sep 18, 2020

I've been struggling a bit with working on a way in a sample asp.net project to use the CommandLineBuilder / hosting project to dynamically create hosted services using IHost/IHostBuilder based on the parse result.

I'd see myself registering sub commands in ConfigureCommandLine and then getting access to the parse result in ConfigureServices or some kind of callback before the container is created to dynamically add my hosted services too. Has this been done before or considered?

@NikiforovAll
Copy link
Contributor Author

@niemyjski I think this under consideration, please see #1025 (comment)

@StephenCleary
Copy link

I have a different desired usage: I want to use the hosted services injected in the host to define which commands are available. In the bigger context, this is for a .NET Core worker app that normally runs as a Win32 service and performs scheduled jobs, and the command line interface is for manually executing one of those jobs.

Currently, I'm doing this by creating the host with the full command line (it ignores unknown arguments), and then I create a command line with dynamically added commands which directly invoke the appropriate hosted service. In order to allow dotnet host/app arguments (.NET command line configuration provider), I also have a catch-all new Argument<string[]>("dotnet runtime options", "Configuration value overrides") on my root command. Which is never used; it just is there to prevent System.CommandLine parse errors.

The drawback to my current approach is that the error handling isn't great. The host attempts to parse any System.CommandLine arguments into configuration values, and the System.CommandLine ignores any unknown arguments to allow for host configuration. So typos are not obvious.

I tried creating a separate RootCommand which would allow specifying the dotnet arguments explicitly in a --dotnet argument, then building the host, and then creating the "real" RootCommand, but that had the drawback that short-circuited execution like help output would only understand the first root command (and not display help for the subcommands of the "real" root command).

Anyway, it seems like there are two different and possibly conflicting desires in this thread:

  1. Allow the command line options to change the DI setup.
  2. Allow the DI setup to define the command line options.

Good luck! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-App Models enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants