-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add clear error message (MSB4233) for .NET runtime tasks on MSBuild 17.14 #12662
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
Add clear error message (MSB4233) for .NET runtime tasks on MSBuild 17.14 #12662
Conversation
|
Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
baronfel
left a comment
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.
@copilot some things to clean up
src/Build.UnitTests/BackEnd/NodeProviderOutOfProcTaskHost_Tests.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
|
@copilot please also add end-to-end tests for the MSBuild CLI and MSBuild API usage that load a Project that consumes a .NET Runtime Task and fails appropriately. You won't be able to run these because they are windows-only, so follow existing patterns for Framework-only end-to-end tests and we can let CI try to verify them. |
Added end-to-end tests for both BuildManager API and MSBuild CLI usage:
(commit: 4c8c0d7) |
|
@copilot compilation is failing on the
If this type actually visible when targeting .NET Framework? |
Yes, The code at line 57 is inside a Can you confirm which commit SHA the failing build is using? The |
|
@copilot remove the NodeProviderOutOfProcTaskHost_Tests - they've served their purpose and we can't directly use the HandshakeOptions type. |
Removed |
|
This is ready for review now - once reviews are good we'll want to take this to VS QB |
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.
Pull Request Overview
Adds a specific error (MSB4233) when a task declares Runtime="NET" on MSBuild 17.14 (.NET Framework build) to replace a previously confusing failure, plus tests validating behavior and localized resource entries.
- Introduces early runtime check in NodeProviderOutOfProcTaskHost.AcquireAndSetUpHost to throw a clear InvalidProjectFile error.
- Adds new resource string (TaskRuntimeNET) with localization placeholders.
- Provides end-to-end API and CLI tests for both failing (.NET runtime task) and succeeding (normal task) scenarios.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/MSBuild.UnitTests/DotNetRuntimeTask_CLI_Tests.cs | Adds CLI tests verifying MSB4233 appears for Runtime="NET" and normal tasks still succeed. |
| src/Build/Resources/xlf/Strings.zh-Hant.xlf | Adds localized (untranslated initial) entry for new MSB4233 error. |
| src/Build/Resources/xlf/Strings.zh-Hans.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.tr.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.ru.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.pt-BR.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.pl.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.ko.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.ja.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.it.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.fr.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.es.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.de.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/xlf/Strings.cs.xlf | Adds localized entry for MSB4233 error. |
| src/Build/Resources/Strings.resx | Defines the new TaskRuntimeNET resource string and localization locking comments. |
| src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs | Adds runtime flag check and throws MSB4233 before host launch. |
| src/Build.UnitTests/BackEnd/DotNetRuntimeTask_EndToEnd_Tests.cs | Adds BuildManager and ProjectInstance API tests for MSB4233 behavior and success path. |
src/Build.UnitTests/BackEnd/DotNetRuntimeTask_EndToEnd_Tests.cs
Outdated
Show resolved
Hide resolved
src/Build.UnitTests/BackEnd/DotNetRuntimeTask_EndToEnd_Tests.cs
Outdated
Show resolved
Hide resolved
src/Build.UnitTests/BackEnd/DotNetRuntimeTask_EndToEnd_Tests.cs
Outdated
Show resolved
Hide resolved
…al variable Extract logger.Errors[0].Message to a local variable before calling ShouldContain to resolve CS8604 nullable reference warning. The variable assignment ensures the null-state analysis understands the value is not null. Co-authored-by: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com>
Fixed the nullable reference warning by extracting |
|
@copilot I see other issues |
- Add null-forgiving operator and ShouldNotBeNull assertion for error message - This resolves CS8600 and CS8604 nullable reference warnings - Tests now properly handle nullable string from logger.Errors[0].Message Co-authored-by: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com>
|
@copilot the test fails with Project "C:\Users\VssAdministrator\AppData\Local\Temp\wcxc1rpv.cs3\MSBuildTemp\Temporary138a2b57c7794ffe8000debafd185ea1\test.proj" on node 1 (TestTask target(s)). Build FAILED. "C:\Users\VssAdministrator\AppData\Local\Temp\wcxc1rpv.cs3\MSBuildTemp\Temporary138a2b57c7794ffe8000debafd185ea1\test.proj" (TestTask target) (1) -> Time Elapsed 00:00:00.43 ============== |
The test was failing because it tried to load the assembly from the wrong directory. Now it properly navigates to the sibling Microsoft.Build.Engine.UnitTests output directory to find the DLL. Co-authored-by: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com>
Fixed the assembly path construction in the CLI test. The test was looking for |
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
- Update comment to clarify that all .NET Core/5+ MSBuilds support .NET runtime tasks - Change error message to suggest "dotnet build" instead of MSBuild 18.0 - Remove assertion on error message string (not needed since it can be localized) Co-authored-by: rainersigwald <3347530+rainersigwald@users.noreply.github.com>
The localization has to be enabled to service the change with the new error message (#12662)

Summary
This PR adds a clear error message (MSB4233) when users try to use tasks with
Runtime="NET"on MSBuild 17.14 (.NET Framework), which doesn't support .NET runtime tasks.Changes
NodeProviderOutOfProcTaskHost.AcquireAndSetUpHost()to detectHandshakeOptions.NETflagInvalidProjectFileExceptionwith clear error message (MSB4233) before attempting to launch task host#if NETFRAMEWORKsince .NET Core/5+ builds support .NET runtime tasksTesting
WindowsFullFrameworkOnlyFactand will run in CI on WindowsFixes #12661
Original prompt
Fixes #12661
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.