search

NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

Enable repeatable package restores using a lock file #5602

Closed anangaur closed 5 years ago

anangaur commented 7 years ago

Discussions should happen on this issue. Please link other issues with similar asks to this one.

July 2018 - Announced feature December 2018 - Blog

jainaashish commented 5 years ago

this is to do with dotnet lzma tool which generates a different nupkg for the same package (id/version) in fallback folder which gives a different hash for the same package, where one coming from fallback folder and another from nuget.org. You can find more details in this issue# https://github.com/NuGet/Home/issues/7414#issuecomment-431997934

anangaur commented 5 years ago

Update: I am mistaken in the below suggestion. Please ignore it.

@Flavien , @springy76

~Just another pointer - You do not need to delete the packages, instead just point NuGet to not look at fallback folder. See details here: https://docs.microsoft.com/en-us/nuget/release-notes/nuget-4.9-rtm#packages-in-fallbackfolders-installed-by-net-core-sdk-are-custom-installed-and-fail-signature-validation---7414~

springy76 commented 5 years ago

I inserted <RestoreAdditionalProjectSources/> into autoexec.bat but did not work -- seriously: If someone writes a xml snippet into documentation then PLEASE give a hint where to place it (file -> parent-node/hierarchy).

jainaashish commented 5 years ago

@anangaur RestoreAdditionalProjectSources has nothing to do with fallback folders, Not sure where did you get this idea? If you really want to suggest skipping fallback folders then RestoreFallbackFolders is the right property. More details at https://github.com/NuGet/Home/wiki/%5BSpec%5D-NuGet-settings-in-MSBuild

anangaur commented 5 years ago

@springy76 My apologies. My earlier suggestion was misplaced. I guess, for now, you can try the deletion or what @jainaashish suggested. We will investigate the issue and get back.

@springy76 , @Flavien Can you tell us more details like SDK versions (and repro steps)?

ericnewton76 commented 5 years ago

EDIT: Damn I wish I'd have seen this issue sooner. First I saw of this was the blog post on Nuget.org. Its great you guys are so much more transparent about things but I have a real bone to pick with y'all on the Nuget team.

Omg. Whats the point of checkin of package.json.lock if you're not going to use it by default! We need packages to be locked down at the time the developer picks a package via install or updates via update.

Do you guys ever check out npm? They did this several years ago via npm shrinkwrap and enhanced it over a year ago to always write out a lock file to fix the same scenario... repeatable builds... even repeatable on developers machines. They use the package.lock when doing npm restore and they even created an npm ci command that bypasses all package dependency resolutions in favor of just restoring the exact package dependency tree.

It feels like the Nuget team is so opinionated about things that keep being proven wrong again and again. I've had my clashes with various members in the past about issues like this and I'm continually overruled then vindicated several months if not year or so later on issues.

I realize Nuget has some significant challenges especially with the multitude of platforms and whatnot. But this particular feature is a no-brainer guys...

Should be OPT-OUT if not desired and I can't imagine any scenario where that is legitimately the case.

Now its baked in as OPT-IN so everybody keeps going forward slamming their heads into their desks while the Build server starts installing unwanted new versions of DLLs that are different from the developer's machines...

Here's how its gonna go: Developer: "Works on my machine!"
Manager: "It sure does... damn... what the heck???" Developer 2: "Did you set that option in Nuget to only restore the exactly whats in package.lock file so that new versions of some of the transient dependencies dont get installed?"
Developer: "...Wait... what? I thought having the lock file would do that... I already had to set something in the CSPROJ file to turn this on."

ericnewton76 commented 5 years ago

And to be clear:

package lock files should only be generated at INSTALL or UPDATE time. When a restore happens, and a package lock doesnt exist (solve issue of old libraries) then CREATE IT. If this happens on build server, nothing you can do about it. If this happens on DEV machine, it shows up as new file. (Except on TFS where you HAVE to remember to promote the thing... another id10tic feature) You should have to OPT-OUT of this behavior.

Secondly I recommend, like npm restore, to only install exactly whats in the package lock file. npm ci command was created to make this process even faster by not evaluating any dependency. It simply deletes node_modules and restores EXACTLY whats in package.lock. For nuget, the restore command should ALWAYS respect the package.lock file and so that if run on the build system and it cant restore EXACTLY whats in packages.json.lock, because a transient dependency exact version went unlisted or similar (hash fail maybe?) then it should fail the build, so developers can evaluate the situation, come back to their developer machine and effectively evaluate the situation, by themselves running nuget restore which precisely matches up the packages to packages.lock file. What I'm saying is... the same thing should happen on my developer machine as on the build server... otherwise its nondeterministic even at checkin. A transitive dependency can update its version literally one second after checkin, and bam the build server is pulling newer version. And you've made this OPT-IN so that developer may or may not end up replicating the build server package tree. So again whats the point?

