Use BCL APIs to create symlinks and fix error reporting.

[teo-tsirpanis]

Fixes #

Context

MSBuild has used platform-specific APIs to create symbolic links. However, since .NET 6 there is an API in System.IO.File for this purpose. This PR makes use of this API when available. It also fixes a bug in the Copy task where the error message was not getting propagated, and removed an unused parameter.

Changes Made

Testing

Existing testing coverage.

Notes

A BCL API to create hard links has been added in .NET 11. We can similarly use it when MSBuild targets this framework.

[Copilot]

Pull request overview

This PR updates MSBuild’s symlink creation to use the .NET BCL (System.IO.File.CreateSymbolicLink) when building for NET, and adjusts the Copy task’s link-creation plumbing to better propagate link-creation error text while removing an unused parameter.

Changes:

• Use File.CreateSymbolicLink in Microsoft.Build.Framework.NativeMethods.MakeSymbolicLink under #if NET.
• Refactor Copy.TryCopyViaLink to use a ref string-based delegate for error propagation and remove an unused importance parameter.

Reviewed changes

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

File	Description
src/Tasks/Copy.cs	Refactors link-copy helper to use a ref error message delegate and updates call sites.
src/Framework/NativeMethods.cs	Switches symlink creation to BCL API for NET builds and maps exceptions to error text.

[ViktorHofer]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Performance & Allocation — LGTM

No concrete regression found:

• CreateLinkProc named delegate allocates identically to the Func<string,string,string,bool> it replaces; no measurable difference.
• The ref string errorMessage parameter avoids an extra string slot; neutral to slightly better.
• File.CreateSymbolicLink + try/catch in NativeMethods.cs runs at most once per file, opt-in only; e.Message allocation is on the failure path only.
• No LINQ, no recomputed cacheable values, no wrong collection types, nothing on a hot path.

[teo-tsirpanis]

Do we need the path for .NET Framework on non-Windows?

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

🔴 MAJOR — Test Coverage Gap

The PR fixes a bug (error messages from failed link creation were silently not propagated) and introduces a new #if NET implementation for MakeSymbolicLink. Neither has a regression test.

Gap 1 — MakeSymbolicLink failure path (new #if NET branch)

No test invokes NativeMethodsShared.MakeSymbolicLink (or NativeMethods.MakeSymbolicLink) in a scenario that triggers failure and verifies errorMessage is set. The catch block at NativeMethods.cs:819 is completely uncovered. If that block silently dropped the message (return false without setting errorMessage), no existing test would detect it.

Gap 2 — Symlink failure error message in the Copy task

The Windows hard-link tests (CopyToDestinationFolderWithHardLinkFallback*) guard the analogous hard-link path with AssertLogContains("0x80070011"). No equivalent test exists for the symlink failure path — nothing asserts that when a symbolic link creation fails, the RetryingAsFileCopy log message contains a non-empty error description.

Suggested additions:

1. A Copy_Tests.cs test (gated with [RequiresSymbolicLinksFact] or arranged to fail deterministically) that triggers a symlink creation failure and asserts AssertLogContains includes a non-empty error detail in the RetryingAsFileCopy message.
2. A direct unit test for NativeMethods.MakeSymbolicLink in Framework.UnitTests covering the failure path (e.Message is propagated back, false is returned).

Generated by Expert Code Review (command) for #14143 · 2.6K AIC · ⊞ 31.9K · ◷
_{Comment /review to run again}

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

⚠️ MODERATE — Error Message Text Accuracy (Copy.NonWindowsLinkErrorMessage)

The resource string Copy.NonWindowsLinkErrorMessage reads:

"The {0} library call failed with the following error code: {1}."

This was accurate when {1} received Marshal.GetLastWin32Error().ToString() (a numeric errno like "1"). After the #if NET change, {1} is e.Message — a full prose exception message such as "Access to the path '/foo/link' is denied." Presenting a sentence as an "error code" is misleading.

Suggested fix — update the resource string to use neutral wording, e.g.:

"The {0} library call failed: {1}."

or separately handle the two cases. This is a small string-only change but improves the diagnostic message quality for users on .NET Linux/macOS.

Generated by Expert Code Review (command) for #14143 · 2.6K AIC · ⊞ 31.9K · ◷
_{Comment /review to run again}

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Error Message Quality — ISSUE

One concrete issue introduced by this PR: the #if NET switch to File.CreateSymbolicLink + e.Message makes the pre-existing Copy.NonWindowsLinkErrorMessage format string inaccurate in two ways ("symlink()" is the wrong function name; "error code" is the wrong label for a human-readable description). See inline comment.

Generated by Expert Code Review (command) for #14143 · 2.6K AIC · ⊞ 31.9K
_{Comment /review to run again}

Comments that could not be inline-anchored

src/Tasks/Copy.cs:362

Error Message Quality — ISSUE introduced by this PR

The #if NET path in NativeMethods.MakeSymbolicLink now calls File.CreateSymbolicLink and surfaces e.Message (e.g. "Permission denied"), but line 362 still formats the message with:

Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage)

