StackOverflowException resolving NuGet package without version constraint from large paket.dependencies #1392

Closed
viktor-svub opened this Issue Jan 15, 2016 · 11 comments

Comments

Projects
None yet
4 participants
@viktor-svub
Contributor

viktor-svub commented Jan 15, 2016

StackOverflowException resolving "our.proprietary.component"

  • Paket.exe 2.44.1, version 2.21 works but does not handle prereleases
  • paket.dependencies NuGet, declared without any version constraint
  • there are about 180 declared dependencies, all NuGet, all custom feeds
  • probably also transitive dependency of some of the other component/s
  • SO is thrown in \src\Paket.Core\Domain.fs, line 30, ckeckout cf9c622
    (this.GetCompareString().CompareTo(that.GetCompareString()))
  • stack trace @ https://gist.github.com/viktor-svub/c1563f0c2b98562f67ee
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jan 15, 2016

Member

can you please try to find the last working version?

Member

forki commented Jan 15, 2016

can you please try to find the last working version?

@viktor-svub

This comment has been minimized.

Show comment
Hide comment
@viktor-svub

viktor-svub Jan 15, 2016

Contributor

OK, the issue appears between 2.40.14 (latest working) and 2.40.15 (throws SO).

  • the full command crashing with exception is paket.exe update --no-install
  • we have 4 custom NuGet feeds, often containing different versions of same packages
  • majority of the 180 immediate dependencies are prerelease
  • majority of all the dependencies are using @~> resolution
  • the generated paket.lock is about 360 lines long
Contributor

viktor-svub commented Jan 15, 2016

OK, the issue appears between 2.40.14 (latest working) and 2.40.15 (throws SO).

  • the full command crashing with exception is paket.exe update --no-install
  • we have 4 custom NuGet feeds, often containing different versions of same packages
  • majority of the 180 immediate dependencies are prerelease
  • majority of all the dependencies are using @~> resolution
  • the generated paket.lock is about 360 lines long
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jan 15, 2016

Member
diff --git a/src/Paket.Core/SemVer.fs b/src/Paket.Core/SemVer.fs
index 444e688..d466672 100644
--- a/src/Paket.Core/SemVer.fs
+++ b/src/Paket.Core/SemVer.fs
@@ -144,6 +144,8 @@ interface System.IComparable with
                     | None, None -> 0
                     | Some p, None -> -1
                     | None, Some p -> 1
+                    | Some p, _ when p.ToString() = "prerelease" -> -1
+                    | _, Some p when p.ToString() = "prerelease" -> 1
                     | Some left, Some right -> compare left right

             | _ -> invalidArg "yobj" "cannot compare values of different types"

that's the only change. weird

Member

forki commented Jan 15, 2016

diff --git a/src/Paket.Core/SemVer.fs b/src/Paket.Core/SemVer.fs
index 444e688..d466672 100644
--- a/src/Paket.Core/SemVer.fs
+++ b/src/Paket.Core/SemVer.fs
@@ -144,6 +144,8 @@ interface System.IComparable with
                     | None, None -> 0
                     | Some p, None -> -1
                     | None, Some p -> 1
+                    | Some p, _ when p.ToString() = "prerelease" -> -1
+                    | _, Some p when p.ToString() = "prerelease" -> 1
                     | Some left, Some right -> compare left right

             | _ -> invalidArg "yobj" "cannot compare values of different types"

that's the only change. weird

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jan 15, 2016

Member

but I assume the real issue is that our resolver algorithm is still recursive. Need to implement BFS qith a queue...

Member

forki commented Jan 15, 2016

but I assume the real issue is that our resolver algorithm is still recursive. Need to implement BFS qith a queue...

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jan 15, 2016

Member

@viktor-svub in the latest version I inline the tryToImprove function. This will "free" halve of the stack in your sample and should help to process significantly larger graphs. Does this work for you?

@mrinaldi do you see a chance that we can remove the recursive step function and turn it into something that manages a queue or a stack internally?

Member

forki commented Jan 15, 2016

@viktor-svub in the latest version I inline the tryToImprove function. This will "free" halve of the stack in your sample and should help to process significantly larger graphs. Does this work for you?

@mrinaldi do you see a chance that we can remove the recursive step function and turn it into something that manages a queue or a stack internally?

@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Jan 16, 2016

Contributor

IIRC yes.
I'm on vacation right now, though. I'm not sure when I'll have time to look at it.
Maybe sometime next week.

Contributor

mrinaldi commented Jan 16, 2016

IIRC yes.
I'm on vacation right now, though. I'm not sure when I'll have time to look at it.
Maybe sometime next week.

@gpomykala

This comment has been minimized.

Show comment
Hide comment
@gpomykala

gpomykala Jan 18, 2016

I noticed the same problem (SO during paket update --no-install). The last working version is 2.40.14, 2.44.5 still fails

I noticed the same problem (SO during paket update --no-install). The last working version is 2.40.14, 2.44.5 still fails

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jan 18, 2016

Member

can any of you reproduce with a dependencies file that can be made public?

Member

forki commented Jan 18, 2016

can any of you reproduce with a dependencies file that can be made public?

@gpomykala

This comment has been minimized.

Show comment
Hide comment
@gpomykala

gpomykala Jan 22, 2016

We were not able to reproduce it using public packages only yet. it's a mix of public libs and internal libs

We were not able to reproduce it using public packages only yet. it's a mix of public libs and internal libs

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jan 22, 2016

Member

maybe exclude the internal stuff and add public packages until it breaks again?

Member

forki commented Jan 22, 2016

maybe exclude the internal stuff and add public packages until it breaks again?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Feb 11, 2016

Member

This fixed in 2.50.3

Member

forki commented Feb 11, 2016

This fixed in 2.50.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment