Skip to content

Migrate ReadStaticWebAssetsManifestFile - #54791

Merged
AlesProkop merged 3 commits into
dotnet:mainfrom
AlesProkop:migrate-read-static-web-assets-manifest-file
Jun 18, 2026
Merged

Migrate ReadStaticWebAssetsManifestFile#54791
AlesProkop merged 3 commits into
dotnet:mainfrom
AlesProkop:migrate-read-static-web-assets-manifest-file

Conversation

@AlesProkop

@AlesProkop AlesProkop commented Jun 16, 2026

Copy link
Copy Markdown
Member

Fixes dotnet/msbuild#14060

Context

Migrates ReadStaticWebAssetsManifestFile to support MSBuild multithreaded task execution. The task reads the static web assets manifest from disk, so it needs to avoid resolving relative paths against process-wide current directory state.

Changes Made

  • Added [MSBuildMultiThreadableTask] to ReadStaticWebAssetsManifestFile.
  • Implemented IMultiThreadableTask and initialized TaskEnvironment with TaskEnvironment.Fallback.
  • Absolutized ManifestPath with TaskEnvironment.GetAbsolutePath() before file system access.
  • Preserved existing missing-manifest behavior and error text for valid missing paths and empty/invalid paths.
  • Added a focused regression test for empty manifest path handling.

Testing

  • Built D:\msbuild\src\ThreadSafeTaskAnalyzer\ThreadSafeTaskAnalyzer.csproj.
  • Ran the thread-safe task analyzer against src\StaticWebAssetsSdk\Tasks\Microsoft.NET.Sdk.StaticWebAssets.Tasks.csproj.
    • No analyzer diagnostics for ReadStaticWebAssetsManifestFile.
    • The project still reports unrelated diagnostics in other task files.
  • Built src\StaticWebAssetsSdk\Tasks\Microsoft.NET.Sdk.StaticWebAssets.Tasks.csproj.
  • Ran focused tests:
    • Microsoft.NET.Sdk.StaticWebAssets.Tests.ReadStaticWebAssetsManifestFileTest
    • Result: 8 passed, 0 failed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Migrates ReadStaticWebAssetsManifestFile to be safe for MSBuild multi-threaded task execution by eliminating reliance on process-wide current directory when resolving the manifest path.

Changes:

  • Marked ReadStaticWebAssetsManifestFile as [MSBuildMultiThreadableTask] and implemented IMultiThreadableTask with a TaskEnvironment fallback.
  • Resolved ManifestPath to an absolute path via TaskEnvironment.GetAbsolutePath() before doing file-system reads.
  • Added a regression test ensuring empty ManifestPath continues to produce the existing “not found” error behavior instead of throwing.

Reviewed changes

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

File Description
src/StaticWebAssetsSdk/Tasks/ReadStaticWebAssetsManifestFile.cs Makes the task MSBuild-multithread friendly by resolving manifest paths via TaskEnvironment and using the absolute path for file IO.
test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/ReadStaticWebAssetsManifestFileTest.cs Adds a focused regression test for empty manifest path handling to ensure behavior remains stable post-migration.

@AlesProkop

Copy link
Copy Markdown
Member Author

Reviewed this as an MSBuild multithreaded-task migration and I don't see actionable issues.

The task is marked [MSBuildMultiThreadableTask], implements IMultiThreadableTask with TaskEnvironment.Fallback, absolutizes ManifestPath before the file-system calls, keeps user-facing messages on the original ManifestPath, and does not leak absolute paths into outputs. The empty-path test covers the new GetAbsolutePath ArgumentException path. No review comments from me.

@jankratochvilcz jankratochvilcz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a few comments, thanks for the PR!

Comment thread src/StaticWebAssetsSdk/Tasks/ReadStaticWebAssetsManifestFile.cs Outdated
Comment thread src/StaticWebAssetsSdk/Tasks/ReadStaticWebAssetsManifestFile.cs Outdated
Comment thread src/StaticWebAssetsSdk/Tasks/ReadStaticWebAssetsManifestFile.cs Outdated

@jankratochvilcz jankratochvilcz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚢 it!

@AlesProkop
AlesProkop merged commit 8a1dd27 into dotnet:main Jun 18, 2026
25 checks passed
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 11.0-preview6 milestone Jun 23, 2026
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.

[Multithreaded] Migrate ReadStaticWebAssetsManifestFile in SDK

4 participants