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

Error log the invalid the target name #9050

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Error log the invalid the target name #9050

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51fcd86
Parse the invalid the target name
JaynieBai Jul 19, 2023
5e66010
Update related test case
JaynieBai Jul 19, 2023
3f42c63
Revert "Update related test case"
JaynieBai Jul 20, 2023
7f1aeb2
Revert "Parse the invalid the target name"
JaynieBai Jul 20, 2023
4fc8828
Process the invalid target
JaynieBai Jul 20, 2023
5c69881
Merge branch 'main' of https://github.com/dotnet/msbuild into jennyba…
JaynieBai Jul 21, 2023
849a7a1
Add test case
JaynieBai Jul 21, 2023
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
2 changes: 1 addition & 1 deletion src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class ProjectTargetElement_Tests
[Fact]
public void AddTargetInvalidName()
{
Assert.Throws<ArgumentException>(() =>
Assert.Throws<InvalidProjectFileException>(() =>
{
ProjectRootElement project = ProjectRootElement.Create();
project.CreateTargetElement("@#$invalid@#$");
Expand Down
17 changes: 12 additions & 5 deletions src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,12 +412,19 @@ private async Task ProcessTargetStack(ITaskBuilder taskBuilder)
switch (currentTargetEntry.State)
{
case TargetEntryState.Dependencies:
// Ensure we are dealing with a target which actually exists.
var targetName = currentTargetEntry.Name;
int indexOfSpecialCharacter = targetName.IndexOfAny(XMakeElements.InvalidTargetNameCharacters);
if (indexOfSpecialCharacter >= 0)
{
ProjectErrorUtilities.ThrowInvalidProject(currentTargetEntry.ReferenceLocation, "NameInvalid", targetName, targetName[indexOfSpecialCharacter]);
JaynieBai marked this conversation as resolved.
Show resolved Hide resolved
}

// Ensure we are dealing with a target which actually exists
ProjectErrorUtilities.VerifyThrowInvalidProject(
_requestEntry.RequestConfiguration.Project.Targets.ContainsKey(currentTargetEntry.Name),
_requestEntry.RequestConfiguration.Project.Targets.ContainsKey(targetName),
currentTargetEntry.ReferenceLocation,
"TargetDoesNotExist",
currentTargetEntry.Name);
targetName);

// If we already have results for this target which were not skipped, we can ignore it. In
// addition, we can also ignore its before and after targets -- if this target has already run,
Expand All @@ -428,7 +435,7 @@ private async Task ProcessTargetStack(ITaskBuilder taskBuilder)
_targetsToBuild.Pop();

// Push our after targets, if any. Our parent is the parent of the target after which we are running.
IList<TargetSpecification> afterTargets = _requestEntry.RequestConfiguration.Project.GetTargetsWhichRunAfter(currentTargetEntry.Name);
IList<TargetSpecification> afterTargets = _requestEntry.RequestConfiguration.Project.GetTargetsWhichRunAfter(targetName);
bool didPushTargets = await PushTargets(afterTargets, currentTargetEntry.ParentEntry, currentTargetEntry.Lookup, currentTargetEntry.ErrorTarget, currentTargetEntry.StopProcessingOnCompletion, TargetBuiltReason.AfterTargets);

// If we have after targets, the last one to run will inherit the stopProcessing flag and we will reset ours. If we didn't push any targets, then we shouldn't clear the
Expand All @@ -449,7 +456,7 @@ private async Task ProcessTargetStack(ITaskBuilder taskBuilder)
// happen if our current target was supposed to stop processing AND we had no after targets, then our last before target should
// inherit the stop processing flag and we will reset it.
// Our parent is the target before which we run, just like a depends-on target.
IList<TargetSpecification> beforeTargets = _requestEntry.RequestConfiguration.Project.GetTargetsWhichRunBefore(currentTargetEntry.Name);
IList<TargetSpecification> beforeTargets = _requestEntry.RequestConfiguration.Project.GetTargetsWhichRunBefore(targetName);
bool pushedTargets = await PushTargets(beforeTargets, currentTargetEntry, currentTargetEntry.Lookup, currentTargetEntry.ErrorTarget, stopProcessingStack, TargetBuiltReason.BeforeTargets);
if (beforeTargets.Count != 0 && pushedTargets)
{
Expand Down
16 changes: 8 additions & 8 deletions src/Build/Construction/ProjectTargetElement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public string Name
int indexOfSpecialCharacter = unescapedValue.IndexOfAny(XMakeElements.InvalidTargetNameCharacters);
if (indexOfSpecialCharacter >= 0)
{
ErrorUtilities.ThrowArgument("OM_NameInvalid", unescapedValue, unescapedValue[indexOfSpecialCharacter]);
JaynieBai marked this conversation as resolved.
Show resolved Hide resolved
ProjectErrorUtilities.ThrowInvalidProject(ElementLocation.Create("MSBUILD"), "NameInvalid", unescapedValue, unescapedValue[indexOfSpecialCharacter]);
}

SetOrRemoveAttribute(XMakeAttributes.name, unescapedValue, "Set target Name {0}", value);
Expand Down Expand Up @@ -171,7 +171,7 @@ public string KeepDuplicateOutputs
if (String.IsNullOrEmpty(value) && !BuildParameters.KeepDuplicateOutputs)
{
// In 4.0, by default we do NOT keep duplicate outputs unless they user has either set the attribute
// explicitly or overridden it globally with MSBUILDKEEPDUPLICATEOUTPUTS set to a non-empty value.
// explicitly or overridden it globally with MSBUILDKEEPDUPLICATEOUTPUTS set to a non-empty value.
value = "False";
}

Expand Down Expand Up @@ -273,11 +273,11 @@ public string Returns
value,
true); /* only remove the element if the value is null -- setting to empty string is OK */

// if this target's Returns attribute is non-null, then there is at least one target in the
// parent project that has the returns attribute.
// NOTE: As things are currently, if a project is created that has targets with Returns, but then
// all of those targets are set to not have Returns anymore, the PRE will still claim that it
// contains targets with the Returns attribute. Do we care?
// if this target's Returns attribute is non-null, then there is at least one target in the
// parent project that has the returns attribute.
// NOTE: As things are currently, if a project is created that has targets with Returns, but then
// all of those targets are set to not have Returns anymore, the PRE will still claim that it
// contains targets with the Returns attribute. Do we care?
if (returnsAttribute != null)
{
((ProjectRootElement)Parent).ContainsTargetsWithReturnsAttribute = true;
Expand Down Expand Up @@ -313,7 +313,7 @@ public ElementLocation KeepDuplicateOutputsLocation
if ((location == null) && !BuildParameters.KeepDuplicateOutputs)
{
// In 4.0, by default we do NOT keep duplicate outputs unless they user has either set the attribute
// explicitly or overridden it globally with MSBUILDKEEPDUPLICATEOUTPUTS set to a non-empty value.
// explicitly or overridden it globally with MSBUILDKEEPDUPLICATEOUTPUTS set to a non-empty value.
location = NameLocation;
}

Expand Down