Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Resolver error formatting #3862

Merged
merged 14 commits into from
Aug 2, 2015
Merged

Resolver error formatting #3862

merged 14 commits into from
Aug 2, 2015

Conversation

segiddins
Copy link
Member

Closes #3803

@segiddins segiddins force-pushed the seg-resolver-error-formatting branch from 8c9292c to 0163e21 Compare July 21, 2015 07:15
@segiddins
Copy link
Member Author

We should remember to apply the following diff once CocoaPods/Molinillo#24 is merged and released:

diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb
index f21c9cf..ec3b2a4 100644
--- a/lib/bundler/resolver.rb
+++ b/lib/bundler/resolver.rb
@@ -35,6 +35,9 @@ module Bundler
             depth = 2
             tree.each do |req|
               t << "  " * depth << %(#{clean_req req})
+              if spec = conflict.activated_by_name[req.name]
+                t << %(, resolved to #{spec.version},)
+              end
               t << %( depends on) unless tree.last == req
               t << %(\n)
               depth += 1

@segiddins
Copy link
Member Author

@indirect this is now ready for review

@segiddins segiddins force-pushed the seg-resolver-error-formatting branch from c9a82ec to 8be3be3 Compare July 27, 2015 04:53
rack_middleware (>= 0) ruby depends on
rack (= 0.9.1) ruby
rack-obama (= 2.0), resolved to 2.0, depends on
rack (= 1.2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm reading it, I'd prefer rack-obama (= 2.0) was resolved to 2.0, which depends on\n rack(= 1.2). Sorry to nitpick on you. 😝

@indirect
Copy link
Member

This is a huuuuuge improvement in the UX of requirement conflicts. 👏 🙌

@segiddins
Copy link
Member Author

@indirect done

@indirect
Copy link
Member

Woo! @homu r+

@homu
Copy link
Contributor

homu commented Jul 27, 2015

📌 Commit 210f9d9 has been approved by indirect

@homu
Copy link
Contributor

homu commented Jul 27, 2015

⌛ Testing commit 210f9d9 with merge 1401589...

homu added a commit that referenced this pull request Jul 27, 2015
@homu
Copy link
Contributor

homu commented Jul 27, 2015

💔 Test failed - status

@indirect
Copy link
Member

@segiddins looks like it's broken on 1.8.7

@segiddins
Copy link
Member Author

Looks like something has an unstable comparable implementation to me?

@indirect
Copy link
Member

Hashes literally do not have a stable iteration order in 1.8.7; you'll need to sort the hash before printing the conflict message or store conflicts in an array of tuples.

@segiddins
Copy link
Member Author

@indirect should work now!

@indirect
Copy link
Member

@homu r+

@homu
Copy link
Contributor

homu commented Aug 1, 2015

⌛ Testing commit 67b0274 with merge 9bb228f...

homu added a commit that referenced this pull request Aug 1, 2015
@homu
Copy link
Contributor

homu commented Aug 1, 2015

💔 Test failed - status

@segiddins segiddins force-pushed the seg-resolver-error-formatting branch from 67b0274 to f09b2c6 Compare August 1, 2015 04:00
@segiddins segiddins force-pushed the seg-resolver-error-formatting branch from f09b2c6 to ef5b0ea Compare August 1, 2015 04:01
@segiddins
Copy link
Member Author

@homu r+

@homu
Copy link
Contributor

homu commented Aug 1, 2015

📌 Commit ef5b0ea has been approved by segiddins

@homu
Copy link
Contributor

homu commented Aug 1, 2015

⌛ Testing commit ef5b0ea with merge fbae548...

homu added a commit that referenced this pull request Aug 1, 2015
@homu
Copy link
Contributor

homu commented Aug 1, 2015

💔 Test failed - status

if conflict.locked_requirement
o << %( In snapshot (#{Bundler.default_lockfile.basename}):\n)
o << %( #{clean_req conflict.locked_requirement}\n)
o << %( #{conflict.locked_requirement}\n)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a Gem::Dependency, whose to_s method includes "runtime" on RubyGems 1.5 and older. How is it not a DepProxy? No one knows...

@segiddins
Copy link
Member Author

@homu r+

@homu
Copy link
Contributor

homu commented Aug 2, 2015

📌 Commit 3fad388 has been approved by segiddins

@homu
Copy link
Contributor

homu commented Aug 2, 2015

⌛ Testing commit 3fad388 with merge ae575fb...

homu added a commit that referenced this pull request Aug 2, 2015
@homu
Copy link
Contributor

homu commented Aug 2, 2015

☀️ Test successful - status

@homu homu merged commit 3fad388 into master Aug 2, 2015
@segiddins segiddins deleted the seg-resolver-error-formatting branch August 2, 2015 05:09
@segiddins
Copy link
Member Author

Wooooo! Finally! Thanks for the help on this one, @indirect !

@indirect
Copy link
Member

indirect commented Aug 2, 2015

ugh, just realized that I should be reading the platform from the dep... the output will be wrong if the platform isn't Ruby. :P

@segiddins
Copy link
Member Author

mhmm :P

@segiddins
Copy link
Member Author

Actually, @indirect, do we even have the platform for locked gems?

@indirect
Copy link
Member

indirect commented Aug 2, 2015

no idea! that's... kind of distressing, actually.

@indirect
Copy link
Member

indirect commented Aug 3, 2015

Opened #3906 to print platform(s) correctly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants