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

Use packages.lock.json instead of project.assets.json, if available #35556

Closed
gtbuchanan opened this issue Sep 19, 2023 · 8 comments
Closed

Use packages.lock.json instead of project.assets.json, if available #35556

gtbuchanan opened this issue Sep 19, 2023 · 8 comments
Assignees
Milestone

Comments

@gtbuchanan
Copy link

Is your feature request related to a problem? Please describe.

NuGet restore is required to generate project.assets.json even when a project-level lock file (e.g. packages.lock.json) already exists. When using the Cache task in Azure Pipelines, the recommendation is to add a condition to your NuGet restore task to prevent it from running. However, I've found this doesn't actually work in practice because the build process always expects project.assets.json to exist (the aforementioned link even mentions this like it might not happen).

Running a NuGet restore adds at least 30 seconds to our build time even when disabling all remote NuGet sources. I would expect the lock file to contain all the necessary information to build.

Describe the solution you'd like

The build process should detect and use packages.lock.json, if available, in place of project.assets.json instead of triggering NETSDK1004.

Additional context

See also: NuGet/Home#11770, NuGet/Home#12409 (comment)

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Sep 19, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@marcpopMSFT marcpopMSFT self-assigned this Oct 9, 2023
@marcpopMSFT
Copy link
Member

@nkolev92 and @zivkan is this a nuget suggestion or sdk suggestion? Not sure there is a lot of investment around lock files these days. CC @baronfel

@nkolev92
Copy link
Contributor

It's a NuGet suggestion.

The lock file does not contain enough information to setup the build.

  • It does not have the compatibility checks. The asset selection is not part of it, so you don't know which assemblies from the package to pass to the compiler.
  • It does not contain anything related to props/targets files from packages. When packages carry props/targets, the author's expectation is that these packages get imported at build time.
  • It doesn't contain information about the global packages folder. Restore ensures that all packages are in the global packages folder, so the build can complete.

I don't see us adding these to the lock file at this point. The lock file is pretty large as is, and readability is a key aspect of it.

@zivkan
Copy link
Member

zivkan commented Oct 13, 2023

It's a NuGet suggestion.

To be pedantic, if this suggestion was feasible, it'd probably require a code change in the SDK repo, not NuGet. However, as discussed the lock file doesn't contain all the information that the ResolvePackageAssets task needs, so it's not feasible, and NuGet really does need to run a restore. There's more stuff going on behind the scenes than what most customers expect.

It could be interesting to capture a PerfView trace to see what's taking "so long", and therefore get ideas for what perf improvements might be possible.

@gtbuchanan
Copy link
Author

gtbuchanan commented Oct 13, 2023

I don't see us adding these to the lock file at this point. The lock file is pretty large as is, and readability is a key aspect of it.

it's not feasible, and NuGet really does need to run a restore

Understood. I submitted this request based on how lock files work in other package managers (e.g. npm) without really understanding the full process for .NET. It just seemed odd to me to have to run a restore after already restoring the packages with the Pipelines Cache task, but I can see why it is necessary.

It could be interesting to capture a PerfView trace to see what's taking "so long", and therefore get ideas for what perf improvements might be possible.

The 30+ seconds (39s at the moment) I mentioned are not a huge deal in the scheme of things. However, I noticed much of the time is spent upfront:
image

Perhaps this is because I'm using MSBuild for the restore? We still have some legacy projects.
image

I'll see if I can gather some more information. It may make more sense for us to combine the build and NuGet steps (or use Nuget.exe directly?) if it's actually MSBuild that is taking up the time.

@marcpopMSFT marcpopMSFT removed the untriaged Request triage from a team member label Jan 8, 2024
@marcpopMSFT marcpopMSFT removed their assignment Jan 8, 2024
@marcpopMSFT marcpopMSFT added this to the Discussion milestone Jan 8, 2024
@marcpopMSFT
Copy link
Member

Leaving it in the SDK for now but assigning to nkolev92 and zivkan if they want to engage. Moved it to the discussion milestone. I can't mark it for nuget as we have a bot that will close the issue and route people to the nuget repo.

@zivkan
Copy link
Member

zivkan commented Jan 15, 2024

I'm closing this issue, as previous comments explain information that the SDK needs from the assets file that is missing from the lock file. Unfortunately, it's just not feasible to avoid restore to generate the additional intermediate files.

The 30+ seconds (39s at the moment) I mentioned are not a huge deal in the scheme of things. However, I noticed much of the time is spent upfront:

_GetAllRestoreProjectPathItems is Nuget's MSBuild code to get information about all projects in the solution, before running the restore task, that actually downloads packages, creates intermediate files, etc. A while ago MSBuild created different APIs, in attempt to make it more efficient, called static graph. Nuget has an opt-in mode to use these APIs, rather than getting project information via MSBuild script. You could try it out to see if it reduces the overall time: https://learn.microsoft.com/en-us/nuget/reference/msbuild-targets#restoring-with-msbuild-static-graph-evaluation

@zivkan zivkan closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
@gtbuchanan
Copy link
Author

Thanks for the reference. Static graph evaluation did not improve our restore times, but our restore times are not that bad to begin with.

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

No branches or pull requests

4 participants