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

SDK doesnt publish exes correctly when used as a project reference #1675

Closed jaredpar closed 3 years ago

jaredpar commented 7 years ago

Often a group of exes need to be built and deployed together. This is common in applications like Roslyn where we ship several exes as a single unit: csc, vbc and VBCSCompiler.

Rather than building each project separately and then copying the outputs together we create deployment projects. Essentially dummy exes projects that just reference all of the exes that we need via project reference elements

<ProjectReference Include="..\..\src\Compilers\CSharp\csc\csc.csproj" />
<ProjectReference Include="..\..\src\Compilers\VisualBasic\vbc\vbc.csproj" />
<ProjectReference Include="..\..\src\Compilers\Server\VBCSCompiler\VBCSCompiler.csproj" />

This approach works fine in desktop MSBuild as it will correctly deploy all of the runtime assets. The same approach does not work on SDK projects though as the publish step fails to include critical files like deps.json and runtimeconfig.json. That completely breaks this approach.

If this isn't meant to be supported by SDK that would be unfortunate but understandable. I would expect an error though positively asserting that this case is not supported. Today everything builds and publishes without error but the output is completely unusable.

Repo:

Clone https://github.com/jaredpar/roslyn and checkout the branch repro/group-exe. Then run the following commands:

> dotnet restore build/ToolsetPackages/BaseToolset.csproj
> dotnet restore build/ToolsetPackages/CoreToolset.csproj
> dotnet restore build/Toolset/CoreToolset.csproj
> dotnet publish -o ${HOME}/temp/test --framework netcoreapp2.0 build/Toolset/CoreToolset.csproj

The directory ${HOME}/temp/test should contain the deps.json / runtimeconfig.json for csc, vbc and VBCSCompiler but it does not. That means the output of publish is unusable.

CC @khyperia

livarcocc commented 7 years ago

So, if I understand this correct, you have a project that references three other self-contained netcoreapp projects as project references and you expect that publish this aggregator project will also publish all its references as an app?

If that's the case, you are right, we do not support that. P2P references are treated as libraries and not applications. In order to accomplish this I think you could change your aggregator project to have a list of projects that it needs to publish and for you to invoke publish in each of them separately.

jaredpar commented 7 years ago

. In order to accomplish this I think you could change your aggregator project to have a list of projects that it needs to publish and for you to invoke publish in each of them separately.

That doesn't quite get the behavior we want though. Part of the advantage to using the <ProjectReference> route is that MSBuild hepls ensure we have consistent dependencies across the projects. If any dependencies are wrong the standard warnings around conflicts will come into play.

This is important because the versions need to be consistent in order to ensure the exes will run correctly when all their dependencies are dumped into a single directory.

If that's the case, you are right, we do not support that. P2P references are treated as libraries and not applications

In that case why do the app.config files get deployed at all? Thats only relevant when running the application. I would assume, possibly wrong, that app.config, runtimeconfig.json and deps.json would all be deployed or none.

dsplaisted commented 7 years ago

I would also like some way to publish multiple apps to the same folder (and correctly handle the dependencies).

However, it's not as simple as copying the additional files when an app project is referenced. The deps.json and runtimeconfig.json will have been generated within the context of the app's dependencies. If the "publish" project ends up resolving different versions of those dependencies, then the versions deployed won't match what the app is expecting, which I think could lead to failures.

jaredpar commented 7 years ago

If the "publish" project ends up resolving different versions of those dependencies, then the versions deployed won't match what the app is expecting, which I think could lead to failures.

Exactly. We want these failures. It tells us that we screwed up our dependencies :smile:

dsplaisted commented 7 years ago

Here's a potential workaround that includes the deps.json and runtimeconfig.json in the files that referenced projects will copy.

  <!-- Include MSBuild.deps.json and MSBuild.runtimeconfig.json in ContentWithTargetPath so they will be copied to the output folder of projects
       that reference this one. -->
  <Target Name="AddRuntimeDependenciesToContent" Condition=" '$(TargetFrameworkIdentifier)' == '.NETCoreApp'" BeforeTargets="GetCopyToOutputDirectoryItems">
    <ItemGroup>
      <ContentWithTargetPath Include="$(ProjectDepsFilePath)" CopyToOutputDirectory="PreserveNewest" TargetPath="$(ProjectDepsFileName)" />

      <ContentWithTargetPath Include="$(ProjectRuntimeConfigFilePath)" CopyToOutputDirectory="PreserveNewest" TargetPath="$(ProjectRuntimeConfigFileName)" />
    </ItemGroup>
  </Target>
