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

Spec that demonstrates effect of swapping on order of resolution #10

Closed

Conversation

iangreenleaf
Copy link

Swapping can, in certain cases, affect the order in which dependencies are considered and thus potentially the resolution that is first reached. This spec resolves differently when swapping is active versus when it is removed.

There are two possible resolutions that could be reached for this gemset, [cool-gem-1.0.1 weird-gem-0.2.0] or [cool-gem-0.0.1 weird-gem-0.3.0]. Since cool-gem is listed first by sort_dependencies, the backtracking algorithm should end with a stack like this:

cool-gem-1.0.1
weird-gem-0.3.0
weird-gem-0.2.0

But with swapping, the stack gets modified:

cool-gem-1.0.1 cool-gem-0.0.1 (swapped)
weird-gem-0.3.0 ⮵

I guess the question is if it's important that the resolution algorithm have predictable backtracking behavior based on the order of the dependencies. Swapping will still always reach a valid resolution, but it takes the dependency sort as more of a friendly suggestion than a rule 🙂

For more context: I discovered this through a project I'm working on that sorts the dependencies in an unusual order and expects the results to be dependent on that order. So while the above example is one symptom of this, it's more likely to arise for anything using the Molinillo resolver with a non-standard sort order.

Swapping can, in certain cases, affect the order in which dependencies
are considered and thus potentially the resolution that is first
reached.
@chrismo
Copy link

chrismo commented Mar 29, 2017

I have no real understanding of the underpinnings here, @segiddins is the person for all that, but when you mentioned, "I discovered this through a project I'm working on that sorts the dependencies in an unusual order" ... that could be similar to changes I made in Bundler to favor, for example, newer patch versions instead of newest major version ... which meant changing the order FED into Molinillo ... I'm just curious about your project and would love to hear more about it. :)

@iangreenleaf
Copy link
Author

@chrismo good guess! I'm doing something slightly different but similar. I'm trying to build a tool that determines the minimal set of upgrades needed to upgrade one specific gem (including overriding other Gemfile requirements if necessary). This does indeed involve changing the order of things fed into Molinillo.

For example, the specs for the "target" dependency that we want to upgrade are sorted according to fairly standard rules that favor the newest version first. But all other specs are sorted with the currently locked version first, then patch-level upgrades, then minor, then major. I've run into some unintended consequences of swapping because of this unusual sorting, where an older version of a gem will be swapped in instead of backtracking all the way out to hit the optimal combination.

@chrismo
Copy link

chrismo commented Mar 29, 2017

@iangreenleaf the bulk of what I did in my bundler-patch plugin is now in Bundler itself, but bundler-patch still has some additional functionality, including some modifications to the Gemfile in order to do a vulnerability update: https://github.com/livingsocial/bundler-patch - take a gander if anything seems helpful to you.

@chrismo
Copy link

chrismo commented Mar 29, 2017

also, fwiw, my changes were completely external to molinillo ... since it depends on being fed the order of things externally. dunno if that would help you at all - maybe you don't need to worry about how molinillo does its magicks either?

@iangreenleaf
Copy link
Author

@chrismo yes, thanks! I looked through your plugin as I was learning the Bundler internals and it was a big help in helping me understand how one might use the interface to tweak things. I didn't realize you were doing Gemfile modifications, so I'll have to take another look to see how you're handling that piece.

Unfortunately, I don't think I can accomplish what I'm looking for without mucking with Molinillo—I got very close by adjusting the order of things I feed in, and it works most of the time, but the swapping interacts badly with my sorting order and basically makes things happen in an unexpected way, which causes some subpar results. And at least so far I have not been able to think of a way to get around it.

@segiddins
Copy link
Member

thanks! Only thing left is to fix Molinillo :P

@iangreenleaf
Copy link
Author

Haha, yeah just that one "small" addition.

I am working on it, but it is proving tricky. Tweaking one part of the algorithm tends to have effects on other parts, so I may have to do a big change all at once. Right now I've got simple backtracking mostly working and am in the process of adding some performance optimizations.

@greysteil
Copy link
Collaborator

greysteil commented Jul 20, 2017

@iangreenleaf - we've removed swapping from Molinillo, in favour of processing similar dependencies together as possibility sets (see CocoaPods/Molinillo#69 for details).

As a result, it looks like this PR is out-of-date - Molinillo now resolves this case to ["a (1.0.0)", "b (1.0.1)", "c (1.0.0)", "x (0.0.2)"] in all but the ReverseBundlerIndex case (which is understandably different). Are you happy for this PR to be closed? I can't see a way we're eve going to be able to merge it, given that the expected behaviour will always be different results from different indices.

@greysteil greysteil closed this Jul 26, 2017
@iangreenleaf
Copy link
Author

Yes, it's probably ok to close this without a merge. It would still be a somewhat useful case to include, since having the algorithm follow a predictable order of operations is still a good property to check for. But since the specific bug it was exposing is fixed, it's not as urgent. I was thinking about adding an ability to the specs to run some specs only against certain indices, which would be a bit cleaner than the current blacklisting stuff. But I can't say I'll get to that any time soon.

BTW, great work on that change @greysteil. Swapping has been haunting me but I couldn't quite crack the solution. It looks like you came up with a win-win.

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 this pull request may close these issues.

None yet

4 participants