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

"Possible Performance degration, V3 was not working" error when using with myget v3 feeds #2700

Closed
eiriktsarpalis opened this issue Aug 31, 2017 · 16 comments · Fixed by #2708
Assignees

Comments

@eiriktsarpalis
Copy link
Member

Description & Repro

Running paket update on a dependency file with the following content:

source https://www.myget.org/F/dotnet-core-svc/api/v3/index.json

nuget Microsoft.CSharp prerelease

produces the following output

Paket version 5.91.0
Resolving packages for group Main:
 - Microsoft.CSharp 4.0.1-beta-23708
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
 - System.Runtime 4.0.21-beta-23708
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
 - System.Runtime.InteropServices 4.0.21-beta-23708
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
Possible Performance degration, V3 was not working: Value cannot be null.
Parameter name: uriString
Possible Performance degration, V3 was not working: Value cannot be null.
(..truncated..)

Expected Behaviour

It seems that the behaviour is caused by a slight difference in schemata returned by nuget and myget.
For instance, compare this myget feed with a regular nuget feed. Notice that the "catalogEntry" field is missing in the former, which is actually what triggers the null exception because paket deserializes the response as a Registration type.

Would provide a PR to fix this, but not sure what the appropriate course of action would be here.

@eiriktsarpalis eiriktsarpalis changed the title Possible Performance degration, V3 was not working error when using with myget v3 feeds "Possible Performance degration, V3 was not working" error when using with myget v3 feeds Aug 31, 2017
@matthid
Copy link
Member

matthid commented Aug 31, 2017

One question: Is paket working ok or crashing eventually?

Nice analysis, thanks!
I think I'd either make the field an option (if our serialization logic can handle that) or create another type where this is an option and convert to it. And then handle the option appropriately - Or handle the null correctly :). I think we would accept all.

These are the kind of issues I expected to find after we switched our warning and fallback strategy...

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Aug 31, 2017 via email

@matthid
Copy link
Member

matthid commented Aug 31, 2017

I looked at the json, can you even download and use v3 on myget? Where is the download link and the dependency information? Maybe you should use v2 with myget as they are not ready yet?

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Aug 31, 2017 via email

@Gonnagle
Copy link
Contributor

Gonnagle commented Sep 1, 2017

We've struggling with similar issues lately. It seems that for example running "paket install -f" sometimes fails, sometimes completes seemingly successfully, but even then sometimes drops many of the transitive dependencies unexpectedly. Sources are nuget v3 and our private myget-feed in v3 as well.

Haven't been able to come up with repro yet, so not 100% if the issue of missing transitive deps is related to this, but my hunch is that issues started to appear around the same time that these V3 warnings started to appear (somewhere

@maartenba
Copy link

catalogEntry should not be used during install/restore and can safely be made optional

@eiriktsarpalis
Copy link
Member Author

@maartenba In this case it is not being used for either installation or restore. What would be the approved way of accessing the myget equivalent of catalog data?

@maartenba
Copy link

There is none. The catalog on NuGet is a way of replicating data between website and search, but should not be used for fetching data during install/restores either.

@maartenba
Copy link

You probably want the registration root (index.json): https://dotnet.myget.org/F/dotnet-core-svc/api/v3/registration1/microsoft.csharp/index.json

@matthid
Copy link
Member

matthid commented Sep 1, 2017

@maartenba I don't get it - where is the download link? Or how do we get dependency information for a single version? Or is this no longer supported and we should download all?

@maartenba
Copy link

Ok let's step back a bit: what info do you need in a request here? The registration version (x.y.z.json) will have a link to the .nupkg. need dependency info etc? The index.json will happily provide it.

@matthid
Copy link
Member

matthid commented Sep 1, 2017

@maartenba I think I understand: Basically we should make a single request per package which will resolve us ALL versions and ALL download links. I think that would actually be an improvement over the current state.

It seems like for packages with lot's of versions the situation is a bit more complicated (https://api.nuget.org/v3/registration3/fake/index.json).
I think implementing this properly might take a while (but is valuable).

First, I will try to figure out why V2 is not working which should unblock you (obviously we would accept a PR implementing this properly in the mean time)

Let's just hope the situation is better for V3 and not every implementation is doing it's own stuff with the change I suggested above.

Thanks for helping us understand what the proper solution would be.

@matthid matthid self-assigned this Sep 2, 2017
@matthid
Copy link
Member

matthid commented Sep 2, 2017

I tried changing to v2 but somehow it converted it to v3 and got the same error

@eiriktsarpalis I cannot reproduce this

source https://www.myget.org/F/dotnet-core-svc

nuget Microsoft.CSharp prerelease

works just fine in my limited testing

@maartenba
Copy link

In case needed:

  • The https://api.nuget.org/v3/index.json will contain a "flatcontainer" entry
  • Which can be used to resolve, e.g. https://api.nuget.org/v3/flatcontainer/fake/index.json
  • Which holds all versions (but does not distinguish between listed/unlisted (!))
  • Has all downloads as well, by convention https://api.nuget.org/v3/flatcontainer/{packageid-lower}/{version-lower}/{packageid-lower}.{version-lower}.nupkg (e.g. https://api.nuget.org/v3/flatcontainer/fake/1.46.1/fake.1.46.1.nupkg)

For more info, the registration blobs (per version and their index.json) are the way to go.

@matthid
Copy link
Member

matthid commented Sep 3, 2017

@maartenba Thanks for clarifying again, yes that's pretty much how I tried to implement it now. (At least I think it is)
Because we need these additional infos we only request the versions from flatcontainer and everything else (dependencies, downloadlink, ...) from PackageDisplayMetadataUriTemplate.

Also I noticed that for example for the package Nunit contains a version "5.0.0-beta03-tryoutmutex" in flatcontainer but the version field in the PackageDisplayMetadataUriTemplate is "5.0.0-beta03-tryoutMutex" (notice the differently cased "M")

@eiriktsarpalis Please retry with latest stables. Also please provide a repro for the case where a fallback to V2 was not working (as that is still missing)

If someone want's to review my commits here they are:

@eiriktsarpalis
Copy link
Member Author

@matthid thanks! this fixed the issue for me.

Re: the V2 issue, perhaps I bungled api path. I should point out that I used our private feed for that, which also incorporates an auth token.

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

Successfully merging a pull request may close this issue.

4 participants