Disallow applying updates to FBA MSBuildWorkspace - #84722
Conversation
|
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.
Pull request overview
This PR changes MSBuildWorkspace.TryApplyChanges behavior for file-based apps by preventing apply-operations that would otherwise load/save a “virtual” project and materialize it to disk, and adds a regression test plus localized resource strings for the new failure diagnostic.
Changes:
- Block apply-changes for file-based apps when the change would require project-file updates, reporting a workspace diagnostic instead.
- Add/adjust a file-based app test to validate the new behavior.
- Add a new localized resource string for the failure message across multiple
.xlffiles.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Workspaces/MSBuild/Test/NetCoreTests.cs | Updates file-based app test expectations around TryApplyChanges, including new diagnostics and filesystem assertions. |
| src/Workspaces/MSBuild/Core/MSBuild/MSBuildWorkspace.cs | Disallows applying project-file-affecting updates to file-based apps and reports a failure diagnostic. |
| src/Workspaces/MSBuild/Core/WorkspaceMSBuildResources.resx | Adds new user-facing resource string for the file-based app apply-updates failure. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.de.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.es.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.fr.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.it.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.ja.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.ko.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.pl.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.pt-BR.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.ru.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.tr.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.zh-Hans.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.zh-Hant.xlf | Adds localization entry for the new resource string. |
| src/Workspaces/MSBuild/Core/xlf/WorkspaceMSBuildResources.cs.xlf | Adds localization entry for the new resource string. |
Suppressed comments (1)
src/Workspaces/MSBuild/Test/NetCoreTests.cs:1023
- The assertions after TryApplyChanges are using the pre-apply
programProjectsnapshot and also rely on an exact directory listing. This can miss regressions if the workspace solution changes but the old snapshot still has the original text, and the directory assertion is brittle to unrelated files. Consider re-acquiring the project fromworkspace.CurrentSolutionafter the apply attempt and assert specifically that no.csprojwas materialized (and that the expected source files still exist).
Assert.Equal(["Program", "Util"], workspace.CurrentSolution.Projects.Select(p => p.Name).Order());
var programText = await programProject.Documents.Single(d => d.Name == "Program.cs").GetTextAsync();
AssertEx.Equal(programSource, programText.ToString());
Assert.Collection(Directory.EnumerateFileSystemEntries(SolutionDirectory.Path).Order(),
entry => Assert.Equal(Path.Combine(SolutionDirectory.Path, ".packages"), entry),
entry => Assert.Equal(Path.Combine(SolutionDirectory.Path, "Program.cs"), entry),
entry => Assert.Equal(Path.Combine(SolutionDirectory.Path, "Util.cs"), entry));
programProject = workspace.CurrentSolution.Projects.Single(p => p.Name == "Program");
Assert.Empty(programProject.ProjectReferences);
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
| @@ -992,14 +1001,34 @@ public static class Util | |||
| var solution = programProject.AddProjectReference(new ProjectReference(utilProject.Id)).Solution; | |||
| Assert.True(workspace.TryApplyChanges(solution)); | |||
There was a problem hiding this comment.
nit: feels reasonable to also assert we did not write a '.cs.csproj' file to disk.
There was a problem hiding this comment.
I'm asserting that below with
Assert.Collection(Directory.EnumerateFileSystemEntries(SolutionDirectory.Path).Order(),
entry => Assert.Equal(Path.Combine(SolutionDirectory.Path, ".packages"), entry),
entry => Assert.Equal(Path.Combine(SolutionDirectory.Path, "Program.cs"), entry),
entry => Assert.Equal(Path.Combine(SolutionDirectory.Path, "Util.cs"), entry));| @@ -992,14 +1001,34 @@ public static class Util | |||
| var solution = programProject.AddProjectReference(new ProjectReference(utilProject.Id)).Solution; | |||
| Assert.True(workspace.TryApplyChanges(solution)); | |||
There was a problem hiding this comment.
It's kinda strange to me that this isn't returning false and we're not throwing an exception, but I guess that's the pattern this code uses generally....strange. If you have to touch this PR for any other reason consider adding a comment here, otherwise let it be.
There was a problem hiding this comment.
I agree this is weird but also seems by design. Per its doc comment (and implementation), TryApplyChanges
- returns false only
if (newSolution.SolutionStateContentVersion != oldSolution.SolutionStateContentVersion) - throws only if
CanApplyChange(ApplyChangesKind feature)returns false, but that only takes the kind of the change, so we can't determine whether we are adding a project reference to a file-based app yet.
Fixes #84721.
Microsoft Reviewers: Open in CodeFlow