New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move definitions of BeforeBuild and AfterBuild targets to .props instead of .targets files #1680
Comments
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> |
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- 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. |
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 |
I couldn't disagree more strongly with this:
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 |
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. |
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. |
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. |
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 Like was mentioned, from all of the tutorials out there I just assumed those special names were how you were supposed to setup these. |
I want to add my 2cents; I've been involved in this project from day 1 - yet I was utterly confused when my |
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. |
The most direct replacement is <Target Name="MyCode" BeforeTargets="Build" /> (note That is often appropriate for a "just needs to get done eventually" target. In many cases, I would recommend using <Target Name="RewriteILUsingMagic" AfterTargets="CoreCompile">
<Exec Command="magic.exe @(IntermediateRefAssembly) -rewrite" />
</Target> |
So for <Target Name="MyCode" BeforeTargets="Build" /> What about |
<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 |
Makes sense. I supposed it would be useful to have a table with a few entries, plus the recommendation to check |
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? |
If we don't want to move forward
The target will not run and you'll see the following in the log file.
What's the value of defining an empty target in Also I'd like to echo @davkean comment.
I was really confused when my 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. |
I've migrated a solution with the old csproj format to the new one, but the lack of 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 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: This works for me. |
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.
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. |
We just had a project migrated to Sdk style csproj that had a codegen target defined like this that failed to run:
I don't see /cc @danzil |
Yes. |
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. |
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 In that way, the import order is preserved, allowing properties and targets to be overridden as they have been historically. |
@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). |
@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 |
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.
|
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. |
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 I've deleted my original comment to avoid confusion. |
Going way back to #1680 (comment)
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> |
Yes, that's true. |
Using that could we maybe do this:
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. |
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. |
I would be more in favor of making these targets work if they were originally given the more accurate names |
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 |
…r MS recommendations for csproj (dotnet/msbuild#1680) - Naming convention in UpdateVerify.cs
…r MS recommendations for csproj (dotnet/msbuild#1680)
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 |
@dashesy no, |
@dashesy In general I recommend that you change to The I would not recommend |
@rainersigwald is any work planned to help people migrating avoid this pain? It's so easy to get tripped up. |
This was caused on the migration of the project format by this issue: dotnet/msbuild#1680 It means we can no longer use AfterBuild and BeforeBuild targets like this, because in the new project format they will be ignored.
Ditto. We've solved it locally by adding the following to the root Directory.Build.targets our repos:
Note that GeneratePackageOnBuild uses that dreaded
And when migrating projects, this:
Gets renamed and added to AfterBuildDependsOn, e.g.:
|
@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! |
I support this, so thumbs up, but I don't work on .NET anymore so I can't speak for the timeline or anything. |
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. |
I just ran into this when I happened to name my target I see some people talking about dotnet build
build log file
The target just doesn't appear at all in the build logs, with no warning that it was skipped or overridden. Where was I supposed to see the warning? |
Based on feedback from dotnet/msbuild#1680 (comment)
The
BeforeBuild
andAfterBuild
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 theProject
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
andAfterBuild
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.
The text was updated successfully, but these errors were encountered: