dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.04k forks source link

csc.runtimeconfig.json is not correctly generated and is broken by additional deps light-up #24816

Closed pakrym closed 6 years ago

pakrym commented 6 years ago

In Azure AppService ASP.NET Core ligthup scenario we bring additional deps and runtime store for 2.1-* runtime and set DOTNET_ADDITIONAL_DEPS/ASPNETCORE_HOSTINGSTARTUPASSEMBLIES environment variables that affect all dotnet processes

csc.runtimeconfig.json has TFM set to netcoreapp2.0 but shared framework version 2.1.0-preview1-26116-04 which causes additional deps file to be resolved from 2.1.0-preview1-26116-04 hive while runtime store is resolved from netcoreapp2.0 hive that doesn't even exists (we only care about 2.1). Corehost running csc.dll is unable to resolve libraries from additional deps and crashes.

csc.runtimeconfig.json

{
  "runtimeOptions": {
    "tfm": "netcoreapp2.0",
    "framework": {
      "name": "Microsoft.NETCore.App",
      "version": "2.1.0-preview1-26116-04"
    },
    "configProperties": {
      "System.GC.Server": true
    }
  }
}

/cc @eerhardt

eerhardt commented 6 years ago

Who generates the csc.runtimeconfig.json? Is it hard-coded to tfm: netcoreapp2.0? If so, and you are targeting netcoreapp2.1, it should be updated.

If the dotnet/sdk generated an invalid runtimeconfig.json, then we have larger issues.

pakrym commented 6 years ago

/cc @jaredpar this affects preview1 release.

jaredpar commented 6 years ago

This behavior is by design. The 2.1 runtime is supposed to automatically roll 2.0 apps to 2.1. Until such a time that it's implemented the hosting components of Roslyn, such as CLI, simply overwrite our config file to have 2.1.

eerhardt commented 6 years ago

The issue is the mismatch of tfm: netcoreapp2.0 and "framework/version": 2.1.0-xxx. These two versions should be aligned, if I'm not mistaken.

/cc @steveharter @livarcocc

jaredpar commented 6 years ago

The 2.1 runtime should allow for apps that target 2.0 to silently roll forward. That is the design we're operating on and confirmed earlier with CLI.

If it's not the design then csc will literally have to multi-target for every runtime that exists going forward. That's not a sensible design.

eerhardt commented 6 years ago

@steveharter - have you given thought to how the store and additional-deps features will work in the roll forward case?

@jaredpar - so what you are saying is that when roll-forward comes in (which I think it is already in the latest 2.1 builds), then the CLI will no longer need to overwrite the csc.runtimeconfig.json file. So the file should always look like:

{
  "runtimeOptions": {
    "tfm": "netcoreapp2.0",
    "framework": {
      "name": "Microsoft.NETCore.App",
      "version": "2.0.0"
    },
    "configProperties": {
      "System.GC.Server": true
    }
  }
}

(Note the framework/version is set to 2.0.0.)

jaredpar commented 6 years ago

I'm less familiar with the runtimeconfig.json file format so can't speak with confidence to what it will look like. But my understanding is we will be able to build apps with a TF of netcoreapp2.0 and have them run on any 2.X runtime.

pakrym commented 6 years ago

Reopened on CLI: https://github.com/dotnet/cli/issues/8613

steveharter commented 6 years ago

have you given thought to how the store and additional-deps features will work in the roll forward case?

Anything more specifically?

The additional-deps is processed after the app's deps file, but before the framework deps, so any additional-deps entries will "win" over framework entries by default. However, pending completion of https://github.com/dotnet/core-setup/issues/3546 for 2.1, if a minor roll-forward has occurred on a framework, then the framework may win pending AssemblyVersion\FileVersion comparison checks (newest wins).

The store is a probing path and will only be selected if the given asset is not in the app or framework locations. There is planned work for 2.1 via https://github.com/dotnet/core-setup/issues/3461 that on a roll-forward will forward a request for a version of a package\assembly that doesn't exist anywhere (including the store) and instead look in the framework other probing locations and pick a newer version if it exists. This helps ASP.NET deployment since they don't need to deploy new bits to the store anymore.

eerhardt commented 6 years ago

Anything more specifically?

Specifically this exact issue. The app says tfm: netcoreapp2.0 and framework/version: 2.0.0. But only a netcoreapp2.1 store and the 2.1 shared framework are installed on the machine. If additional-deps are passed, these additional dependencies won't be found because there is no netcoreapp2.0 store on the machine.

steveharter commented 6 years ago

Issue dotnet/core-setup#3461 will handle the store-to-framework redirects. However that work is not yet started, although next week I should have an update. It may entail a mapping of package\version to a specific framework\version (Microsoft.AspNetCore.All\2.1.0).

pakrym commented 6 years ago

@steveharter possible issue here is not about store to framework redirects, it's about a situation where app targeting netcoreapp2.0 and shared runtime 2.0.x gets rolled to 2.1.x and picks up 2.1.x additional deps, but tfm isn't rolled and it still picks up old netcoreapp2.0 runtime store that doesn't have required packages.

steveharter commented 6 years ago

@pakrym

Is the sample csc.runtimeconfig.json in the issue description above correct? Should framework version be targeting 2.0.0 instead of 2.1.0-preview*?

How is %DOTNET_ADDITIONAL_DEPS% set, and what is it set to?

pakrym commented 6 years ago

@steveharter it's not correct and it will get fixed, but we are now supporting rolling the minor of shared runtime so the situation like this could happen even when runtime config points to 2.0 of everything.

DOTNET_ADDITIONAL_DEPS is set by AspNet Core Site Extension for Azure AppServices (https://azure.microsoft.com/en-us/blog/writing-a-site-extension-for-azure-websites/)

https://github.com/aspnet/AzureIntegration/blob/7706619fc1979b62c24e5db841e837fd279664e8/extensions/Microsoft.AspNetCore.AzureAppServices.SiteExtension/applicationHost.xdt#L10-L12

steveharter commented 6 years ago

Since the app is targeting tfm=netcoreapp2.0, we try to load those first. We don't "roll-forward" on store + tfm (currently, and no plans to do so AFAIK).

I assume the store doesn't contain the netcoreapp2.0 packages that you want to use. If so, then the store-to-framework redirect pending feature should handle that.

Does the store contain the netcoreapp2.1 packages that you want to use? Note that ASP.NET isn't currently installing store packages in 2.1.0-preview and probably won't for RTM pending https://github.com/dotnet/core-setup/issues/3461. If they did install store packages, it would be just for netcoreapp2.0 as for 2.1+ they are using the layered framework feature and not the store.

steveharter commented 6 years ago

FWIW if we need to support "roll-forward" on tfm, we would need to add the tfm entry to netcoreapp's runtimeconfig.json (and add that file), and then use that instead of the app's tfm entry. We'd have to discuss cases when the full framework chain has different entries for tfm (aspnet, netcoreapp) although netcoreapp, being the lowest level framework, should always have the highest tfm.

steveharter commented 6 years ago

@pakrym are the assets in the store from ASP.NET or are you placing your own assets there?

pakrym commented 6 years ago

@steveharter This is the site extension I'm talking about: https://dotnet.myget.org/feed/aspnetcore-release/package/nuget/Microsoft.AspNetCore.AzureAppServices.SiteExtension