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.

Sign up for GitHub

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

Jump to bottom

Lazily load and vendor optparse #4881

Merged
merged 3 commits into from Nov 2, 2021
Merged

Lazily load and vendor optparse #4881

merged 3 commits into from Nov 2, 2021

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Aug 26, 2021 •
edited

What was the end-user or developer problem that led to this PR?

When BUNDLE_AUTO_INSTALL is setup, bundle exec will also load all the bundle install machinery to install gems. That means also eventually loading the optparse gem, and causing activation conflicts in the Gemfile also includes a non compatible version of optparse.

What is your fix for the problem, implemented in this PR?

My fix means lazily loading optparse from top-level to the places that need it, and that seems to fix this issue.

It also requires #4887 from the bundler side, which was also loading optparse. If that change in bundler is not in place, Gem::Command usages in bundler will result in a missing constant error. Meaning this PR will break using bundler versions prior to 2.2.29 in combination with the rubygems version that ships this change. To fix that, I used an inherited hook to backfill the missing require from the rubygems side.

However, the inherited hook breaks things again in the same way, so I finally went with also vendoring optparse inside rubygems which should completely fix the issue in all cases.

Fixes #4880.

Make sure the following tasks are checked

@deivid-rodriguez deivid-rodriguez force-pushed the lazily_load_optparse branch 2 times, most recently from 2504048 to a1257b6 Compare August 26, 2021 14:46
This was referenced Aug 26, 2021
Closed
Merged
@deivid-rodriguez deivid-rodriguez force-pushed the lazily_load_optparse branch 2 times, most recently from 0ac7730 to f5d3438 Compare September 22, 2021 18:08
@deivid-rodriguez
Copy link
Member Author

Still lacking a spec, but I rebased this PR and filled its description.

@doodzik
Copy link
Contributor

doodzik commented Oct 29, 2021

That means also eventually loading the optparse gem, and causing activation conflicts in the Gemfile also includes a non compatible version of optparse.

This sounds like the same issue as the tsort activation problem in #5006. I wonder if it would make more sense to vendor in the optparse gem so we don't have too many different approaches to solving the activation problem. Keeping the approaches somewhat similar should allow us to keep the complexity of the codebase to a minimum.

@deivid-rodriguez
Copy link
Member Author

Yeah, it's the same thing, but I usually try lazy loading first, since lazy loading is a good thing by itself and if it's enough to fix the issue, then why not.

@deivid-rodriguez
Copy link
Member Author

That said, I think lazily loading optparse here might not be good enough, so maybe we can go ahead and directly vendor it. Vendoring also has the advantage of making rubygems behave the same regardless of which version of optparse the user has installed, thus potentially protecting us from breaking changes in the optparse gem.

@deivid-rodriguez
Lazily load optparse
136cc7f 
The `optparse` library is loaded by `rubygems/command`, which should
only be loaded when using the `gem` CLI. However, it was also being
loaded in some other cases.

This commit tries to lazily load it, so that it doesn't interfere with
other usages. For example, when `rubygems/installer` is required
directly from `bundler`.
@deivid-rodriguez deivid-rodriguez force-pushed the lazily_load_optparse branch 2 times, most recently from 616541d to 1c7a68e Compare November 2, 2021 20:45
@deivid-rodriguez deivid-rodriguez changed the title Lazily load optparse Lazily load and vendor optparse Nov 2, 2021
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review November 2, 2021 20:47
@deivid-rodriguez
Copy link
Member Author

Alright, in the end I went with also vendoring optparse because only lazily loading it caused some issues, and I don't think it was going to work in all cases.

I also added a spec, so this should be ready.

@deivid-rodriguez
Vendor optparse
47c4d12
@deivid-rodriguez
Add a spec to cover optparse improvements in rubygems
a227e1c 
Make sure using `auto_install` setting in combination with a `Gemfile`
depending on `optparse` doesn't create activation conflicts.
@deivid-rodriguez
Copy link
Member Author

Thanks for the review @simi!

@deivid-rodriguez deivid-rodriguez merged commit 1f0d78d into master Nov 2, 2021
@deivid-rodriguez deivid-rodriguez deleted the lazily_load_optparse branch November 2, 2021 22:29
deivid-rodriguez added a commit that referenced this pull request Nov 8, 2021
@deivid-rodriguez
Merge pull request #4881 from rubygems/lazily_load_optparse
eafe704 
Lazily load and vendor `optparse`

(cherry picked from commit 1f0d78d)
deivid-rodriguez added a commit that referenced this pull request Nov 8, 2021
@deivid-rodriguez
Merge pull request #4881 from rubygems/lazily_load_optparse
f672ab5 
Lazily load and vendor `optparse`

(cherry picked from commit 1f0d78d)
deivid-rodriguez added a commit that referenced this pull request Nov 8, 2021
@deivid-rodriguez
Merge pull request #4881 from rubygems/lazily_load_optparse
d39f7b0 
Lazily load and vendor `optparse`

(cherry picked from commit 1f0d78d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simi simi simi approved these changes

Assignees
No one assigned
Labels
Bundler rubygems: enhancement RubyGems
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bundle exec <foo> with BUNDLE_AUTO_INSTALL enable fails; running bundle install separately works
4 participants
@deivid-rodriguez @doodzik @simi @bundlerbot