-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Allow minimal host to be created without default HostBuilder behavior #46040
Conversation
This adds a new Hosting API to reduce startup and app size, and ensures the default behavior is NativeAOT compatible. Fix dotnet#32485
Refactor the WebHostBuilder classes to share more code.
b4e091d
to
ef62c6d
Compare
I think this PR is ready to be reviewed, if anyone wants to take a look. |
It is breaking too much functionality that will need to be reimplemented from the Extensions.HostBuilder.
options => | ||
{ | ||
// We've already applied "ASPNETCORE_" environment variables to hosting config | ||
options.SuppressEnvironmentConfiguration = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have not already applied "ASPNETCORE_" environment variables to hosting config in this code path. Only "DOTNET_". We should do both. I bet this is why the macOS template test is failing to load the server certificate.
We had an issue a while back with the template tests using expired dev certs on macOS. That was fixed by using "ASPNETCORE_"-prefixed environment variables to set a default cert, so the tests shouldn't rely on the development cert being up to date.
aspnetcore/src/ProjectTemplates/Shared/AspNetProcess.cs
Lines 91 to 95 in 40dd230
var finalEnvironmentVariables = new Dictionary<string, string>(environmentVariables) | |
{ | |
["ASPNETCORE_Kestrel__Certificates__Default__Path"] = _developmentCertificate.CertificatePath, | |
["ASPNETCORE_Kestrel__Certificates__Default__Password"] = _developmentCertificate.CertificatePassword, | |
}; |
I'm guessing the dev cert is working as a fallback on the other OS's, but we still have that original issue on macOS when it falls back to the dev cert. @adityamandaleeka I wonder if this is related to our wider macOS dev cert issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested how it interacts with WebApplicationFactory? My assumption is that it will get the IHostBuilder from HostApplicationBuilder.HostBuilderAdapter and then add back the GenericWebHostBuilder rather than the SlimWebHostBuilder here.
aspnetcore/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs
Lines 206 to 211 in c77deeb
hostBuilder.ConfigureWebHost(webHostBuilder => | |
{ | |
SetContentRoot(webHostBuilder); | |
_configuration(webHostBuilder); | |
webHostBuilder.UseTestServer(); | |
}); |
Not a big deal in terms of this PR, but something to follow up on. I was talking to @davidfowl about creating a more "minimal" version of WebApplicationFactory, so that could tie in.
I'm also interested in seeing how much emptier we could possibly make this. I know there's a lot more trimming to do in lower layers even for the "slim" mode.
get | ||
{ | ||
yield return new[] { (CreateBuilderFunc)CreateBuilder }; | ||
yield return new[] { (CreateBuilderFunc)CreateSlimBuilder }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: How easy is it to determine which builder failed if only one does with this member data? It might be nicer to use strings for the MemberData and have it index into a static Dictionary<string, CreateBuilderFunc>
. We are often bad at using serializable member data. But it might be worth it here given how often this is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually works pretty well, from what I can tell. In Test Explorer:
2) Microsoft.AspNetCore.Tests.WebApplicationTests.AddingMemoryStreamBackedConfigurationWorks(createBuilder: CreateBuilderFunc { Method = Microsoft.AspNetCore.Builder.WebApplicationBuilder CreateSlimBuilder(), Target = null })
Duration: 1 ms
1) Microsoft.AspNetCore.Tests.WebApplicationTests.AddingMemoryStreamBackedConfigurationWorks(createBuilder: CreateBuilderFunc { Method = Microsoft.AspNetCore.Builder.WebApplicationBuilder CreateBuilder(), Target = null })
Duration: 3 ms
And when the test fails:
Explicitly add ASPNETCORE environment variables.
No, I haven't. I don't really know much about it and can try it next week. Or I can log a follow up issue to track ensuring it works as we want it to.
Yeah, there is much more work to make it smaller. This is just the start to get the API in. Now other work items (one example: #46142) can build on this to trim more out. My next plan is to get a new API into Extensions.Hosting for an "empty" HostApplicationBuilder. This code can then use that, and trim even more stuff out by default. |
The `dotnet new api -aot` template is getting updated to use the new slim API in dotnet/aspnetcore#46040. Updating the benchmark to match.
This adds a new Hosting API to reduce startup and app size, and ensures the default behavior is NativeAOT compatible.
Fix #32485
Notes: