Support ItemDefinitionGroup in Choose/When by JaynieBai · Pull Request #7979 · dotnet/msbuild

JaynieBai · 2022-09-16T08:14:56Z

Context

Changes Made

There is a significant error in the original Constuctor ProjectItemDefinitionGroupElement(XmlElement xmlElement, ProjectRootElement parent, ProjectRootElement containingProject). The second parameter and third parameter types are the same. Fix the constructor bug and parse ItemDefinationGroup in when and otherwise.

Testing

Add one test SupportItemDefinationGroupInWhenOtherwise()

Notes

dnfadmin · 2022-09-16T08:15:10Z

All CLA requirements met.

danmoseley · 2022-09-16T11:01:20Z

Hello, assuming the repo owners would like to take this change, they will ask for unit test coverage too.

danmoseley · 2022-09-19T14:00:30Z

src/Build.UnitTests/Parser_Tests.cs

+            MockLogger ml = ObjectModelHelpers.BuildProjectExpectSuccess(@"
+                    <Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`msbuildnamespace`>
+                        <Choose>
+                            <When Condition=` '$(OutputType)'=='Library' `>


I would expect tests that
-verify that the IDG works, and isn't ignored
-verify that multiple IDG are supported and they are processed top to bottom
-this for multiple When, as well as Otherwise

I think testing the same way as ItemGroup might cover all the scenarios. That could be a start.

Also put in some PropertyGroup and ItemGroup in there. Everything is evaluated in passes -- normally, all properties, then all IDG, then all items. each pass can "see" the previous, even if the elements were "lower" in the project file. So if there are PG and IG in here, whatever the ordering, the IG should see the IDG.

This comment is now wrong:

msbuild/src/Build/Evaluation/Evaluator.cs

Line 1495 in 5d102ae

/// We enter here in both the property and item passes, since Chooses can contain both.

it's certainly important to verify that the IDG are "working" inside Choose.

@danmoseley Should we add PropertyGroup and ItemGroup in the project definition, and verify the itemdefinitiongroup works there in the test?

/// We enter here in both the property and item passes, since Chooses can contain both.

Will remove this line or update to "We enter here in the property, itemDefinition, and item passes, since Chooses can contain all of them" ?

@danmoseley Should we add PropertyGroup and ItemGroup in the project definition, and verify the itemdefinitiongroup works there in the test?

Sorry I'm not sure what you mean. Yes, I would expect tests that verify the IDG work, including consuming property expressions

There's some confusion here, so I'll just lay out roughly what I'd want the test to look like:
Project
Choose
When
ItemGroup
ItemDefinitionGroup
PropertyGroup
Otherwise
ItemDefinitionGroup
PropertyGroup
ItemGroup

For each ItemDefinitionGroup, PropertyGroup, and ItemGroup, there should be things actually defined in it, so like for a PropertyGroup:

<PropertyGroup> <Foo>bar</Foo> </PropertyGroup

That's important because you can have something that parses without throwing an error but doesn't actually work, and you want to verify that it works.

So then you also want to make sure each of the Property/Item/ItemDefinitionGroups that you'd expected to execute actually did execute. It would be good to do that for both the When and the Otherwise case, so a Theory with one case for each would be appropriate here.

Is that clearer? (And I'm guessing that hits what you wanted, right danmoseley?)

@danmoseley I have updated the test case, please have a look. Any suggestions, please let me know. Thank you.

This looks much closer to what I'd expect, but the When and Otherwise conditions are currently identical. That means we don't know if it's following the When, Otherwise, or both. It would be good to disentangle those.

Add one condition to distinguish when it is following when or otherwise

Forgind · 2022-09-19T15:08:03Z

src/Build/Construction/ProjectItemDefinitionGroupElement.cs

        /// Initialize a parented ProjectItemDefinitionGroupElement
        /// </summary>
-        internal ProjectItemDefinitionGroupElement(XmlElement xmlElement, ProjectRootElement parent, ProjectRootElement containingProject)
+        internal ProjectItemDefinitionGroupElement(XmlElement xmlElement, ProjectElementContainer parent, ProjectRootElement containingProject)


We should probably add a check to make sure the parent isn't a Target, since we still reject ItemDefinitionGroups under targets.

In the function ParseProjectTargetElement，there is

msbuild/src/Build/Evaluation/ProjectParser.cs

Lines 629 to 631 in cc3db35

case XMakeElements.itemDefinitionGroup:

ProjectErrorUtilities.ThrowInvalidProject(childElement.Location, "ItemDefinitionGroupNotLegalInsideTarget", childElement.Name);

break;

, before to call the Itemdefinationgroup constructor, the error "MSB4163: is not allowed inside a target" has been thrown when parse the child item under the target element.

Nirmal4G · 2022-09-27T07:27:09Z

src/Build.UnitTests/Parser_Tests.cs

+        {
+            var projectContent = $@"
+                <Project ToolsVersion= `msbuilddefaulttoolsversion` xmlns= `msbuildnamespace`>
+                    <Choose>


This might be out of topic but the only child element that doesn't support Condition attribute is Choose. Is it on purpose? I find myself, on some occasions, needing to add a conditional attribute to the Choose element itself.

If I don't have that option, the code kinda becomes error prone and cumbersome.

For Example

<Choose Condition="'$(RootCheck)' == 'true'"> <When Condition="'$(ActualCheck)' == 'value1'"> </When> <When Condition="'$(ActualCheck)' == 'value2'"> </When> <Otherwise> </Otherwise> </Choose>

is better, less repetitive and less error prone than the following...

<Choose> <When Condition="'$(RootCheck)' == 'true' AND '$(ActualCheck)' == 'value1'"> </When> <When Condition="'$(RootCheck)' == 'true' AND '$(ActualCheck)' == 'value2'"> </When> <Otherwise Condition="'$(RootCheck)' == 'true'"> </Otherwise> </Choose>

This test is for the ItemDefinitionGroup supported under choose/when and choose/otherwise, and not for the choose when or otherwise condition expression testing. So, didn't add condition expression for the Choose.

only child element that doesn't support Condition attribute is Choose. Is it on purpose?

Reaching back over 15 years .. no particular reason, it just didn't occur to anyone. I don't see why it couldn't be added if they want it.

only child element that doesn't support Condition attribute is Choose. Is it on purpose?

This should be on purpose from this test case

msbuild/src/Build.OM.UnitTests/Construction/ProjectChooseElement_Tests.cs

Lines 43 to 61 in 8872ed6

/// <summary>

/// Read choose with unexpected Condition attribute.

/// Condition is not currently allowed on Choose.

/// </summary>

[Fact]

public void ReadInvalidConditionAttribute()

{

Assert.Throws<InvalidProjectFileException>(() =>

{

string content = @"

<Project>

<Choose Condition='true'/>

</Project>

";

ProjectRootElement.Create(XmlReader.Create(new StringReader(content)));

}

);

}

@JaynieBai Nice Find... Thanks for looking it up.

Seems on purpose but WHY? Instead of supporting it, they made sure it didn't. Still, WHY? I'm puzzled.

I need this badly. Any chance that this can be allowed?

I'm not sure either. @Forgind @rainersigwald Do you know anything about this?

I said why 🙂 I implemented it. It just didn't occur to me at the time that it would be useful. Today I see no reason why it can't be added.

It just didn't occur to me at the time that it would be useful.

So, were you one of devs who worked on MSBuild? 😲 I thought you were just speaking for the team. Sorry If I misunderstood you. 😥

Today I see no reason why it can't be added.

Thank you!

Ha.. no problem. Yes, but I don't speak for the current team. I work on ASPNET now. My knowledge is historical.

@Nirmal4G, as far as I'm concerned, you're free to file a bug to add conditions as an option. My first impulse is that we'd treat it the same as ItemDefinitionGroups: if you want to implement it, and you do it properly, we can take it, but we aren't likely to prioritize it. But file the bug first so we can have an official team opinion.

Forgind

Thanks for taking care of this!

Forgind · 2022-10-03T14:59:09Z

src/Build.UnitTests/Parser_Tests.cs

+            var project = ObjectModelHelpers.CreateInMemoryProject(projectContent);
+
+            var projectItem = project.GetItems("A").FirstOrDefault();
+            Assert.Equal("bar", projectItem.EvaluatedInclude);


nit:
We're trying to use Shouldly for new tests, but I'm not blocking on that.

Forgind · 2022-10-07T14:59:52Z

Thanks @JaynieBai!

Support ItemDefinitionGroup in Choose/When

48f63c4

JaynieBai requested review from Forgind and rainersigwald September 17, 2022 08:08

Add one test

cbb2d4a

danmoseley reviewed Sep 19, 2022

View reviewed changes

Forgind reviewed Sep 19, 2022

View reviewed changes

JaynieBai added 2 commits September 26, 2022 16:53

Update test case

10da885

Add one condition to disentangle when and otherwise

483ca4b

Nirmal4G reviewed Sep 27, 2022

View reviewed changes

Forgind approved these changes Oct 3, 2022

View reviewed changes

benvillalobos approved these changes Oct 3, 2022

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 3, 2022

Forgind merged commit 06db703 into main Oct 7, 2022

Forgind deleted the v-jennybai/FixIssue#5436 branch October 7, 2022 14:59

Nirmal4G mentioned this pull request Oct 7, 2022

[Feature] Support Condition attribute on Choose element #8032

Open

	case XMakeElements.itemDefinitionGroup:
	ProjectErrorUtilities.ThrowInvalidProject(childElement.Location, "ItemDefinitionGroupNotLegalInsideTarget", childElement.Name);
	break;

	/// <summary>
	/// Read choose with unexpected Condition attribute.
	/// Condition is not currently allowed on Choose.
	/// </summary>
	[Fact]
	public void ReadInvalidConditionAttribute()
	{
	Assert.Throws<InvalidProjectFileException>(() =>
	{
	string content = @"
	<Project>
	<Choose Condition='true'/>
	</Project>
	";

	ProjectRootElement.Create(XmlReader.Create(new StringReader(content)));
	}
	);
	}

Conversation

JaynieBai commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

dnfadmin commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nirmal4G Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

For Example

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nirmal4G Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind commented Oct 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JaynieBai commented Sep 16, 2022 •

edited

Loading

dnfadmin commented Sep 16, 2022 •

edited

Loading

Forgind Sep 20, 2022 •

edited

Loading

Nirmal4G Sep 27, 2022 •

edited

Loading

Nirmal4G Sep 29, 2022 •

edited

Loading