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

try to fix https://github.com/fsprojects/Paket/issues/2640 #2721

Merged
merged 8 commits into from Sep 5, 2017

Conversation

Projects
None yet
2 participants
@matthid
Member

matthid commented Sep 4, 2017

by always calling getVersions.

#2640

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Sep 4, 2017

Member

Ok here is the explanation for this:

  • "Assuming" a version as the resolver did for "pinned" version broke my assumption that "GetDetails" cannot fail (because GetVersions already returned a version)
  • Apparently this was a "feature", because it is the only way to install packages which are not indexed yet by NuGet

Therefore, the solution is:

  • We always call GetVersions before
  • If there are NO results (maybe the case where package is not indexed) we save a bool to indicate that we "assumed" the version and forward that info to GetDetails
  • GetDetails will no longer blacklist if it sees that the version was "assumed"

I noticed another problem where NuGet sources are asked when the package only existed in a local one, I hope this is solved by this as well (and was only a special case of this).
Will add a test for this case with pinned version.

Member

matthid commented Sep 4, 2017

Ok here is the explanation for this:

  • "Assuming" a version as the resolver did for "pinned" version broke my assumption that "GetDetails" cannot fail (because GetVersions already returned a version)
  • Apparently this was a "feature", because it is the only way to install packages which are not indexed yet by NuGet

Therefore, the solution is:

  • We always call GetVersions before
  • If there are NO results (maybe the case where package is not indexed) we save a bool to indicate that we "assumed" the version and forward that info to GetDetails
  • GetDetails will no longer blacklist if it sees that the version was "assumed"

I noticed another problem where NuGet sources are asked when the package only existed in a local one, I hope this is solved by this as well (and was only a special case of this).
Will add a test for this case with pinned version.

@matthid matthid merged commit 04810ed into master Sep 5, 2017

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@@ -1,5 +0,0 @@
{
"sdk": {
"version": "2.1.0-preview1-007002"

This comment has been minimized.

@0x53A

0x53A Sep 5, 2017

Contributor

maybe we should instead lock to 2.0.0 (the release version)? Do you know why this was originally added?

@0x53A

0x53A Sep 5, 2017

Contributor

maybe we should instead lock to 2.0.0 (the release version)? Do you know why this was originally added?

This comment has been minimized.

@matthid

matthid Sep 5, 2017

Member

I have no idea, I guess it was before 2.0.0 was released. yes we should probably lock and downgrade the fake script.

This was just to get rid of the very annoying blocker locally.

@matthid

matthid Sep 5, 2017

Member

I have no idea, I guess it was before 2.0.0 was released. yes we should probably lock and downgrade the fake script.

This was just to get rid of the very annoying blocker locally.

This comment has been minimized.

@matthid

matthid Sep 5, 2017

Member

I have no idea, I guess it was before 2.0.0 was released. yes we should probably lock and downgrade the fake script.

This was just to get rid of the very annoying blocker locally.

@matthid

matthid Sep 5, 2017

Member

I have no idea, I guess it was before 2.0.0 was released. yes we should probably lock and downgrade the fake script.

This was just to get rid of the very annoying blocker locally.

matthid added a commit that referenced this pull request Sep 17, 2017

make the resolver request only sources returned by GetVersion (anothe…
…r case similar to #2721 but in the 'pre-fetcher')
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment