-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[release/7.0] Do not override content root with default #79411
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-hosting Issue DetailsBackport of #79242 to release/7.0 /cc @halter73 Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
Ensure the temp directory used is always empty, so it doesn't pick up appsettings.json files randomly. Fix #79453
@halter73 there's a merge conflict with the base branch. Can you please address it so I can merge this? |
Approved by Tactics (7.0.3). |
I've fixed the merge conflict. Letting CI run now to ensure it still builds. 😄 |
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.
@halter73 @eerhardt the CI failure seems related to the changes in this PR:
artifacts\bin\testPackages\projects\Microsoft.Extensions.Hosting\project.csproj(0,0): error NU1102: (NETCORE_ENGINEERING_TELEMETRY=Build) Unable to find package Microsoft.Extensions.Configuration.Binder with version (>= 7.0.2)
- Found 1562 version(s) in dotnet7 [ Nearest version: 7.0.0-rtm.22511.4 ]
- Found 109 version(s) in dotnet-public [ Nearest version: 7.0.1 ]
- Found 1 version(s) in dotnet7-transport [ Nearest version: 7.0.0-rc.1.22426.10 ]
- Found 0 version(s) in D:\a\_work\1\s\artifacts\packages\Debug\
- Found 0 version(s) in richnav
- Found 0 version(s) in darc-pub-dotnet-emsdk-d71ea7c
- Found 0 version(s) in dotnet-eng
- Found 0 version(s) in dotnet-libraries
- Found 0 version(s) in dotnet-tools
@ViktorHofer - is this a libraries infrastructure problem? If we are shipping 2 OOB packages, with one depending on the other, in the same release? |
Looking... |
The issue here is that the previous release (7.0.2) is supposed to provide the Microsoft.Extensions.Configuration.Binder/7.0.2 version but apparently hasn't yet shipped to nuget.org. The current release (7.0.3) now already depends on that package but can't find it on nuget.org and neither in the artifacts/packages directory because the project doesn't build in this servicing release. AFAIK we never had overlapping servicing releases in the same band (i.e. 7.0.2 and 7.0.3 at the same time). I can think of couple of workarounds to unblock the PR:
cc @ericstj for awareness |
Thanks for looking into this @ViktorHofer. It sounds like for both the first 2 workarounds, it would take 2 changes:
For the 3rd workaround, I'd be concerned about nuget caching the "staged packages" on people's machines. If someone restores this "intermediate" version before the official 7.0.2 version is shipped, they could get the non-official package cached. I kind of think I like the 1st workaround the best, and then we make a revert before we ship 7.0.3. |
You might even be able to avoid reverting if we have more binder changes coming in servicing this release cc @tarekgh One generalization here might be to change how we sequence branding changes. We could make the product version changes first, but wait to remove |
You shouldn't need the above mentioned workaround anymore, given that the 7.0.2 nuget packages already got released to nuget.org |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Ok, thanks @ViktorHofer. Just to be clear, I'm still seeing the following failure in the
The rest of the CI failures are a known unrelated issue affecting Apple OSs where openssl fails to install, showing the error message:
|
@ericstj can you please take a look? I think you know more about package testing. |
That indicates that Microsoft.Extensions.Hosting.dll built here is referencing I think that's happening because Microsoft.Extensions.Hosting uses types in DiagnosticSource but doesn't directly reference it. It ends up getting the transitive reference to DiagnosticSource through the ProjectReference to To fix this can you try adding a ProjectReference to System.Diagnostics.DiagnosticSource in Microsoft.Extensions.Hosting? |
CI failures related to this PR are gone. Only a couple of timeout cancellations. |
Backport of #79242 to release/7.0
/cc @halter73
Customer Impact
A customer reported this .NET 7 regression after noticing their ASP.NET Core application using
WebApplication
stopped respecting theASPNETCORE_CONTENTROOT
environment variable after upgrading. See #78583.There is a workaround of using the
DOTNET_CONTENTROOT
environment variable instead, but a lot of existing documentation suggests using theASPNETCORE_
-prefix which has been supported longer thanDOTNET_
.This only affects
ASPNETCORE_CONTENTROOT
. All otherASPNETCORE_
-prefixed environment variables are respected. And this also only affects applications usingWebApplication
. None of the older web hosts are affected.It might seem odd that an issue specific to
ASPNETCORE_
-prefixed environment variables requires a runtime fix, but that's because the mechanism thatWebApplication
is using to pass in this configuration (HostApplicationBuilderSettings.Configuration
) incorrectly allows a manually configured content root to be overwritten by the default value which is the root cause of the bug.Testing
I added regression tests and manually tested the fix with a ASP.NET Core application by adding a Microsoft.Extensions.Hosting 7.0.1 PackageReference with
artifacts/packages/Debug/Shipping/
as a NuGet source.Risk
Low. This is a small, targeted fix that has been well tested.
This does touch code that ships in a NuGet package. I followed the package authoring instructions, but I still need to get the package authorization changes reviewed.