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.

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

Lazily require readline #689

Merged
merged 1 commit into from
Nov 11, 2019
Merged

Lazily require readline #689

merged 1 commit into from
Nov 11, 2019

Conversation

deivid-rodriguez
Copy link
Contributor

The current implementation of readline has issues under Windows, and those issues sometimes prevent thor from being used.

For example, in our bundler Windows CI, we had to patch rb-readline to workaround some issues
because rb-readline (the default readline provider on Windows) couldn't be required. Moreover, we still get some issues when requiring rb-readline like

HOME environment variable (or HOMEDRIVE and HOMEPATH) must be set and point to a directory

Since in some situations the part of thor using readline (Thor::LineEditor) is not required, for example, in most of CI situations. I thought it could be a good idea to lazily require readline.

Just to be clear, this is not a problem with thor at all, but since requiring unnecessary things lazily is usually a good thing, I figured we could do this.

Also, an alternative version of this patch would be to autoload Thor::LineEditor, like

diff --git a/lib/thor/base.rb b/lib/thor/base.rb
index 26132de..3bbbbd2 100644
--- a/lib/thor/base.rb
+++ b/lib/thor/base.rb
@@ -4,7 +4,6 @@ require_relative "error"
 require_relative "invocation"
 require_relative "parser"
 require_relative "shell"
-require_relative "line_editor"
 require_relative "util"
 
 class Thor
diff --git a/lib/thor/shell/basic.rb b/lib/thor/shell/basic.rb
index 49e4cb2..9a48e8b 100644
--- a/lib/thor/shell/basic.rb
+++ b/lib/thor/shell/basic.rb
@@ -1,4 +1,6 @@
 class Thor
+  autoload :LineEditor, "thor/line_editor"
+
   module Shell
     class Basic
       DEFAULT_TERMINAL_WIDTH = 80

The current implementation of readline has issues under Windows, and
those issues prevent thor from being used. For example, in our bundler
Windows CI, we had to patch `rb-readline` to workaround some issues
because `rb-readline` (the default `readline` provider on Windows)
couldn't be required. Moreover, we still get some issues when requiring
`rb-readline` like

```
HOME environment variable (or HOMEDRIVE and HOMEPATH) must be set and point to a directory
```

Since in some situations the part of thor using `readline`
(`Thor::LineEditor`) is not required, for example, in most of CI
situations. I thought it could be a good idea to lazily require
`readline`.
@rafaelfranca rafaelfranca merged commit a1b8eaa into rails:master Nov 11, 2019
@deivid-rodriguez deivid-rodriguez deleted the lazily_load_readline branch November 11, 2019 17:02
deivid-rodriguez added a commit to dependabot/dependabot-core that referenced this pull request Aug 12, 2022
The reason is that the vendored copy of thor included with Bundler
1.17.3 eargerly loads `readline` and its dependencies and that causes
gem activation issues if the Gemfile also depends on those gems (which
it does).

This is not easy to patch at runtime, so I'm applying
rails/thor#689 through a .patch file to fix
things. It's a bit hacky, but only needed for development so I think it
works.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants