microsoft / service-fabric

Service Fabric is a distributed systems platform for packaging, deploying, and managing stateless and stateful distributed applications and containers at large scale.
https://docs.microsoft.com/en-us/azure/service-fabric/
MIT License
3.03k stars 401 forks source link

Multitargeted ASP.NET Core apps not supported by Microsoft.VisualStudio.Azure.Fabric.MSBuild #180

Open idg10 opened 6 years ago

idg10 commented 6 years ago

I have a Service Fabric application that currently runs on .NET 4.7.1. We're looking at migrating to .NET Core 2.1, but as a stepping stone, we want to enable our build to target both .NET FX and .NET Core, instead of switching irreversibly to .NET Core.

All our C# projects use the new .NET core style project system so this should be simple: we just specify multiple frameworks, e.g.:

<TargetFrameworks>net471;netcoreapp2.1</TargetFrameworks>

However, although this works fine as far as the .NET parts of the build are concerned, this causes problems when a .sfproj refers to such a project. In theory we can deal with this by having multiple configurations, e.g. ReleaseNetFx, ReleaseNetCore etc. configurations. However, this causes subtle problems with the new-style projects system: it has built-in support for multitargetting, and the tooling expects you to use it. If you change the target based on the config, subtle things can go wrong, e.g., dotnet restore becomes configuration-sensitive (because your dependency tree for a given project can vary by target) and there are certain tools that don't expect that to be the case. (E.g., when you put VSTS's built-in dotnet build task into restore mode it actually removes the option to specify a configuration!) And when you ask the team about this, they tell you that config-sensitive package dependencies are something they advise you to avoid.

Moreover, if you try to specify the target framework based on the configuration in ASP.NET Core applications, you can sometimes get weird Razor-related errors from the build system.

The bottom line is that you're not really expected to do this.

So for this reason, you don't want .csproj files to change their targets based on the configuration: it's best to just state in your .csproj that you want to build for multiple target frameworks, as shown above.

Since .sfproj files don't have any such concept, the obvious solution would be for the .csproj files to just support ordinary Release and Debug configurations, but still to have ReleaseNetFx, ReleaseNetCore, DebugNetFx and DebugNetCore configurations for the .sfproj, and then have the .sfproj indicate which target framework it wants to use.

And this very nearly looks possible. If you just do the naive thing, and try setting <TargetFramework> inside the .sfproj it doesn't work: you get errors at the point where it invokes the GetTargetPath task, complaining that you're trying to use a multi-target project in a scenario where you can't. However, if you modify your .sfproj to use references like this:

<ProjectReference
    Include="..\My.WebApp\My.WebApp.csproj"
    SetTargetFramework="TargetFramework=net471" />

those particular errors go away. This SetTargetFramework attribute is a feature of the new project system, where you can specify that you want to use the build for a particular target framework in the referenced project.

That makes the first set of errors go away. However, a new error appears:

C:\Program Files\dotnet\sdk\2.1.301\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.CrossTargeting.targets(31,5): The 'Publish' target is not supported without specifying a target framework. The current project targets multiple frameworks, please specify the framework for the published application. [E:\dev\Test\My.WebApp\My.WebApp.csproj]

The basic problem here is that the Service Fabric tooling is not honouring the SetTargetFramework attribute on the ProjectReference.

This seems to arise from the behaviour of the custom GetServiceProjectReferences task. When building the list of project references to package, it adds a CommonBuildProperties property that passes in the Configuration and Platform to the child build. You can see it being used here:

<Target Name="PackageDeployableServices"
        Condition=" '@(DeployableServiceProjectReference)' != '' ">

  <MSBuild Projects="%(DeployableServiceProjectReference.Identity)"
           Properties="%(DeployableServiceProjectReference.CommonBuildProperties);DeployOnBuild=true;PublishUrl=%(DeployableServiceProjectReference.PublishUrl);WebPublishMethod=FileSystem"
           Condition=" '%(DeployableServiceProjectReference.IncludeCodePackage)' == 'true' "/>
  </Target>

That's line 318 of the Microsoft.VisualStudio.Azure.Fabric.Application.targets file in the NuGet Microsoft.VisualStudio.Azure.Fabric.MSBuild (v1.6.6) package.

If the custom task had also included the TargetFramework from the ProjectReference (which is what the .NET Core SDK does when launching child MSBuild tasks like this) then this error wouldn't appear. But since the Service Fabric tooling basically loses the target framework specification when it launches the child MSBuild task to create the package, you get an error from the .NET Core SDK parts of the system because they are (correctly) complaining that you support multiple targets and haven't told it which particular one you want to build a package for. Obviously, I have specified one, but the Service Fabric has erased my choice.

Ideally the Service Fabric tooling would support multitargetting just like the .NET Core SDK, but in the absence of that, it would at least be possible to work around the lack of support if the SetTargetFramework attribute on ProjectReference elements was correctly honoured.

kichalla commented 3 years ago

We are hitting the same issue in my team currently :-( .sfproj files not supporting the new SDK style format has been a pain.

kichalla commented 3 years ago

Thanks a lot for the nice write-up @idg10 . Your post helped me fix the issue at my end.

In our team we checked-in the package Microsoft.VisualStudio.Azure.Fabric.MSBuild to our repo itself (Yeah, in general this is a big NO but we had to be practical given Service Fabric projects do not support "SDK" style and they caused friction with rest of the projects which were on "SDK" style and also my team is planning to move to Kubernetes and remove Service Fabric dependency entirely :-))

So since we have the package checked-in, I did the following modifications to the Microsoft.VisualStudio.Azure.Fabric.Application.targets file based on your post:

And the above changes worked well for following way of referencing projects:

<ProjectReference Include="..\MyService\MyService.csproj" SetTargetFramework="TargetFramework=net472" />

OR if the target service project did not target multiple target frameworks

<ProjectReference Include="..\MyService\MyService.csproj"  />
Angr1st commented 3 years ago

If this fix is relativly simple to do, wouldn't it be quit easy to send a pr fixing this for everyone with this Problem?

kichalla commented 3 years ago

@Angr1st Good point. Probably the Service Fabric team can handle this. cc @dbreshears @ravipal

dbreshears commented 3 years ago

Thanks all for the investigation. Does seem like this is something we can add to our targets fairly easily but it is in our internal VS extension repo so we will need to make the change. I've added a workitem on our backlog and will look into soon.

andrey-noskov commented 3 years ago

@dbreshears , could you please provide an update? my team seems to be affected by this as well

wouterroos commented 3 years ago

@dbreshears Any update on this?

NCarlsonMSFT commented 3 years ago

Sorry for not updating here earlier. Basic support for this has been added in VS 16.11 and the NuGet package 1.7.6