jaredpar commented 6 years ago

@dsplaisted

How supported is that work around? This is a feature we sorely miss since switching to the new SDK / Exes (lots of duplication + missed errors). If this is fairly supported we'd consider switching to use it.

dsplaisted commented 6 years ago

@jaredpar I wouldn't call it supported. The problem is going to be if the different Exes have different versions of shared dependencies. If that's not the case, then I think everything should work well.

jaredpar commented 6 years ago

If it's not supported I'm rather hesitant to move to it. Depending on unsupported features is equally painful to both our teams when they eventually get broken

ericstj commented 6 years ago

@dsplaisted what part of this are you feeling like is unsupported? Ensuring the workaround stays working? Seems like we can just add a test case to make sure that is true. If the SDK does add support for this then you can make sure you won't break the workaround. I don't think there is anything fundamentally wrong with flowing the deps/runtimeconfig along with other things. I can see how this might be important for other project types like NuProj/WixProj/Azure-packages/etc.

It sounds like @jaredpar's not asking us to vouch for the deployment strategy of combining apps into a single directory as "supported" since he's expecting it to fail sometimes if they do the wrong thing and mentioned they'll need to test for that.

ViktorHofer commented 4 years ago

@dsplaisted can we please adjust the milestone? We have a tracking issue against this in dotnet/runtime: https://github.com/dotnet/runtime/blob/ec9d33daf4f1922b2f36f548752ac4539f370c91/eng/testing/runtimeConfiguration.targets#L19

MartyIX commented 4 years ago

This helped me https://github.com/dotnet/runtime/blob/ec9d33daf4f1922b2f36f548752ac4539f370c91/eng/testing/runtimeConfiguration.targets#L19 for my .NET Core 3.1 application:

  <ItemGroup Condition="'$(GenerateRuntimeConfigurationFiles)' == 'true'">
    <None Include="$(TestRuntimeConfigurationFile)"
          Condition="Exists('$(TestRuntimeConfigurationFile)')"
          Link="$(TargetName).exe.config"
          CopyToOutputDirectory="PreserveNewest"
          Visible="false" />
    <!--
      Include deps.json and runtimeconfig.json in ContentWithTargetPath so they will
      be copied to the output folder of projects that reference this one.
      Tracking issue: https://github.com/dotnet/sdk/issues/1675
    -->
    <ContentWithTargetPath Include="$(ProjectDepsFilePath)"
                           Condition="'$(TargetsNetCoreApp)' == 'true' and '$(GenerateDependencyFile)' == 'true'"
                           CopyToOutputDirectory="PreserveNewest"
                           TargetPath="$(ProjectDepsFileName)" />
    <ContentWithTargetPath Include="$(ProjectRuntimeConfigFilePath)"
                           Condition="'$(TargetsNetCoreApp)' == 'true'"
                           CopyToOutputDirectory="PreserveNewest"
                           TargetPath="$(ProjectRuntimeConfigFileName)" />
  </ItemGroup>

https://github.com/sbomer/linker/commit/fda18bc142d5ea44d0931e0bfdf50eeb6b5a0425

edit: useful link for my future reference: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

ViktorHofer commented 4 years ago

That's me implementing (mostly copying) what @dsplaisted suggested above 😄

MartyIX commented 4 years ago

@ViktorHofer I've just spent an hour trying to fix the original issue, so I posted your version because it's not obvious that one needs to use the latest master version in git to get to a working solution.

Thank you for your work!

ViktorHofer commented 4 years ago

Sure, np.

because it's not obvious that one needs to use the latest master version in git to get to a working solution.

what do you mean by that?

MartyIX commented 4 years ago

