Make multi-targeting constants local properties

[m0sa]

The DefineConstants properties for multi targeting builds are a bit confusing, since they are global and not local. This means that if there is a custom -p:DefineConstants=WHATEVER on the command line, the multi targeting constants won't get added.

What I'm asking here is to add a TreatAsLocalProperty="DefineConstants" to Microsoft.NET.Sdk.CSharp.targets, (and possibly other .target / .props files where DefineConstants gets updated), so that it works consistently when the consuming build modifies it via a PropertyGroup as well as via command line args.

---

Let me elaborate. We have an old non-SDK style solution that we are gradually refactoring into AspNetCore and SDK-style. We were confronted with the build on the CI server failing miserably, because the target framework constants weren't defined during the build (e.g. #if NET462 and #if NETCOREAPP2_2 were not working as expected). Turned out that we had some build configurations passing in a custom DefineConstants property. As this property is global, this line Microsoft.NET.Sdk.CSharp.targets does not work like a non-msbuild expert would would expect from reading it. It took us multiple days of digging through MSBuildBinLog, and collective WTFing, to finally figure out what's going on. Ironically, we did skim over the SDK.CSharp.targets, as it was seemingly not doing what it was supposed to do, but it looked OK at first glance.

Adding to the confusion was the fact that if you add a custom <PropertyGroup><DefineConstants>WHATEVER</DefineConstants></PropertyGroup>, it gets concatenated, and not overridden.

Ideally, it would behave the same way (concatenate the existing value) when it gets passed in via the command line arg.

[dasMulli]

Passing global parameters on the command line (or just having conflicting environment variables by accident - such as Platform which happens on some laptops) can have multiple aspects that can mess up your build quite easily.
Just yesterday @jchannon ran into something else - https://twitter.com/jchannon/status/1087414442013405184.

Three behaviors of global properties seem to surprise people:

1. They override / disable a lot of defaults (e.g. you won't have DEBUG / RELEASE etc. added by default, possibly rendering Debug.Assert() or similar useless)
2. They also apply to referenced projects. (-p:AssemblyName=foo case)
3. They usually aren't considered for incremental compation. Up-to-date checking relies on file timestamps. Since you don't change the project and it happens to be something that doesn't affect file names then you may not cause a rebuild when you actually wanted to. DefineConstants is a great example. (It will be considered for incremental compilation in MSBuild 16 - DefineConstants should be considered for CoreCompile incrementality msbuild#3350)

Now we could start a list with all the properties that you may want to pass via command line and try to find the best way to deal with each of them.
TreatAsLocalProperty may be a quick fix for most of these but also require build authors to know which properties are marked as such.
Note that the inverse expectation to yours also exists. Build authors that know how global properties work will believe that -p:DefineConstants will override all defaults and allow them to specify the exact set of constants that they want. If this is changed now, it will be a breaking change (!) to existing builds.

My personal suggestion is to use custom properties wherever possible, like

<DefineConstants Condition="'$(AdditionalDefineConstants)' != ''">$(DefineConstants );$(AdditionalDefineConstants)</DefineConstants>

or adding a logical switch to the project (-p:EnableFeatureXYZ=true).

But I also believe that some areas like DefineConstants need a proper extensibility design.
In this case i think adding a proper AdditionalDefineConstants somewhere in the SDK or even core MSBuild targets seems reasonable as it states what it is: "add these".

[m0sa]

I don't think the "inverse expectation makes" sense in the context of a multi-target build, though.

Multi-target builds branch out / build the same project with in a slightly different way. I'd expect this to be stable, regardless of what gets passed in the command line.

The inverse case would only work if the build author would specify an explicit TargetFramework (to prevent branching) in addition to DefineConstants. Otherwise they'd break all the incorrect #if <FrameworkVersion> pragmas.

[dasMulli]

It's a very tricky situation.
If you build using -p:DefineConstants=FOO you clear out all the defaults. the TF-specific constans are also missing in case of single-targeting (since user code could technically use them, e.g. when having shared code files that are included in multiple projects).
Simply changing from single-targeting to mulit-targeting would then change the effect of the global property, even if you don't add any additional target frameworks.
My goal would be to have something that works similar in all cases - in this case not adding TF-specific constants and removing all defaults. Even though i agree that this may be unsuspected and not a very extensible design.

[m0sa]

Hmn, yes, it's a bit tricky since TreatAsLocalProperty works on all subsequent imports, not only on the .targets file it's declared in. If the import is at the bottom, before the global property groups, it does scope it. This seems to be the case here in the chain of Sdk.targets -> Microsoft.NET.Sdk.targets -> Microsoft.NET.Sdk.CSharp.targets imports, though.

[github-actions]

Due to lack of recent activity, this issue has been labeled as 'stale'. It will be closed if no further activity occurs within 30 more days. Any new comment will remove the label.

[KalleOlaviNiemitalo]

#45638 describes the same problem, although with a different solution. It is being fixed in #45696.

[teo-tsirpanis]

#45696 was just merged. @dsplaisted, can we close this?

[dsplaisted]

Sounds like this will be fixed be #45696.