search

dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.63k stars 1.04k forks source link

Handle targeting packs in PreserveCompilationContext #10012

Open dsplaisted opened 5 years ago

dsplaisted commented 5 years ago

When doing a build with PreserveCompilationContext, referenced assemblies are copied to the refs folder of the output directory. However, assemblies coming from NuGet packages aren't copied, and are looked up in the NuGet cache at runtime.

With Targeting packs for .NET Core 3.0, the Framework reference assemblies don't go through the NuGet assets pipeline, and so they are now being copied to the refs folder in the output.

We should figure out what to do here. Possibly the DependencyModel code needs to be updated to understand how to locate .NET Core targeting packs, the same way that it can do with .NET Framework targeting packs.

cc @natemcmaster @eerhardt

natemcmaster commented 5 years ago

cc @NTaylorMullen @pranavkm - this will affect Mvc

nguerrera commented 5 years ago

See also https://github.com/dotnet/sdk/issues/2122

I thought I heard that PreserveCompilationContext was no longer needed by default for 3.0, but I think I misunderstood the details.

What I see is that it is still turned on by default here:

https://github.com/aspnet/AspNetCore-Tooling/blob/f6f6e164672c0488115423f2db3d7ce1c1c92a1b/src/Razor/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Sdk.Razor.CurrentVersion.props#L71

However, there is code to trim the refs/ assemblies from the published output here:

https://github.com/aspnet/AspNetCore-Tooling/blob/f6f6e164672c0488115423f2db3d7ce1c1c92a1b/src/Razor/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Sdk.Razor.CurrentVersion.targets#L688-L691

Without any further steps, dotnet/sdk#2774 will cause 14 MB of framework reference assemblies to be copied to refs/ of build output of a default MVC app. This is because they are no longer seen as being able to be located by the dependency model in nuget cache. However, I am seeing RazorCompile run on build, due to RazorCompileOnBuild=true, and produce a .Views.dll, which I suspect means that these aren't needed. I further suspect there was no need to work around this in the build case pre-3.0 because it wasn't copying the refs on build anyway.

If I've got this all right, I think we should proceed in this priority order

(3) would not be needed in the common, default case. Furthermore, I think we should make refs/ copying consistent with nuget libs. In 3.0, we are saying that build output can be copied to another machine and used as-is by default. In that case, I believe that even if we had support for finding framework reference assemblies in .NET Core targeting packs in dependency model, it should not be turned on by default for 3.0. cc @peterhuene

1 and 2 should be cheap and I think we should prioritize landing them now. Thoughts?

nguerrera commented 5 years ago

cc @DamianEdwards @rynowak

rynowak commented 5 years ago

@nguerrera - for inner loop we still need PreserveCompilationContext and reference assemblies. Views are compiled during the build, but can also be recompiled during development when they change, which means that the runtime needs to be able to find the compilation references. For production, it's the users choice whether or not they need runtime compilation support. Many users still like the runtime compilation features and use them in production.

As a separate matter, MVC currently uses PreserveCompilationContext for some assembly lookup things (dating back to 2.1) - so we effectively always need it to be on. I actually wasn't aware of this until I talked to a few other people on the team, and I don't think it's the right thing for us to going forward. Unfortunately that doesn't help 2.1 and 2.2.

So, in general we expect PreserveCompilationContext and related features to be updated and continue to be relevant when framework composition changes happen. This has caused friction whenever we introduce new concepts like shared frameworks. Anything my team can do to try and push this feedback and testing upstream would benefit us all I think. These runtime composition changes have a really big effect on MVC because we need to be able to load all of the app's assemblies as well as recreate the build environment on demand. When it breaks a bug gets reported to us and then find out what changed upstream.

Responding to your comments in detail:

Implement PreserveCompilationReferences as proposed on dotnet/sdk#2122 Set PreserveCompilationReferences appropriately in Web SDK

Yes please 👍

Look into optimizing the case where compilation references are preserved, but we're not building a deployable output. This would probably amount to "Possibly the DependencyModel code needs to be updated to understand how to locate .NET Core targeting packs, the same way that it can do with .NET Framework targeting packs."

Somebody like @DamianEdwards can comment on the tradeoffs about copying refs to the output during inner loop vs making the resolver smarter. I want to emphasize the being able to runtime-compile code during inner loop is primary scenario for us.

nguerrera commented 5 years ago

