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

Set stdin in UpstreamProcess to os.Stdin (instead of /dev/null) #18

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

3v0k4
Copy link
Contributor

@3v0k4 3v0k4 commented Apr 15, 2024

Fixes #16

As explained in the issue, debug depends on irb, which depends on reline. There's a call to @@input.winsize in reline:

  def self.get_screen_size
    s = @@input.winsize
    return s if s[0] > 0 && s[1] > 0
    s = [ENV["LINES"].to_i, ENV["COLUMNS"].to_i]
    return s if s[0] > 0 && s[1] > 0
    [24, 80]
  rescue Errno::ENOTTY
    [24, 80]
  end

The above fails because thruster uses /dev/null as stdin:

$ irb

STDIN.winsize
 => [39, 172]

devnull = File.open(File::NULL, "w")
devnull.winsize
(irb):3:in `winsize': Operation not supported by device - /dev/null (Errno::ENODEV)

@3v0k4 3v0k4 changed the title WIP: Fix stdin Set stdin in UpstreamProcess to os.Stdin (instead of /dev/null) Apr 15, 2024
@3v0k4 3v0k4 changed the title Set stdin in UpstreamProcess to os.Stdin (instead of /dev/null) Set stdin in UpstreamProcess to os.Stdin (instead of /dev/null) Apr 15, 2024
Copy link
Collaborator

@kevinmcconnell kevinmcconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, @3v0k4! 🙏

I left a small suggestion, let me know what you think. Otherwise this looks great to me.

@@ -15,9 +15,11 @@ type UpstreamProcess struct {
}

func NewUpstreamProcess(name string, arg ...string) *UpstreamProcess {
cmd := exec.Command(name, arg...)
cmd.Stdin = os.Stdin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently connect stdout and stderr inside Run(). It would be good to handle these all in the same place, for consistency.

Maybe move the stdin case to Run() too, to match the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that 🤦 Moved it inside Run().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks again for diagnosing and fixing! 🎉

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.

Thruster not working in new projects due to "irb" gem
2 participants