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

Make restore command check if dependencies and lock file are in sync #1750

Closed
tebeco opened this issue Jun 23, 2016 · 40 comments
Closed

Make restore command check if dependencies and lock file are in sync #1750

tebeco opened this issue Jun 23, 2016 · 40 comments

Comments

@tebeco
Copy link
Contributor

tebeco commented Jun 23, 2016

This may be an issue in Paket.VisualStudio in fact so i'll post in both and well see where it goes

Description

Open a project with changes on Paket.dependencies does not impact paket.lock
The Idea should be :
When you get the Latest source control version, open it and rebuild it (the solution).
You should be using the "dependencies" version, even if it means updation paket.lock
(see repro step)

Repro steps

Please provide the steps required to reproduce the problem

  1. Create a solution that use paket and where paket.dependencies have something like :
    nuget MyLib 1.0

    paket install

The paket.lock is created, Build your solution and the close Visual Studio
2. To simulation a change on SourceControl open your "paket.dependencies" and change it :
nuget MyLib 2.0

  1. Re open your solution in Visual Studio Try to rebuild all ...
    You'll see that "paket.lock" did not took the v2.0 of the lib but is still using 1.0
    But paket.dependencies indicate v2.0

Expected behavior

Please provide a description of the behavior you expect.

Actual behavior

Known workarounds