Maybe theres more to it, I cant imagine it... thats 99% of developer intended workflow regarding a package manager in relation to the build/release phase and SHOULD utilize LOCKED package modes. You made this opt-in instead of opt-out so the build output of a lot of projects will continue to be nondeterministic as time moves on which is just silly.

TL;DR the RESTORE should ALWAYS respect package.lock thats why it exists. INSTALL and UPDATE should always WRITE and UPDATE the dependency versions within packages.lock if needed, and any changes get exposed in the VCS checkin. If you didnt intend for that to happen... DONT CHECKIN THE PACKAGES.LOCK file! Very simple. You guys have made it mind numbingly irritating by now having to double check a csproj setting upon install/update/restore to even write the file, AND you make it so we have to now go into every build definition and double check that this feature is turned on.

forki commented 5 years ago

@ericnewton76 check out https://fsprojects.github.io/Paket/ - .NET has this for years.

anangaur commented 5 years ago

@ericnewton76 Thanks for your detailed feedback. Let me try to answer your queries (as I understand them) one by one and we can delve into specifics, as required.

Why opt-in and and not have it opt-out? Why the presence of lock file is enough?

We did not want to suddenly start producing a new file with an incremental release i.e. as part of 4.x. In addition, the lock file feature is not complete unless we solve the problem of centrally managing package versions and thereby having a central lock file per repo/solution. Once done, we will evaluate to make the lock file default.

Btw, the presence of lock file is enough to opt in to this feature. The property RestoreWithLockFile is only to bootstrap into this feature. See the note section here for more details.

Install and Update should write to lock file and Restore should always restore from the lock file

Today there are various ways to install and update the packages - not just from various tools but also directly writing the PackageReference node into the csproj file:

To summarize, there were 2 options: Option 1: Continue with the current NuGet model of installs and restores and introduce thi feature in most non-breaking way as possible. Option 2: Re work on the notions of installs vs. restores and break the current way of adding a package by directly writing PackageReference node into the project file as the restore immediately after such writes/edits of PackageReferences would fail.

We chose Option 1. One can get the Option 2 behavior by setting the locked mode to true. Once set, restore will fail if it cannot get the exact packages as mentioned in the lock file. I feel this is what you want but as default?

nuget ci similar to npm ci

This seems like a good idea. We can definitely evaluate this option if you and others feel this would be useful and if this helps in improved performance of restore (theoretically this has promise but we would need to run a few experiments to understand the quantum of gain).

Do let me know if this answers your concerns? Happy to discuss more and/or delve deeper into any of the topics elaborated above.

ericnewton76 commented 5 years ago

Thanks for the reply. Sorry if my tone seemed too adversarial. I can be passionate about this stuff and its hard to guard my words sometimes.

We did not want to suddenly start producing a new file with an incremental release i.e. as part of 4.x. In addition, the lock file feature is not complete unless we solve the problem of centrally managing package versions

Granted, npm formally brought on the package-lock.json by moving to npm v5... read their release notes and releases after v5.0.0 It should be a very interesting read for another package manager in a slightly different ecosystem.

However, if you introduce this lock file in v4.8 then whats the difference really? It seems like MS in general is afraid to kick the major version up due to marketing concerns or something else instead of technology needs. I won't say thats what Nuget team does but it seems to be the norm. Nothing wrong with Nuget v10... LOL. Anybody that complains just doesnt understand how software is built then.

Directly writing PackageReference node into the csproj file is so easy and has become very common

That's fine... that'll happen on the developer's machine and the RESTORE command should complain like a stuck pig. At least squeal with a warning message that says a new packagereference is found and should be installed properly to have a proper dependency graph analysis performed. And then go ahead and basically do an install. When nuget restore --ci is running and the same occurs, then fail the build. Call a time out. And say "this is for your own good," and point them right at these messages as to why failing the build is necessary when a package lock file exists and there's packages being installed in a build server scenario that shouldn't be there. Again, complainers about this don't understand the problem until they smack into it when a transitive dependency floats on them and breaks the runtime behavior somehow.

Problem is: Option 1 is still non-deterministic builds.
Option 2 is better but still requires that specific opt-in that should be a default, and thus non-deterministic builds

The goal is a deterministic build. Both of your current options listed don't solve that... they just add more configuration switches to underlying mechanisms that don't help you achieve a stable build. And when the feature releases formally, now you have this extra cruft that has to be supported ad-infinium to preserve that exact behavior when it might not be true later.

Probably a difference of opinion here... you guys are trying to go for least disruptive change for something that will 100% make their lives better, but by not jumping in feet first, you're equivocating on a feature that is a must-have. In addition, did you try this out in-house first? Did you guys scratch their heads saying "omg! this would be bad for it to precisely match up my dependencies! and when it doesnt match, its notifying me that my hand edited package reference accidentally checked in due to TFS lunacy is warning me that something is awry!" I have a feeling it was the opposite. Please note I'm trying to be humorous here, to keep this deep subject in the realm of amusement.

