dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.21k stars 1.35k forks source link

PrepareForRunDependsOn is unconditionally set #2393

Open sharwell opened 7 years ago

sharwell commented 7 years ago

Currently PrepareForRunDependsOn is unconditionally set, hindering the ability for users to modify this value in their project files. When attempting to develop a Visual Studio extension with the new SDK, the relative ordering of Microsoft.VsSDK.targets results in extensions not getting deployed.

rainersigwald commented 7 years ago

I don't think we want to start setting those conditionally. They must be at least what is specified for the targets to work, and it's hard to define what to do if there's already some value defined: overwriting it has surprised you here, but how would we know whether to prepend or append to it? Or some other more complicated scenario?

MSBuild's extensibility model for this has relied on order of imports. That continues to work with NuGet packages (package .targets are imported late in common.targets, so they can override) and Directory.Build.targets (ditto). It's somewhat harder for manual imports, but there's a mechanism to control import order of SDK targets more precisely: import from the SDKs explicitly:

diff --git "a/msbuild\\before.proj" "b/msbuild\\after.proj"
index d6082d1..72c60e8 100644
--- "a/msbuild\\before.proj"
+++ "b/msbuild\\after.proj"
@@ -1,9 +1,12 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project>
+  <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props">

   <PropertyGroup>
     <TargetFramework>net452</TargetFramework>
   </PropertyGroup>

+  <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.targets">
+
   <Import Project="$(VSToolsPath)\VSSDK\Microsoft.VsSDK.targets" Condition="Exists('$(VSToolsPath)\VSSDK\Microsoft.VsSDK.targets')" />

 </Project>
\ No newline at end of file
MiYanni commented 2 years ago

@rainersigwald Based on what you said, it seems our repo doesn't do your recommended workaround, but uses your information for a different workaround. As you mentioned, Directory.Build.targets works with the order of imports. Thus, instead of changing the Project node to no longer use the Sdk attribute, what our repo does is load the Microsoft.VsSDK.targets in the Directory.Build.targets. This is a much cleaner solution as it doesn't involve changing the Project node and importing both the Sdk.props and Sdk.targets explicitly in the file. This seems to allow the Microsoft.VsSDK.targets to load late enough to workaround this issue appropriately.

Example

after.proj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net452</TargetFramework>
    <IsVsixProject>true</IsVsixProject>
  </PropertyGroup>

</Project>

Directory.Build.targets

<Import Project="$(VSToolsPath)\VSSDK\Microsoft.VsSDK.targets" Condition="Exists('$(VSToolsPath)\VSSDK\Microsoft.VsSDK.targets') and '$(IsVsixProject)' == 'true'" />
tmeschter commented 2 years ago

Is there a reason we can't just modify Microsoft.Common.CurrentVersion.targets to add to PrepareForRunsDependsOn rather than replacing it?

<PropertyGroup>
  <PrepareForRunDependsOn>
    $(PreareForRunDependsOn);
    CopyFilesToOutputDirectory
  </PrepareForRunDependsOn>
</PropertyGroup>