Avoid configuring PreserveCompilationContext=true in 6.0 apps #16656
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
pranavkm
force-pushed
the
prkrishn/preservecompilationcontext
branch
3 times, most recently
from
09bd4cb to
0341547
Compare
| @@ -106,5 +108,43 @@ public virtual void Publish_IncludesRefAssemblies_WhenCopyRefAssembliesToPublish | |||
|
|
|||
| new FileInfo(Path.Combine(outputPath, "refs", "System.Threading.Tasks.Extensions.dll")).Should().Exist(); | |||
| } | |||
|
|
|||
| [CoreMSBuildOnlyFact] | |||
| public void Build_ProducesDepsFileWithCompilationContext_ButNoReferences() | |||
There was a problem hiding this comment.
These tests run on .NET Core 2.1, 3.1 and 5.0. So we know we haven't regressed this scenario there. We don't have a netframework app in our test matrix, but I manually verified that the behavior there is also unchanged.
| Optional targets used to initialize PreserveCompilationContext. | ||
| This property is configured by the RazorSDK if it is referenced by the app. | ||
| --> | ||
| <Import Project="$(_NETSdkInitializePreserveCompilationTargets)" Condition="'$(_NETSdkInitializePreserveCompilationTargets)' != ''" /> |
There was a problem hiding this comment.
I could make this a proper ImportBeforeMicrosoftNetSdkPreserveCompilationContext property if we that's more appropriate. I used a "private" property since I don't really expect user projects to ever want to configure this
|
@dsplaisted any chance you could have a look at this PR this week? |
|
These properties don't need to be set exactly before I would suggest importing next to this: That way it's importing in a more generic location, and if Razor ends up shipping in a workload then it's a simple switch. I'd suggest naming the new file something like |
The goal is to avoid producing the side-effects of setting PreserveCompilationContext=true (additional content in deps file and ref assemblies) unless the app expects it. Previously the option was configured as part of Razor's SDK.props. This was done as a hack around target evaluation ordering - GenerateDependencyFile and PreserveCompilationReferences are defined in M.NET.Sdk's targets and use PreserveCompilationContext during their initialization. Unfortunately configuring it there makes it difficult to make any tfm specific decisions or de-activate it if the user did not explicitly configure it. This change adds a hook to initialize PreserveCompilationContext early on during .NET SDK that allows configuring defaults for 5.0 and earlier apps.
pranavkm
force-pushed
the
prkrishn/preservecompilationcontext
branch
2 times, most recently
from
8072c3f to
071a308
Compare
pranavkm
force-pushed
the
prkrishn/preservecompilationcontext
branch
from
071a308 to
2112f3c
Compare
|
Updated the PR based on your feed. It looks a lot cleaner now! |
|
|
||
|
|
||
| <!-- Import targets from RazorSDK if referenced --> | ||
| <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.Razor.BeforeCommon.targets" Condition="'$(UsingMicrosoftNETSdkRazor)' == 'true'" /> |
There was a problem hiding this comment.
Could you put this file under the Microsoft.NET.Sdk.Razor folder? It should be fine to use a relative import path for it.
pranavkm
added
auto-merge
|
Hello @pranavkm! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
The goal is to avoid producing the side-effects of setting PreserveCompilationContext=true
(additional content in deps file and ref assemblies) unless the app expects it.
Previously the option was configured as part of Razor's SDK.props. This was done as a hack around
target evaluation ordering - GenerateDependencyFile and PreserveCompilationReferences are defined
in M.NET.Sdk's targets and use PreserveCompilationContext during their initialization. Unfortunately
configuring it there makes it difficult to make any tfm specific decisions or de-activate it if
the user did not explicitly configure it.
This change adds a hook to initialize PreserveCompilationContext early on during .NET SDK that allows
configuring defaults for 5.0 and earlier apps.
Fixes dotnet/aspnetcore#5068