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.72k stars 1.07k forks source link

Setting PreserveCompilationContext=false trims Microsoft.AspNetCore.App shared runtime in deps files #2092

Closed eerhardt closed 5 years ago

eerhardt commented 6 years ago

From @pranavkm on March 26, 2018 17:18

Steps to reproduce

  1. Create an webapi \ mvc \ emptyweb application using preview1 (or preview2) Sdk
  2. Build the project

The generated deps file will say that the application references both Microsoft.NETCore.App and Microsoft.AspNetCore.App:

"blah/1.0.0": {
    "dependencies": {
      "Microsoft.AspNetCore.App": "2.1.0-preview1-final",
      "Microsoft.NETCore.App": "2.1.0-preview1-26216-03"
    },
    "runtime": {
      "blah.dll": {}
    },
    "compile": {
      "blah.dll": {}
    }
},
  1. Delete the bin directory. Edit the csproj <PreserveCompilationContext>false</PreserveCompilationContext>. Rebuild the project
  2. The generated deps file will say that the app no longer references Microsoft.AspNetCore.App:
 "blah/1.0.0": {
   "dependencies": {
     "Microsoft.NETCore.App": "2.1.0-preview1-26216-03"
   },
   "runtime": {
     "blah.dll": {}
   }
 },

Environment data

.NET Command Line Tools (2.1.300-preview1-008174)

Product Information:
 Version:            2.1.300-preview1-008174
 Commit SHA-1 hash:  b8df89a54f

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.16299
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Users\Pranav\Downloads\dotnet-sdk-2.1.300-preview1-008174-win-x64\sdk\2.1.300-preview1-008174\

Microsoft .NET Core Shared Framework Host

  Version  : 2.1.0-preview1-26216-03
  Build    : f2c3216183d20416568a4bbf5bb7d153e826f153

More information:

The bulk of MVC's controller discovery model is based on finding libraries that directly or transitively depend on any one of Mvc's libraries. In 2.1.0-preview2, the Razor.Sdk only sets PreserveCompilationContext=true if the app has any files that require runtime compilation. For projects without views, such as webapi or empty web template, the produced deps file claims that the app does not reference Microsoft.AspNetCore.App -> Microsoft.AspNetCore.Mvc. Consequently no controllers are discovered.

Copied from original issue: dotnet/core-setup#3902

eerhardt commented 6 years ago

From @pranavkm on March 26, 2018 17:57

Spoke to @eerhardt, the missing shared runtime isn't necessarily the cause for concern here - a console app's deps file also does not include a reference to the shared runtime Microsoft.NETCore.App. The issue is that DependencyContext.Load does not merge shared fx deps files. The suggested fix is to add a new overload to DependencyContext.Load that walks up the fx chain and merges the runtime deps files. MVC can consume this API once this is available.

cc @rynowak \ @steveharter \ @mkArtakMSFT

eerhardt commented 6 years ago

Looking more, the DependencyContext.Load DOES merge in the shared fx deps files.

The issue is that the Dependency information is getting trimmed, so there is no link from the application's library to the MVC library. Or similarly, if the application references another project that contains controllers, that project won't have a Dependency on the MVC library either. This trimming logic is in the dotnet/sdk repo. Moving this issue there.

eerhardt commented 6 years ago

See https://github.com/dotnet/sdk/blob/935ac5fde8d917b1de3b18d038232f15527572aa/src/Tasks/Microsoft.NET.Build.Tasks/ProjectContext.cs#L63-L66 for where the PlatformLibrarys get trimmed.

livarcocc commented 6 years ago

@dsplaisted please take a look.

eerhardt commented 6 years ago

Some more thoughts about this:

Assuming the plan is to preserve the "Dependency" entries, even when the Platform library is trimmed:

  1. You will probably want to only enable this behavior with a switch that is disabled by default. ASP.NET can enable the switch as necessary, when they want this behavior. Leaving the "Dependency" entries pointing to trimmed libraries could be considered a breaking change. This is because someone could load the .deps.json file without also loading the shared fx .deps.json. When this happens, if there is code trying to resolve those dependencies it will fail to find the referenced library.
  2. The "Version" property of a Dependency may be problematic, depending on how the consumer's dependency resolution logic works. For example, if there was a Dependency for MVC v2.1.0 and the asp.net shared fx rolled forward to v2.1.6 or even v2.2.0, using the Dependency.Version to match on strings will not work.
pranavkm commented 6 years ago

Stepping back, the primary driver for turning off PreserveCompilationContext is to avoid publishing the refs directory when an application does not require it (e.g. when you're writing an application not using runtime compiled views). Right now, there's a single switch that determines the structure of the deps file and the published refs directory. Could we possibly introduce a new switch that controls publishing the refs directory?

// Microsoft.NET.Sdk.Razor.targets
<!-- Use PreserveCompilationContext's value if it was explicitly set in the user's project -->
<PreserveCompilationReferences Condition="'$(PreserveCompilationContext)' != ''">$(PreserveCompilationContext)</PreserveCompilationReferences>

<!-- Initialize PreserveCompilationContext -->
<PreserveCompilationReferences Condition="'$(PreserveCompilationReferences )' == ''">true</PreserveCompilationReferences>

Targets in Microsoft.NET.Sdk that control publishing would be modified to rely on this new switch: https://github.com/dotnet/sdk/blob/6945e3694c918eea4c8c4fb6217e1485b179994b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PreserveCompilationContext.targets#L45-L46 https://github.com/dotnet/sdk/blob/6945e3694c918eea4c8c4fb6217e1485b179994b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PreserveCompilationContext.targets#L84-L85

It's a breaking change for users relying on the refs directory in non-web scenarios, but I'm not sure if that's a common scenario.

nguerrera commented 5 years ago

Duplicate of dotnet/sdk#2122

nguerrera commented 5 years ago

Going with https://github.com/dotnet/sdk/issues/2092#issuecomment-377050440, which is tracked by #2122.