Fix installation from gemfile shared across different directories#5508
Fix installation from gemfile shared across different directories#5508faucct wants to merge 4 commits intorubygems:masterfrom
Conversation
| bundle "install" | ||
| in_app_root_custom "app" do | ||
| bundle "install" | ||
| expect(out).to include("Bundle complete!") |
There was a problem hiding this comment.
please also add expect(the_bundle).to include_gem "foo 1.0"?
There was a problem hiding this comment.
I believe I am having some troubles with that: it seems that the match you are suggesting to add does not care about .bundle/config and tries to validate the empty app/Gemfile.
The only reason I have created this empty Gemfile is to make app directory the bundler root, not the app/...
There was a problem hiding this comment.
Bundler.root should probably be changed instead of creating this empty Gemfile: it should choose the first directory containing Gemfile, gems.rb or .bundle.
There was a problem hiding this comment.
I did an attempt to improve Bundler.root and added the check you've suggested.
faucct
force-pushed
the
bugfix/gemfile_sharing
branch
3 times, most recently
from
264892e to
aa3e8e2
Compare
| given_gemfile = ENV["BUNDLE_GEMFILE"] | ||
| return Pathname.new(given_gemfile).dirname if given_gemfile && !given_gemfile.empty? | ||
| search_up('') do |path| | ||
| if path.join("Gemfile").file? || path.join("gems.rb").file? || path.join(".bundle").directory? |
There was a problem hiding this comment.
this will return ~ if the user is not in a bundle, which I don't think we want?
There was a problem hiding this comment.
Okay, so I should put the check from SharedExamples#default_bundle_dir here, right?
faucct
force-pushed
the
bugfix/gemfile_sharing
branch
from
aa3e8e2 to
a44a196
Compare
| def default_root | ||
| given_gemfile = ENV["BUNDLE_GEMFILE"] | ||
| return Pathname.new(given_gemfile).dirname if given_gemfile && !given_gemfile.empty? | ||
| search_up("") do |path| |
There was a problem hiding this comment.
I have rebased the thing and it looks like my changes here weren't necessary anymore, so I have removed them.
|
@faucct ping on my last comment about |
|
Hello, had no time to finish the thing yet. Will be done in couple weeks. |
|
☔ The latest upstream changes (presumably #5719) made this pull request unmergeable. Please resolve the merge conflicts. |
|
ping @faucct is this still being worked on? |
faucct
force-pushed
the
bugfix/gemfile_sharing
branch
from
a44a196 to
fd7e379
Compare
|
Finally rebased. Please, review this once again. |
|
ping |
colby-swandale
left a comment
colby-swandale
left a comment
There was a problem hiding this comment.
The PR needs a description about what exactly this is fixing and link and issues this relates to. See the PR template for more info.
|
Any news @faucct? I copied the test you added to both 2-0-stable and current master (in both 2.x and 3.x mode), and it passed in all cases, so we really need a description of what this is fixing. Thanks! |
|
I guess this can be closed if the tests are passing. |
|
Ok, if you run into related trouble, make sure to let us know! |
5 participants
No description provided.