Migrate Common.GenerateSourceLinkFile to multithreaded task model#1733
Migrate Common.GenerateSourceLinkFile to multithreaded task model#1733jankratochvilcz wants to merge 6 commits into
Conversation
Add the MSBuild multithreaded task marker and IMultiThreadableTask implementation with a TaskEnvironment fallback for direct unit-test construction. Resolve OutputFile through TaskEnvironment once for file operations while preserving the original OutputFile value for the SourceLink output and log messages. Sanitize exception messages that may contain the absolutized path back to the original path, and preserve the empty-output/null-or-empty OutputFile no-file behavior before absolutization. Add a decoy-CWD test that proves a relative OutputFile is resolved against the project directory while SourceLink remains the original relative value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Automated MT review (MT-migration reviewer + expert code review): both approved, no blocking/actionable findings. Verified: Sin 1 ( |
The empty-OutputFile/null-content guard only calls string.IsNullOrEmpty and Log.LogMessage and returns before outputPath is assigned, so none of it can throw. Move it above the try so the try scopes exactly the code that can throw (GetAbsolutePath + file I/O). No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| Log.LogWarning(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty); | ||
| } | ||
|
|
||
| if (content == null && string.IsNullOrEmpty(OutputFile)) |
There was a problem hiding this comment.
I'd make it a requirement on OutputFile to be an absolute path. The value of this parameter is controlled by our target and set from _SourceLinkFilePath defined here:
There was a problem hiding this comment.
@tmat I changed the implementation, please check it out
There was a problem hiding this comment.
@tmat Are you confident on this one? We have failing tests that suggest that currently absolute paths are not always being passed 👀
Address review feedback: rather than resolving OutputFile against the task's working directory via TaskEnvironment.GetAbsolutePath, require it to already be a fully qualified path. OutputFile is set by the SourceLink targets from _SourceLinkFilePath, so it is always fully qualified in practice, and file access must not depend on the shared process current directory under the multithreaded task model. - Make the MT migration attribute-only: drop IMultiThreadableTask and the TaskEnvironment property; keep [MSBuildMultiThreadableTask]. OutputFile is the only filesystem path the task touches, so rejecting a non-fully-qualified value makes the task MT-safe. - Reject a non-fully-qualified OutputFile via PathUtilities.IsPathFullyQualified. - Replace the relative-path resolution test with one asserting rejection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GenerateSourceLinkFile now requires OutputFile to be fully qualified. The targets set OutputFile from _SourceLinkFilePath, which was relative because IntermediateOutputPath is relative. Normalize it against the project directory so all source link tasks operate on absolute paths, and update integration tests to expect the absolute $(SourceLink) value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4 participants
Summary
Migrates
Microsoft.SourceLink.Common.GenerateSourceLinkFileto the MSBuild multithreaded task model for dotnet/msbuild#14093.Details
[MSBuildMultiThreadableTask]and implementsIMultiThreadableTaskwithTaskEnvironment.Fallbackso direct unit-test construction keeps existing semantics.OutputFileviaTaskEnvironment.GetAbsolutePathbefore allFile.Exists,File.Delete,File.ReadAllText, andFile.WriteAllTextcalls, so relative paths are based on the project directory instead of process CWD.[Output] SourceLinkvalue as the originalOutputFilestring, not the absolute path.OutputFilevalue and sanitizing exception messages that include the absolutized path back to the original path.OutputFileno-existing-file behavior before absolutization; non-empty content still fails through the existing catch path.Tests
./build.sh --restore --build --verbosity minimal./.dotnet/dotnet test src/SourceLink.Common.UnitTests/Microsoft.SourceLink.Common.UnitTests.csproj -f net11.0 --no-restoreWriteSourceLinkFileResolvesRelativeOutputFileAgainstProjectDirectory, a decoy-CWD test that verifies a relative output file is written under theTaskEnvironmentproject directory, not process CWD, whileSourceLinkremains the original relative value.