Fix FBA restore mismatch issue - #85204
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
jjonescz
added
Area-IDE
Feature - Run File
There was a problem hiding this comment.
🔵 Needs a closer look
The implementation depends on a draft SDK change with failing CI, while Roslyn validation remains incomplete.
Pull request overview
Fixes file-based app package restore mismatches when ArtifactsPath is explicitly configured.
Changes:
- Detects SDK support using
_SupportsFileBasedAppArtifactsPath. - Adds an LSP restore/reload regression test.
- Updates the synchronized SDK commit.
Assessment: The approach is sound, but one stale compatibility comment remains. The paired SDK PR is draft with failing CI, and Roslyn CI is incomplete.
File summaries
| File | Description |
|---|---|
VirtualProjectBuilder.cs |
Uses the SDK capability flag for artifact-path compatibility. |
commitid.txt |
Updates the synchronized SDK revision. |
FileBasedProgramsWorkspaceTests.cs |
Tests restore with an explicit artifacts path. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
9c2bd4b to
46d68a7
Compare
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
| }, | ||
| ], | ||
| }); | ||
| await Task.Delay(100); |
There was a problem hiding this comment.
this seems like it could cause flakiness - we should likely have an explicit condition we're expecting
There was a problem hiding this comment.
There are other tests in this file which explicitly create a watcher and await the watch event. We should use that.
We might have some lingering usage of await Task.Delay(100) which ideally should be replaced with this also.
If possible, it would be great to extract a test helper, which takes a file path and returns a task that awaits a watch event for the file.
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM modulo the test feedback
There was a problem hiding this comment.
🔵 Needs a closer look
The new SDK-dependent regression test currently resolves an ambient SDK instead of the repository-pinned SDK.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/MiscellaneousFiles/FileBasedProgramsWorkspaceTests.cs:114
- This is the only file-based-program test in this class that bypasses
CreateTempDirectoryWithGlobalJson; the other FBA tests pin the repository SDK (for example, line 36). Because this regression specifically depends on SDK artifact-path capabilities, resolving an ambient installed SDK can exercise a different compatibility branch or fail on machines with a different SDK set. Use the existing helper so the test is deterministic.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Adds a test for the issue discovered in dotnet/vscode-csharp#9734 (comment).
And brings in dotnet/sdk#56160 to fix it.
The previous attempt to support legacy artifacts path didn't work when the user sets ArtifactsPath explicitly. This better fix should work in all cases, except in preview versions between dotnet/sdk#55697 and dotnet/sdk#56160 (everything should mostly work fine, it's just that artifacts might end up in wrong places, or IDE might not detect FBA restore correctly - but it's really limited to just .NET 11.rc1, I think).
Microsoft Reviewers: Open in CodeFlow