-
Notifications
You must be signed in to change notification settings - Fork 525
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
Forcing lowercase with tolower(Id) yields poor Proget performance during resolution #2466
Comments
Mhm I am not sure why we even do that. The first request for the package name should actually return the correct casing. But maybe that's not the case in one of the other feeds. |
Is it possible that this could be changed? The behavior I've been seeing implies that the toLower is being used off the bat with nuget. |
I assume it is possible. But I can't remember if why we added it. Please send a pull request and we see what it breaks. |
I don't think we don't think we propagate the casing of the first query, only the casing from the @Christoba can you check if casing correctly in I'm not sure where paket can find the proper casing, the only other call we do is |
@Christoba can you please retry with latest? |
We've seen a definite improvement (we're all using 5.6.6 right now) and resolution is down to seconds. Casing is something that I aligned early on (shortly after the ticket was entered) and it didn't seem to have a direct benefit, but this was a couple of weeks back. Thanks for the update! |
Ok I think we should probably add a warning when we use the fallback |
Instead of a warning we could look casing up in the lockfile. So only very
first resolution would be slow. Or we could alternatively patch the deps
file.
Am 20.07.2017 23:41 schrieb "Matthias Dittrich" <[email protected]>:
… Reopened #2466 <#2466>.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2466 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADgNNaJgpWYh62hF_2X_X8y3rnhzwNyks5sP8mPgaJpZM4OF9yu>
.
|
Yes I agree, but warning is simpler to implement ;) Long-term we could fixup casing directly while resolving, so warnings will disappear with next install or update (at least that was the thinking when I suggested the warning) |
Or maybe we need to use a query/find API first to find the properly cased Id for each source, no idea. |
We are also seeing this with our ProGet server. And along with #1777 it makes Paket almost impossible to use at our organisation. |
@rasmus do you have correct casing in your dependencies file? I changed it to try the given casing first and only fallback to lower when nothing was found |
@forki So as a workaround we'll have to specify correct casing for all of our indirect dependencies? |
No indirect deps are probed with the casing from the nuspec definitions of the direct deps. So in most cases they should be correct. |
@forki So the problem is that some package in our chain of dependencies have listed |
This or we have a bug somewhere. But paket should honor casing from deps file and nuspec files/ your proget api. |
@forki I tried added the line
|
Mmm. I think we need to invest this then. I'm currently very short on time since we got a baby. But maybe I find a minute. |
@forki Congratulations 😄 I understand difficulties of finding time to open source when having kids |
Investigate. Damn phone. |
ok found 5min. can you please retry with latest? It should at least try with correct casing now and the number that is given in the lock file. So if you manually change that to 1.0 then it should at least now work with ProGet. |
@forki tried it out, but the initial search URL looks like |
Oups. That's what you get for doing this in 5min. Can you please take a
look at the very last commit. Then you can see the change and just remove
the to lower in that url call and send a PR? Then I will released asap.
Sorry.
Am 09.08.2017 12:14 nachm. schrieb "Rasmus Mikkelsen" <
[email protected]>:
… @forki <https://github.com/forki> tried it out, but the initial search
URL looks like http://dev-nuget/nuget/Default/Packages?$filter=(Id eq
'nunit') and (NormalizedVersion eq '3.7.1'). Shouldn't that be NUnit
(with a capital N and U)? It seems that it will always do a fallback as nunit
!= NUnit
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2466 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNLMSkJT5HRrr5ubW6T1i0nQrObBSks5sWYaSgaJpZM4OF9yu>
.
|
@forki no problem, it happens 😉 |
Ok fixed it. Hopefully. Can you please retry |
@forki Very nice, that fixed the performance problems. Thanks. |
Can you all please retry with 5.86.0-alpha002. Otherwise we might break you on next release. |
Description
When resolving packages on a Proget Nuget feed with 150k package versions, the use of tolower(Id) in tryGetAllVersionsFromNugetODataWithFilter breaks performance optimizations around "Id eq 'package'" in Proget. This forces full enumeration of packages in the feed, which is orders of magnitude slower.
See related question posted to http://inedo.com/support/questions/6810
Repro steps
Please provide the steps required to reproduce the problem
paket.dependencies references a package with source as a Proget feed with 100s of thousands of package version
Issue a get using Id='', Version='' and compare performance to a paket update on the same package.
Expected behavior
Paket could provide syntax or a keyword override within paket.dependencies to disable case insensitivity handling like tolower()
The text was updated successfully, but these errors were encountered: