Skip to content

Migrate GetAssemblyIdentity to multithreaded execution model#13588

Open
jankratochvilcz wants to merge 3 commits intomainfrom
jankratochvilcz/multithreaded/get-assembly-identity-migration
Open

Migrate GetAssemblyIdentity to multithreaded execution model#13588
jankratochvilcz wants to merge 3 commits intomainfrom
jankratochvilcz/multithreaded/get-assembly-identity-migration

Conversation

@jankratochvilcz
Copy link
Copy Markdown
Contributor

Migrates the GetAssemblyIdentity task to MSBuild's multithreaded execution model, applying the absolutize-at-boundary pattern.

Fixes #13571.

What changed

Relative AssemblyFiles paths are now resolved via TaskEnvironment.GetAbsolutePath() instead of relying on Environment.CurrentDirectory. This makes the task safe for concurrent execution across projects with different working directories.

Validation

  • GetAssemblyIdentity_Tests — 8 tests covering baseline behavior, project-relative resolution, output integrity (Sin 1), and metadata copying.
  • Full Tasks.UnitTests build passes with 0 warnings.

Absolutize item.ItemSpec via TaskEnvironment.GetAbsolutePath() before
AssemblyName.GetAssemblyName. Add tests for baseline behavior,
project-relative resolution, output integrity, and metadata copying.

Fixes #13571

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 21, 2026 13:17
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

Migrates the GetAssemblyIdentity built-in task to MSBuild’s multithreaded execution model by resolving AssemblyFiles relative paths via TaskEnvironment.GetAbsolutePath() rather than Environment.CurrentDirectory, making path handling safe under concurrent project execution.

Changes:

  • Mark GetAssemblyIdentity as [MSBuildMultiThreadableTask], implement IMultiThreadableTask, and add a TaskEnvironment property defaulting to TaskEnvironment.Fallback.
  • Absolutize each AssemblyFiles item at the task boundary before calling AssemblyName.GetAssemblyName(...), preserving existing per-item error/continue semantics.
  • Add new unit tests covering success, failure modes, project-directory relative resolution, and output/metadata behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Tasks/GetAssemblyIdentity.cs Implements the multithreaded task contract and switches path resolution to TaskEnvironment.GetAbsolutePath() before reading assemblies.
src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs Adds initial unit coverage for baseline behavior, error handling, project-relative resolution, and output integrity/metadata copying.

Comment thread src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Generated by Expert Code Review (on open) for issue #13588 · ● 27.6M

Comment thread src/Tasks/GetAssemblyIdentity.cs
Comment thread src/Tasks/GetAssemblyIdentity.cs
Comment thread src/Tasks/GetAssemblyIdentity.cs
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Generated by Expert Code Review (on open) for issue #13588 · ● 27.6M

Comment thread src/Tasks/GetAssemblyIdentity.cs
Comment thread src/Tasks/GetAssemblyIdentity.cs
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Review: Migrate GetAssemblyIdentity to multithreaded execution model

Verdict: Looks good

This is a clean, well-executed migration that follows the established IMultiThreadableTask pattern (matching Copy.cs, Delete.cs, AssignTargetPath.cs).

Compatibility Analysis

No breaking changes detected. Key observations:

  1. Error behavior preserved: The original catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e)) already catches ArgumentException, so invalid paths (empty strings, illegal characters) were already handled gracefully. The new GetAbsolutePath catch block on line 79 intercepts the same exception type before it reaches AssemblyName.GetAssemblyName, producing the same MSB3441 error code via the same resource string.

  2. Output property safe: The [Output] Assemblies items use AssemblyName.FullName as their ItemSpec — not any file path — so absolutized paths cannot leak into downstream task inputs.

  3. Error messages use original paths: All Log.LogErrorWithCodeFromResources calls use item.ItemSpec (the user's original input), not the absolutized assemblyPath.

Migration Pattern Compliance

Check Status
[MSBuildMultiThreadableTask] attribute
IMultiThreadableTask interface
TaskEnvironment default = Fallback
GetAbsolutePath with ArgumentException catch
Error messages use original path
Output properties not contaminated
No environment variable usage to migrate ✅ N/A
No process start to migrate ✅ N/A

Test Coverage

Excellent — 8 tests covering:

  • ✅ Happy path (absolute path to real assembly)
  • ✅ Non-existent file → MSB3441
  • ✅ Non-assembly file → MSB3441
  • ✅ Mixed batch (good + bad items processed independently)
  • ✅ Empty ItemSpec → MSB3441
  • Relative path resolution against project directory (the migration's core behavioral change)
  • ✅ Output items don't contain absolutized paths
  • ✅ Input metadata copied to output

Minor Nit

The test file has #nullable disable which conflicts with the "new files should always use nullable reference types" guidance, but matches the overwhelming convention in src/Tasks.UnitTests/. Low priority.

Generated by Expert Code Review (on open) for issue #13588 · ● 3.4M

Comment thread src/Tasks/GetAssemblyIdentity.cs Outdated
Comment thread src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs
Comment thread src/Tasks/GetAssemblyIdentity.cs Outdated
Comment thread src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs
Comment thread src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs
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.

Enlighten GetAssemblyIdentity task for multithreaded mode

3 participants