@dsplaisted can we please adjust the milestone? We have a tracking issue against this in dotnet/runtime: https://github.com/dotnet/runtime/blob/ec9d33daf4f1922b2f36f548752ac4539f370c91/eng/testing/runtimeConfiguration.targets#L19

@ViktorHofer This refers to some commit from 2019. This one is newer: https://github.com/dotnet/runtime/blob/ec9d33daf4f1922b2f36f548752ac4539f370c91/eng/testing/runtimeConfiguration.targets#L19 and that code works for me.

ViktorHofer commented 4 years ago

Got it, thanks for clarifying.

kirsan31 commented 4 years ago

The same approach does not work on SDK projects though as the publish step fails to include critical files like deps.json and runtimeconfig.json.

Just to note, that the problem appear not only on publish step, but on simple build too, see #11207.

urosjovanovic commented 4 years ago

Yes, it is also not copied on build action.

I have tried the workaround proposed above but had no luck. It throws a build error that it cannot copy .deps.json and .runtimeconfig.json because they do not exist.

I've added the target at the end of referenced .csproj (.net console app).

ViktorHofer commented 4 years ago

@urosjovanovic do you have a repro that you can share? The workaround above should work.

urosjovanovic commented 4 years ago

@ViktorHofer here is a minimal repro case, it's an asp.net core web app referencing net core console app. Thanks in advance.

ReferencedConsoleApp.zip

ViktorHofer commented 4 years ago

OK the reason why the proposed workaround doesn't work in your repro is because you don't have any other content items in the lib that are copied over. Please try this one:

  <Target Name="AddRuntimeDependenciesToContent"
          Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'"
          BeforeTargets="GetCopyToOutputDirectoryItems"
          DependsOnTargets="GenerateBuildDependencyFile;
                            GenerateBuildRuntimeConfigurationFiles">
    <ItemGroup>
      <ContentWithTargetPath Include="$(ProjectDepsFilePath)"
                            Condition="'$(GenerateDependencyFile)' == 'true'"
                            CopyToOutputDirectory="PreserveNewest"
                            TargetPath="$(ProjectDepsFileName)" />
      <ContentWithTargetPath Include="$(ProjectRuntimeConfigFilePath)"
                            Condition="'$(GenerateRuntimeConfigurationFiles)' == 'true'"
                            CopyToOutputDirectory="PreserveNewest"
                            TargetPath="$(ProjectRuntimeConfigFileName)" />
    </ItemGroup>
  </Target>
urosjovanovic commented 4 years ago

@ViktorHofer Thank you, this works!

However, I also noticed that some dependencies are not copied for the referenced project as well.

For example, I have a reference to System.Security.Permissions package in the console app. The System.Security.Permissions.dll is present in the output folder of the console app but is not copied over to the output of the project referencing the console app project which causes a runtime error when trying to run the console app.

Is the same workaround considered OK for this use-case, for example if I add the line:

<ContentWithTargetPath Include="$(OutDir)System.Security.Permissions.dll" CopyToOutputDirectory="PreserveNewest" TargetPath="System.Security.Permissions.dll" />

it works, but at which point this becomes too much of a hack?

What is also weird, is that for example Newtonsoft.Json package gets copied OK, both in console app outdir and the referencing project's outdir.

Don't know if these issues are related at all but I couldn't find any similar cases reported.

ViktorHofer commented 4 years ago

I guess that would be a question for @dsplaisted and @ericstj. I would say if the hack works for you, use it but beware of the risk that it might break in the future.

ericstj commented 4 years ago

However, I also noticed that some dependencies are not copied for the referenced project as well.

Dependencies shouldn't be copied. Dependencies should be allowed to flow transitively into the referenced project. That's what @jaredpar was specifically calling out as something he wanted to accomplish with this strategy here https://github.com/dotnet/sdk/issues/1675#issuecomment-338283234.

In this case System.Security.Permissions wasn't copied because your referencing project is an ASPNetCore project and AspNetCore's shared framework includes System.Security.Permissions.dll. You'd run into the same problem when referencing any package which is in the AspNetCore.App shared framework and not NetCore.App shared framework.

ltrzesniewski commented 4 years ago

