This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
Fix more leaks to default copy of bundler #7274
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Unfortunately this patch seems to introduce some flakyness on a single realworld spec And I'm not yet sure why 😬. |
deivid-rodriguez
force-pushed
the
fix_more_leaks_to_default_copy_of_bundler
branch
from
9f72bc2
to
9e135d4
Compare
|
Actually, nevermind, the flakyness is already present in master, so it's unrelated to this PR. I'll investigate how the flaky got started separately. |
|
I still want to review some things in this PR, so please don't merge it just yet :) |
4 tasks
hsbt
reviewed
Aug 4, 2019
deivid-rodriguez
force-pushed
the
fix_more_leaks_to_default_copy_of_bundler
branch
2 times, most recently
from
86ca451
to
547a116
Compare
|
@hsbt I addressed your feedback and now hopefully all paths are independent from the specific folder structure where the tests are run, thanks for pointing that out 👍. I still want to improve a few other things in this PR, so I'll let you know when it's ready. |
deivid-rodriguez
force-pushed
the
fix_more_leaks_to_default_copy_of_bundler
branch
3 times, most recently
from
dbd8141
to
300dcf7
Compare
|
I'm now happy with this PR, @hsbt. You can have another look :) |
|
@hsbt If you're ok, I'd like to merge this. |
|
@deivid-rodriguez I'm going to try to merge this pull-request into ruby repo soon. |
|
Thank you @hsbt, let me know how it goes! Hopefully it allows removing some of those |
ghost
pushed a commit
that referenced
this pull request
Aug 16, 2019
7301: Track changes from ruby core master r=hsbt a=hsbt ### What was the end-user problem that led to this PR? I'm going to merge #7274. But the ruby-core source has some of the changes for bundler source. ### What was your diagnosis of the problem? ### What is your fix for the problem, implemented in this PR? ruby core team fixed them: * Removed circular require warning at `shared_helper.rb` * Support test at GitHub Actions, It helps that bundler will migrate Actions from Azure Pipelines too. * Fixed broken examples at ruby core repository ### Why did you choose this fix out of the possible options? Co-authored-by: ohbarye <over.rye@gmail.com> Co-authored-by: Hiroshi SHIBATA <hsbt@ruby-lang.org> Co-authored-by: Yusuke Endoh <mame@ruby-lang.org>
|
We need to rebase this after merging #7303 |
Yes, I'll take care of that 👍. |
deivid-rodriguez
force-pushed
the
fix_more_leaks_to_default_copy_of_bundler
branch
4 times, most recently
from
f2f81de
to
97ddcb4
Compare
If we use system bundler, when booting the "outermost" bundler process, bundler will save the path to the system bundler in BUNDLE_BIN_PATH, and use it again when booting the "innermost" bundler process (`bundle exec echo foo`). That means that second process will use the system bundler path again. However, we have `-rsupport/hax` in RUBYOPT, so that file will load from the local copy of bundler, and that file will load `bundler/version` from the project (not from system), because -Ilib is in the LOAD_PATH. That will end up causing redefinition errors because the same constant will be loaded from two different locations. In general, this is expected behavior, normally you will wrap the process with `Bundler.with_original_env` to reset the environment. However, the easiest fix here is to not use system bundler, because it's not really necessary and thus doesn't help the readability of the spec.
Instead, make sure we always load the local copy of bundler during specs, and never end up using the default copy.
The version we're vendoring actually relaxed this restriction back to 2.3.0+, so we can always use the vendored version.
deivid-rodriguez
force-pushed
the
fix_more_leaks_to_default_copy_of_bundler
branch
from
7eca509
to
8ef571e
Compare
|
@hsbt This one is ready, can I merge it? |
hsbt
approved these changes
Aug 17, 2019
|
@bundlerbot r=hsbt |
ghost
pushed a commit
that referenced
this pull request
Aug 18, 2019
7274: Fix more leaks to default copy of bundler r=hsbt a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that in some places, it was still possible to end up requiring files in a different copy of bundler (the default copy). I noticed this when I removed a rubygems monkeypatch from the test suite that was preventing the default copy of bundler from being activated when requiring files. This thing: https://github.com/bundler/bundler/blob/e1c518363641208429f397170354054b3d28effd/spec/support/hax.rb#L15-L20 ### What was your diagnosis of the problem? My diagnosis was that I should use relative requires wherever they were missing. ### What is your fix for the problem, implemented in this PR? My fix is to remove the rubygems hack, migrate the rest of the internal requires to be relative, and also introduce some hacks on our specs to make sure we never load the incorrect copy of bundler. I think this PR should fix the issues in rubygems/rubygems#2863. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit
ghost
pushed a commit
to rubygems/rubygems
that referenced
this pull request
Mar 11, 2020
7301: Track changes from ruby core master r=hsbt a=hsbt ### What was the end-user problem that led to this PR? I'm going to merge rubygems/bundler#7274. But the ruby-core source has some of the changes for bundler source. ### What was your diagnosis of the problem? ### What is your fix for the problem, implemented in this PR? ruby core team fixed them: * Removed circular require warning at `shared_helper.rb` * Support test at GitHub Actions, It helps that bundler will migrate Actions from Azure Pipelines too. * Fixed broken examples at ruby core repository ### Why did you choose this fix out of the possible options? Co-authored-by: ohbarye <over.rye@gmail.com> Co-authored-by: Hiroshi SHIBATA <hsbt@ruby-lang.org> Co-authored-by: Yusuke Endoh <mame@ruby-lang.org>
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the end-user problem that led to this PR?
The problem was that in some places, it was still possible to end up requiring files in a different copy of bundler (the default copy). I noticed this when I removed a rubygems monkeypatch from the test suite that was preventing the default copy of bundler from being activated when requiring files.
This thing:
https://github.com/bundler/bundler/blob/e1c518363641208429f397170354054b3d28effd/spec/support/hax.rb#L15-L20
What was your diagnosis of the problem?
My diagnosis was that I should use relative requires wherever they were missing.
What is your fix for the problem, implemented in this PR?
My fix is to remove the rubygems hack, migrate the rest of the internal requires to be relative, and also introduce some hacks on our specs to make sure we never load the incorrect copy of bundler.
I think this PR should fix the issues in rubygems/rubygems#2863.