Skip to content

Migrate ValidateExecutableReferences to IMultiThreadableTask#55

Closed
SimaTian wants to merge 1 commit intomultithreading-polyfillsfrom
migrate-validate-executable-references
Closed

Migrate ValidateExecutableReferences to IMultiThreadableTask#55
SimaTian wants to merge 1 commit intomultithreading-polyfillsfrom
migrate-validate-executable-references

Conversation

@SimaTian
Copy link
Copy Markdown
Owner

WIP — Migration in progress.

@SimaTian
Copy link
Copy Markdown
Owner Author

Closing: PR is empty (no code changes). Task is available for migration.

@SimaTian SimaTian closed this Feb 10, 2026
@SimaTian
Copy link
Copy Markdown
Owner Author

Migration Review: ValidateExecutableReferences

Status: ❌ No migration implemented

This PR contains 0 changed files (empty commit). The PR was closed with the note "PR is empty (no code changes). Task is available for migration."

Required for migration (per migration guide):

  1. Determine pattern: Analyze the existing ValidateExecutableReferences task code for forbidden API usage (Path.GetFullPath, File.*, Environment.*, ProcessStartInfo, etc.)

  2. If no forbidden APIs (Pattern A - Attribute-Only):

    • Add [MSBuildMultiThreadableTask] attribute to the task class
  3. If forbidden APIs are used (Pattern B - Interface-Based):

    • Implement IMultiThreadableTask interface
    • Add TaskEnvironment property (public ITaskEnvironment TaskEnvironment { get; set; })
    • Replace all forbidden API calls:
      • Path.GetFullPath(path)TaskEnvironment.GetAbsolutePath(path)
      • Environment.GetEnvironmentVariable(name)TaskEnvironment.GetEnvironmentVariable(name)
      • Environment.SetEnvironmentVariable(name, value)TaskEnvironment.SetEnvironmentVariable(name, value)
      • Environment.CurrentDirectoryTaskEnvironment.ProjectDirectory
      • new ProcessStartInfo(...)TaskEnvironment.GetProcessStartInfo()
    • Ensure all File/Directory APIs use absolute paths
    • Do NOT null-check TaskEnvironment
  4. Tests required:

    • Verify IMultiThreadableTask implementation (for Pattern B)
    • Verify MSBuildMultiThreadableTask attribute
    • Verify correct path resolution with non-default project directory
    • Tests must FAIL on improperly migrated task

This PR needs to be reopened with actual migration code before it can be reviewed for correctness.

Copy link
Copy Markdown
Owner Author

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

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

Code Review: Migration to IMultiThreadableTask

Checklist

  • Base class changed from Task to MultiThreadableTask
  • Path.GetFullPath() replaced with TaskEnvironment.GetAbsolutePath()
  • Environment.GetEnvironmentVariable() replaced with TaskEnvironment.GetEnvironmentVariable()
  • Environment.SetEnvironmentVariable() replaced with TaskEnvironment.SetEnvironmentVariable()
  • No remaining global process state usage
  • ProcessStartInfo uses TaskEnvironment.GetProcessStartInfo() where applicable

Findings

This PR contains no code changes. The diff is empty (0 additions, 0 deletions, 0 changed files). The commit 887c591 is an empty WIP commit with the message ""WIP: Migrate ValidateExecutableReferences to multithreading"" but no actual modifications were made to any files.

Analysis of the current (unmigrated) source ValidateExecutableReferences.cs:

This task extends TaskBase and performs in-memory validation operations only. It:

  • Uses BuildEngine6.GetGlobalProperties() to check for SelfContained and RuntimeIdentifier global properties — this is a BuildEngine API, not a global process state API, so it should be thread-safe (MSBuild manages per-project build engine instances).
  • Parses XML from ITaskItem metadata (AdditionalPropertiesFromProject)
  • Compares boolean properties and logs errors
  • Does not use Path.GetFullPath(), File.*, Directory.*, Environment.*, ProcessStartInfo, or any other forbidden APIs

This task likely only needs an attribute-only migration:

  • Add [MSBuildMultiThreadableTask] attribute
  • No need to implement IMultiThreadableTask interface (BuildEngine6.GetGlobalProperties() is a per-instance BuildEngine API, not global process state)
  • Write a basic thread-safety test verifying the attribute is present

Note: The plan.md mentions this task ""uses BuildEngine6.GetGlobalProperties()"" under ""Tasks using Environment.*"", but upon source analysis, this is a BuildEngine API call, not an Environment API call. It should be safe for multithreaded execution without interface-based migration.

Verdict

NEEDS CHANGES — PR is empty. No migration work has been implemented. This should be a straightforward attribute-only migration since no forbidden APIs are used — BuildEngine6.GetGlobalProperties() is a per-instance BuildEngine API that is thread-safe.

@SimaTian
Copy link
Copy Markdown
Owner Author

Migration Review: ValidateExecutableReferences (PR #55)

Status: No migration implemented — empty commit with 0 file changes.

Source Analysis

The existing ValidateExecutableReferences task (inherits TaskBase) uses NO forbidden APIs:

  • No Path.GetFullPath
  • No File.* or Directory.*
  • No Environment.*
  • No ProcessStartInfo
  • No Console.*

The task only operates on ITaskItem metadata, XElement parsing, and BuildEngine6.GetGlobalProperties() — pure in-memory logic with no file system or environment access.

Required Migration (Pattern A — Attribute-Only)

This is a simple migration:

  • Add [MSBuildMultiThreadableTask] attribute to the class
  • No IMultiThreadableTask interface needed
  • No TaskEnvironment property needed
  • Add tests: verify MSBuildMultiThreadableTask attribute is present, tests must FAIL if attribute is removed

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.

1 participant