You have to honestly ask yourself, would a developer whos tested precise versions of packages for days, possibly weeks, on his own machine, be okay with an algorithm making decisions for him about inaccurate package versions by library developers that probably will introduce inadvertent breaking changes into his runtime when he presses that build button to release to production on a thursday night at 10pm?

nuget restore --ci or nuget ci Just check out npm ci and some of the release notes about it. Makes perfect sense, and has legitimately locked down that crazy world of javascript package management! Quite amazing if you ask me... considering how fast things move over there too.

rrelyea commented 5 years ago

This didn't get documented in the release notes for 4.9.0 because it was mistakenly in 5.0 milestone. (working to fix)

For now, setting to 4.9.3 - will fix release notes in 4.9.0 when we ship release notes for 4.9.3 and then reset this back to 4.9.0

Will be closing this issue. Please spin off any follow up discussions in other issues.

Flavien commented 5 years ago

Wasn't the problem referenced above the same problem as this?

fowl2 commented 5 years ago

are there plans for a nugetrestore --locked-mode? Without this it seems like it's impossible to use the lock file on Azure Pipelines for framework/non-dotnetcore projects.

anangaur commented 5 years ago

@fowl2 Can you try msbuild instead:

msbuild.exe /t:restore /p:RestoreLockedMode=true

Flavien commented 5 years ago

I'm still struggling with this error:

error NU1403: The package System.Collections.Specialized.4.3.0 sha512 validation failed. The package is different than the last restore.

(happens with any package randomly)

I'm using NuGet 5.0.0.6 and have cleared my caches and fallback folders numerous times. I am still getting this error on my CI build no matter what I try.

Any ideas what's wrong?

anangaur commented 5 years ago

Can specify the exact steps? Or may be provide a repro?

anangaur commented 5 years ago

It would also be good to know the sources you are using. One of the reasons could be that the sources have different packages (with different SHA) and depending on which source was used to restore, you might see failures.

Flavien commented 5 years ago

Here is a repro: https://github.com/Flavien/nuget-lockfile-repro.

I have generated the lock file by building the project on Visual Studio (Windows) 16.0.0.0 Preview 5.0. NuGet version is 5.0.0.

When I clone this on my Ubuntu WSL and run dotnet restore --locked-mode, I get this:

flavien@LAPTOP-FLAVIEN:~/NuGet/NuGetLockFile$ dotnet restore --locked-mode
  Restore completed in 163.45 ms for /home/flavien/NuGet/NuGetLockFile/NuGetLockFile.csproj.
/home/flavien/NuGet/NuGetLockFile/NuGetLockFile.csproj : error NU1403: The package Microsoft.AspNetCore.2.2.0 sha512 validation failed. The package is different than the last restore.
  Restore failed in 5.25 sec for /home/flavien/NuGet/NuGetLockFile/NuGetLockFile.csproj.

Here is dotnet --info on my WSL install:

.NET Core SDK (reflecting any global.json):
 Version:   2.2.202
 Commit:    8a7ff6789d

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  18.04
 OS Platform: Linux
 RID:         ubuntu.18.04-x64
 Base Path:   /usr/share/dotnet/sdk/2.2.202/

Host (useful for support):
  Version: 2.2.3
  Commit:  6b8ad509b6

.NET Core SDKs installed:
  2.2.202 [/usr/share/dotnet/sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.2.3 [/usr/share/dotnet/shared/Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.2.3 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.2.3 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

The only source I have on Visual Studio is: nuget.org (https://api.nuget.org/v3/index.json). Sources on WSL:

Registered Sources:

  1.  https://www.nuget.org/api/v2/ [Enabled]
      https://www.nuget.org/api/v2/
StingyJack commented 5 years ago

We did not want to suddenly start producing a new file with an incremental release i.e. as part of 4.x

@anangaur - Yet it was OK to start breaking/normalizing version numbers in a minor release?

MarkKoz commented 4 years ago

There seem to be discrepancies across platforms or systems. I locked on Windows 10 and can restore fine on that same system. However, restoring on Arch Linux fails for System.Collections.NonGeneric. If I restore and let it update the lock file, then I can see it produces a different hash for the aforementioned dependency. Furthermore, I use an Ubuntu agent for my CI pipeline and there System.Collections.NonGeneric fails too, but so does Microsoft.NETCore.Platforms (which is fine on Arch).

Edit: solved by following https://github.com/NuGet/Home/issues/7921#issuecomment-478152479

ghost commented 3 years ago

@MarkKoz Just a heads up, the package Dotnet.ReproducibleBuilds.Isolated aims to configure these msbuild settings for you to avoid non-reproducibility issues. Among other things, it turns off that hidden nuget cache you ran into.

MarkKoz commented 3 years ago

Thanks @aaronla-ms. I'm glad MS is investing in solving this issue. I'll look into adding this to my projects.