Override default properties with custom file-level directives #49767
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes default MSBuild properties into a single s_defaultProperties dictionary, applies file‐level directives to override them, and updates tests to reflect the new XML output format.
- Extract default
<PropertyGroup>values intos_defaultPropertiesand write only non‐overridden defaults. - Update
RunFileTestsandDotnetProjectConvertTeststo remove hardcoded<TargetFramework>entries and remove interpolation. - Add a new
Directives_AllDefaultOverriddentest to verify that overriding all defaults yields exactly one<PropertyGroup>.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Removed the default <TargetFramework> line from the expected XML |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Switched expectedProject to a non‐interpolated literal and removed <TargetFramework> |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Consolidated default property output into s_defaultProperties and adjusted XML writing logic |
Comments suppressed due to low confidence (2)
test/dotnet.Tests/CommandTests/Run/RunFileTests.cs:1860
- The test no longer expects the default
<TargetFramework>property, but the code always emits it froms_defaultProperties. Update this test to re‐introduce<TargetFramework>net10.0</TargetFramework>(or the current default) in the expected XML.
<OutputType>Exe</OutputType>
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:670
- [nitpick] The indentation for these dynamically generated property tags may not align with other XML elements. Consider using a consistent approach like
writer.WriteLine($" <{name}>{EscapeValue(value)}</{name}>");to enforce two spaces for the element and two more for its children.
<{name}>{EscapeValue(value)}</{name}>
| } | ||
| writer.WriteLine(" </PropertyGroup>"); | ||
| writer.WriteLine(); | ||
| } | ||
|
|
||
| if (isVirtualProject) | ||
| { | ||
| writer.WriteLine(""" |
There was a problem hiding this comment.
[nitpick] The code currently mixes raw string literals and direct WriteLine calls for XML fragments. For consistency, you could unify on one style (either always use raw string literals or always use WriteLine with interpolated strings).
| writer.WriteLine(""" | |
| writer.WriteLine($""" |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Personally, I would also agree with copilot here.
There was a problem hiding this comment.
either always use raw string literals or always use WriteLine with interpolated strings
I'm not sure what is being suggested here. Why would I use WriteLine with interpolated strings when I don't need anything interpolated? And when I'm using interpolated strings, they are raw string literals.
There was a problem hiding this comment.
Oh, the suggested fix is bogus. I'm talking about the mix of raw vs non-raw
| private static readonly FrozenDictionary<string, string> s_defaultProperties = FrozenDictionary.Create<string, string>(StringComparer.OrdinalIgnoreCase, | ||
| [ | ||
| new("OutputType", "Exe"), | ||
| new("TargetFramework", "net10.0"), |
There was a problem hiding this comment.
Any reason we can't use ToolsetInfo.CurrentTargetFramework here? I think that's the right value.
There was a problem hiding this comment.
ToolsetInfo is only available in tests. I'm introducing a product equivalent in #49528 and once that's merged I expect it to be used here (there will be conflicts that will remind me to do that).
| #!/program | ||
| #:sdk Microsoft.NET.Web.Sdk | ||
| #:property OutputType=Exe | ||
| #:property TargetFramework=net11.0 |
There was a problem hiding this comment.
main for the SDK is not net11 yet.
There was a problem hiding this comment.
I know, this is supposed to be a different version for testing, I can probably use something like net472 to make that clearer.
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net11.0</TargetFramework> |
|
@jaredpar for a review, thanks |
|
@333fred for a review, thanks |
|
/backport to release/10.0.1xx-preview7 |
|
Started backporting to release/10.0.1xx-preview7: https://github.com/dotnet/sdk/actions/runs/16450060171 |
Resolves #49177.
I've also consolidated the project writing logic so each "section" is responsible for its own trailing blank line.