-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Adding target platform preprocessor symbols #11635
Conversation
@dsplaisted this is my first pass at adding preprocessor symbols. I haven't included backwards compatibility for target platforms yet, since a way to check for supported target platforms hasn't been implemented yet (afaik). |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
<_NetCoreAppFrameworkVersions Include="@(SupportedNETCoreAppTargetFramework->'%(Identity)'->TrimStart('.NETCoreApp,Version=v'))" /> | ||
<_NetCoreAppCompatibleVersions Include="@(_NetCoreAppFrameworkVersions)" Condition=" $([MSBuild]::VersionGreaterThan(%(Identity), 3.1)) and $([MSBuild]::VersionLessThan(%(Identity), $(TargetFrameworkVersion))) " /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice approach to this problem! But since it has to happen on every build, it's making me think about having some precomputation at SDK build time for faster execution time. We can keep that in our back pocket for the future.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
@dsplaisted do you have any more feedback here? I've updated the issue to track the other half of this work, which I'll make in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would kind of like one test that tests the compilation constants end to end. IE compile an app with the following code and verify that you get the expected output when you run it:
#if NETCOREAPP
Console.WriteLine("NETCOREAPP");
#endif
#if NETCOREAPP3_1
Console.WriteLine("NETCOREAPP3_1");
#endif
#if NET
Console.WriteLine("NET");
#endif
#if NET5_0
Console.WriteLine("NET5_0");
#endif
// etc.
The tests that expect WINDOWS to be defined will need to be updated once we fix that issue, though we don't need to block this PR on that.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Show resolved
Hide resolved
itemGroup.Add(supportedFramework); | ||
}); | ||
|
||
AssertDefinedConstantsOutput(testAsset, targetFramework, new[] { "NETCOREAPP", "NET", "WINDOWS", "WINDOWS7_0" }.Concat(expectedDefines).ToArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem we didn't realize: The common targets set the TargetPlatformIdentifier
to Windows
if it's not already set. This is not what we planned for, so we'll have to figure out how to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I noticed that while implementing this. Once we add the condition for including those defaults I'll have to come back and change this test (remove the WINDOWS
assertions in this line).
What about the "OS flavored" preprocessors like NET6_0_WINDOWS. I don´t get these to work. When will these implemented? |
@Maxyeah There are separate symbols defined for the OS. So you can do something like |
Ok thanks, but what are the combined prepocessors for? |
I'm not sure what you mean. If you're talking about something like |
Adds the first half of #11236
Fixes #11544