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

Tail Recursive Package Resolution #2066

Merged
merged 12 commits into from
Dec 13, 2016
Merged

Tail Recursive Package Resolution #2066

merged 12 commits into from
Dec 13, 2016

Conversation

cloudRoutine
Copy link
Member

I checked the IL to make sure that it's tail recursive
I tested it against the repos that were causing stackoverflows in #2030, it successfully resolved for both

Although the output for

source https://nuget.org/api/v2

github logary/logary src/Logary.Facade/Facade.fs 4b21d888e83d72d1238ab0f36110e2a3946240e7

nuget Argu
nuget FsCheck
nuget FSharp.Core ~> 3
nuget FSharpx.Core
nuget BenchmarkDotNet
nuget NuGet.CommandLine
nuget Microsoft.NET.Sdk alpha 
nuget FSharp.NET.Sdk 1.0.0-alpha-000007 alpha framework: netstandard1.6
nuget NETStandard.Library 1.6.1 framework: netstandard1.6
nuget System.Diagnostics.TraceSource

group netcore 
    source https://www.nuget.org/api/v2

    nuget FSharp.Core 4.0.1.7-alpha alpha framework: netstandard1.6

group build
    source https://www.nuget.org/api/v2

    nuget FAKE

show that something is still clearly wrong

@cloudRoutine
Copy link
Member Author

@matthid @forki I ran the integration tests again in debug mode to see what other errors might pop up without tail recursive optimization and the #1174 ninject test failed

Any chance either of you remember what part of the resolution function is failing when this kind of output occurs?

@cloudRoutine
Copy link
Member Author

The verbose output for the failing test is

Test Name:	#1579 update allows unpinned
Test Outcome:	Failed
Result Message:	
System.Exception : Paket failed with:
	There was a version conflict during package resolution.
  Resolved packages:
   - MvvmLight 4.2.32.7
   - Paket.Test.A 1.0.0-prerelease
  Could not resolve package Mvvmlight >= 4.0 < 5.0:
   - Paket.Test.A 1.0.0-prerelease requested package Mvvmlight: 4.2.30

  Please try to relax some conditions.
