Migrate Microsoft.NET.ProjectData.Tasks and the associated tests - #85154
Conversation
44cc278 to
01bf9ae
Compare
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
New sources contain tab indentation (repo style disallows tabs) and MergeProjectDataSlicesTask uses ProjectFilePath unconditionally despite documenting it as optional when OutputPath is provided.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — ProjectFilePath is documented as only required when OutputPath is empty, but the success path… |
|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — This file (and many others added in this PR) uses tab indentation. Roslyn style is 4-space… |
What changed in this PR
This PR migrates the Microsoft.NET.ProjectData.Tasks MSBuild task assembly and its associated unit tests into the Roslyn repo under src/LanguageServer/ProjectData, wiring the new projects into the repo solution and package versioning so ProjectData cache generation/validation/merging logic can be built and tested in-repo.
Changes:
- Add
Microsoft.NET.ProjectData.TasksMSBuild tasks (write slice, merge slices, validate packages, write receipts, unsupported marker) plus supporting helpers. - Add
Microsoft.NET.ProjectData.Tasks.UnitTestscovering cache merging, path portability, analyzer config filtering, and smoke/invariant tests. - Register the new projects in
Roslyn.slnxand add a central package version forNuGet.Versioning.
| File | Description |
|---|---|
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/AnalyzerConfigFileFilter.cs | Filters/normalizes analyzer config file inputs for cache writing. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/CachePathResolver.cs | Converts absolute paths to portable sentinel forms and normalizes embedded paths. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/ForwardCompat.cs | Forward-compatibility helpers for reading/writing cache formats. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs | MSBuild task wrapper that merges per-TFM slice files into a merged cache. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/Microsoft.NET.ProjectData.Tasks.csproj | New task project (netstandard2.0) with packaging/output-closure support for tests. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/PooledCacheRender.cs | Pooled writer to render cache content without allocating intermediate strings/arrays. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/ProjectDataBuildCompletionLogger.cs | MSBuild logger that records aggregate structured evidence for ProjectDataBuild attempts. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/ProjectDataMerger.cs | Core merge/dedupe logic for multi-targeting cache slices. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/ProjectDataWriter.cs | Writer-side cache emission logic used by the tasks. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/StringComparers.cs | OS-sensitive path comparison helpers used by tasks utilities. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/ValidateProjectDataPackagesTask.cs | Validates evaluated package requests against restore graph and package folder existence. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/WriteProjectDataBuildReceiptTask.cs | Writes completion receipts after ProjectDataBuild completion. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/WriteProjectDataSliceTask.cs | MSBuild task wrapper that writes a single project-data slice file. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/WriteUnsupportedProjectDataMarkerTask.cs | Writes the “intentionally unsupported” marker into the user cache. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/build/Microsoft.NET.ProjectData.Schema.props | Generated property allow-list imported by the targets. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/build/Microsoft.NET.ProjectData.targets | Targets/UsingTask wiring and ProjectDataBuild/Build hooks for producing cache files. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/AnalyzerConfigFileFilterTests.cs | Unit tests for analyzer config filtering and applicability logic. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/CachePathResolverTests.cs | Unit tests for sentinel/path portability rewriting. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/ForwardCompatTests.cs | Tests for forward-compat parsing/behavior across cache versions. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/LscacheInvariants.cs | Invariant assertions for well-formed .lscache output (and their tests). |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/Microsoft.NET.ProjectData.Tasks.Tests.csproj | New unit test project and dist-file copy target for task smoke tests. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/ProjectDataMergerTests.cs | Unit tests for multi-TFM slice merging/deduplication behavior. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/ProjectDataWriterTests.cs | Unit tests for writer output and edge cases. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/TargetsFileSmokeTests.cs | Smoke tests for targets integration/output shape. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/TaskExecutionTests.cs | End-to-end task execution tests under MSBuild-like layout. |
| src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks.Tests/XunitV2TestContext.cs | Test-only shim to satisfy shared test infrastructure expectations. |
| Roslyn.slnx | Adds the new Tasks and Tasks.Tests projects to the solution. |
| eng/Packages.props | Adds a central package version for NuGet.Versioning. |
| this.Succeeded = true; | ||
| UnsupportedProjectDataMarker.Delete(this.ProjectFilePath); | ||
| this.RecordDonorIndexEntry(); | ||
| this.Log.LogMessage(MessageImportance.Low, "ProjectData: merged {0} slices into {1}.", count, this.ResolvedOutputPath); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
MergeProjectDataSlicesTask can incorrectly delete the output file when OutputPath is set but ProjectFilePath is empty, due to path comparison using Path.GetFullPath("").
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — DeleteOutputPathIfNotProjectFolder assumes ProjectFilePath is set to detect the project-folder… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — ProjectFilePath is documented as only required when OutputPath is empty, but the success path… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — This file (and many others added in this PR) uses tab indentation. Roslyn style is 4-space… View resolved comment |
| private void DeleteOutputPathIfNotProjectFolder() | ||
| { | ||
| if (string.IsNullOrEmpty(this.ResolvedOutputPath) || | ||
| PathsEqual(this.ResolvedOutputPath, Path.GetFullPath(this.ProjectFilePath) + ".lscache")) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It introduces new MSBuild tasks/targets and cache-generation behavior that should be validated by a human reviewer across build scenarios and platforms.
Review tier: Lite
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — DeleteOutputPathIfNotProjectFolder assumes ProjectFilePath is set to detect the project-folder… View comment |
|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — ProjectFilePath is documented as only required when OutputPath is empty, but the success path… View comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs:84
- MergeProjectDataSlicesTask supports callers specifying OutputPath without ProjectFilePath, but if SliceFiles isn't provided the task falls back to ProjectDataMerger.FindSlices(SliceGlob). When SliceGlob is empty (the default), FindSlices ends up passing an empty search pattern to Directory.EnumerateFiles, which throws and is swallowed by the catch-all, leaving the task to report only a low-value warning and potentially keep stale output.
Since you already treat some misconfigurations as errors (OutputPath/ProjectFilePath), consider also failing fast when neither SliceFiles nor SliceGlob is provided so configuration problems are diagnosable.
This migrates this code from the internal repository.
Since this repo isn't what's deploying this, let's not worry about it.
f082558 to
45090f3
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are a couple of concrete behavioral issues in the new task wrappers (misleading logging and a success-path dependency on an optional input) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — MergeProjectDataSlicesTask's doc comment says ProjectFilePath is only required when OutputPath is… |
|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/WriteUnsupportedProjectDataMarkerTask.cs — When Reason is empty/whitespace, UnsupportedProjectDataMarker.Write normalizes it to "Unsupported",… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — DeleteOutputPathIfNotProjectFolder assumes ProjectFilePath is set to detect the project-folder… View comment |
|
src/LanguageServer/ProjectData/Microsoft.NET.ProjectData.Tasks/MergeProjectDataSlicesTask.cs — ProjectFilePath is documented as only required when OutputPath is empty, but the success path… View comment |
| this.Succeeded = true; | ||
| UnsupportedProjectDataMarker.Delete(this.ProjectFilePath); | ||
| this.RecordDonorIndexEntry(); | ||
| this.Log.LogMessage(MessageImportance.Low, "ProjectData: merged {0} slices into {1}.", count, this.ResolvedOutputPath); |
| this.MarkerPath = UnsupportedProjectDataMarker.Write(this.ProjectFilePath, this.Reason); | ||
| this.Log.LogMessage( | ||
| MessageImportance.Low, | ||
| "ProjectData: wrote unsupported marker for {0}: {1} ({2})", | ||
| this.ProjectFilePath, | ||
| this.MarkerPath, | ||
| this.Reason); |



This migrates this code from the internal repository.
Microsoft Reviewers: Open in CodeFlow