-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Extend ItemGroup/PropertyGroup grammar to allow more user-friendly Conditions for common use cases #11329
Comments
Make |
This proposal makes a meaningful step towards user bliss. I resonate with the motivation. A few points in favor of making the syntax
For example, right off the bat I would miss being able to use this new syntax with TargetFrameworkIdentifier. I also like the idea of |
What will the experience be when |
Definitely yearning for Properties I'd like to see:
Ideally we'd be able to have conditions beside from just Lots of ways that syntax could be invented around this.... @StephaneDelcroix may have some thoughts given this is xml based, as he's thought through a lot of similar patterns for XAML. |
The default LangVersion and TargetFrameworkVersion are set by .NET SDK after the project has been evaluated and TargetFramework is known. So they are usable in ItemGroup conditions but generally not in PropertyGroup. I do not want MSBuild to special-case |
We already have <Choose>
<When Condition="$(Configuration)=='Test'">
<PropertyGroup>
<DebugSymbols>true</DebugSymbols>
<DebugType>full</DebugType>
<Optimize>false</Optimize>
<DefineConstants>DEBUG;TRACE</DefineConstants>
</PropertyGroup>
</When>
</Choose> I think it would be intuitive to use <PropertyGroup When.Configuration="Test">
<DebugSymbols>true</DebugSymbols>
<DebugType>full</DebugType>
<Optimize>false</Optimize>
<DefineConstants>DEBUG;TRACE</DefineConstants>
</PropertyGroup> This can even be extended to support multiple conditions: <PropertyGroup>
<When>
<Configuration>Release</Configuration>
<Platform>x64</Platform>
</When>
<DebugSymbols>true</DebugSymbols>
<DebugType>full</DebugType>
<Optimize>false</Optimize>
<DefineConstants>DEBUG;TRACE</DefineConstants>
</PropertyGroup> |
I expected this to be a breaking change; I expected it to define a property Still, it looks so much like a regular property definition that I'm not a fan of this approach. |
See also related #820. |
my .02€ It's always a headache to invent a comparison DSL to extend XML... the issue we're trying to solve is pattern matching with property pattern, and in that context, 'When' ( Multiple conditions could indeed be specified like in #11329 (comment), or simply using About multiple values, I read While we're inventing a syntax, we could have pattern matching In any form, a feature like this would be a big win for developers with non-trivial cases, and would make conditions easier to read, and write. (feel free to delete those thoughts if they derail the OP's intend) |
Re tooling changes, consider also how https://source.dot.net/ generated by https://github.com/KirillOsenkov/SourceBrowser searches for references to MSBuild properties and items. If an MSBuild project is using the |
Summary
The MSBuild
Condition
syntax is very powerful, but it's also very open-ended and hard to create tooling around. There is value in making syntax for common project- and MSBuild-logic constraints more easy-to-use, because a more-restricted but easier-to-use syntax would be both more understandable to users (especially casual MSBuild users) and could disallow entire categories of errors that exist today (e.g. typos in commonCondition
usage).Background and Motivation
Consider a user that wants to make a number of changes to Property values when they are targeting the
Release
configuration. For example, they might want to opt in toToday, this is all doable. A user might write some code like
Things to note here:
Making changes
What if the user needs to add a pivot to the condition for any reason? Say for .NET 9 they need an additional NoWarn. The code might look like this:
This change is hard to spot-check for most users.
Consolidating changes
Experienced MSBuild users might reach for a
Condition
on the PropertyGroup to reduce the amount of duplication on the properties, like so:This is great! However, many users do not realize that Conditions can be applied to these nodes, and the problem of not being able to easily-classify the changes remains. If you need to add another TFM, now you either have two levels of Condition (one at the PropertyGroup, one on the NoWarn Property) or you have two PropertyGroups.
We need a syntax that is easy to use for common pivots that users experience daily.
Proposed Feature
We should extend the grammar of PropertyGroups and ItemGroups to allow for metadata to be set that the engine would interpret as an automatically-correct Conditions. I propose that the initial set of Properties we consider for this treatment be
With these properties, users would be able to construct targeted PropertyGroups or ItemGroups that scope changes easily. An example of the above in this fashion:
During parsing, the engine would read this attribute and generate a Condition on this PropertyGroup equivalent to
Condition="$(Configuration) == 'Release'"
. Implicit conditions would be additive - setting bothConfiguration="Release"
andTargetFramework="net8.0"
would result in a compound Condition ofCondition="$(Configuration) == 'Release' and $(TargetFramework) == 'net8.0'"
.The Groups would be read and applied in-order in the file according to the existing MSBuild passes, so this change would be a purely-semantic transformation - no changes to passes or the resulting project would be intended here.
Open Questions
Why only PropertyGroups/ItemGroups?
Properties cannot have metadata today, and Items already have metadata that may clobber the existing properties - it would be hard to do this in a safe way. It's natural to group changes to properties and items based on these properties, so this extension felt natural.
Is this set of properties sufficient?
We've chosen the common user pivots today, but the syntax isn't limited to/exhaustive. This feels like a kind of change that we should start small on to keep the problem space limited/well-understood. An immediate suggestion to add might be
LangVersion
Should we limit the set of properties at all?
Yes - if we make this change we'll be supporting the syntax forever. We need to be conservative in the set of values accepted here so that we have room to evolve in the future.
Should we support comparisons on things other than Properties?
Item-comparisons are common, but due to the grammar of MSBuild Item-based comparisons can only happen on ItemGroups - is this clear to users? Would it be a foot-gun?
Should the TFM condition sematics be
Exactly Equal
orIsTargetFrameworkCompatible
?The other property checks use exact-matching, and this seems like a safer/more limited default. We could change to use an is-compatible check based on user feedback.
How should this be represented in tooling/API?
The Object Model objects should have a fully-realized
Condition
, and there should be a new Well-Known metadata key on ItemGroups and PropertyGroups that provides a tooling-visible representation of the implicit Condition terms and their values.Other notes
This syntax is very regular and I think could be more easily understood and well-formed-synthesized by LLM tools. The MSBuild language is not well known today by these tools, so something that is more simple and likely to be correct is a plus.
Could this be pluggable in some way?
It feels odd for MSBuild itself to know about specific Properties that may only be known to/used by a specific SDK. Could a given MSBuild SDK somehow provide this data?
Should compound values be allowed in the property comparisons (e.g.
TargetFramework="net8.9;net9.0"
)No - the intent of these comparisons is singular values only.
Alternative Designs
There are other ways to tackle this for sure -
Condition
syntax intactThe text was updated successfully, but these errors were encountered: