Open m0sa opened 5 years ago
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:
DEBUG
/ RELEASE
etc. added by default, possibly rendering Debug.Assert()
or similar useless)-p:AssemblyName=foo
case)DefineConstants
is a great example. (It will be considered for incremental compilation in MSBuild 16 - https://github.com/Microsoft/msbuild/issues/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".
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.
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.
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.
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 whereDefineConstants
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 customDefineConstants
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.