Force "Install manually" when you open the solution (it's a bit f*** up right ?)

Related information

  • Operating system : Win10
  • Release 3.1.9
  • Lastest Vs Extension also
  • .NET Framework 4.5.x
@forki forki changed the title Paket.lock is not updated after changes in paket.dependencies Make restore command check if dependencies and lock file are in sync Jun 23, 2016
@forki
Copy link
Member

forki commented Jun 23, 2016

Paket.lock is not updated after changes in paket.dependencies

That's not going to happen automatically. The lock file is the single truth and the most important file in the system. All changes will remain explicit via install or update commands.

I change the title of this issue since I think we can make restore check if something changed in the deps file. If that happend then we can let restore fail. I think this is a sensible way to detect inconsistencies.

@isaacabraham
Copy link
Contributor

@forki you mean deps vs lock file inconsistency?

@forki
Copy link
Member

forki commented Jun 23, 2016

yes.

@tebeco
Copy link
Contributor Author

tebeco commented Jun 23, 2016

I do agree ... almost all points here
May be the "restore" should Warn (with an option "as error") is it detect that "dependencies" file is not synch with "lock" file

so when you "paket restore" at least in your shell or IDE you have some tips to let you know something is probably bad
Just like NPM does when you "NPM install" and tries to helps you when you're using deprecated version

@isaacabraham
Copy link
Contributor

No. Paket has outdated for that. We don't want to do version checking and stuff like that when doing a paket restore.

@forki
Copy link
Member

forki commented Jun 23, 2016

No, I don't want to do version checking like outdated does.
I would just use the internal "smart install" logic and perform the same test that install does before it starts resolver. I mean install command only runs resolver for a given group if it finds a inconsistency between lock file and deps file for that group. We could use this and let restroe fail if that's the case. We would not need any webrequests for that check.

@tebeco
Copy link
Contributor Author

tebeco commented Jun 23, 2016

Well then is there an existing command to run this check ?
So that it will break if there's sync issue between lock & deps

@tebeco
Copy link
Contributor Author

tebeco commented Jun 23, 2016

I mean, paket solves issue about multi "packages" version in multiple "packages.config" but is not smart enought to tell (or let you know if there's a command for that) if the 2 file of paket "lock" and "dependencies" have different version.
It's hard to defend ...

"We fix the multipe version issue, but know you're not sure of the version you use depending on the file you're looking at"

@baronfel
Copy link
Contributor

Well, that seems to be to be a bit of user education. If you as a user change the requirements for a package (like adding the 2.0 version constraint that you did above) you must tell paket to recompute the dependency graph. This works the same for when you add a new package, remove a package, change an install option (like target framework version).

@isaacabraham
Copy link
Contributor

@forki
Copy link
Member

forki commented Jun 23, 2016

you're not sure of the version you use depending on the file you're looking at

You need to look into the lock file. That's the file that fixes all versions and contains the valid resolution.

forki added a commit that referenced this issue Jun 23, 2016
@forki
Copy link
Member

forki commented Jun 23, 2016

So in the latest Paket 3.2 alpha we have the following check in paket restore:

  • We check if there are any changes in paket.dependencies after the last paket install/update
  • If we detect that paket.lock and paket.dependencies are out of sync we throw an error:

image

@tebeco
Copy link
Contributor Author

tebeco commented Jun 23, 2016

So now a spark shine,
Do you think having a new feature (that is disable by default) in the paket.VisualStudio to run this the first time the solution is opened ?
So we could be able to warn the user one way or another ?

Or in paket.config (or both)
Or as error ? (just like Warn as error does allready ?)

All the pipeline would be the same as today, it would just give hints about potential technical debts

@forki
Copy link
Member

forki commented Jun 23, 2016

Paket.VisualStuio will fail on restore with the correct message. I think that's fine and will tell you that something is fucky.

forki added a commit that referenced this issue Jun 23, 2016
@forki forki closed this as completed Jun 23, 2016
@matthid
Copy link
Member

matthid commented Jun 23, 2016

Ok, but please make that opt-out or opt-in. I actually depend on this behavior from time to time when debugging hard to find dependency issues or to find when an incompatibility was introduced. What I do: I update the paket.lock file by hand (i know this is evil) to force paket to install some specific versions (even when the new dependency graph is not even valid). Please let me keep doing this as it helps a lot from time to time. It is easier than updating paket.dependencies and putting == everywhere until I get what I want. Like paket restore --force?

@forki
Copy link
Member

forki commented Jun 23, 2016

Your use case is still possible as long as the manually changed versions
don't violate the original restrictions.

But you are right we definitely should add a parameter which just brutally
uses the lock file. Ideas for names?
On Jun 23, 2016 8:11 PM, "Matthias Dittrich" [email protected]
wrote:

Ok, but please make that opt-out or opt-in. I actually depend on this
behavior from time to time when debugging hard to find dependency issues or
to find when an incompatibility was introduced. What I do: I update the
paket.lock file by hand (i know this is evil) to force paket to install
some specific versions (even when the new dependency graph is not even
valid). Please let me keep doing this as it helps a lot from time to time.
It is easier than updating paket.dependencies and putting == everywhere
until I get what I want. Like paket restore --force?


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1750 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNEkfJ3SjjFlTSbeeXoLa3VjTtWRIks5qOsxDgaJpZM4I8w6o
.

@isaacabraham
Copy link
Contributor

Agreed, I've done this from time to time as well.

@isaacabraham
Copy link
Contributor

@forki -unsafe? -manualLock?-ignoreDepsFile?

@forki
Copy link
Member

forki commented Jun 23, 2016

(also we need to give people a way out for when our changedetection is
buggy)
On Jun 23, 2016 8:30 PM, "Isaac Abraham" [email protected] wrote:

Agreed, I've done this from time to time as well.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1750 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNN3oYEPeyCzr74xhuztTBD9RVgqUks5qOtDWgaJpZM4I8w6o
.

@forki
Copy link
Member

forki commented Jun 23, 2016

I think it should be something around "ignore checks". Or we reuse - f
On Jun 23, 2016 8:33 PM, "Steffen Forkmann" [email protected] wrote:

(also we need to give people a way out for when our changedetection is
buggy)
On Jun 23, 2016 8:30 PM, "Isaac Abraham" [email protected] wrote:

Agreed, I've done this from time to time as well.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1750 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNN3oYEPeyCzr74xhuztTBD9RVgqUks5qOtDWgaJpZM4I8w6o
.

@matthid
Copy link
Member

matthid commented Jun 23, 2016

I suggested --force above but --ignoreDeps (or something like it) might be better. There possibly are use cases where people don't want to have a deps file (because it is managed somewhere else or whatever)

@matthid
Copy link
Member

matthid commented Jun 23, 2016

Thinking about it overloading -f for multiple things is probably not a good thing (or at least have the other parameters as well and document like "-f shortcut for --ignore-dependencies-file ... --... ")

@matthid
Copy link
Member

matthid commented Jun 23, 2016

@tebeco What nobody has said jet: You should really add the lockfile to your source control if it isn't (and your report sounds like it isn't). That way you get exactly the same version as the one running paket update or paket install. That's really the way to use paket. There is never a reason to not have the lockfile added to your source control. Plus what @baronfel has already said. You cannot expect paket to do something differently when neither paket install nor paket update has been called.
That's exactly the advantage paket tries to deliver.

This issue shows that throwing an error is probably the better default behavior (but imho its wrong because restore should not need to read paket.dependencies).

@tebeco
Copy link
Contributor Author

tebeco commented Jun 23, 2016

@matthid the lock file is on git and the paket update did not get updated so no push was done on it.

I can't explain that but anyway I think that an optional settings that could warm the user about out synch may be a good idea to track version issue

I'm not saying it should be by default, but for example as an option in either the IDE / one of the option if paket.dependemcies (header block) or paket.config in the roaming (or all of it)

@forki
Copy link
Member

forki commented Jun 24, 2016

going with '--ignore-checks'

@vbfox
Copy link
Contributor

vbfox commented Jun 24, 2016

Just to weight in, except if there are hidden consequences (I don't think there is any risk but i'm thinking about edge cases like https://blogs.msdn.microsoft.com/oldnewthing/20110504-00/?p=10743/ ) the principle seem right to me.

It would stop bad situations on the build server right at the start when paket restore is launched instead of later on a random developer machine when paket install is needed.

@Martin-Bohring
Copy link

Hmmh, I get that very error message after the automatic update to paket 3.2.0.0.
But I can't get out of it. No install, update, clear-cache or other commands seem to fix it. So what is the best way to solve it? Delete the lock file in that case?

I have some version constraints for transitive dependencies in there, but not to complex ones.

@Martin-Bohring
Copy link

Deleting the lock file and doing an install afterwards produced the same lock file again. And I have the error message as well. I am a bit clueless how to resolve the problem.

Are there changes in the package resolution strategy that could cause the error message as well?

@forki
Copy link
Member

forki commented Jun 24, 2016

Can you please send me the lock file and deps file?
On Jun 24, 2016 5:21 PM, "Martin Bohring" [email protected] wrote:

Deleting the lock file and doing an install afterwards produced the same
lock file again. And I have the error message as well. I am a bit clueless
how to resolve the problem.

Are there changes in the package resolution strategy that could cause the
error message as well?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1750 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNP4dVxbn_XsmkPHcvvnNn7QUrFxuks5qO_YKgaJpZM4I8w6o
.

@Martin-Bohring
Copy link

Wow that is quick 👍

You can find the lock and deps file at:
https://github.com/Martin-Bohring/fixieSpec

Just trying out now, if AppVeyor can build it.

@Martin-Bohring
Copy link

Well AppVeyor creates the same error message and I think I have package caching disabled.

https://ci.appveyor.com/project/Martin-Bohring/fixiespec/build/0.0.1.203

@forki
Copy link
Member

forki commented Jun 24, 2016

please retry with latest version.

@Martin-Bohring
Copy link

Wow, that is a turn around time like magic. 💯
Works know.

What was the issue?

@forki
Copy link
Member

forki commented Jun 24, 2016

auto-detect was triggering the outdated check

@Martin-Bohring
Copy link

Martin-Bohring commented Jun 24, 2016

I see framwork auto detection. Than you very much for that super quick response

@halcwb
Copy link
Contributor

halcwb commented Jun 26, 2016

I am afraid something is broken for my projects. When I tried to rebuild previous successfully build projects in appveyor or travis, I get the sync error. I have to delete and rebuild the lock file push the changed file and it is fixed. But I have quite a lot or projects. So, I am not really happy with this.

@tebeco
Copy link
Contributor Author

tebeco commented Jun 26, 2016

This is why I asked of visual studio extension could do a force install when the solution is opened (juste like nuget does)

And let it optional in settings ;)

@halcwb
Copy link
Contributor

halcwb commented Jun 26, 2016

But I do have a paket.lock file in my repository that should be in sync with dependencies as it is the direct result of a paket install. Still it's not working and also, I cannot fix this.

@ltvan
Copy link

ltvan commented Jun 27, 2016

This is the most breaking change ever. Now every succeeded build is failed if I try to build it again. Why don't you go with a --check or give error only when doing paket install, not restore?

@forki
Copy link
Member

forki commented Jun 27, 2016

yes sorry.

I changed ti to print a warning instead of failing the restore. I will continue to fix the out-of-date check.
Sorry for causing so much trouble,

337407

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

9 participants