Allow MSBuildWorkspace to open file-based apps - #84139
Conversation
jjonescz
added
Area-IDE
Feature - Run File
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 61 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/Workspaces/MSBuild/Core/MSBuild/ProjectFileExtensionRegistry.cs:82
- TryGetLanguageNameFromProjectPath holds _dataGuard while calling IFileBasedProgramService.IsValidEntryPointPath(projectFilePath). That method performs file I/O (File.Exists + potentially opening the file to check for a shebang), so doing it under the lock can block other threads trying to query/associate extensions and can introduce responsiveness issues when the path is on a slow/unavailable filesystem.
Consider doing the dictionary lookup under the lock, then (only if not found) release the lock and perform the file-based-app check outside of it.
src/Workspaces/MSBuild/Test/NetCoreTests.cs:886
- This timeout is extremely short and is likely to be flaky on slow or contended test machines (100ms can be exceeded just by scheduling delays). Since the purpose here is to guard against the known hang, consider using a more forgiving timeout (seconds) to keep the test stable while still preventing an indefinite hang.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/Sha256Hasher.cs:15 - The XML doc comment appears to be a copy/paste artifact: this method hashes an arbitrary string, not specifically a MAC address. This is misleading for future maintainers trying to understand why this helper exists.
Consider updating the comment to describe the actual contract (produce the same SHA-256 hex string format as other tooling for the same input, including normalized casing in HashWithNormalizedCasing).
/// <summary>
/// The hashed mac address needs to be the same hashed value as produced by the other distinct sources given the same input. (e.g. VsCode)
/// </summary>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 61 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/Workspaces/MSBuild/Test/NetCoreTests.cs:887
- The 100ms cancellation timeout is extremely tight and risks intermittent failures on slower/loaded test machines (the call may not reliably observe cancellation that quickly). Using a more generous timeout keeps the test validating the hang workaround without introducing flakiness.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/Sha256Hasher.cs:15 - The XML doc comment mentions a "hashed mac address", but this helper hashes arbitrary text (paths, etc.). This looks like a copy/paste artifact and is misleading for future maintainers.
/// <summary>
/// The hashed mac address needs to be the same hashed value as produced by the other distinct sources given the same input. (e.g. VsCode)
/// </summary>
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
Didn't look closely at the msbuildhost changes. But what I saw in LS layer looked great. Great work, I'm glad we are moving to this!
| // App2.cs | ||
|
|
||
| var tempDir = _tempRoot.CreateDirectory(); | ||
| DeferDeleteCacheDirectory(tempDir.Path); |
There was a problem hiding this comment.
I guess this was just a no-op in the tests that don't DTB?
There was a problem hiding this comment.
I moved the DeferDeleteCacheDirectory call inside CreateDiscoveryTestServerAsync (to simplify the same pattern everywhere but also because it now needs access to the lsp server instance).
| """; | ||
| var sourceFile = tempDir.CreateFile("SomeFile.cs").WriteAllText(sourceText); | ||
|
|
||
| // Until we can discover the `#:ref`erenced project, build it so it works as metadata reference. |
There was a problem hiding this comment.
hmm. Do you think discovery should parse directives when we have a positive entry point match and walk the #:refs? (not for this PR obviously.) That feels viable since we would only pay the cost after identifying the #!.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 61 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/FileLevelDirectiveHelpers.cs:165
DirectiveDeduplicator.CheckDirectivereturnsshouldKeep=falsefor duplicate sdk/property/package directives with the same value (to avoid emitting duplicate imports/properties). However,FindLeadingDirectivescurrently ignoresshouldKeepand always adds the directive to the builder, so identical duplicates still flow into project generation and can cause duplicate<Sdk>, property, or package items.
src/Workspaces/MSBuild/Test/NetCoreTests.cs:913- Using a 100ms cancellation timeout here is very likely to be flaky on slower/loaded CI machines. Since this is guarding against a known hang, a longer timeout (seconds) keeps the test stable while still preventing indefinite hangs.
… merge CI (CodeQL "Analyze (csharp)") has been failing since the upstream-main merge with CS0433 errors in FormatterBenchmarks.cs: CSharpSyntaxFormattingOptions and NewLinePlacement exist in both the fork's own local build of Workspaces\CSharp\Portable (FollyOfTheUnbound.CodeAnalysis.CSharpUnbound.Workspaces, via ProjectReference) and the real released Microsoft.CodeAnalysis.CSharp.Workspaces 4.12.0 NuGet package. Root cause: Microsoft.CodeAnalysis.LanguageServer.UnitTests.csproj (which IdeCoreBenchmarks references for net10.0) has a PackageReference to Microsoft.CodeAnalysis.CSharp.Analyzer.Testing, added upstream in dotnet#84139 ("Allow MSBuildWorkspace to open file-based apps"). That package transitively depends on Microsoft.CodeAnalysis.CSharp.Workspaces, centrally pinned to 4.12.0 in eng/Packages.props (deliberately, for stable analyzer-testing infrastructure) -- which only started colliding with this fork's own local-build wiring once the upstream commit and this fork's IdeCoreBenchmarks project graph were merged together in this branch's recent upstream-main merge. Fixed by excluding that transitive package's compile assets in IdeCoreBenchmarks.csproj for the net10.0 TFM -- it doesn't need the NuGet build, since it already references the local one directly.
Resolves #78887.
Resolves #84588 (← public API issue).
Fixes #83304 (we don't use run-api anymore).
Closes #78879 (
VirtualProjectXmlProviderTestsare removed).SDK counterpart: dotnet/sdk#54773
Microsoft Reviewers: Open in CodeFlow