Copy.NonWindowsLinkErrorMessage reads: "The {0} library call failed with the following error code: {1}."

On .NET non-Windows a failure now prod…

src/Tasks/Copy.cs:400

Naming Precision — NIT

The Proc suffix is unused anywhere else in this codebase for delegate declarations. The pervasive convention here is the Delegate suffix (e.g., NodeShutdownDelegate, RequestCompleteDelegate, BuildRequestEntryStateChangedDelegate). Consider renaming to CreateLinkDelegate to stay consistent.

src/Framework/NativeMethods.cs:1

Idiomatic C# Patterns — ISSUE

catch (Exception e) swallows every exception kind, including OutOfMemoryException and StackOverflowException. The established pattern in Copy.cs uses a filtered catch:

catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))

(See Copy.cs lines 995 and 1025.) A File.CreateSymbolicLink failure will always be IO-related, so the filter fits and keeps the codebase consistent while avoiding silently masking unexpected exceptio…

src/Framework/NativeMethods.cs:819

[MODERATE] Correctness & Edge Cases — overly broad exception catch

catch (Exception e) is too broad: it silently swallows fatal exceptions (OutOfMemoryException, ThreadAbortException, ExecutionEngineException) that should propagate and terminate the process.

File.CreateSymbolicLink fails in normal operation with IOException (file exists, path too long) or UnauthorizedAccessException (missing privilege / developer mode not enabled on Windows). These are the only exception typ…

src/Tasks/Copy.cs:400

[NIT] Naming Precision — Proc suffix is not idiomatic in C#

CreateLinkProc uses the Proc suffix, which originates from Win32 / WNDPROC conventions and is rarely seen in modern .NET codebases.

Since Func<> does not support ref parameters, a custom delegate is correct here. The idiomatic convention is to use the Delegate suffix or describe the shape without a suffix:

// Option A: explicit 'delegate' suffix
private delegate bool CreateLinkDelegate(string source, string…

</details>

<details><summary>src/Framework/NativeMethods.cs:819</summary>

**⚠️ MODERATE — Idiomatic C# / Correctness**

Bare `catch (Exception e)` silently swallows fatal exceptions (`OutOfMemoryException`, `AccessViolationException`, etc.). The established pattern throughout this assembly is:

```csharp
catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))

ExceptionHandling lives in src/Framework/ (same assembly) and this exact pattern is used in FileMatcher.cs, FileUtilities.cs, FrameworkLocationHelper.cs, and others. The when filter…

src/Tasks/Copy.cs:400

NIT — Naming

CreateLinkProc is the only delegate type in the entire Tasks assembly using a Proc suffix (Win32 API nomenclature). No single naming convention dominates in the codebase, but the suffix is a clear outlier. Consider CreateLinkDelegate or simply dropping the suffix (e.g., CreateLink) to stay closer to existing patterns in Copy.cs and neighboring files.