Prevent duplicate targets Fixes #5071 #5109

Forgind · 2020-02-05T18:09:57Z

Duplicate targets break the build; this prevents MSBuild's default targets from being added if they already exist.

jeffkl · 2020-02-05T18:23:47Z

src/Build/Construction/Solution/SolutionProjectGenerator.cs

-            AddReferencesBuildTask(target, targetName, outputItem);
+            if (!traversalProject.Targets.Select(target => target.Key).Contains(targetName ?? "Build"))
+            {
+                ProjectTargetInstance target = traversalProject.AddTarget(targetName ?? "Build", string.Empty, string.Empty, outputItemAsItem, null, string.Empty, string.Empty, string.Empty, string.Empty, false /* legacy target returns behaviour */);


This fix looks right, but can you add a unit test that proves it does what we think?

src/Build.UnitTests/Construction/SolutionProjectGenerator_Tests.cs

rainersigwald · 2020-02-05T22:17:10Z

src/Build/Construction/Solution/SolutionProjectGenerator.cs

@@ -1969,8 +1969,11 @@ private static void AddTraversalReferencesTarget(ProjectInstance traversalProjec
                outputItemAsItem = "@(" + outputItem + ")";
            }

-            ProjectTargetInstance target = traversalProject.AddTarget(targetName ?? "Build", String.Empty, String.Empty, outputItemAsItem, null, String.Empty, String.Empty, String.Empty, String.Empty, false /* legacy target returns behaviour */);
-            AddReferencesBuildTask(target, targetName, outputItem);
+            if (!traversalProject.Targets.Select(target => target.Key).Contains(targetName ?? "Build"))


nit: extract targetName ?? "Build" to a variable since it's used twice

rainersigwald · 2020-02-06T05:37:00Z

src/Build.UnitTests/Construction/SolutionProjectGenerator_Tests.cs

+                instances.ShouldHaveSingleItem();
+                if (!name.Equals("name.that.does.Not.Affect.The.Build.targets"))
+                {
+                    instances[0].Targets["Build"].AfterTargets.ShouldBe("NonsenseTarget");


I don't think this is right. When defined in before, the dummy target from the bug should be overridden by the generated target "in the body" of the metaproject.

Have you validated that this change behaves correctly using the repro steps from #5071 (comment)?

Ah, I think this was a design issue. I thought it would be best for the user's version to override the generated version no matter what. I can change it.

…s.cs Co-Authored-By: Rainer Sigwald <raines@microsoft.com>

In 16.3, the default version overrode before/after.<sln>.targets versions. This changes back to that mode.

rainersigwald · 2020-02-06T19:53:17Z

src/Build/Construction/Solution/SolutionProjectGenerator.cs

+            if (traversalProject.Targets.Select(target => target.Key).Contains(correctedTargetName))
+            {
+                traversalProject.RemoveTarget(correctedTargetName);
+            }


You can just do this unconditionally; RemoveTarget doesn't throw if it's not there.

But this depends on the "how did pre-16.4 behave" question WRT having stub targets in before/after.

Sorry—I should have mentioned this rather than just pushing the change. In 16.3, if you have a before...targets or an after...targets with a Build target, the Build target is overwritten by the generated target.

tg73 · 2021-07-04T10:39:27Z

I've just run into the situation where I can no longer redefine the Build target, for example via Directory.Solution.targets - my redefinition of the Build target is silently ignored. This appears to be due to this 'fix'. I can redefine any other target (such as Restore). Why is this considered a desirable restriction?

UPDATE:

I've come up with a workaround of sorts:

<Target
  Name="_CustomBeforeBuild"
  BeforeTargets="Build"
  Outputs="@(CollectedBuildOutput)">

  <!-- DO YOUR ACUTAL Build TARGET WORK HERE (or via DependsOnTargets etc) -->

  <!-- Remove all @(ProjectReference) which effectively turns the default Build target into a no-op -->
  <ItemGroup>
    <_OriginalProjectReference Include="@(ProjectReference)"/>
    <ProjectReference Remove="@(ProjectReference)" />
  </ItemGroup>

</Target>

<Target
  Name="_CustomAfterBuild"
  AfterTargets="Build">

  <!-- Restore all @(ProjectReference) in case another target is due to be executed after Build -->
  <ItemGroup>
    <ProjectReference Include="@(_OriginalProjectReference)" />
  </ItemGroup>

</Target>

Forgind · 2021-07-13T23:56:51Z

Can you define the Build target by putting it in a separate file that you import in Directory.Build/Solution.props? I think Directory.Solution.targets comes in after the dummy Build target was added.

tg73 · 2021-07-25T17:17:24Z

@Forgind Apologies for the delay, vacation. No, your suggestion does not work. The custom build target is ignored. Looking at the code, and with the disclaimer that this is my naive, non-expert understanding:

These lines in CreateTraversalInstance appear to seek to retain any custom definition of the standard targets, removing only the placeholder empty targets, and then call AddStandardTraversalTargets apparently with the intention to add in the full versions only of those targets that have already been removed:

https://github.com/Forgind/msbuild/blob/1d845f30213e9ba4f36d4d5a366c0cc8285eed6e/src/Build/Construction/Solution/SolutionProjectGenerator.cs#L986-L997

However, AddStandardTraversalTargets calls AddTraversalReferencesTarget for each standard target, which forcibly removes any existing target. This appears to undermine the intent of CreateTraversalInstance:

https://github.com/Forgind/msbuild/blob/3e49893950c9a5e9f39a4d887b93810946361eef/src/Build/Construction/Solution/SolutionProjectGenerator.cs#L1961-L1977

An earlier version of AddTraversalReferencesTarget appears to have suitable conditional logic in place:

https://github.com/Forgind/msbuild/blob/fb82bc8216051c467850de04fe24e42026a02167/src/Build/Construction/Solution/SolutionProjectGenerator.cs#L1964-L1977

Prevent duplicate targets

0c362a4

Duplicate targets break the build; this prevents MSBuild's default targets from being added if they already exist.

jeffkl reviewed Feb 5, 2020

View reviewed changes

Added test

fb82bc8

rainersigwald requested changes Feb 6, 2020

View reviewed changes

Forgind and others added 3 commits February 6, 2020 08:31

Update src/Build.UnitTests/Construction/SolutionProjectGenerator_Test…

8f76c6d

…s.cs Co-Authored-By: Rainer Sigwald <raines@microsoft.com>

Update src/Build.UnitTests/Construction/SolutionProjectGenerator_Test…

7010a2c

…s.cs Co-Authored-By: Rainer Sigwald <raines@microsoft.com>

Flipped overwriting

1598b4f

In 16.3, the default version overrode before/after.<sln>.targets versions. This changes back to that mode.

rainersigwald reviewed Feb 6, 2020

View reviewed changes

Remove condition

3e49893

rainersigwald approved these changes Feb 12, 2020

View reviewed changes

Forgind merged commit 77da97f into dotnet:master Feb 12, 2020

Forgind deleted the duplicate-build-target branch February 12, 2020 23:34

Forgind restored the duplicate-build-target branch March 11, 2020 21:07

Forgind deleted the duplicate-build-target branch March 12, 2020 15:42

rainersigwald mentioned this pull request May 13, 2021

Add solution-validation targets as hook points #6454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplicate targets Fixes #5071 #5109

Prevent duplicate targets Fixes #5071 #5109

Forgind commented Feb 5, 2020

jeffkl Feb 5, 2020

rainersigwald Feb 5, 2020

rainersigwald Feb 6, 2020

Forgind Feb 6, 2020

rainersigwald Feb 6, 2020

rainersigwald Feb 6, 2020

Forgind Feb 6, 2020

tg73 commented Jul 4, 2021 •

edited

Loading

Forgind commented Jul 13, 2021

tg73 commented Jul 25, 2021

Prevent duplicate targets Fixes #5071 #5109

Prevent duplicate targets Fixes #5071 #5109

Conversation

Forgind commented Feb 5, 2020

jeffkl Feb 5, 2020

Choose a reason for hiding this comment

rainersigwald Feb 5, 2020

Choose a reason for hiding this comment

rainersigwald Feb 6, 2020

Choose a reason for hiding this comment

Forgind Feb 6, 2020

Choose a reason for hiding this comment

rainersigwald Feb 6, 2020

Choose a reason for hiding this comment

rainersigwald Feb 6, 2020

Choose a reason for hiding this comment

Forgind Feb 6, 2020

Choose a reason for hiding this comment

tg73 commented Jul 4, 2021 • edited Loading

Forgind commented Jul 13, 2021

tg73 commented Jul 25, 2021

tg73 commented Jul 4, 2021 •

edited

Loading