Note that the workaround doesn't handle a self-contained publish correctly: the dependency won't be published in self-contained mode.

To reproduce:

dotnet new console -n ConsoleAppA
dotnet new console -n ConsoleAppB
dotnet add ConsoleAppA reference ConsoleAppB

Paste the workaround to ConsoleAppB, then:

dotnet publish ConsoleAppA -r win-x64

Now diff the two .deps.json files in ConsoleAppA\bin\Debug\netcoreapp3.1\win-x64\publish:

This can be fixed by setting the RuntimeIdentifier or RuntimeIdentifiers property in ConsoleAppB.


I'm currently using a... less subtle workaround though as I haven't seen this issue before, and I don't really want to set the RuntimeIdentifier property in the project. I'm posting it here in case it may help someone else:

In ConsoleAppA, add this:

<Target Name="AddExeDependencyForBuild" BeforeTargets="CopyFilesToOutputDirectory">
  <MSBuild Projects="..\ConsoleAppB\ConsoleAppB.csproj"
           Targets="Publish"
           Properties="PublishDir=$([System.IO.Path]::GetFullPath($(OutDir)));PublishReadyToRun=false"
           BuildInParallel="$(BuildInParallel)" />
</Target>

<Target Name="AddExeDependencyForPublish" BeforeTargets="CopyFilesToPublishDirectory">
  <MSBuild Projects="..\ConsoleAppB\ConsoleAppB.csproj"
           Targets="Publish"
           Properties="PublishDir=$([System.IO.Path]::GetFullPath($(PublishDir)))"
           BuildInParallel="$(BuildInParallel)" />
</Target>

The drawback of this approach is that the project gets built twice.

I suppose you can combine both workarounds this way:


Here's yet another fix I tried, it's to be used with the original one. Add this to ConsoleAppA:

<Target Name="ForceRidAgnosticToFalse" AfterTargets="_GetProjectReferenceTargetFrameworkProperties">
  <ItemGroup>
    <_MSBuildProjectReferenceExistent Update="@(_MSBuildProjectReferenceExistent)"
                                      UndefineProperties="$([MSBuild]::ValueOrDefault('%(UndefineProperties)', '').Replace(';RuntimeIdentifier', ''))" />
  </ItemGroup>
</Target>

This is not great either, as it depends on and modifies "internal" data (targets and items whose names start with an underscore).


Anyway, nothing I posted here is particularly pleasant... I'd be grateful if the SDK was fixed to be able to handle exe references correctly. 😉

AaronRobinsonMSFT commented 4 years ago

Fixing this would be helpful for testing the DllImport source generation with DNNE.

See https://github.com/dotnet/runtimelab/pull/124.

TonyValenti commented 4 years ago

We're running into challenges because of this too. It would be really nice if this could get resolved.

MartyIX commented 4 years ago

How can I use https://github.com/dotnet/sdk/issues/1675#issuecomment-658779827 in .NET 5?

I believe that '$(TargetFrameworkIdentifier)' == '.NETCoreApp' needs to be modified but I'm not sure what the new identifier is.

ltrzesniewski commented 4 years ago

@MartyIX $(TargetFrameworkIdentifier) is still .NETCoreApp in .NET 5.

ChrisUnityArto commented 4 years ago

I'm glad to see this is due to be worked on.

ViktorHofer commented 3 years ago

Fixed by https://github.com/dotnet/sdk/pull/14488

kirsan31 commented 3 years ago

@ViktorHofer In What VS version it will be included?

dsplaisted commented 3 years ago

It should be in .NET SDK 5.0.200, which will ship with VS 16.9

RandomEngy commented 3 years ago

VS 16.9.0 is out and it works! Thanks for the fix.

MartyIX commented 3 years ago

Fixed by #14488

This works for me in .NET 5. However, I wonder whether this issue is back in .NET 6 RC 2 as I see the following issue: https://github.com/dotnet/efcore/issues/26350?

Would anybody know?

jeremy-visionaid commented 4 months ago

Looks like this is still a problem for Managed C++ dependencies, but the workarounds suggested here for adding it to ContentWithTargetPath manually work for those too. I'll open a new issue if I get the chance.