From 5a30b3b6300df2f13e634a1fde4ec66d7af039f6 Mon Sep 17 00:00:00 2001 From: Jeff Kluge Date: Thu, 19 Jan 2017 12:41:06 -0800 Subject: [PATCH 1/2] Ensure built-in and imported targets are respected by the solution project generator. The goal is to take whatever target the user specified from the command-line and add it to the metaproj. However, we add targets and targets can be imported. This change ensures that command-line targets are added last after everything else and they're only added if they don't already exist. This removes the need to know what built-in targets are there. If the user specifies /t:Clean, then it's still added as a built-in target and at the end when adding user specified targets it is ignored. Another case is for projects. We add a targets like "Project_Name", "Project_Name:Clean" where the project name becomes a target. However we also add sub-targets which need to be respected. These are also now added before the user specified targets are added. If you specify /t:Project_Name:Clean, it won't be added to the project as a user specified target. Unit test has been updated to hopefully test all built-in targets. Closes #1587 --- .../Solution/SolutionProjectGenerator.cs | 58 +++++++------------ .../SolutionProjectGenerator_Tests.cs | 31 ++++++++-- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs b/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs index 123a97e2b73..ad3f152e033 100644 --- a/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs +++ b/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs @@ -65,17 +65,6 @@ internal class SolutionProjectGenerator "Publish" ); - /// - /// Internal implementation targets that can't be used by users - /// - internal static readonly ISet _solutionGeneratedTargetNames = ImmutableHashSet.Create(StringComparer.OrdinalIgnoreCase, - "ValidateSolutionConfiguration", - "ValidateToolsVersions", - "ValidateProjects", - "GetSolutionConfigurationContents", - "GetFrameworkPathAndRedistList" - ).Union(_defaultTargetNames); - /// /// Version 2.0 /// @@ -162,27 +151,10 @@ IReadOnlyCollection targetNames if (targetNames != null) { - _targetNames = GetUserTargets(targetNames, solution.ProjectsInOrder.Select(_ => _.ProjectName)); + _targetNames = targetNames.Select(i => i.Split(new[] {':'}, 2, StringSplitOptions.RemoveEmptyEntries).Last()).ToList(); } } - private List GetUserTargets(IReadOnlyCollection targetNames, IEnumerable projectNames) - { - // Special target names are generated for each project like My_Project:Clean. If the user specified a value like - // that then we need to split by the first colon and assume the rest is a real target name. This works unless the - // target has a colon in it and the user is not trying to run it in a specific project. At the time of writing this - // we figured it unlikely that a target name would have a colon in it... - - var userTargets = - targetNames.Select(i => i.Split(new[] {':'}, 2, StringSplitOptions.RemoveEmptyEntries).Last()) - // The known target names are also removed from the list in case something like /t:Build was specified it is just ignored - .Except(_solutionGeneratedTargetNames, StringComparer.OrdinalIgnoreCase) - // Ignore targets that have the same name as projects, since their meaning is special. Whatever else remains, it must be a user target (e.g. the MyBuild target) - .Except(projectNames, StringComparer.OrdinalIgnoreCase).ToList(); - - return userTargets; - } - #endregion // Constructors #region Methods @@ -800,8 +772,9 @@ private void EvaluateAndAddProjects(List projectsInOrder, Lis AddTraversalTargetForProject(traversalInstance, project, projectConfiguration, "Rebuild", "BuildOutput", canBuildDirectly); AddTraversalTargetForProject(traversalInstance, project, projectConfiguration, "Publish", null, canBuildDirectly); - // Add any other targets specified by the user - foreach (string targetName in _targetNames) + + // Add any other targets specified by the user that were not already added + foreach (string targetName in _targetNames.Where(i => !traversalInstance.Targets.ContainsKey(i))) { AddTraversalTargetForProject(traversalInstance, project, projectConfiguration, targetName, null, canBuildDirectly); } @@ -813,6 +786,12 @@ private void EvaluateAndAddProjects(List projectsInOrder, Lis projectInstances.Add(metaProject); } } + + // Add any other targets specified by the user that were not already added + foreach (string targetName in _targetNames.Where(i => !traversalInstance.Targets.ContainsKey(i))) + { + AddTraversalReferencesTarget(traversalInstance, targetName, null); + } } /// @@ -828,10 +807,6 @@ private void AddStandardTraversalTargets(ProjectInstance traversalInstance, List AddTraversalReferencesTarget(traversalInstance, "Clean", null); AddTraversalReferencesTarget(traversalInstance, "Rebuild", "CollectedBuildOutput"); AddTraversalReferencesTarget(traversalInstance, "Publish", null); - foreach (string targetName in _targetNames) - { - AddTraversalReferencesTarget(traversalInstance, targetName, null); - } } /// @@ -1116,7 +1091,7 @@ private ProjectInstance CreateMetaproject(ProjectInstance traversalProject, Proj AddMetaprojectTargetForWebProject(traversalProject, metaprojectInstance, project, "Rebuild"); AddMetaprojectTargetForWebProject(traversalProject, metaprojectInstance, project, "Publish"); - foreach (string targetName in _targetNames) + foreach (string targetName in _targetNames.Where(i => !metaprojectInstance.Targets.ContainsKey(i))) { AddMetaprojectTargetForWebProject(traversalProject, metaprojectInstance, project, targetName); } @@ -1132,7 +1107,7 @@ private ProjectInstance CreateMetaproject(ProjectInstance traversalProject, Proj AddMetaprojectTargetForManagedProject(traversalProject, metaprojectInstance, project, projectConfiguration, "Rebuild", targetOutputItemName); AddMetaprojectTargetForManagedProject(traversalProject, metaprojectInstance, project, projectConfiguration, "Publish", null); - foreach (string targetName in _targetNames) + foreach (string targetName in _targetNames.Where(i => !metaprojectInstance.Targets.ContainsKey(i))) { AddMetaprojectTargetForManagedProject(traversalProject, metaprojectInstance, project, projectConfiguration, targetName, null); } @@ -1144,7 +1119,7 @@ private ProjectInstance CreateMetaproject(ProjectInstance traversalProject, Proj AddMetaprojectTargetForUnknownProjectType(traversalProject, metaprojectInstance, project, "Rebuild", unknownProjectTypeErrorMessage); AddMetaprojectTargetForUnknownProjectType(traversalProject, metaprojectInstance, project, "Publish", unknownProjectTypeErrorMessage); - foreach (string targetName in _targetNames) + foreach (string targetName in _targetNames.Where(i => !metaprojectInstance.Targets.ContainsKey(i))) { AddMetaprojectTargetForUnknownProjectType(traversalProject, metaprojectInstance, project, targetName, unknownProjectTypeErrorMessage); } @@ -1942,6 +1917,13 @@ private void AddTraversalTargetForProject(ProjectInstance traversalProject, Proj actualTargetName += ":" + targetToBuild; } + // Don't add the target again. The user might have specified /t:Project:target which was already added but only this method knows about Project:Target so + // after coming up with that target name, it can check if it has already been added. + if (traversalProject.Targets.ContainsKey(actualTargetName)) + { + return; + } + // The output item name is the concatenation of the project name with the specified outputItem. In the typical case, if the // project name is MyProject, the outputItemName will be MyProjectBuildOutput, and the outputItemAsItem will be @(MyProjectBuildOutput). // In the case where the project contains characters not allowed as Xml element attribute values, those characters will diff --git a/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs b/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs index 13b97d6192e..31ec2f4c24b 100644 --- a/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs +++ b/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs @@ -2106,14 +2106,35 @@ public void IllegalUserTargetNamesDoNotThrow() EndGlobal "); - // "GetFrameworkPathAndRedistList" is for web projects only - var illegalTargetNamesForCsproj = SolutionProjectGenerator._solutionGeneratedTargetNames.Union(new []{"ClassLibrary1"}).Except(new []{ "GetFrameworkPathAndRedistList" }).ToList(); - ProjectInstance[] instances = SolutionProjectGenerator.Generate(solution, null, null, BuildEventContext.Invalid, null, illegalTargetNamesForCsproj); + ProjectInstance[] instances; - foreach (var illegalTargetName in illegalTargetNamesForCsproj) + foreach (string builtInTargetName in new[] { - Assert.Equal(1, instances[0].Targets.Count(target => String.Compare(target.Value.Name, illegalTargetName, StringComparison.OrdinalIgnoreCase) == 0)); + null, + "Build", + "Rebuild", + "Clean", + "Publish", + "ClassLibrary1", + "ClassLibrary1:Clean", + "ClassLibrary1:Rebuild", + "GetSolutionConfigurationContents", + "ValidateProjects", + }) + { + instances = SolutionProjectGenerator.Generate(solution, null, null, BuildEventContext.Invalid, null, builtInTargetName == null ? null : new [] { builtInTargetName }); + + Assert.Equal(1, instances.Length); + + Assert.Equal(12, instances[0].TargetsCount); } + + + instances = SolutionProjectGenerator.Generate(solution, null, null, BuildEventContext.Invalid, null, new[] { "Foo" }); + + Assert.Equal(1, instances.Length); + + Assert.Equal(14, instances[0].TargetsCount); } #region Helper Functions From 06c8ce1e776917f20ee7111e85c248b6e15b2994 Mon Sep 17 00:00:00 2001 From: Jeff Kluge Date: Fri, 20 Jan 2017 09:22:40 -0800 Subject: [PATCH 2/2] Add unit test for after targets --- .../Solution/SolutionProjectGenerator.cs | 4 +- .../SolutionProjectGenerator_Tests.cs | 56 +++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs b/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs index ad3f152e033..1613aa14af4 100644 --- a/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs +++ b/src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs @@ -872,7 +872,7 @@ private ProjectInstance CreateTraversalInstance(string wrapperProjectToolsVersio // These are just dummies necessary to make the evaluation into a project instance succeed when // any custom imported targets have declarations like BeforeTargets="Build" // They'll be replaced momentarily with the real ones. - foreach (string targetName in _defaultTargetNames.Union(_targetNames)) + foreach (string targetName in _defaultTargetNames) { traversalProject.AddTarget(targetName); } @@ -897,7 +897,7 @@ private ProjectInstance CreateTraversalInstance(string wrapperProjectToolsVersio ); // Make way for the real ones - foreach (string targetName in _defaultTargetNames.Union(_targetNames)) + foreach (string targetName in _defaultTargetNames) { traversalInstance.RemoveTarget(targetName); } diff --git a/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs b/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs index 31ec2f4c24b..e5776f6b2b4 100644 --- a/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs +++ b/src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs @@ -2137,6 +2137,62 @@ public void IllegalUserTargetNamesDoNotThrow() Assert.Equal(14, instances[0].TargetsCount); } + /// + /// Verifies that when a user has an after.solution.sln.targets that the targets are not overridden by the solution project generator. + /// + [Fact] + public void AfterTargetsComeFromImport() + { + string baseDirectory = Guid.NewGuid().ToString("N"); + + string solutionFilePath = ObjectModelHelpers.CreateFileInTempProjectDirectory(Path.Combine(baseDirectory, $"{Guid.NewGuid():N}.sln"), @" + Microsoft Visual Studio Solution File, Format Version 14.00 + # Visual Studio 2015 + Project(""{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}"") = ""ClassLibrary1"", ""ClassLibrary1.csproj"", ""{6185CC21-BE89-448A-B3C0-D1C27112E595}"" + EndProject + Global + GlobalSection(SolutionConfigurationPlatforms) = preSolution + Debug|Any CPU = Debug|Any CPU + Release|Any CPU = Release|Any CPU + EndGlobalSection + GlobalSection(ProjectConfigurationPlatforms) = postSolution + {6185CC21-BE89-448A-B3C0-D1C27112E595}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {6185CC21-BE89-448A-B3C0-D1C27112E595}.Debug|Any CPU.Build.0 = Debug|Any CPU + {6185CC21-BE89-448A-B3C0-D1C27112E595}.Release|Any CPU.ActiveCfg = Release|Any CPU + {6185CC21-BE89-448A-B3C0-D1C27112E595}.Release|Any CPU.Build.0 = Release|Any CPU + EndGlobalSection + EndGlobal + "); + + ObjectModelHelpers.CreateFileInTempProjectDirectory(Path.Combine(baseDirectory, $"after.{Path.GetFileName(solutionFilePath)}.targets"), @" + + + + + "); + + try + { + var solutionFile = SolutionFile.Parse(solutionFilePath); + + ProjectInstance projectInstance = SolutionProjectGenerator.Generate(solutionFile, null, null, BuildEventContext.Invalid, null, new[] { "MyTarget" }).FirstOrDefault(); + + Assert.NotNull(projectInstance); + + Assert.True(projectInstance.Targets.ContainsKey("MyTarget")); + + Assert.Equal(1, projectInstance.Targets["MyTarget"].Children.Count); + + ProjectTaskInstance task = Assert.IsType(projectInstance.Targets["MyTarget"].Children[0]); + + Assert.Equal("MyTask", task.Name); + } + finally + { + ObjectModelHelpers.DeleteTempProjectDirectory(); + } + } + #region Helper Functions ///