Skip to content

add enum for nodemode and support using it with ints and readable strings#13148

Merged
baronfel merged 5 commits intomainfrom
nicer-parsing-for-nodemode
Feb 9, 2026
Merged

add enum for nodemode and support using it with ints and readable strings#13148
baronfel merged 5 commits intomainfrom
nicer-parsing-for-nodemode

Conversation

@baronfel
Copy link
Copy Markdown
Member

Context

The team was discussing nodemodes in sync this morning and we were all getting confused. This PR gives names to the existing modes and has us use them consistently across the codebase for clarity.

Copilot AI review requested due to automatic review settings January 29, 2026 16:46
@baronfel baronfel force-pushed the nicer-parsing-for-nodemode branch 2 times, most recently from 61cf314 to e0100f3 Compare January 29, 2026 16:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request introduces a strongly-typed NodeMode enum to replace the use of magic numbers throughout the MSBuild codebase for identifying different types of out-of-process nodes. The change improves code clarity and maintainability by giving meaningful names to the previously unnamed node mode values (1, 2, 3, and 8).

Changes:

  • Added a new NodeMode enum in src/Shared/NodeMode.cs with four values: OutOfProcNode (1), OutOfProcTaskHostNode (2), OutOfProcRarNode (3), and OutOfProcServerNode (8)
  • Enhanced XMake.cs to parse node mode values from both integers and enum names (case-insensitive) for backward compatibility and improved developer experience
  • Updated all node launching code to use the enum values via NodeModeHelper.ToCommandLineArgument() instead of hardcoded integers

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/Shared/NodeMode.cs New file defining the NodeMode enum and NodeModeHelper utility class for converting enum values to command-line arguments
src/MSBuild/XMake.cs Added ParseNodeMode method to support parsing both integers and enum names; refactored StartLocalNode to use switch expression on enum values; added (unused) public NodeModeToCommandLineArgument wrapper method
src/Build/Microsoft.Build.csproj Added reference to the new NodeMode.cs shared file
src/Build/BackEnd/Components/Communications/RarNodeLauncher.cs Replaced hardcoded "/nodemode:3" with NodeModeHelper.ToCommandLineArgument(NodeMode.OutOfProcRarNode)
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs Replaced hardcoded "/nodemode:2" with NodeModeHelper.ToCommandLineArgument(NodeMode.OutOfProcTaskHostNode)
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs Replaced hardcoded "/nodemode:1" with NodeModeHelper.ToCommandLineArgument(NodeMode.OutOfProcNode)
src/Build/BackEnd/Client/MSBuildClient.cs Replaced hardcoded "/nodemode:8" with NodeModeHelper.ToCommandLineArgument(NodeMode.OutOfProcServerNode)
Comments suppressed due to low confidence (1)

src/MSBuild/XMake.cs:123

  • This public method appears to be unused throughout the codebase. All callers use NodeModeHelper.ToCommandLineArgument directly instead. Additionally, the cast from NodeMode to Shared.NodeMode is redundant since NodeMode (imported via "using Microsoft.Build.Shared;") already refers to Shared.NodeMode. Consider removing this method or clarifying its intended purpose if it's meant to be a public API for external consumers.
        /// <summary>
        /// The object used to synchronize access to shared build state
        /// </summary>
        private static readonly LockType s_buildLock = new LockType();

@baronfel
Copy link
Copy Markdown
Member Author

baronfel commented Feb 5, 2026

@copilot implement @JanProvaznik's feedback:

  • move the new type down to the framework project
  • add unit tests covering parsing
  • unify with the DebugUtils enum as well
  • make sure that if parsing fails we throw the same error message as we did before this change

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 5, 2026

@baronfel I've opened a new pull request, #13205, to work on those changes. Once the pull request is ready, I'll request review from you.

@baronfel
Copy link
Copy Markdown
Member Author

baronfel commented Feb 5, 2026

@JanProvaznik mind taking another look now that some of your feedback has been integrated?

@baronfel baronfel enabled auto-merge (squash) February 5, 2026 17:10
baronfel and others added 5 commits February 5, 2026 11:10
### Context

The NodeMode enum was in Shared namespace, duplicated logic existed in
DebugUtils, and parsing used string.Format placeholders instead of
direct construction.

### Changes Made

- **Moved NodeMode to Framework**: Relocated from
`Microsoft.Build.Shared` to `Microsoft.Build.Framework` for better
accessibility
- **Added NodeModeHelper.TryParse()**: Safe parsing supporting both
integers (1, 2, 3, 8) and enum names (case-insensitive) with
`Enum.IsDefined` validation
- **Refactored XMake.ParseNodeMode()**: Now uses TryParse pattern, made
internal for testability, preserves original error messages
- **Unified DebugUtils with NodeMode**: Removed the private
`ProcessMode` enum from DebugUtils and replaced it with `NodeMode?` from
Framework. The nullable type allows `null` to represent the central/main
MSBuild process (not running as a node), while actual node modes use the
shared enum. Updated parsing to use `NodeModeHelper.TryParse()` for
consistency across the codebase.
- **Cleaned up command line construction**: Replaced string.Format
placeholders in NodeProviderOutOfProcTaskHost with direct string
interpolation matching NodeProviderOutOfProc pattern

### Testing

Added 6 unit tests covering:
- Valid integers: 1, 2, 3, 8
- Valid enum names: case-sensitive and case-insensitive
- Error cases: undefined integers, negative numbers, invalid strings,
empty strings

### Notes

- The `Enum.IsDefined` check after `Enum.TryParse` is required because
TryParse succeeds for string representations of undefined integer values
(e.g., "5" parses successfully even though only 1, 2, 3, and 8 are
defined).
- DebugUtils now uses `NodeMode?` (nullable) where `null` represents the
central/main process, eliminating the need for a separate `ProcessMode`
enum and ensuring a single source of truth for node mode values.

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
@baronfel baronfel force-pushed the nicer-parsing-for-nodemode branch from 7ec5f59 to 2bfb167 Compare February 5, 2026 17:10
@baronfel baronfel merged commit 356bf5d into main Feb 9, 2026
9 checks passed
@baronfel baronfel deleted the nicer-parsing-for-nodemode branch February 9, 2026 14:16
Copilot AI added a commit that referenced this pull request Feb 17, 2026
…ings (#13148)

### Context
The team was discussing nodemodes in sync this morning and we were all
getting confused. This PR gives names to the existing modes and has us
use them consistently across the codebase for clarity.

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Co-authored-by: Jan Provazník <janprovaznik@microsoft.com>
JanProvaznik added a commit to JanProvaznik/msbuild that referenced this pull request Feb 25, 2026
…ings (dotnet#13148)

### Context
The team was discussing nodemodes in sync this morning and we were all
getting confused. This PR gives names to the existing modes and has us
use them consistently across the codebase for clarity.

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Co-authored-by: Jan Provazník <janprovaznik@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants