Automatically detect missing NuGet packages and restore #70851

dibarbet · 2023-11-16T02:07:00Z

Implements basic support for automatic nuget restore in the C# standalone project system. This is done by checking the project.assets.json to ensure all project dependencies are present.

Client PR: dotnet/vscode-csharp#6676

Looks like:

sharwell · 2023-11-17T14:08:10Z

Does this still work if the only change is removing a package reference? Why not just always run a restore after open? NuGet is expected to have a fast-path NOP if you run a restore after it was already up-to-date.

dibarbet · 2023-11-17T18:37:44Z

Does this still work if the only change is removing a package reference? Why not just always run a restore after open? NuGet is expected to have a fast-path NOP if you run a restore after it was already up-to-date.

It works for both - we'll detect a missing project.assets.json and run a restore if that is the case.

...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs

jasonmalinowski · 2023-11-21T23:15:26Z

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

+
+        if (projectFileInfo.PackageReferences.IsEmpty)
+        {
+            // If there are no package references then there are no unresolved dependencies.


What about stuff from the SDK itself? I admit I don't know the details here but I'm just asking.

Missing stuff from the SDK should be covered under the presence of the project.assets.json. I think the only potential problem would be if you change TFM, but I'm not quite certain how to detect that...

Looks like there is some information in the assets file on the target frameworks that change if you modify TFMs. Going to take a look to see if its as simple as checking if the TFMs in the project match the TFMs in the lock file.

Missing stuff from the SDK should be covered under the presence of the project.assets.json.

I wasn't sure about the case where we've seen during dogfooding where you upgrade to some SDK version and the various implicit packs aren't actually in feeds yet.

I added some code to look at target framework changes as well. However I don't think it will be easy to detect SDK version changes - we'd have to track and listen to changes in the global.json or look at which SDK the build host used to load the project (if you manually upgrade without having a global.json). I don't think that is necessarily worth doing.

...Server/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreParams.cs

src/VisualStudio/Core/Test.Next/Options/VisualStudioOptionStorageTests.cs

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

jasonmalinowski · 2023-11-21T23:27:45Z

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

+        {
+            var message = string.Format(LanguageServerResources.Project_0_has_unresolved_dependencies, projectFileInfo.FilePath)
+                + Environment.NewLine
+                + string.Join(Environment.NewLine, unresolved.Select(r => $"{r.Name}-{r.VersionRange}"));


Should we be indenting this or something?

jasonmalinowski · 2023-11-22T00:18:37Z

@dibarbet: Regarding @sharwell's point:

What about something like:

Open a project, a restore happens (any number of ways.)
Then remove a project reference

At that point the project.assets.json will exist and will contain all the things restored. Does that need to run a restore again to remove the reference from the project? I don't know if the underlying SDK tasks will ignore that or will still operate on the last restore.

dibarbet · 2023-11-22T01:08:53Z

After offline discussion - to ensure we run restore when a package reference is removed or a version changes (but still satisfies the range), going to look at diffing the last set of the package reference items we saw on the project, than fall back to project.assets.json checks (in case things change on disk).

jasonmalinowski

Feedback is all naming/ordering things and could be done as a follow-up.

jasonmalinowski · 2023-12-04T21:46:21Z

src/Workspaces/Core/MSBuild.BuildHost/MSBuild/ProjectFile/Extensions.cs

+            if (packageReferenceItems == null)
+            {
+                return ImmutableArray<PackageReference>.Empty;
+            }


This can happen? We aren't doing that for any of the other GetItems() checks above; do those need to be corrected too?

jasonmalinowski · 2023-12-04T21:47:33Z

src/Workspaces/Core/MSBuild.BuildHost/MSBuild/ProjectFile/ProjectFileInfo.cs

@@ -154,6 +160,7 @@ public override string ToString()
            ImmutableArray<DocumentFileInfo> additionalDocuments,
            ImmutableArray<DocumentFileInfo> analyzerConfigDocuments,
            ImmutableArray<ProjectFileReference> projectReferences,
+            ImmutableArray<PackageReference> packageReferences,


Did you want to keep the constructor sorted with the properties?

jasonmalinowski · 2023-12-04T21:54:32Z

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

+{
+    private const string UnresolvedProjectDependenciesName = "workspace/_roslyn_projectHasUnresolvedDependencies";
+
+    internal static bool HasUnresolvedDependencies(ProjectFileInfo newProjectFileInfo, ProjectFileInfo? previousProjectFileInfo, ILogger logger)


Suggested change

internal static bool HasUnresolvedDependencies(ProjectFileInfo newProjectFileInfo, ProjectFileInfo? previousProjectFileInfo, ILogger logger)

internal static bool NeedsRestore(ProjectFileInfo newProjectFileInfo, ProjectFileInfo? previousProjectFileInfo, ILogger logger)

Since this does a lot more than just check for unresolved dependencies now.

jasonmalinowski · 2023-12-04T21:57:33Z

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

+        if (newProjectFileInfo.TargetFramework != previousProjectFileInfo.TargetFramework)
+        {
+            // If the target framework has changed then we need to run a restore.
+            return true;
+        }


This will never happen, since we use TFM as a part of the identity of the project:

roslyn/src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs

Line 86 in 73b4ebd

Contract.ThrowIfFalse(newProjectInfo.TargetFramework == _mostRecentFileInfo.TargetFramework);

If we want to diff on TFM addition/removal we'd need to do that elsewhere.

Ah will remove, but this will still work- it will hit the case where we don't have any previous project file info and we'll trigger a restore.

jasonmalinowski · 2023-12-04T22:01:18Z

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

+            return true;
+        }
+
+        if (!newPackageReferences.SetEquals(previousPackageReferences))


We should really just teach SetEquals to do the length check first, but that can be later.

jasonmalinowski · 2023-12-04T22:02:15Z

...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs

+    private static bool CheckProjectAssetsForUnresolvedDependencies(ProjectFileInfo projectFileInfo, ILogger logger)
+    {
+        var projectAssetsPath = projectFileInfo.ProjectAssetsFilePath;
+        if (!File.Exists(projectAssetsPath))


Today I learned File.Exists() takes a null. I'm not sure how I feel about that.

jasonmalinowski · 2023-12-04T22:05:00Z

...t.CodeAnalysis.LanguageServer/HostWorkspace/ProjectTelemetry/ProjectLoadTelemetryReporter.cs

@@ -26,7 +26,7 @@ internal class ProjectLoadTelemetryReporter(ILoggerFactory loggerFactory, Server
    /// so that we are able to compare data accurately.
    /// See https://github.com/OmniSharp/omnisharp-roslyn/blob/b2e64c6006beed49460f063117793f42ab2a8a5c/src/OmniSharp.MSBuild/ProjectLoadListener.cs#L36
    /// </summary>
-    public async Task ReportProjectLoadTelemetryAsync(Dictionary<ProjectFileInfo, (ImmutableArray<CommandLineReference> MetadataReferences, OutputKind OutputKind)> projectFileInfos, ProjectToLoad projectToLoad, CancellationToken cancellationToken)
+    public async Task ReportProjectLoadTelemetryAsync(Dictionary<ProjectFileInfo, (ImmutableArray<CommandLineReference> MetadataReferences, OutputKind OutputKind, bool HasUnresolvedDependencies)> projectFileInfos, ProjectToLoad projectToLoad, CancellationToken cancellationToken)


Suggested change

public async Task ReportProjectLoadTelemetryAsync(Dictionary<ProjectFileInfo, (ImmutableArray<CommandLineReference> MetadataReferences, OutputKind OutputKind, bool HasUnresolvedDependencies)> projectFileInfos, ProjectToLoad projectToLoad, CancellationToken cancellationToken)

public async Task ReportProjectLoadTelemetryAsync(Dictionary<ProjectFileInfo, (ImmutableArray<CommandLineReference> MetadataReferences, OutputKind OutputKind, bool NeedsRestore)> projectFileInfos, ProjectToLoad projectToLoad, CancellationToken cancellationToken)

jasonmalinowski · 2023-12-04T22:06:25Z

...Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs

@@ -185,11 +185,13 @@ public async ValueTask<(ImmutableArray<CommandLineReference>, OutputKind)> Updat

        WatchProjectAssetsFile(newProjectInfo, _fileChangeContext);

+        var hasUnresolvedDependencies = ProjectDependencyHelper.HasUnresolvedDependencies(newProjectInfo, _mostRecentFileInfo, logger);


Also rename to "needs restore".

jasonmalinowski · 2023-12-04T22:08:43Z

...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs

@@ -195,7 +212,7 @@ private async ValueTask LoadOrReloadProjectsAsync(ImmutableSegmentedList<Project
        return binaryLogPath;
    }

-    private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)
+    private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind, bool HasUnresolvedDependencies)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)


Suggested change

private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind, bool HasUnresolvedDependencies)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)

