Move generating implicit framework defines to a target. #45696
Conversation
ghost
added
|
Would this (or, would you want to help) fix #43908 too by any chance? 😅 |
There was a problem hiding this comment.
This looks like it could be good, but I'm not aware that setting properties in a target lets you override global properties. @rainersigwald, Do you know if this is the case?
I know that you can use the TreatAsLocalProperty attribute on the Project element to override global properties during evaluation. It sounds like moving these to a target may have other benefits so if the target approach works probably it's best to stick with that.
It would also be nice to add a test case covering this. Would you like some help with that?
teo-tsirpanis
force-pushed
the
fix-implicit-defines
branch
from
c014571 to
3d58279
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR moves the logic for generating implicit framework define constants from individual language-specific targets to a centralized GenerateTargetFrameworkDefineConstants target. This resolves an issue where framework defines were not being set when DefineConstants was specified as a global property.
- Centralizes implicit framework define generation in a new target instead of directly modifying
DefineConstantsin language-specific files - Uses the
_ImplicitDefineConstantitem pattern consistently with other targets - Adds test coverage to verify the fix works when
DefineConstantsis provided via CLI
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Microsoft.NET.Sdk.BeforeCommon.targets | Moves framework define logic to new GenerateTargetFrameworkDefineConstants target and updates target dependencies |
| Microsoft.NET.Sdk.CSharp.targets | Removes duplicated framework define logic |
| Microsoft.NET.Sdk.FSharp.targets | Removes duplicated framework define logic |
| Microsoft.NET.Sdk.VisualBasic.targets | Removes duplicated framework define logic |
| GivenThatWeWantToBuildALibrary.cs | Adds test cases to verify framework defines work with CLI-provided DefineConstants |
| .Pass(); | ||
|
|
||
| var definedConstants = getValuesCommand.GetValues(); | ||
| var expectedConstants = expectedDefines.Concat(addDefineFromCli ? ["HELLOWORLD"] : ["DEBUG", "TRACE"]); |
There was a problem hiding this comment.
Not sure if skipping TRACE and the configuration define is the best possible behavior, but I'm not going to change it in this PR. We can either open an issue about it, or issue guidance to discourage people from setting DefineConstants from the command line.
|
@dsplaisted I added tests and CI is passing. Can you take another look? |
There was a problem hiding this comment.
LGTM. @rainersigwald or @baronfel might want to take a quick look at this in case I'm missing anything.
If you want this to go into .NET 10, it needs to be retargeted or backported to the release/10.0.1xx branch.
|
This LGTM - it's important that we try and separate the user-influenceable knobs like DefineConstant from critical functionality, like the TFM/Runtime-specific constants, and this is a good step! |
|
/backport to release/10.0.1xx |
|
Started backporting to release/10.0.1xx: https://github.com/dotnet/sdk/actions/runs/17239981884 |
6 participants
This PR moves the logic that sets the implicit framework defines to the newly added
GenerateTargetFrameworkDefineConstantstarget. Instead of directly modifying theDefineConstantsproperty, this target modifies the_ImplicitDefineConstantitem like the other targets do, which causes the defines to be set even ifDefineConstantsis specified as a global property. Language-specific logic was also deduplicated.Fixes #45638. Validated locally by building the repro attached in the issue with a patched installation of the .NET SDK.