StackTrace:
     at [email protected](String message) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\PackageResolver.fs:line 212
   at [email protected](GroupName groupName, DependenciesGroup dependenciesGroup) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\UpdateProcess.fs:line 149
   at Microsoft.FSharp.Collections.MapTreeModule.mapiOpt[a,b,c](FSharpFunc`3 f, MapTree`2 m)
   at Microsoft.FSharp.Collections.FSharpMap`2.Map[b](FSharpFunc`2 f)
   at Paket.UpdateProcess.selectiveUpdate(Boolean force, FSharpFunc`2 getSha1, FSharpFunc`2 getSortedVersionsF, FSharpFunc`2 getPackageDetailsF, LockFile lockFile, DependenciesFile dependenciesFile, UpdateMode updateMode, SemVerUpdateMode semVerUpdateMode) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\UpdateProcess.fs:line 139
   at Paket.UpdateProcess.SelectiveUpdate(DependenciesFile dependenciesFile, UpdateMode updateMode, SemVerUpdateMode semVerUpdateMode, Boolean force) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\UpdateProcess.fs:line 200
   at Paket.UpdateProcess.SmartInstall(DependenciesFile dependenciesFile, UpdateMode updateMode, UpdaterOptions options) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\UpdateProcess.fs:line 215
   at <StartupCode$Paket-Core>[email protected](Unit unitVar0) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\PublicAPI.fs:line 206
   at Paket.Utils.RunInLockedAccessMode[a](String rootFolder, FSharpFunc`2 action) in C:\Users\jared\Github\Forks\Paket\src\Paket.Core\Utils.fs:line 733
   at [email protected](ParseResults`1 results) in C:\Users\jared\Github\Forks\Paket\src\Paket\Program.fs:line 432
   at Paket.Program.processWithValidation[T](Boolean silent, FSharpFunc`2 validateF, FSharpFunc`2 commandF, ParseResults`1 result) in C:\Users\jared\Github\Forks\Paket\src\Paket\Program.fs:line 36
   at Paket.Program.main$cont@414(ParseResults`1 results, Boolean silent, Boolean fromBootstrapper, Unit unitVar) in C:\Users\jared\Github\Forks\Paket\src\Paket\Program.fs:line 432
   at Paket.Program.main() in C:\Users\jared\Github\Forks\Paket\src\Paket\Program.fs:line 414
Result StandardOutput:	
C:\Users\jared\Github\Forks\Paket\integrationtests\scenarios\i001579-unlisted\temp already exists.
C:\Users\jared\Github\Forks\Paket\bin\paket.exe pack templatefile paket.A.template version 1.0.0-prerelease output bin
C:\Users\jared\Github\Forks\Paket\bin\paket.exe update -v
Paket version 4.0.0-alpha030
found: C:\Users\jared\Github\Forks\Paket\integrationtests\scenarios\i001579-unlisted\temp\paket.dependencies
Parsing C:\Users\jared\Github\Forks\Paket\integrationtests\scenarios\i001579-unlisted\temp\paket.dependencies
Resolving packages for group Main:
   0 packages in resolution.
   2 requirements left
     - Mvvmlight, >= 4.0 < 5.0
     - Paket.Test.A, 

  Trying to resolve Mvvmlight >= 4.0 < 5.0 (from C:\Users\jared\Github\Forks\Paket\integrationtests\scenarios\i001579-unlisted\temp\paket.dependencies)
  - fetching versions for Mvvmlight
 - Mvvmlight 4.2.32.7
   1 packages in resolution.
     - MvvmLight, 4.2.32.7
   2 requirements left
     - Paket.Test.A, 
     - MvvmLightLibs, >= 4.4.32.7

  Trying to resolve Paket.Test.A  (from C:\Users\jared\Github\Forks\Paket\integrationtests\scenarios\i001579-unlisted\temp\paket.dependencies)
  - fetching versions for Paket.Test.A
 - Paket.Test.A 1.0.0-prerelease
   2 packages in resolution.
     - MvvmLight, 4.2.32.7
     - Paket.Test.A, 1.0.0-prerelease
   2 requirements left
     - Mvvmlight, 4.2.30
     - MvvmLightLibs, >= 4.4.32.7

  Trying to resolve Mvvmlight 4.2.30 (from Paket.Test.A 1.0.0-prerelease)
  Could not resolve package Mvvmlight >= 4.0 < 5.0:
   - Paket.Test.A 1.0.0-prerelease requested package Mvvmlight: 4.2.30

    ==> Trying different resolution.
 - Mvvmlight 4.2.30.0
     unlisted
 - Mvvmlight 4.1.27.1
     unlisted
 - Mvvmlight 4.1.27.0
     unlisted
 - Mvvmlight 4.1.26.1
     unlisted
 - Mvvmlight 4.1.26.0
     unlisted
 - Mvvmlight 4.1.23.0
     unlisted

I spent all day yesterday working on this, but I have no idea what the issue is 😞

@forki
Copy link
Member

forki commented Dec 4, 2016 via email

@cloudRoutine cloudRoutine force-pushed the overflow branch 2 times, most recently from eb4c1f2 to 41a2d2f Compare December 5, 2016 22:25
@cloudRoutine cloudRoutine changed the title [WIP] Tail Recursive Package Resolution Tail Recursive Package Resolution Dec 5, 2016
@cloudRoutine cloudRoutine force-pushed the overflow branch 6 times, most recently from 2b784cb to b14dcc3 Compare December 5, 2016 23:57
@forki
Copy link
Member

forki commented Dec 12, 2016

you disabled magic mode?
now both CIs are red?

@cloudRoutine
Copy link
Member Author

I can't remember where I first saw it, but magic mode was causing travis builds to fail so I disabled it here.
I have no idea why dotnet pack is failing now

@forki
Copy link
Member

forki commented Dec 12, 2016 via email

@cloudRoutine
Copy link
Member Author

@forki it's back to appveyor passing

@forki
Copy link
Member

forki commented Dec 13, 2016

thanks so much for this!

s

@cloudRoutine
Copy link
Member Author

I'm still a bit concerned about how the packages are resolving, even though it works properly

It's going through a lot more packages than it should be. Even after I got the unlisted package traversal stage working it's still going through a larger package list than it should. I think something is wrong with the list of compatible packages being provided for a specific version constraint.

@forki
Copy link
Member

forki commented Dec 13, 2016

it's still going through a larger package list than it should

more than before?

@cloudRoutine
Copy link
Member Author

I think so, but it's hard to tell since the tests aren't giving full output when they ran on appveyor in previous PRs. This is another reason #2070 would be useful.

@forki forki merged commit 7d6f95c into fsprojects:core3 Dec 13, 2016
@cloudRoutine cloudRoutine mentioned this pull request Dec 13, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cloudRoutine @forki