Skip to content
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

Resolve net task host params from the project properties #11543

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented Mar 5, 2025

based on: #11393

experimental VS instance https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/616996
This PR contains logic that helps to resolve NetTaskHost execution params based on the project properties.
DOTNET_EXPERIMENTAL_HOST_PATH => resolves path to dotnet.exe
RuntimeIdentifierGraphPath => gives the path to the sdk version resolved for the given project.

The values are passed to NodeProviderOutOfProcTaskHost by using taskHostParameters.
The exception is generated on attempt to use this functionality for sdk9 and lower.

For further flexibility the packet versioning support was added in NodePacketType.

MSBuild Packet Schema with Version Information

Packet Structure Overview

MSBuild's packet communication protocol supports versioning through an extended header mechanism. This document outlines the binary structure of packets with and without version information.

Basic Packet Format

Position Size (bytes) Description
0 1 Packet Type Byte
1-4 4 Packet Length (32-bit integer)
5+ Variable Packet Body

Packet Type Byte (Position 0) Bit Layout

Bits Purpose
0-5 Base packet type (values 0-63)
6 (0x40) Extended header flag - When set, indicates version information follows
7 (0x80) Reserved for future use or special packet types

Important Note: Special server command packets (0xF0-0xFF) ignore the extended header flag regardless of its value.

Packet Formats

Standard Packet (No Version Information)

Position Size (bytes) Description
0 1 Packet Type (bit 6 not set)
1-4 4 Packet Length
5+ Variable Packet Body (in original format)

Versioned Packet (With Version Information)

Position Size (bytes) Description
0 1 Packet Type with bit 6 set
1-4 4 Packet Length (includes version byte)
5 1 Version Byte
6+ Variable Packet Body (format depends on version)

Version Byte Values

Value Description
1 Original packet format (baseline)
2 Second generation format with extended fields
3+ Future format extensions

{AC560B48-040C-46AC-8A1A-AF237D127AFB}

Implementation Notes

  • Older clients will ignore bit 6 in the packet type byte
  • Newer clients will check bit 6 to determine if version information follows
  • The packet length field ensures all clients can properly navigate the packet data
  • Version 1 is considered the baseline format compatible with older clients

Migration Strategy

When adding new fields to existing packet types:

  1. Create a new version of the packet format
  2. Set the extended header flag when writing packets
  3. Include version byte with the new version number
  4. Ensure packet readers check for version information
  5. Maintain backward compatibility with version 1 format

msbuildArchitecture = msbuildArchitecture == String.Empty ? XMakeAttributes.MSBuildArchitectureValues.any : msbuildArchitecture.Trim();
msbuildRuntime = msbuildRuntime == String.Empty ? XMakeAttributes.MSBuildRuntimeValues.any : msbuildRuntime.Trim();

taskIdentityParameters.Add(XMakeAttributes.runtime, msbuildRuntime);
taskIdentityParameters.Add(XMakeAttributes.architecture, msbuildArchitecture);
}

string hostPath = lookup.GetProperty("DOTNET_EXPERIMENTAL_HOST_PATH")?.EvaluatedValue;
string msBuildAssemblyPath = Path.GetDirectoryName(lookup.GetProperty("RuntimeIdentifierGraphPath")?.EvaluatedValue) ?? string.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should be able to find a better property - using the RID graph as a side effect may be prone to breakage or re-arrangement in the SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a workaround. we plan to add a separate property later

@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review March 6, 2025 14:58
/// <summary>
/// Task host runtime.
/// </summary>
private string _runtime;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private string _runtime;
private readonly string _runtime;


// Set the configuration bytes just before serialization in case the SetConfigurationBytes was invoked during lifetime of this instance.
if (translator.Mode == TranslationDirection.WriteToStream)
// Skip AppDomain configuration when targeting .NET Task Host (Runtime="Net").
Copy link
Member

Choose a reason for hiding this comment

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

Is there a list of all the runtime names and their meaning?

@@ -115,6 +116,8 @@ internal abstract class NodeEndpointOutOfProcBase : INodeEndpoint
/// </summary>
private BinaryWriter _binaryWriter;

private readonly IList<string> _versionHandshakeGroup = ["fileVersionMajor", "fileVersionMinor", "fileVersionBuild", "fileVersionPrivate"];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private readonly IList<string> _versionHandshakeGroup = ["fileVersionMajor", "fileVersionMinor", "fileVersionBuild", "fileVersionPrivate"];
private readonly ImmutableArray<string> _versionHandshakeGroup = ["fileVersionMajor", "fileVersionMinor", "fileVersionBuild", "fileVersionPrivate"];

Given this collection doesn't change consider a non-mutating type to make future mutations an explicit decision.

break;
// NET Task host allows to connect to MSBuild.dll with the different handshake version.
// We agreed to hardcode a value of 99 to bypass the protection for this scenario.
if (_versionHandshakeGroup.Contains(handshakeComponents[i].Key) && handshakeComponents[i].Value == Handshake.NetTaskHostHandshakeVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Why is the runtime host name treated as ordinal ignore case but the version handshake groups are ordinal?

public static byte CreateExtendedHeaderType(NodePacketType type) => (byte)((byte)type | ExtendedHeaderFlag);

// Read extended header (returns version)
public static byte ReadVersion(Stream stream) => (byte)stream.ReadByte();
Copy link
Member

Choose a reason for hiding this comment

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

TIL: that ReadByte returns int not byte.

/// </summary>
Arm64 = 128,
}

internal class Handshake
{
public static int NetTaskHostHandshakeVersion = 99;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static int NetTaskHostHandshakeVersion = 99;
public const int NetTaskHostHandshakeVersion = 99;

Or possible static readonly

// for NET the executable is resolved from DOTNET_EXPERIMENTAL_HOST_PATH
s_msbuildName = IsHandshakeOptionEnabled(HandshakeOptions.NET)
? string.Empty
: Environment.GetEnvironmentVariable("MSBUILD_EXE_NAME") ?? Constants.MSBuildExecutableName;
Copy link
Member

Choose a reason for hiding this comment

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

The comment says DOTNET_EXPERIMENTAL_HOST_PATH but the code is using MSBUILD_EXE_NAME. Not quite following that bit.

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.

None yet

3 participants