Skip to content

Enlighten ResolveNonMSBuildProjectOutput for multithreaded mode #13614

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.

Sign up for GitHub

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

Open
jankratochvilcz wants to merge 1 commit into
base: main
Choose a base branch
from
+1 −0
Open

Enlighten ResolveNonMSBuildProjectOutput for multithreaded mode #13614

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Tasks/ResolveNonMSBuildProjectOutput.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ namespace Microsoft.Build.Tasks
/// references (i.e. calling into specific targets of references to get the manifest file name)
/// which would not be possible with a mixed list of MSBuild and non-MSBuild references.
/// </remarks>
[MSBuildMultiThreadableTask]
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

Before marking this task as multi-threadable, please verify it doesn’t rely on process-global state for relative path resolution. The task calls GetAssemblyName(resolvedPath.ItemSpec) where resolvedPath comes from the XML inner text (not item FullPath), and existing unit tests build that XML with relative paths (e.g., "obj\correct.dll"). In in-proc multi-threaded execution, relative paths implicitly depend on Environment.CurrentDirectory and can become non-deterministic if any concurrently-running task changes it. Consider requiring/validating rooted paths for PreresolvedProjectOutputs (or normalizing to an absolute path via FileUtilities/TaskEnvironment) before enabling [MSBuildMultiThreadableTask].

Suggested change
[MSBuildMultiThreadableTask]

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@jankratochvilcz jankratochvilcz Apr 27, 2026

Choose a reason for hiding this comment

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

Good catch on the careful audit. The relative-path test inputs are unit-test artifacts; in production, PreresolvedProjectOutputs is generated by Visual Studio and always contains absolute paths (the IDE pre-resolves them precisely so MSBuild doesn't have to). The XML schema is set by the VS project system and the path values come from the IDE's resolved project output cache — there's no code path that would inject a relative path here in real builds.

That said, normalizing inside the task would protect against:

  • Custom hosts that synthesize PreresolvedProjectOutputs with relative paths (theoretical, no known consumer)
  • Future regressions if VS ever changed its contract

I'd argue against doing it in this PR for two reasons:

  1. Behavioral change risk: Normalizing relative paths via TaskEnvironment.GetAbsolutePath() would change the value flowing into AssemblyName.GetAssemblyName() and into the [Output] ResolvedOutputPaths items (Sin 1 risk — the resolved path is consumed downstream by the targets). That's a separate, independently-justified change.
  2. Scope: This issue (Enlighten ResolveNonMSBuildProjectOutput task for multithreaded mode #13612) is explicitly scoped as attribute-only. Adding path normalization changes the migration shape and warrants its own design discussion.

If the multithreaded execution model needs stronger guarantees here, a follow-up issue could (a) document the contract that PreresolvedProjectOutputs must contain absolute paths, or (b) add normalization with explicit [Output] preservation per Sin 1. Happy to file that follow-up if reviewers think it's worthwhile.

Copy link
Copy Markdown
Contributor

@OvesN OvesN Apr 29, 2026

Choose a reason for hiding this comment

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

About behavioral change risk: how so calling AssemblyName.GetAssemblyName() with absolutized path violates the sin 1? Value from AssemblyName.GetAssemblyName() does not go to [Output] ResolvedOutputPaths, we discard it.

I do not really see the reason not to be defensive here and do not use the absolutized path in GetAssemblyName

Copy link
Copy Markdown
Contributor

@OvesN OvesN Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

About PreresolvedProjectOutputs - same comment as here #13615 (comment)

public class ResolveNonMSBuildProjectOutput : ResolveProjectBase
Comment on lines +25 to 26
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

Adding [MSBuildMultiThreadableTask] changes execution routing in multi-threaded mode (in-proc vs TaskHost), but this PR doesn’t add a concurrency regression test. Please add a unit test that runs two ResolveNonMSBuildProjectOutput instances concurrently with different PreresolvedProjectOutputs/ProjectReferences and asserts each produces the correct, independent ResolvedOutputPaths and UnresolvedProjectReferences (no cross-contamination).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@jankratochvilcz jankratochvilcz Apr 27, 2026

Choose a reason for hiding this comment

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

Pushing back on this — a concurrency test is not justified for an attribute-only migration:

  1. Per-instance state: MSBuild creates a fresh task instance per Execute() call. All mutable state (base class _cachedProjectReferencesByAbsolutePath, _solutionConfiguration) is instance-scoped, so concurrent task instances cannot interfere by construction.

  2. No process-global state touched: No Environment.CurrentDirectory, no Path.GetFullPath, no File.*/Directory.*, no ProcessStartInfo, no static mutable state. The only file API call is AssemblyName.GetAssemblyName() on paths from PreresolvedProjectOutputs (see comment 2 thread for that discussion).

  3. Pattern consistency: ~27 other attribute-only migrations in the codebase ship without concurrency tests for the same reason — the attribute is a metadata declaration, not behavior. A concurrency test for an attribute-only migration would exercise MSBuild's task instantiation model, not this task.

{
#region Constructors
Expand Down
Loading