Msbuild: Move definitions of BeforeBuild and AfterBuild targets to .props instead of .targets files

[dsplaisted]

The BeforeBuild and AfterBuild targets are currently defined in Microsoft.Common.CurrentVersion.targets. I presume that the intention was for you to override them in your project files after the .targets import near the bottom of the file.

However, if you're using the new Sdk attribute on the Project element, it's not possible to put a target definition after the default .targets import. This can lead to targets that people put in their project files unexpectedly not running, with no indication why unless you examine the log file and see the message that the target has been overridden (for example, https://github.com/dotnet/sdk/issues/841).

It would be better to define the empty BeforeBuild and AfterBuild targets in a .props file so that if they occur in the body of a project the ones from the project take precedence.

@AndyGerlicher @rainersigwald @cdmihai What do you think about the compat implications of this and when we could make such a change? If we changed it for all situations, then targets defined in the "wrong" place in project files would start running where they hadn't previously. If we are not OK with that, we could change to conditionally defining these targets where they currently are, and then define them in a .props file of the .NET SDK along with a property telling the default MSBuild targets not to define them.

[cdmihai]

A workaround is to use SDK imports:

<Project>
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />
...
  <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />

  <Target Name="BeforeBuild">
...
  </Target>
</Project>

[rainersigwald]

I'm very worried about the compat impact of moving things around in common targets. I do not think we should make this change, ever.

The condition on a target is considered when the target is run, not when it is evaluated. That means there's no such thing as a conditional target definition, so there's no way to mitigate the compat impact as you describe.

Additionally, I do not think we should make changes to encourage the use of the confusing override-BeforeBuild/AfterBuild extensibility mechanism over the clearer alternative of adding a new target that runs at the right time defined by AfterTargets or BeforeTargets. I know that the comment in the old default csproj template encouraged this. That was wrong then and is wrong now.

If a user really wants to override an SDK-delivered target, they should switch to the explicit import form to make the ordering clear, as @cdmihai showed.

[dsplaisted]

I wasn't explicit in the original issue description, but you can do a conditional target definition by putting the target in a separate file and conditionally importing it. That's what I was thinking of doing for this.

I think it's going to be common for people to hit this as they port projects to the new SDK. If you just had to add AfterTargets="Build" to your target, that wouldn't be bad, but you also have to choose a different name for the target, when AfterBuild or BeforeBuild are the natural choices. So we end up with MyAfterBuild targets in the projects.

[rainersigwald]

I couldn't disagree more strongly with this:

AfterBuild or BeforeBuild are the natural choices

The natural name for a target, like the natural name for a function, is a description of what the target does--for all the same reasons. I would hard reject any code review I saw with a pattern like MyAfterBuild.

[dsplaisted]

Good point that BeforeBuild and AfterBuild are not great names. Unfortunately we've trained people to use them over 10 or so years, so in that sense I do think they are names that people will naturally use.

In general, I think we should prefer avoiding friction when adopting the new Sdk-style projects, as opposed to imposing better practices on patterns that worked fine before.

[rainersigwald]

That's definitely the line of argument that's going to convince me, if anything will. But it doesn't yet.

I don't want to saddle all new projects with the failures of the past. We've taken the new Sdk as a place to make many breaks from old behavior, and I think should be one of them.

[karolz-ms]

As Daniel said, this is going to break a lot of people and there is a lot of docs on the net telling people to use these magic names. Just run into this myself--very confusing.

If we do not want the magic, at least we should make sure that BeforeBuild/AfterBuild names are not "cursed" (they currently are, meaning, if your target is named AfterBuild, it won't be executed even if it has AfterTargets attribute set). This feels just mean and silly.

[jayoungers]

I've just started switching my project file formats over and thank god I ran into these threads: for the life of me I couldn't figure out why none of my build scripts were running.

Going from Target Name="AfterCompile" to Target Name="MyTask" AfterTargets="Compile" or from Target Name="AfterBuild" to Target Name="MyTask" AfterTargets="Build" resolved all of my issues.

Like was mentioned, from all of the tutorials out there I just assumed those special names were how you were supposed to setup these.

[davkean]

I want to add my 2cents; I've been involved in this project from day 1 - yet I was utterly confused when my AfterBuild target didn't run. If someone closely involved in the project runs into this - I can't imagine how 3rd parties can understand this change.

[terrajobst]

What is the recommended replacement?

<Target Name="MyCode" AfterTargets="AfterBuild" />

-- or --

<Target Name="MyCode" AfterTargets="Build" />

-- or --

<PropertyGroup>
  <BuildDependsOn>$(BuildDependsOn);MyCode</BuildDependsOn>
</PropertGroup>

<Target Name="MyCode" />

They aren't equivalent as they have different behavior (and there are probably more ways to do that). We should agree what the right replacement is and recommend it in our docs.

[rainersigwald]

The most direct replacement is

<Target Name="MyCode" BeforeTargets="Build" />

(note Before, not After).

That is often appropriate for a "just needs to get done eventually" target. In many cases, I would recommend using AfterTargets on the target that produces the output that needs to be modified instead, for better understandability and future extensibility. For example, you might have

<Target Name="RewriteILUsingMagic" AfterTargets="CoreCompile">
  <Exec Command="magic.exe @(IntermediateRefAssembly) -rewrite" />
</Target>

[terrajobst]

So for AfterBuild it sounds like the recommendation is

<Target Name="MyCode" BeforeTargets="Build" />

What about BeforeBuild?

[rainersigwald]

<Target Name="MyCode" BeforeTargets="CoreBuild" />

Is the direct replacement, though in this case I'd recommend even more strongly that you should refer directly to the target that will consume the output you're producing (perhaps BeforeTargets="CoreCompile" if you're generating code).

[terrajobst]

Makes sense. I supposed it would be useful to have a table with a few entries, plus the recommendation to check Microsoft.Common.targets for details, as most of it is probably not going to be documented for now...

[magol]

One issue i have with AfterBuild is when i use multiple frameworks. The target is run efter each target framework has been compiled. But how do i do if i want a target to run after all target has been compiled?

[sayedihashimi]

If we don't want to move forward BeforeBuild/AfterBuild that's one thing, but users should be able to name the targets BeforeBuild/AfterBuild as they have before. Today if you create the following target in your .csproj

<Target Name="BeforeBuild" BeforeTargets="Build">
  <Message Text="Inside BeforeBuild" Importance="high"/>
</Target>

The target will not run and you'll see the following in the log file.

Overriding target "BeforeBuild" in project "C:\Users\sayedha\source\repos\TuneTag\TuneTag
\TuneTag.csproj" with target "BeforeBuild" from project "C:\Program Files (x86)\Microsoft Visual 
Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets".

What's the value of defining an empty target in Microsoft.Common.CurrentVersion.targets for BeforeBuild and AfterBuild?

Also I'd like to echo @davkean comment.

I want to add my 2cents; I've been involved in this project from day 1 - yet I was utterly confused when my AfterBuild target didn't run. If someone closely involved in the project runs into this - I can't imagine how 3rd parties can understand this change.

I was really confused when my BeforeBuild target didn't execute, especially after adding AfterTargets. I don't think a typical customer would be able to figure out that they would need to change the name of the target. It just appears as yet another thing that doesn't work with core.

I understand we would like customers to move away from BeforeBuild/AfterBuild but there is an extensive amount of docs/info available that point users to that.

[MarkusAmshove]

I've migrated a solution with the old csproj format to the new one, but the lack of BeforeBuild did break my build.
I now have a mix of old and new format in that solution, because a project depends on a code generation tool to run before the compiler kicks in.
Is there any way (in the new format) to force a traget to be run before the compiler kicks in, without having to specify the target directly via MSBuild flags (because the build in VS should do it by default too)?

The workaround with import targets and props doesn't work for me anymore, just giving some warnings and not invoking the target.

The project is targeting net461 and I'm using VS 15.4.5

Edit:

I've just tried to go through the Properties of the project in VS and when adding a target to run before the build it adds:
<Target Name="PreBuild" BeforeTargets="PreBuildEvent">

This works for me.

[weltkante]

You did not only break BeforeBuild/AfterBuild but also BuildDependsOn. And probably all other {Target}DependsOn Variables defined in MSBuild, like CoreBuildDependsOn, RunDependsOn, and whatever else there may exist. Now we have to redo our entire Build Process which worked fine for classic projects for 10+ years. Very annoying.

@rainersigwald

I'm very worried about the compat impact of moving things around in common targets. I do not think we should make this change, ever.

The compact impact of not moving them around is that all After{Target}, Before{Target} and {Target}DependsOn Variables do not work anymore at all. Any targets files which made use of these are now subtly broken. Think of targets inside nuget packages which may be included in either classic or new project types.

[vatsan-madhavan]

We just had a project migrated to Sdk style csproj that had a codegen target defined like this that failed to run:

<PropertyGroup>
<CoreCompileDependsOn>
GenerateTemplates;
$(CoreCompileDependsOn);
</CoreCompileDependsOn>
</PropertyGroup>

I don't see CoreCompileDependsOn called out on this thread explicitly - is this a variant of the problem being discussed here?

/cc @danzil

[davkean]

Yes.

[vatsan-madhavan]

Do we have a plan for prevent these sorts of surprises? When projects are migrated over to Sdk style csproj, we should expect these to just work. This feels like a significant compatibility loss to me - esp. one that can often be a silent problem in sufficiently complex systems.

[rainersigwald]

As a general rule, migrated projects should use explicit SDK imports, rather than the implicit form, for exactly these reasons. That is, replace the old project template's

<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />

with

<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

and likewise replace

<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />

with

<Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />

After doing this, _do not_ add an SDK to the Project element; that is syntactic sugar for adding an Sdk.props import as the first line of the file and Sdk.targets as the last line.

In that way, the import order is preserved, allowing properties and targets to be overridden as they have been historically.

[weltkante]

@rainersigwald I'm not sure its just a problem for project migration, what about targets imported from nuget packages, like code generators?

I worry that this can subtly break packages which rely on After{Target}, Before{Target} or {Target}DependsOn variables. It definitely did happen for our own build customization and we didn't notice for months that some rules did not run anymore when imported into SDK projects.

For us it basically means we cannot use implicit SDK projects at all (regardless of whether they are migrated or newly created).

[rainersigwald]

@weltkante Can you please provide a concrete example? The relative import order of NuGet-package-delivered imports should not change between SDK and non-SDK projects (the "import all NuGet .targets" import comes after the common.currentversion.targets import), so I don't think I understand the problem you're describing.

Note also that it's bad practice for NuGet packages to override an extension target like AfterBuild, independently of SDK projects--what if you want to use two such packages?

[weltkante]

the "import all NuGet .targets" import comes after the common.currentversion.targets import

I wasn't aware of the concrete ordering, sorry, I guess that means it should work. Our scenario was manually referencing a targets file and I was assuming that targets referenced by nuget packages may have the same problem, but I didn't try to build an example. Given the ordering you mention it makes sense that only the explicit references in the project file itself are broken and not those included by other mechanics.

Note also that it's bad practice for NuGet packages to override an extension target like AfterBuild, independently of SDK projects--what if you want to use two such packages?

{Target}DependsOn is composable (you include the previous value of the variable when extending it) but suffers from the same bug.

[rainersigwald]

{Target}DependsOn is composable (you include the previous value of the variable when extending it) but suffers from the same bug.

That should be handled correctly for NuGet packages by the import order, too, and can be handled with explicit Sdk imports for the manual (or ported-from-non-SDK-project) case.

[rainersigwald]

@herebebeasties That's not correct. The docs describe the exact target-ordering algorithm, but the critical part is that BeforeTargets is considered _after_ DependsOnTargets.

To prove this to yourself here's a simple example:

<Project DefaultTargets="Build">
 <Target Name="Build" DependsOnTargets="BeforeBuild;CoreBuild;AfterBuild" />

 <Target Name="BeforeBuild" />
 <Target Name="CoreBuild" />
 <Target Name="AfterBuild" />

 <Target Name="Custom" BeforeTargets="Build" />
</Project>

S:\msbuild>msbuild -verbosity:detailed ordering.proj
Microsoft (R) Build Engine version 16.0.443+g5775d0d6bb for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 3/1/2019 9:46:14 AM.
     1>Project "S:\msbuild\ordering.proj" on node 1 (default targets).
     1>Building with tools version "Current".
     1>Target "BeforeBuild" in project "S:\msbuild\ordering.proj" (target "Build" depends on it):
     1>Done building target "BeforeBuild" in project "ordering.proj".
     1>Target "CoreBuild" in project "S:\msbuild\ordering.proj" (target "Build" depends on it):
     1>Done building target "CoreBuild" in project "ordering.proj".
     1>Target "AfterBuild" in project "S:\msbuild\ordering.proj" (target "Build" depends on it):
     1>Done building target "AfterBuild" in project "ordering.proj".
     1>Target "Custom" in project "S:\msbuild\ordering.proj" (target "Build" depends on it):
     1>Done building target "Custom" in project "ordering.proj".
     1>Target "Build" in project "S:\msbuild\ordering.proj" (entry point):
     1>Done building target "Build" in project "ordering.proj".
     1>Done Building Project "S:\msbuild\ordering.proj" (default targets).

Build succeeded.

I recommend against ever using AfterTargets="Build". It has very confusing semantics and buggy behavior in multiproc builds (#3345).

If you wish to be very explicit in ordering,

<Target Name="CustomBeforeBuild" BeforeTargets="BeforeBuild" />
<Target Name="CustomAfterBuild" AfterTargets="AfterBuild" />

is a better approach.

[herebebeasties]

Thank you very much for clarifying this. I hadn't realised the ordering there, which obviously makes sense when you think about it, but nonetheless gives a rather counterintuitive result that a <Target> with BeforeTargets="Build" runs _after_ the build. We'll use your suggestion as it's less likely to be inadvertently changed by other devs to "fix" it.

I've deleted my original comment to avoid confusion.

[nguerrera]

Going way back to https://github.com/Microsoft/msbuild/issues/1680#issuecomment-278504147

That means there's no such thing as a conditional target definition, so there's no way to mitigate the compat impact as you describe.

Isn't this possible like so?

Place where you want to put target but only on some condition:

<Import Project="ConditionalTargets.targets" Condition="$(SomeCondition)" />

ConditionalTargets.targets

<Project>
  <Target Name="ConditionalTarget" />
</Project>

[rainersigwald]

Yes, that's true.

[nguerrera]

Using that could we maybe do this:

1. Have a targets file with all the overridable stubs in common.targets. Have it set a flag that it has already been imported.
2. In common.props, set a variable to the path of this targets file.
3. In common.targets, import it conditioned on whether it's already been imported.
4. In sdk.props for 3.0, import the stubs early.

This should be compatible with sdk < 3.0 and all classic projects, and only break some exotic cases going from 2.0 to 3.0 SDK.

[rainersigwald]

My initial reaction to that plan is utter revulsion, followed by agreeing that we should do it since it's a bunch of gymnastics we can do to avoid user pain. I think that's the plan now.

[sharwell]

I would be more in favor of making these targets work if they were originally given the more accurate names MaybeBeforeBuild and MaybeAfterBuild. 😄

[NinoFloris]

Before this becomes legacy that carries over into new sdk forever. Would a new sdk.compat that does that and possibly more gymnastics not be preferable? Then there's at least some hint that at some point in the future this could cease to exist. Nudging people to gradual migration

[dashesy]

We have a common dependency nuget that have this in the nuspec

  <Target Name="CreateModelConfig" AfterTargets="Build" Outputs="%(Model.Identity)">
  <SomeTask Input="%(Model.Input)"/>
  </Target>

Should we change it to AfterTargets="CoreCompile" if we want to to be usable in VS2019?

[weltkante]

@dashesy no, AfterTargets attributes work just fine, this issue is about Before{TargetName} and After{TargetName} extension points (which are targets you can override) not about the BeforeTargets and AfterTargets _attributes_ (which are attributes on the target itself).

[rainersigwald]

@dashesy In general I recommend that you change to BeforeTargets="AfterBuild" (or a more specific target, depending on details).

The Build target is the very last thing to run in the default configuration; it does nothing and exists only to call BeforeBuild, CoreBuild, and AfterBuild targets (in that order). Hooking after Build has confusing semantics because it means work happens after the project is "done". That leads to bugs like #3345.

I would not recommend AfterTargets="CoreCompile" because it makes it harder for a user to inject itself into the middle (if for example they want to use an IL rewriter after the compiler but before packaging).

[ericsampson]

@rainersigwald is any work planned to help people migrating avoid this pain? It's so easy to get tripped up.

[aaronla-ms]

Ditto. We've solved it locally by adding the following to the root Directory.Build.targets our repos:

<!-- 
Alternate beforebuild/afterbuild that's compatible with both old style projects and sdk projects. See
issue https://github.com/dotnet/msbuild/issues/1680 for details: 
-->
<Target Name="XyzBeforeBuild" BeforeTargets="BeforeBuild" DependsOnTargets="$(BeforeBuildDependsOn)" />
<Target Name="XyzAfterBuild" BeforeTargets="AfterBuild" DependsOnTargets="$(AfterBuildDependsOn)" />

Note that GeneratePackageOnBuild uses that dreaded AfterTargets="Build" causing phasing issues in repos that have multiple projects (e.g. 3 nuget projects, then one down-stream project that validates them all). So when we pack as part of build, we would generally write

<PropertyGroup>
  <AfterBuildDependsOn>$(AfterBuildDependsOn);Pack;MyCustomPackageValidation;Etc</AfterBuildDependsOn>
<PropertyGroup>

And when migrating projects, this:

<Target Name="AfterBuild">
  ...
</Target>

Gets renamed and added to AfterBuildDependsOn, e.g.:

<PropertyGroup>
  <AfterBuildDependsOn>$(ThisProjectAfterBuild);AfterBuild_</AfterBuildDependsOn>
</PropertyGroup>
<Target Name="AfterBuild_">
   ...
</Target>

[ericsampson]

My initial reaction to that plan is utter revulsion, followed by agreeing that we should do it since it's a bunch of gymnastics we can do to avoid user pain. I think that's the plan now.

@rainersigwald @nguerrera or @KirillOsenkov, is this something that could be considered for .NET 6? Given the number of people that are hopefully going to be migrating off Framework onto .NET 6 and potentially run into this little footgun, it would be great to avoid their pain :) Cheers!

[nguerrera]

I support this, so thumbs up, but I don't work on .NET anymore so I can't speak for the timeline or anything.

[trivalik]

In case this is not fixed I would suggest to give a warning that target is not called or out of scope, if easy possible of course.