Migrate StaticWebAssetsGeneratePackagePropsFile to IMultiThreadableTask#54617
Migrate StaticWebAssetsGeneratePackagePropsFile to IMultiThreadableTask#54617jankratochvilcz wants to merge 3 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
Area-AspNetCore
|
Thanks for your PR, @jankratochvilcz. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Static Web Assets SDK task StaticWebAssetsGeneratePackagePropsFile to be safe under MSBuild’s multi-threaded task execution model by resolving filesystem paths via TaskEnvironment rather than relying on the process current directory.
Changes:
- Mark
StaticWebAssetsGeneratePackagePropsFileas multi-threadable ([MSBuildMultiThreadableTask],IMultiThreadableTask). - Resolve
BuildTargetPathusingTaskEnvironment.GetAbsolutePath(...)before performing file IO. - Add a regression test that changes the process CWD and verifies the props file is still written under the task-environment project directory.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/StaticWebAssetsSdk/Tasks/StaticWebAssetsGeneratePackagePropsFile.cs |
Makes the task MSBuild-multithreadable and ensures file output uses TaskEnvironment-resolved paths. |
test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/StaticWebAssetsGeneratePackagePropsFileMultiThreadingTest.cs |
Adds a regression test to validate correct output location when process CWD differs from the project directory. |
- Fix awkward log message grammar (Copilot comment 3363270444) - Remove redundant test [Collection]/[CollectionDefinition] now that parallelization is disabled assembly-wide via LegacyStaticWebAssetsV1IntegrationTest.cs (Copilot comments 3363270493, 3363270509) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid leaking the resolved absolute path into log output ("sin" of
MT migrations — preserve original caller shape for user-visible
strings; only use the resolved absolute path for file I/O).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jankratochvilcz
left a comment
jankratochvilcz
left a comment
There was a problem hiding this comment.
Self-review notes on intentional path-handling decisions (this PR was authored by an autonomous agent — reviewer, please push back if any of this sounds off).
| AbsolutePath buildTargetPath = TaskEnvironment.GetAbsolutePath(BuildTargetPath); | ||
|
|
||
| var document = new XDocument(new XDeclaration("1.0", "utf-8", "yes")); | ||
| var elements = (AdditionalImports ?? []).Select(e => e.ItemSpec).Prepend(PropsFileImport) |
There was a problem hiding this comment.
Intentionally NOT routed through TaskEnvironment.GetAbsolutePath.
PropsFileImport and each AdditionalImports.ItemSpec end up here verbatim as the value of <Import Project="..."/> in the generated .props file (line 35 below). That generated .props file is packed into a NuGet (e.g., build/<PackageId>.props) and consumed on a different machine. MSBuild resolves the Project attribute at consume time, relative to the directory of the importing file — so the value must stay package-relative.
Example. A consumer authors <PackageReference Include="Contoso.AspNetCore.StaticWebAssets" />. NuGet drops build/Contoso.AspNetCore.StaticWebAssets.props which contains:
<Import Project="Microsoft.AspNetCore.StaticWebAssets.props" />That resolves to <nuget-cache>/contoso.aspnetcore.staticwebassets/<ver>/build/Microsoft.AspNetCore.StaticWebAssets.props on the consumer's machine. If we absolutized at pack time it would become e.g. /Users/jan/src/.../Microsoft.AspNetCore.StaticWebAssets.props and break every consumer.
The only path with a real CWD-vs-project-dir dependency in this task is BuildTargetPath (the write target), which is correctly routed through TaskEnvironment.GetAbsolutePath above. Imports are pure pass-through string content of the generated file.
Reviewer: if this reasoning is wrong (e.g., a call site does pass relative imports that need resolution at task time, not at consume time), please flag — this PR was authored by an autonomous agent and I'd like a human pair of eyes on this decision specifically.
| { | ||
| Log.LogMessage(MessageImportance.Low, $"Creating file '{BuildTargetPath}' does not exist."); | ||
| File.WriteAllBytes(BuildTargetPath, data); | ||
| Log.LogMessage(MessageImportance.Low, $"Creating file '{BuildTargetPath}' because it does not exist."); |
There was a problem hiding this comment.
Intentionally logs the caller-shape BuildTargetPath, not the resolved buildTargetPath.
The local buildTargetPath is always an absolute path (the output of TaskEnvironment.GetAbsolutePath). Logging it would leak machine-specific paths into customer-visible diagnostics and change historical log output for any caller that passed a relative path. File.Exists/File.WriteAllBytes below correctly use the resolved absolute path; only the user-visible string keeps the original shape — same pattern recent migrations (#54444) use via AbsolutePath.OriginalValue for output metadata.
Reviewer: if there's a stronger convention here (e.g., always log the absolute path because that's what hits disk), let me know. Autonomous-agent-authored PR — please challenge if this looks wrong.
Context
MSBuild's multi-threaded task execution model does not guarantee that the process current directory matches the project directory. Tasks that touch the filesystem need to resolve paths through the injected TaskEnvironment.
Changes
Safety
The existing fallback behavior is preserved through TaskEnvironment.Fallback, so legacy absolute paths continue to work. The task has no output path items that need OriginalValue preservation.
Testing