private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind, bool NeedsRestore)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)

dibarbet · 2023-12-04T22:56:38Z

responding to feedback in a followup

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 16, 2023

dibarbet mentioned this pull request Nov 16, 2023

Enable automatic nuget restore on the client dotnet/vscode-csharp#6676

Merged

dibarbet force-pushed the automatic_nuget_restore branch from f69f9c2 to 98a3d40 Compare November 16, 2023 20:33

build-analysis bot mentioned this pull request Nov 16, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

dibarbet marked this pull request as ready for review November 21, 2023 21:51

dibarbet requested review from a team as code owners November 21, 2023 21:51

jasonmalinowski reviewed Nov 21, 2023

View reviewed changes

Automatically detect missing NuGet packages and restore

98552e2

dibarbet force-pushed the automatic_nuget_restore branch from 98a3d40 to 696bf99 Compare November 28, 2023 22:00

dibarbet requested a review from jasonmalinowski November 28, 2023 22:01

Review feedback

809fd2f

dibarbet force-pushed the automatic_nuget_restore branch from 696bf99 to 809fd2f Compare November 28, 2023 23:02

jasonmalinowski approved these changes Dec 4, 2023

View reviewed changes

dibarbet merged commit 02de7d7 into dotnet:main Dec 4, 2023

