Fix AppHost pack prebuilt usage in source-build#37672
Conversation
Pilchie
added
the
area-infrastructure
There was a problem hiding this comment.
This actually doesn't work because of props/targets ordering. This is a .props file, but the SDK sets UseAppHost to true in Microsoft.NET.RuntimeIdentifierInference.targets. 🙁 At the point this condition is evaluted, UseAppHost is still empty string. I'm not sure how I didn't catch it not working in my test source-build. @MichaelSimons caught it after the 6.0.1xx patch version of this PR went through a build.
I think all I need to do is remove the or '$(UseAppHost)' != 'true' part of the condition and change this:
to match the condition on the runtime project:
(I noticed that only ref packs were being "installed" during source-builds, not runtime packs. We don't care about tests in source-build, so might as well disable both.)
There was a problem hiding this comment.
Yeah, that's my thought above as well.
There was a problem hiding this comment.
Because the SDK can download targeting packs and reference them from the .nuget/packages/ folder, this isn't entirely true.
There was a problem hiding this comment.
I pulled this from the commit that added this line via git blame to put more context in the comment itself alongside the source-build comment. Would this be better? (Basically, remove required:)
Override the SDK default and point to local .dotnet folder. This is done to work around limitations in the way the .NET SDK finds shared frameworks and targeting packs. It allows tests to use the shared frameworks and targeting packs that were just built.
There was a problem hiding this comment.
Ick, I can see how that would be a problem.
There was a problem hiding this comment.
Leave this alone. This property controls where Microsoft.AspNetCore.App.Ref and Microsoft.AspNetCore.App.Runtime lay out their content for local tests. Should instead conditionalize the App.Ref target _InstallTargetingPackIntoLocalDotNet i.e. copy over https://github.com/dotnet/aspnetcore/blob/main/src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.csproj#L514
There was a problem hiding this comment.
Ah, yeah, in addition to removing the '$(UseAppHost)' != 'true' part, I should be able to move the source-build condition onto the NetCoreTargetingPackRoot property because that's the one that's actually giving us trouble. (We ended up with the same conclusion about _InstallTargetingPackIntoLocalDotNet but I hadn't thought about the simplification on this end. 👍)
There was a problem hiding this comment.
Yeah, that's my thought above as well.
dagood
force-pushed
the
fix-prebuilt-apphost
branch
from
00c1aae to
3f477ec
Compare
dagood
force-pushed
the
fix-prebuilt-apphost
branch
from
3f477ec to
2662685
Compare
|
@dougbu @wtgodbe This is ready for another pass. I'm running it through a tarball build in dotnet/installer#12469 and locally. |
| <!-- Workaround https://github.com/dotnet/sdk/issues/2910 by copying targeting pack into local installation. --> | ||
| <Target Name="_InstallTargetingPackIntoLocalDotNet" | ||
| DependsOnTargets="_ResolveTargetingPackContent" | ||
| Condition="'$(DotNetBuildFromSource)' != 'true'" |
ghost
added this to the
4 participants
Fix AppHost pack prebuilt usage in source-build
This change removes the
NetCoreTargetingPackRootoverride in some cases during source-build to avoid unnecessarily downloading the apphost pack as a prebuilt dependency. I also updated the comment on the override to be more descriptive (based ongit blame) and added details on why/when it needs to be disabled for source-build./cc @dougbu