Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dotnet list package checks if restore is current #14065

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Nigusu-Allehu
Copy link
Contributor

@Nigusu-Allehu Nigusu-Allehu commented Jan 23, 2025

@Nigusu-Allehu
Copy link
Contributor Author

Thank you @zivkan for the ideas in our offline discussion.

@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review January 23, 2025 19:19
@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner January 23, 2025 19:19
@Nigusu-Allehu Nigusu-Allehu requested a review from jeffkl January 23, 2025 19:20
<!-- Why is this the best design compared to other designs? -->
<!-- What other designs have been considered and why weren't they chosen? -->
<!-- What is the impact of not doing this? -->
- **Force Restore Every Time**: Automatically run restore when dotnet list package is called.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should say "Implicitly run restore which might do a no-op with an option to disable via a command-line argument --no-restore which other commands have. This would probably be me vote personally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#13406 is the tracking issue.

It'd be worth to call out the challenges if we were to go that direction.

We can also see if @OliaG can help with some CD to see if the breaking is worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, as per the spec guide, I think we should make a good argument about why we're not pursuing certain alternative.
In this case, those drawbacks are not obvious.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec looks great, complete and there's plenty of technical detail.

We should explore @jeffkl's idea and get a sense of whether we'd want to make that breaking change (or if we've explored it in more detail, we should details about why we're not pursuing that direction.

<!-- Why is this the best design compared to other designs? -->
<!-- What other designs have been considered and why weren't they chosen? -->
<!-- What is the impact of not doing this? -->
- **Force Restore Every Time**: Automatically run restore when dotnet list package is called.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#13406 is the tracking issue.

It'd be worth to call out the challenges if we were to go that direction.

We can also see if @OliaG can help with some CD to see if the breaking is worth it.

<!-- Why is this the best design compared to other designs? -->
<!-- What other designs have been considered and why weren't they chosen? -->
<!-- What is the impact of not doing this? -->
- **Force Restore Every Time**: Automatically run restore when dotnet list package is called.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, as per the spec guide, I think we should make a good argument about why we're not pursuing certain alternative.
In this case, those drawbacks are not obvious.

@dsplaisted
Copy link

To me, it seems like the list package command should do an implicit restore (which will often be a no-op). If it's already going to check if a restore is needed, why can't it do the restore if necessary rather than give you an error telling you to run restore?

@Nigusu-Allehu
Copy link
Contributor Author

To me, it seems like the list package command should do an implicit restore (which will often be a no-op). If it's already going to check if a restore is needed, why can't it do the restore if necessary rather than give you an error telling you to run restore?

That is a great point. Currently the following is the counter argument to that: The dotnet list package command used to be offline, but now it would be potentially online due to the restore. For example, VS Code uses dotnet list package to list PackageReferences in a user’s project. This works offline, but with the restore, the command could fail if the user has no internet connection.

discussed here #13406 (comment)

@jeffkl
Copy link
Contributor

jeffkl commented Feb 5, 2025

To me, it seems like the list package command should do an implicit restore (which will often be a no-op). If it's already going to check if a restore is needed, why can't it do the restore if necessary rather than give you an error telling you to run restore?

That is a great point. Currently the following is the counter argument to that: The dotnet list package command used to be offline, but now it would be potentially online due to the restore. For example, VS Code uses dotnet list package to list PackageReferences in a user’s project. This works offline, but with the restore, the command could fail if the user has no internet connection.

discussed here #13406 (comment)

Thinking about this more, I'm imagine any scenario where my project XML just has:

<ItemGroup>
  <PackageReference Include="SomePackage" Version="1.0.0" />
</ItemGroup>

I run dotnet list package and it shows me that I reference SomePackage. Then I edit the project XML and change the version and run dotnet list package again but it shows me the old version. The proposal is to tell me that that what I just did to my project XML caused restore to be out of date and tells me that I need to run dotnet restore in order for dotnet list package to work properly.

What does that same scenario in Visual Studio feel like? Implicit restore is on by default but if I turn it off, does PM UI show me what was previously restored? Does it prompt me to manually restore before it will show me the packages?

I struggle because I defined my packages in XML and saved the file. To me, dotnet list package should show me my current state but instead it just reads the project.assets.json (a file generated by restore that I may or may not understand). So does dotnet list package list the packages I configured or does dotnet list package list the packages that are in a file that I do not control? In my opinion, its the former, so implicit restore should have been how it worked from the beginning. The fact that a restore has to take place in order for dotnet list package to give me what I want seems like an implementation detail that an end user should not have to care about.

The offline scenario is potentially something to care about of course. But if I restore my project by simply building it or loading it in Visual Studio, the restore already happened and subsequent implicit restores with dotnet list package would no-op, making it work just fine. If I edit my package graph while I'm disconnected, there's no guarantee dotnet restore would work unless I have all packages on disk. In that case dotnet list package would tell me my restore is out of date, and restore wouldn't work either. For scenarios we control, like VS Code, we could keep track of whether or not a restore completed and add --no-restore to the command-line arguments, just to make it faster.

The real question that I'm interested in knowing the answer to is: How many actual users would experience a negative experience if we changed dotnet list package to do an implicit restore?

Is that impact broad? If those users could just update their script with a --no-restore argument, would that be acceptable? Is making this command a better experience for the majority a better approach? Without more data telling us, its hard to make the decision. Obviously, my opinion is to add the implicit restore but its definitely up for a vote.

@donnie-msft
Copy link
Contributor

PM UI show me what was previously restored? Does it prompt me to manually restore before it will show me the packages?

PM UI shows a banner that "Some packages are missing" and offers a Restore gesture.
You'll also get Error List errors indicating the assets file isn't found. Or if it is found, errors about some packages being missing.

An action (say, Update a package) still triggers restore regardless of whether the setting is disabled. Not sure if this is by design or a bug when an action triggers the restore versus a build.

I believe PM UI is already aligning with your suggestion for dotnet list package and PM UI will show users "the current state" and essentially "Work Offline" as much as it can.


The `dotnet list package` command is used to list packages referenced by a project, but it requires a restore operation to be performed beforehand to ensure accurate information. NuGet currently checks for the existence of an assets file to assert that restore has been performed.
However, if a user modifies the project file after running restore, the assets file may become outdated, leading to misinformation from the dotnet list package command.
This proposal discusses how to ensure that NuGet asserts the assets file is in sync with the project file to prevent such issues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this should actually call out the proposal.


## Summary

The `dotnet list package` command is used to list packages referenced by a project, but it requires a restore operation to be performed beforehand to ensure accurate information. NuGet currently checks for the existence of an assets file to assert that restore has been performed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The `dotnet list package` command is used to list packages referenced by a project, but it requires a restore operation to be performed beforehand to ensure accurate information. NuGet currently checks for the existence of an assets file to assert that restore has been performed.
The `dotnet list package` command is used to list packages referenced by a project, but it requires a restore operation to be performed beforehand to ensure accurate information.
NuGet currently checks for the existence of an assets file to assert that restore has been performed.

inconsistent new lines per sentence


The `dotnet list package` command is used to list packages referenced by a project, but it requires a restore operation to be performed beforehand to ensure accurate information. NuGet currently checks for the existence of an assets file to assert that restore has been performed.
However, if a user modifies the project file after running restore, the assets file may become outdated, leading to misinformation from the dotnet list package command.
This proposal discusses how to ensure that NuGet asserts the assets file is in sync with the project file to prevent such issues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This proposal discusses how to ensure that NuGet asserts the assets file is in sync with the project file to prevent such issues
This proposal discusses how to ensure that NuGet asserts the assets file is in sync with the project file to prevent such issues.

nitpick 🤣

## Summary

The `dotnet list package` command is used to list packages referenced by a project, but it requires a restore operation to be performed beforehand to ensure accurate information. NuGet currently checks for the existence of an assets file to assert that restore has been performed.
However, if a user modifies the project file after running restore, the assets file may become outdated, leading to misinformation from the dotnet list package command.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
However, if a user modifies the project file after running restore, the assets file may become outdated, leading to misinformation from the dotnet list package command.
However, if a user modifies the project file, or a ProjectReference's project file, after running restore, the assets file may become outdated, leading to misinformation from the dotnet list package command.

packages are transitive

Comment on lines +18 to +19
Before this command is executed, users are expected to perform a restore operation.
NuGet expects restore to be run both after project creation and every time a change has been made to the project.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: The first sentence is basically a less detailed duplicate of the second sentence. So one of these can be deleted.


<!-- Explain the proposal in sufficient detail with implementation details, interaction models, and clarification of corner cases. -->

Comparing the contents of the project file and the assets file is the most important step here. We will achieve this by using no-op restore.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Comparing the contents of the project file and the assets file is the most important step here. We will achieve this by using no-op restore.
Comparing the contents of the project file and the assets file is the most important step here.
We will achieve this by using no-op restore.

One sentence per line 😁

Comparing the contents of the project file and the assets file is the most important step here. We will achieve this by using no-op restore.

- Create a DGSpec file from the evaluated project
- To create the DGSpec file, use an MSBuild target, [GenerateRestoreGraphFile](https://github.com/NuGet/NuGet.Client/blob/cb0b5561f4fd98347cedac11919f769da0b767f4/src/NuGet.Core/NuGet.Build.Tasks/NuGet.RestoreEx.targets#L46), that writes the DGSpec to a file and then load it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've given this feedback on a number of feature specs, but I think this is too much detail for a feature or design spec.

My concern is that some people find these design specs, wrongly assume they're still current, even years after they're written or implemented, and then assume that any behaviour differences are a bug.

The first implementation might run msbuild as a child process to run this target, but this has a flaw when trying to get a PackageSpec or DGSpec, but I'm working on making it easier for all NuGet's dotnet * commands to create a DGSpec programatically, in-process. Depending on the timing of the two workstreams, maybe dotnet list package will not need to use this msbuild target.

Basically, I think it's sufficient to give the "high level" overview of how we'll detect if the restore is up to date, which is to use the same logic as no-op restores. How we achieve the no-op restore check is an implementation detail that is subject to change over time.


Log the following

> Error: The project's dependency graph has been altered. Please perform a restore for project '{0}'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "The project's dependency graph has been altered" makes any sense to customers. The DGSpec is a NuGet internal implementation detail, that even expert customers have no need to know about.

A more friendly message would be something like "The project has been modified since the last restore"


## Prior Art

<!-- What prior art, both good and bad are related to this proposal? -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • VS restores automatically on solution load for SDK style projects, or before every build for all other projects
  • dotnet build, dotnet publish, and dotnet test all restore automatically by default, unless --no-restore is provided
  • installing or upgrading packages either in VS or with dotnet add package, restores the project, in addition to modifying project files. dotnet add package has a --no-restore option.

@Nigusu-Allehu Nigusu-Allehu self-assigned this Feb 11, 2025
@jebriede jebriede marked this pull request as draft February 18, 2025 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants