Expand MSBuild properties across launch profiles - #56074
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/Microsoft.DotNet.ProjectTools/LaunchSettings/LaunchProfileParser.cs — MSBuildPropertyRegex (\$\([^)]+\)) treats the first ) as the end of the token, which breaks… |
|
src/Microsoft.DotNet.ProjectTools/LaunchSettings/LaunchSettings.cs — ReadProfileSettingsFromFile only catches JsonException/IOException, but… |
What changed in this PR
Expands launch-profile MSBuild property evaluation beyond commandLineArgs, so dotnet run (project + file-based) and MTP test launches consistently expand MSBuild properties in supported launch-profile string fields, aligning with Visual Studio’s token replacement behavior. Adds Native AOT guards to fall back to managed execution when MSBuild evaluation would be required.
Changes:
- Plumbs an optional
expandMSBuildPropertycallback through launch profile discovery/parsing and expands MSBuild properties across additional fields (executable path, working directory, applicationUrl/launchUrl, env vars). - Updates
dotnet runto lazily evaluate the project only when launch settings contain MSBuild property tokens. - Adds/updates integration and unit tests for project, executable, file-based run, MTP, and Native AOT fallback behavior.
| File | Description |
|---|---|
| test/dotnet.Tests/ProjectTools/LaunchSettingsParserTests.cs | Adds unit coverage for MSBuild expansion in executable/project profile fields. |
| test/dotnet.Tests/CommandTests/Test/GivenDotnetTestBuildsAndRunsTests.cs | Updates MTP test launch scenario to validate MSBuild property expansion via evaluated props. |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildCommands.cs | Adds file-based dotnet run test validating MSBuild property expansion in .run.json. |
| test/dotnet.Tests/CommandTests/Run/RunCommandTests.cs | Extends run command tests to validate env var + property-function expansion and executable profile expansion. |
| test/dotnet.Tests/CommandTests/Run/GivenDotnetRunBuildsCsProj.cs | Updates applicationUrl test to validate MSBuild property expansion feeding ASPNETCORE_URLS. |
| test/dotnet-aot.Tests/AotRunCommandTests.cs | Adds Native AOT fallback tests when MSBuild expansion is required by launch profile fields. |
| src/Microsoft.DotNet.ProjectTools/LaunchSettings/ProjectLaunchProfileParser.cs | Expands MSBuild properties in additional Project profile fields (args/url/env vars). |
| src/Microsoft.DotNet.ProjectTools/LaunchSettings/LaunchSettings.cs | Threads expandMSBuildProperty through file-based profile reading and provider parsing. |
| src/Microsoft.DotNet.ProjectTools/LaunchSettings/LaunchProfileParser.cs | Introduces MSBuild property detection + expansion helper using a regex-based replacer. |
| src/Microsoft.DotNet.ProjectTools/LaunchSettings/ExecutableLaunchProfileParser.cs | Expands MSBuild properties in executable path/args/working dir/env vars. |
| src/Cli/dotnet/Commands/Test/MTP/SolutionAndProjectUtility.cs | Enables MSBuild property expansion in MTP launch profile parsing via project.ExpandString. |
| src/Cli/dotnet/Commands/Run/RunCommand.cs | Lazily supplies MSBuild expansion callback (evaluates project on demand for expansion). |
| src/Cli/dotnet/Commands/Run/CommonRunHelpers.cs | Plumbs expandMSBuildProperty through shared launch-profile reader. |
| src/Cli/dotnet/Commands/Run/AotRunCommand.cs | Adds enforcement that MSBuild-property-containing launch profiles must fall back to managed. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/Cli/dotnet/Commands/Run/RunCommand.cs — ReadLaunchProfileSettings now passes an MSBuild expansion callback into the launch-settings… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/Microsoft.DotNet.ProjectTools/LaunchSettings/LaunchSettings.cs — ReadProfileSettingsFromFile only catches JsonException/IOException, but… View resolved comment |
|
src/Microsoft.DotNet.ProjectTools/LaunchSettings/LaunchProfileParser.cs — MSBuildPropertyRegex (\$\([^)]+\)) treats the first ) as the end of the token, which breaks… View resolved comment |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Found three correctness regressions in expansion timing and project selection. The inline comments cover ignored profile arguments becoming failure sources, expansion occurring before restore/ComputeRunArguments, and run --api using the wrong artifacts path during expansion.
Note
This review was generated with AI assistance.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
The revised implementation resolves the previous expansion-timing and project-selection findings. One user-visible diagnostic regression remains in the Executable-profile reparse path.
Note
This review was generated with AI assistance.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
No further code issues found. I rechecked the deferred expansion paths across dotnet run, run-api, MTP, dotnet watch, and Native AOT; the prior findings are resolved and the new diagnostic-deduplication test covers the last regression.
The remaining red CI result is an unrelated macOS dotnet-new.IntegrationTests.dll.1 hang that Helix classified as InfrastructureError; the affected work item timed out after 48 minutes while unrelated template tests were running.
Note
This review was generated with AI assistance.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@jjonescz fixed the two nits here, should be good to merge after reapproval |
GitHub Actions
Failure
Expands MSBuild properties consistently across all supported launch-profile string fields using the same token replacement behavior as Visual Studio. This covers Project and Executable profiles for project and file-based
dotnet run, plus MTP test launches; Native AOT defers before launch when selected values require MSBuild.This follows up #56051 and addresses the post-merge feedback.
cc @baronfel @jjonescz
Note
This pull request was drafted with GitHub Copilot.