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, dotnet/sdk#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.

[JohnYoungers]

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...