Skip to content

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

Merged
kevinmcconnell merged 1 commit intobasecamp:mainfrom
3v0k4:fix-stdin
Apr 16, 2024
Merged

Set stdin in UpstreamProcess to os.Stdin (instead of /dev/null)#18
kevinmcconnell merged 1 commit intobasecamp:mainfrom
3v0k4:fix-stdin

Conversation

@3v0k4
Copy link
Contributor

@3v0k4 3v0k4 commented Apr 15, 2024 •
edited
Loading

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
kevinmcconnell
kevinmcconnell reviewed Apr 16, 2024
View reviewed changes
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.

internal/upstream_process.go Outdated

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

@kevinmcconnell kevinmcconnell Apr 16, 2024

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

@3v0k4 3v0k4 Apr 16, 2024

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

@kevinmcconnell kevinmcconnell Apr 16, 2024

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! 🎉

@kevinmcconnell kevinmcconnell merged commit 2289628 into basecamp:main Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinmcconnell kevinmcconnell kevinmcconnell left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

2 participants

@3v0k4 @kevinmcconnell