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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Sep 5, 2017
Merged

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

merged 8 commits into from
Sep 5, 2017

Conversation

matthid
Copy link
Member

@matthid matthid commented Sep 4, 2017 •
edited
Loading

by always calling getVersions.

#2640

@matthid
Copy link
Member Author

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
0x53A
0x53A reviewed Sep 5, 2017
View reviewed changes
global.json
@@ -1,5 0,0 @@
{
"sdk": {
"version": "2.1.0-preview1-007002"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
@matthid
make the resolver request only sources returned by GetVersion (anothe…
14695ed 
…r case similar to #2721 but in the 'pre-fetcher')
@matthid matthid mentioned this pull request Sep 17, 2017
Merged
@matthid matthid deleted the try_to_fix_2640 branch July 2, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x53A 0x53A 0x53A left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@matthid @0x53A