ghost added this to the Next milestone Dec 4, 2023

dibarbet deleted the automatic_nuget_restore branch December 5, 2023 19:31

Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically detect missing NuGet packages and restore #70851

Automatically detect missing NuGet packages and restore #70851

dibarbet commented Nov 16, 2023 •

edited

Loading

sharwell commented Nov 17, 2023

dibarbet commented Nov 17, 2023

jasonmalinowski Nov 21, 2023

dibarbet Nov 21, 2023

dibarbet Nov 22, 2023

jasonmalinowski Nov 22, 2023

dibarbet Nov 28, 2023

jasonmalinowski Nov 21, 2023

jasonmalinowski commented Nov 22, 2023

dibarbet commented Nov 22, 2023

jasonmalinowski left a comment

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

dibarbet Dec 4, 2023 •

edited

Loading

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

jasonmalinowski Dec 4, 2023

dibarbet commented Dec 4, 2023

	internal static bool HasUnresolvedDependencies(ProjectFileInfo newProjectFileInfo, ProjectFileInfo? previousProjectFileInfo, ILogger logger)
	internal static bool NeedsRestore(ProjectFileInfo newProjectFileInfo, ProjectFileInfo? previousProjectFileInfo, ILogger logger)

	public async Task ReportProjectLoadTelemetryAsync(Dictionary<ProjectFileInfo, (ImmutableArray<CommandLineReference> MetadataReferences, OutputKind OutputKind, bool HasUnresolvedDependencies)> projectFileInfos, ProjectToLoad projectToLoad, CancellationToken cancellationToken)
	public async Task ReportProjectLoadTelemetryAsync(Dictionary<ProjectFileInfo, (ImmutableArray<CommandLineReference> MetadataReferences, OutputKind OutputKind, bool NeedsRestore)> projectFileInfos, ProjectToLoad projectToLoad, CancellationToken cancellationToken)

		@@ -185,11 +185,13 @@ public async ValueTask<(ImmutableArray<CommandLineReference>, OutputKind)> Updat

		WatchProjectAssetsFile(newProjectInfo, _fileChangeContext);

		var hasUnresolvedDependencies = ProjectDependencyHelper.HasUnresolvedDependencies(newProjectInfo, _mostRecentFileInfo, logger);

	private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind, bool HasUnresolvedDependencies)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)
	private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind, bool NeedsRestore)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)

Automatically detect missing NuGet packages and restore #70851

Automatically detect missing NuGet packages and restore #70851

Conversation

dibarbet commented Nov 16, 2023 • edited Loading

sharwell commented Nov 17, 2023

dibarbet commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Nov 22, 2023

dibarbet commented Nov 22, 2023

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet commented Dec 4, 2023

dibarbet commented Nov 16, 2023 •

edited

Loading

dibarbet Dec 4, 2023 •

edited

Loading