[2.0] Enable specific_platform by default on 2.0 #5818
Conversation
|
Will update the specs on 2.0 once the source changes land, as those already fork a lot of the lockfile output specs |
|
☔ The latest upstream changes (presumably #5820) made this pull request unmergeable. Please resolve the merge conflicts. |
segiddins
force-pushed
the
seg-bundler-2-specific-platform
branch
from
8720a9a
to
c254dff
Compare
|
Holy heck this was hard to get passing, took a bunch of time |
segiddins
force-pushed
the
seg-bundler-2-specific-platform
branch
from
c254dff
to
85c527c
Compare
|
Should be green now. Please review. |
lib/bundler/definition.rb
Outdated
| @@ -904,7 +904,7 @@ def expand_dependencies(dependencies, remote = false) | |||
| "To add those platforms to the bundle, " \ | |||
| "run `bundle lock --add-platform #{mapped_platforms.join " "}`." | |||
| end | |||
| platforms.each do |p| | |||
| Resolver.sort_platforms(platforms).each do |p| | |||
There was a problem hiding this comment.
I'm a little concerned about sorting in definition. Is this on the require path?
We should skip this block entirely if remote == true since the entire functionality is not used.
There was a problem hiding this comment.
I have an idea to speed this up that I'll try out.
There was a problem hiding this comment.
Should be improved now, it only sorts the platforms once instead of once per dependency
| @@ -26,6 +26,9 @@ def message | |||
| end.min_by(&:size) | |||
| trees.reject! {|t| !maximal.include?(t.last) } if maximal | |||
|
|
|||
| trees = trees.sort_by {|t| t.flatten.map(&:to_s) } | |||
There was a problem hiding this comment.
We're doing another sort below, maybe we can combine the sort_by, uniq, and sort_by for perf reasons?
There was a problem hiding this comment.
this is in an error message, so I'm not overly concerned with performance
segiddins
force-pushed
the
seg-bundler-2-specific-platform
branch
2 times, most recently
from
ec93581
to
76a5657
Compare
|
LGTM. 👍 @bundlerbot r+ |
|
📌 Commit 76a5657 has been approved by |
|
☔ The latest upstream changes (presumably #5790) made this pull request unmergeable. Please resolve the merge conflicts. |
|
🔒 Merge conflict |
Also ensure the resolver processes specs in the correct order for error messages
segiddins
force-pushed
the
seg-bundler-2-specific-platform
branch
from
76a5657
to
81ac8ab
Compare
|
Rebased. @bundlerbot r=indirect |
|
📌 Commit 81ac8ab has been approved by |
…rect [2.0] Enable specific_platform by default on 2.0 ### What was the end-user problem that led to this PR? The problem was that Bundler has somewhat suspect handling of multi-platform gems. We'd assume that different platform versions of gems were generally interchangeable, so if Bundler resolved to the "ruby" platform gem we'd just blindly try to swap in the gem for the local platform, which could lead to issues (say if the sets of dependencies were different). ### Was was your diagnosis of the problem? My diagnosis was that we needed to stop only working with the notion of "generic" platforms, which mapped everything to (basically) either java, pure ruby, and windows, and instead keep track of the actual platforms a bundle was being used on, and resolve for those specific platforms. ### What is your fix for the problem, implemented in this PR? My fix enables the changes made in #4836 by default on Bundler 2. ### Why did you choose this fix out of the possible options? I chose this fix because it means Bundler will default to more correct platforms semantics out of the box.
|
☀️ Test successful - status-travis |
What was the end-user problem that led to this PR?
The problem was that Bundler has somewhat suspect handling of multi-platform gems. We'd assume that different platform versions of gems were generally interchangeable, so if Bundler resolved to the "ruby" platform gem we'd just blindly try to swap in the gem for the local platform, which could lead to issues (say if the sets of dependencies were different).
Was was your diagnosis of the problem?
My diagnosis was that we needed to stop only working with the notion of "generic" platforms, which mapped everything to (basically) either java, pure ruby, and windows, and instead keep track of the actual platforms a bundle was being used on, and resolve for those specific platforms.
What is your fix for the problem, implemented in this PR?
My fix enables the changes made in #4836 by default on Bundler 2.
Why did you choose this fix out of the possible options?
I chose this fix because it means Bundler will default to more correct platforms semantics out of the box.