Closed davidfowl closed 1 year ago
4 and 5 appear to be the same config above?
Fixed
Thanks for contacting us.
We're moving this issue to the Next sprint planning
milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
API review will tolerate CreateEmptyBuilder as an API here given the goals here.
I am one of the people who would still like it, so +1! also I'm for CreateBuilder vs CreateBuilderWithDefaults or something like that, because it explicitly says what's the intention here without the confusion. Considering that the WebApplication api is not yet in any stable version of aspnetcore, it's still open for such changes, isn't it? CreareBuilder vs CreateEmptyBuilder looks more confusing by name.
also my non trimming usecase is: for example I would like to create a local webapp, the one that listens on localhost and it makes no sense for it to be exposed or have a reverse proxy in front or actually even have most of the config knops or files. And it would have config in some user profile directory. And things like that. I don't need things like the iis integration, forwarded headers, etc, even for the theoretical future, so I would prefer not to bring all these. Other stuff although it's just personal preference and can likely be done with non empty builder, I do not necessarily want the default logging and kestrel sections to be named "Logging" and "Kestrel". And to be fair, in case of the app I mentioned, maybe I don't want them at all at least the Kestrel one. There is limited utility in setting up kestrel to do anything beyond listening to localhost in this case. Even urls are a bit too much because exposing that would be super dangerous. But that's likely not a problem.
@DamianEdwards suggested adding a boolean to the existing CreateBuilder API
well... could be. I am not even sure my first idea wasn't like that :) but I'd be okay with such a bool.
What I suggested:
namespace Microsoft.AspNetCore.Builder
{
public partial class WebApplication
{
+ public static WebApplicationBuilder CreateBuilder(string[] args = null); // Calls overload below with useDefaults=true
+ public static WebApplicationBuilder CreateBuilder(bool useDefaults, string[] args = null);
}
}
what about only one overload and useDefaults as an optional parameter set to true by default? Is that even acceptable? considering it doesn't have to be compatible with anything...
what about only one overload and useDefaults as an optional parameter set to true by default? Is that even acceptable? considering it doesn't have to be compatible with anything...
It is.
namespace Microsoft.AspNetCore.Builder
{
public partial class WebApplication
{
+ public static WebApplicationBuilder CreateBuilder(string[] args = null, bool useDefaults = true);
}
}
By the way, a parameter might make it hard for trim-analysis for the scenario @webczat laid out. The trimmer would not be able to discern that pieces are unused unless we introduce a feature flag. Having an explicit overload helps that case.
ooh. Wouldn't think about that.
We think this still needs more work:
// Useful but minimal
WebApplication
.CreateBuilder()
// Kitchen-sink. Needs more work
WebApplication
.CreateMaximalBuilder()
// Full control
WebApplication
.CreateEmptyBuilder()
I still prefer the parameter-based approach personally:
// Useful defaults
WebApplication.CreateBuilder()
// Empty builder
WebApplication.CreateBuilder(useDefaults: false)
What is the kitchen-sink/maximal builder about? Why do I want that and what is in it?
well also maximal builder name looks soooo bad to me... maybe let's just stay with two paths, not three?
@DamianEdwards but it defeats the trimming so it's a no-go.
@webczat We replaced Maximal with Default as a strawman because Default isn't a good name to describe what it does.
on a bit unrelated note, empty host is empty, it does not even have host configuration like the normal one does? so I would need to call host's methods directly for it, then use the normal Config property for the app one?
@webczat We replaced Maximal with Default as a strawman because Default isn't a good name to describe what it does.
Well but here I see three variants of CreateBuilder. so not sure...
@davidfowl I thought the trimming issue was only with params that have a default value? I'm not suggesting that, I was suggesting concrete overloads.
API review: rejected, we'll look into linker flags
@eerhardt we need some assistance with linker flags to understand what's possible to do here.
what does that mean?
we need some assistance with linker flags to understand what's possible to do here.
Take a look at https://github.com/dotnet/designs/blob/main/accepted/2020/feature-switch.md and https://github.com/dotnet/designs/blob/main/accepted/2020/linking-libraries.md#feature-switches to get an idea on what is possible to do with feature switches.
Although, they are mainly used when you don't have a public API that can control what is pulled in, and what isn't. A concrete example is the InvariantGlobalization
mode, which allows for culture aware dependencies (like libicu
) to be trimmed. See https://github.com/dotnet/runtime/blob/main/docs/workflow/trimming/feature-switches.md for the current list of feature switches available in the runtime, which you can model after.
My opinion would be to try to figure out a new public method that could be added that did the intended behavior. That seems like the more appropriate design here. Feature switches are hard to discover, and they aren't part of the code. It is less surprising for developers if they called method Foo
and got the same behavior, no matter what settings the application was published with.
also note feature switches of any form don't really cover use cases like mine, unless the suggestion is, in such case of needing more customizability just don't use this api at all. Like, I really do not want iis integration because why? any other form of reverse proxy things even behind a config switch, and things like that. It's not a trimming problem, just customized configuration.
@webczat That's fair. @BrennanConroy we might still need a truly empty builder with nothing pre-configured.
Give that we're going to investigate linker flags for this, here's the latest API proposal:
namespace Microsoft.AspNetCore.Builder
{
public partial class WebApplication
{
+ public static WebApplicationBuilder CreateBuilder(string[] args = null, bool useDefaults = true);
}
}
We can have a single overload that has this bool that determines if we use the defaults or not. If it's set to false then we'll make 2 completely empty builder that need to build fully configured (AKA useless 😄). Then we'll also support linker flags for CreateBuilder
in order to remove default dependencies (IIS etc)
so like one implicit way for trimming and one explicit way for other uses?
@davidfowl can we finalize on the API before bringing it up for review?
Yes, I didn't have time to spend on this
We might be able to lean on this API https://github.com/dotnet/aspnetcore/issues/34837 to pass an explicit option to CreateBuilder. It would be cleaner than a new method but it wouldn't be trim friendly.
public class WebApplicationOptions
{
+ public bool IncludeDefaults { get; init; }
}
var builder = WebApplication.CreateBuilder(new() { IncludeDefaults = false });
cc @eerhardt
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
Removing the API review label as this needs more thought. There are some complications I've encountered while trying to implement this.
We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.
Is this going to be ready in 6.0? I would really appreciate having a truly minimal application builder that does not setup ANY configuration sources or logging providers, just add the necessary asp.net core services.
Issue 1:
Is there any workaround? There is probably reloadOnChange: true
in CreateBuilder
method causing error: The configured user limit (128) on the number of inotify instances has been reached, or the per-process limit on the number of open file descriptors has been reached.
Issue 2:
Although, they are mainly used when you don't have a public API that can control what is pulled in, and what isn't. A concrete example is the
InvariantGlobalization
mode, which allows for culture aware dependencies (likelibicu
) to be trimmed. See https://github.com/dotnet/runtime/blob/main/docs/workflow/trimming/feature-switches.md for the current list of feature switches available in the runtime, which you can model after.
Removing libicu dependency would be great as it isn't included in some images.
EDIT: I found simple fix for the first issue/error:
// Prevent "The configured user limit (128) on the number of inotify instances has been reached, or the per-process limit on the number of open file descriptors has been reached."
Environment.SetEnvironmentVariable("DOTNET_hostBuilder:reloadConfigOnChange", "false", EnvironmentVariableTarget.Process);
var builder = WebApplication.CreateBuilder(args);
// Prevent reading from others app `appsettings.json` file
((IConfigurationBuilder)builder.Configuration).Sources.Clear();
((IConfigurationBuilder)builder.Configuration).AddCommandLine(args);
builder.Configuration.AddJsonFile(Path.Combine(AppContext.BaseDirectory, "appsettings.myapp.json"), optional: false, reloadOnChange: false);
@DamianEdwards had an idea that we could create new types here with the same factory method (CreateBuilder) as an alternative.
Right, with new defaults more aligned with application kind the type represents, e.g.:
var apiBuilder = ApiApplication.CreateBuilder(args);
var apiApp = builder.Builder();
or
var httpBuilder = HttpApplication.CreateBuilder(args);
var httpApp = builder.Builder();
or
var containerBuilder = ContainerApplication.CreateBuilder(args);
var containerApp = builder.Builder();
or
var cloudBuilder = CloudApplication.CreateBuilder(args);
var cloudApp = builder.Builder();
or
var workerBuilder = WorkerApplication.CreateBuilder(args);
var workerApp = builder.Builder();
I think that there is a lot of conceptual overlap between those options, especially ContainerApplication
and the other application types. That overlap might be confusing "which one should I choose?".
ContainerApplication
is probably the closest to aligning with the motivation with reducing the on-disk footprint of the built application.
Aside: sensitivity to size on disk for container scenarios really depends on the specific hosting model. If you are deploying to a K8S cluster for example, deploying a non AOT image is probably the most efficient because its more than likely that you've already got the base layers deployed on the cluster nodes. For this reason, calling it
ContainerApplication
may not be helpful.
@mitchdenny TBC I'm not suggesting we do all or any of those, they were just supposed to illustrate that if we feel we can find a meaningful name we can maintain the API pattern but have it use different defaults.
As for the aside RE container scenario deployment and disk size, we subscribed to that theory ourselves for literally years now but recent customer feedback and investigation has shown that in reality this just doesn't pan out in the real world for many folks, whether it be due to lack of well implemented container image layer caching, desire by the customer to use minimal (or no) base layers at all, or the comparisons between the deployment size of different stacks occurring well before any layering is taken into account.
The next step here is getting more concrete on the set of features this host/method will have on by default. I looked at what we turn on today and will enumerate a list:
Tooling also has dependencies on some of these features so that should be taken into account (how does tooling fail when the required in framework dependency is missing for e.g.).
I should also add, the original proposal does not work to make things more trimmable and that style of API does not allow trimming unused features.
For the appsettings.json/appsettings.{environment}.json configuration file support, should we enumerate the reload on file change support separately? In the same vein for Kestrel, should we enumerate things like HTTP/2, HTTP/3, and TLS separately too?
@DamianEdwards i would make a separate issue for kestrel features. Mostly because I’m assuming any API we design would be for kestrel, not the web host builder.
Then once we have that API, we can discuss what is on by default with this API.
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
I've updated the top post with my current API proposal and marked this "ready for review". Please take a look and let me know if you have any feedback.
API Review Notes:
useDefaults
parameter or property on the options? Adding new variants of CreateBuilder
could be confusing. The trimmer cannot "see" this.SemaphoreSlim
is prior art.IHostingStartup
is removed because it requires using reflection to scan the assembly. Do we need a way to opt into this? Are you forced to add things like App Service logging manually? (e.g. AddAzureWebAppDiagnostics
). It looks like it. IHostingStartup
is unlikely to play well with AOT and trimming.namespace Microsoft.AspNetCore.Builder
{
public class WebApplication
{
public static WebApplication Create(string[] args);
public static WebApplicationBuilder CreateBuilder();
public static WebApplicationBuilder CreateBuilder(string[] args);
public static WebApplicationBuilder CreateBuilder(WebApplicationOptions options);
+ public static WebApplicationBuilder CreateSlimBuilder();
+ public static WebApplicationBuilder CreateSlimBuilder(string[] args);
+ public static WebApplicationBuilder CreateSlimBuilder(WebApplicationOptions options);
}
}
namespace Microsoft.Extensions.Hosting
{
public class GenericHostWebHostBuilderExtensions
{
public static IHostBuilder ConfigureWebHost(this IHostBuilder builder, Action<IWebHostBuilder> configure)
public static IHostBuilder ConfigureWebHost(this IHostBuilder builder, Action<IWebHostBuilder> configure, Action<WebHostBuilderOptions> configureWebHostBuilder)
+ public static IHostBuilder ConfigureSlimWebHost(this IHostBuilder builder, Action<IWebHostBuilder> configure, Action<WebHostBuilderOptions> configureWebHostBuilder)
}
}
When creating an "slim" web application, the following features will be on (✅) or off (❌) by default:
API Approved for preview1! We should have a lot of time to collect feedback from the community.
SLIM 😄
Background and Motivation
The default host builder pulls in a ton of dependencies that are rooted for trimming by default. We should have a mode where the minimal host is dependency minimal as well.
These numbers are native AOT net8.0 compiled on linx-x64:
WebApplication.CreateBuilder()
= 18.7 MBWe can stick to the current naming convention and use the existing Create/CreateBuilder methods as the one with the defaults and CreateEmptyBuilder to be the empty one:
Proposed API
API Usage
Features
When creating an "empty" web application, the following features will be on (✅) or off (❌) by default. Note that these features can be enabled by the app explicitly after creating the builder/application. They just won't be enabled by default, in order to reduce app size.
Alternative Designs
We could create whole new class names for another type of "Application" than a "WebApplication". Something like:
Other possible names include:
Risks
Potential confusion about which one to call. The template should make it obvious that the "empty" builder doesn't have anything in it, and you need to add things like appsettings, console logging, etc.