[Fail] add test for https://github.com/fsprojects/Paket/issues/2294 (Cannot pin NETStandard.Library = 1.6.0) #2305
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0x53A
changed the title
0x53A
force-pushed
the
unit-test-for-2294
branch
from
1d1e52f to
b76bc97
Compare
|
|
Yes this needs to be fixed as indicated by my comment here: https://github.com/fsprojects/Paket/blob/master/src/Paket.Core/PaketConfigFiles/DependenciesFile.fs#L273-L275 I wasn't sure if this case actually happens in real-file (therefore I left it as is for now). Apparently it happens and we need to provide the second resolving step with the info from the first... |
|
I was a bit careful there and would love to hear comments about what the best way to do it actually is (would my suggestion of hooking the |
|
maybe its a bit clearer with code. Let me quickly try to provide a fix with what I mean and then we can discuss (will push on your branch) |
|
please don't push on the performance pr, I think forki want's to get that merged asap. Feel free to branch off it and create a second pr? |
|
@0x53A I meant this one "unit-test-for-2294"? |
|
ah, yeah, sure =) |
|
For me locally its timing out. Which makes me wonder because resolution should now be a lot easier in the second run :/ (because we basically lock a lot of version already) |
|
Most likely I'm doing it all wrong ... |
|
I think #2299 should make it slightly faster (but it still fails at the end), so you could merge that into your local branch. |
|
It also times out for me without the performance pr, no idea how the CI got a result, maybe microsoft pushed new package versions which make the situation even worse? :D |
|
You mean without my commit its timing out as well now? |
|
Seems like it ... @cloudRoutine do you remember which version of paket you used when you could resolve #2294 (comment) in 2 minutes? For me it didn't finish even after half an hour without the performance pr (5.alpha release), and finishes after 5 minutes with the performance pr. |
|
This is what I get when I run it: This is probably why it times out with my (partial) other runtime puzzle. If first takes all the 4.3 versions of the packages and later notices that these aren't the correct ones (notice how I don't have NetStandard in the runtime resolution). Because This might be a good dependencies file for performance testing (however I'd need to print the packages its trying to resolve to actually know what it is trying to resolve...). For now the solution is probably quite simple: In the second runtime resolution I will add all the packages again, but lock their versions (like |
|
And the unit tests are red because we need to add some extra logic to detect runtime dependencies when we just resolve everything in the second step ... (I'm just looking at the failing test case atm) |
|
It looks like you know how to fix this, which is great because I have no clue =) |
|
@0x53A I know how to make it work with the last commit (and that one actually works. But the solution I suggested doesn't work (I really have no idea why the resolver sometimes runs into this "slow mode of operations" state and sometimes not. So the answer is I can make it green but I have no clue why :) |
The issue is that if it finds a conflict, it backtracks ONE. In the performance PR I added a heuristic to go back NOT one, but to the last requirement that has the same name. But that is still not perfect. Let's take a requirement list:
... another 100 requirements ...
... another 100 requirements ...
Previously, it would exhaust all previous 201 requirements, until it finally finds Now it will go to |
|
We have way more information the second time (runtime resolution) can I tell it to start with some packages before doing others? |
|
No idea, it took me about a day to figure out the above. That was my first foray into the resolver. I want to look into ordering next, but haven't found out anything yet. |
|
Can you try to introduce additional requirements? E.g. if package Foo was resolved to be version 3.6.1, then add an artificial requirement Foo=3.6.1? |
|
Tried that. Its still starting with all the 4.3.0 versions :/ |
|
Then the ordering of how the requirements are solved is definitely .... suboptimal. I will see if I can figure out anything in the evening, no time atm. |
|
This is a dependencies file where this should happen. One would assume paket could find a resolution easily considering all the pinned versions: |
|
hm maybe this is a bug. How can the resolver even take |
|
It took 2m36s with paket.exe built from 3347802 |
|
edit: version numbers are confusing, I am confused |
|
I think the case I found needs to be fixed for this to work properly. I think in https://github.com/fsprojects/Paket/blob/master/src/Paket.Core/Dependencies/PackageResolver.fs#L769 we should not just take "head" instead we should filter with the restrictions we already know... |
|
|
|
OK this should work and be slightly faster in theory. The problem was that the strategy in the dependency file was not applied to runtime dependencies as well -> Therefore we had this other case where our resolver currently is really bad at.... If you remove From the dependency file in the #2294 test case the resolver should still be able to resolve it properly, but it isn't. Therefore this was just another manifestation of this. |
matthid
reviewed
Apr 30, 2017
| @@ -693,7 +695,7 @@ let Resolve (getVersionsF, getPackageDetailsF, groupName:GroupName, globalStrate | |||
| currentStep.CurrentResolution.Count | |||
| (currentStep.CurrentResolution |> Seq.map (fun x -> sprintf "\n - %O, %O" x.Key x.Value.Version) |> String.Concat) | |||
| currentStep.OpenRequirements.Count | |||
| (currentStep.OpenRequirements |> Seq.map (fun x -> sprintf "\n - %O, %O" x.Parent x.VersionRequirement) |> String.Concat) | |||
| (currentStep.OpenRequirements |> Seq.map (fun x -> sprintf "\n - %O, %O (from %O)" x.Name x.VersionRequirement x.Parent) |> String.Concat) | |||
There was a problem hiding this comment.
I changed this unrelated output. It seems like it was wrong before?
matthid
reviewed
Apr 30, 2017
| runtimeResolutionDeps | ||
| |> Seq.fold (fun (deps:DependenciesFile) dep -> deps.AddAdditionalPackage(Constants.MainDependencyGroup, dep.Name, dep.VersionRequirement, dep.ResolverStrategyForTransitives, dep.Settings)) runtimeDepsFile | ||
|
|
||
| tracefn "Depsfile: \n%O" runtimeDepsFile |
There was a problem hiding this comment.
This helps printing out the deps file we resolve when handling runtime dependencies, this might help in the future...
|
Closing in favor of #2307, thanks for the test! Sorry for my bad handling of branches :) |
|
If we are on the topic of branches, do you think you or @forki could maybe clean them up? It's slighly impossible to find anything, and it doesn't look like most are actually still worked at:
|
|
I totally agree. Question is if I get in trouble if I just start deleting stuff :P |
|
Everything that's not master or actively worked on can go.
Am 02.05.2017 2:03 vorm. schrieb "Matthias Dittrich" <
[email protected]>:
… I totally agree. Question is if I get in trouble if I just start deleting
stuff :P
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2305 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNAoWBqBTqRdY-6K3BS9qkIL4p2Uiks5r1nLDgaJpZM4NL6V->
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.