@rynowak Thank you. I see where I was mistaken and that's very helpful.

So, (1) is done and (2) is tracked by https://github.com/aspnet/AspNetCore/issues/6512

So we're back to (3), which this originally tracked and still does, and your clarification bumps the priority back to high on it.

Next steps I see:

(1) Agree that it is ok to use the refs\ folder on build for fast approaching preview 2. This means 14 MB of refs\ in an mvc app unless/until we do something in the resolver to find them a different way. (2) Figure out a better testing strategy to avoid waiting as long as we have in the past to find breaks to this scenario.

And for (2), in the immediate term, let's look at getting some tests going early next week since the targeting pack resolution change (dotnet/sdk#2774) is in.

livarcocc commented 5 years ago

@nguerrera is going to sort out what is still left here.

nguerrera commented 5 years ago

@rynowak Sorry, this issue got a little stale, and I'm trying to go back and understand if there is must fix work remaining for 3.0.

for inner loop we still need PreserveCompilationContext and reference assemblies. Views are compiled during the build, but can also be recompiled during development when they change, which means that the runtime needs to be able to find the compilation references.

What are the steps to trigger this recompilation? When I build a 3.0 web app, there are no refs/ being copied on F5.

nguerrera commented 5 years ago

Found it:

https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-core-3-0-preview-3/

    <PackageReference Include="Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation" Version="3.0.0-preview6.19307.2" />
nguerrera commented 5 years ago

Adding this package adds the following to bin\

  1. 8 extra runtime assemblies (8.89 MB total)
"Microsoft.AspNetCore.Razor.Language.dll"
"Microsoft.CodeAnalysis.CSharp.dll"
"Microsoft.CodeAnalysis.dll"
"Microsoft.CodeAnalysis.Razor.dll"
"Microsoft.Extensions.DependencyModel.dll"
"System.Text.Encoding.CodePages.dll"
"Microsoft.AspNetCore.Mvc.Razor.Extensions.dll"
"Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.dll"

  1. 26 extra roslyn satellite assemblies (4.35 MB total)

  2. 279 framework reference assemblies in refs\ (6.89 MB total)

(3) Is about about 2x better than the last time I checked. Looks like the reference assemblies got smaller. :)

Somebody like @DamianEdwards can comment on the tradeoffs about copying refs to the output during inner loop vs making the resolver smarter.

@DamianEdwards Can you comment? I have not heard a single complaint about the size of these refs\ when using runtime compilation in 3.0. Have you? I'm inclined to say that optimizing these 7 MB away can be done post-3.0 if at all.

cc @livarcocc

nguerrera commented 5 years ago

Also, this earlier comment is still relevant: "Furthermore, I think we should make refs/ copying consistent with nuget libs. In 3.0, we are saying that build output can be copied to another machine and used as-is by default. In that case, I believe that even if we had support for finding framework reference assemblies in .NET Core targeting packs in dependency model, it should not be turned on by default for 3.0."

If we were to teach dependency model how to find targeting packs, we would need to decide what is the trigger that tells the SDK that it is OK to rely on this in the build output. This feels complex and harder to explain than "3.0 simply copies all the dlls that are needed by the app to bin\". In other words, these refs\ aren't different than the nuget dependencies we now copy...

So I'm inclined to declare this by design.

DamianEdwards commented 5 years ago

I think it's fine to leave this as-is for (i.e. the extra assemblies are copied to \bin when using the runtime-compilation package). We can keep monitoring customer feedback to see if it's something we want to address in the future.

nguerrera commented 5 years ago

Thanks, @DamianEdwards

Moved to backlog.

I thought about this: "we would need to decide what is the trigger that tells the SDK that it is OK to rely on this in the build output. " and I think if we did this feature, it could be the same trigger as not copying nuget bits: CopyLocalLockFileAssemblies. That should probably be aliased with a better name too.

But it is complexity that we should only add if we get clear feedback on it.

ygoe commented 4 years ago

Can I safely delete the "refs" directory after publishing if I'm sure not to use runtime compilation of views in this environment? I don't want to deploy over 250 dead files.

eerhardt commented 4 years ago

@ygoe - have you tried setting ‘PreserveCompilationContext’ to ‘false’ in your .csproj? The “refs” directory won’t be created at all if you set this property to false. Try that and test your app with it.

ygoe commented 4 years ago

@eerhardt That doesn't change anything at all. The "refs" directory and all the files are still there.