Skip to content
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.

Sign up for GitHub

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

Ensure built-in and imported targets are respected by the solution pr… #1590

Merged
merged 2 commits into from
Jan 20, 2017
Merged

Ensure built-in and imported targets are respected by the solution pr… #1590

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a30b3b
Ensure built-in and imported targets are respected by the solution pr…
jeffkl Jan 19, 2017
06c8ce1
Add unit test for after targets
jeffkl Jan 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 20 additions & 38 deletions src/XMakeBuildEngine/Construction/Solution/SolutionProjectGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,6 @@ internal class SolutionProjectGenerator
"Publish"
);

/// <summary>
/// Internal implementation targets that can't be used by users
/// </summary>
internal static readonly ISet<string> _solutionGeneratedTargetNames = ImmutableHashSet.Create<string>(StringComparer.OrdinalIgnoreCase,
"ValidateSolutionConfiguration",
"ValidateToolsVersions",
"ValidateProjects",
"GetSolutionConfigurationContents",
"GetFrameworkPathAndRedistList"
).Union(_defaultTargetNames);

/// <summary>
/// Version 2.0
/// </summary>
Expand Down Expand Up @@ -162,27 +151,10 @@ IReadOnlyCollection<string> 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<string> GetUserTargets(IReadOnlyCollection<string> targetNames, IEnumerable<string> 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
Expand Down Expand Up @@ -800,8 +772,9 @@ private void EvaluateAndAddProjects(List<ProjectInSolution> 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);
}
Expand All @@ -813,6 +786,12 @@ private void EvaluateAndAddProjects(List<ProjectInSolution> 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);
}
}

/// <summary>
Expand All @@ -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);
}
}

/// <summary>
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances is this not redundant with the filtering you do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the project name contains a . or if you have two projects with the same name (like foo.csproj and foo.vbproj) then only this method knows what the final target names will be.

I tried to explain that in the comment. Should I reword it so it makes more sense?

{
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
Expand Down
31 changes: 26 additions & 5 deletions src/XMakeBuildEngine/UnitTests/Construction/SolutionProjectGenerator_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it was a late regression, I'd love to see an explicit test of the after.sln.targets scenario. Would that be prohibitively hard to produce?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I added a test for this.

}

#region Helper Functions
Expand Down