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

wip: interactive mode #803

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

wip: interactive mode #803

wants to merge 6 commits into from

Conversation

ddollar
Copy link
Owner

@ddollar ddollar commented Aug 2, 2024

No description provided.

@dhh
Copy link

dhh commented Aug 2, 2024

Doesn't seem to be working quite right. I'm testing by creating a new rails application off main using "rails new app -j esbuild -c tailwind", then changing the Procfile.dev to remove the remote debugger env, then doing a "rails generate scaffold post title:string", and then inserting a debugger statement in PostsController#new and then hitting that URL:

image

It's currently swallowing inputs and the output back from the irb debugger is also a bit garbled.

@ddollar
Copy link
Owner Author

ddollar commented Aug 2, 2024

Looks like the irb mode gets hot and heavy with the cursor manipulation. I'll keep poking.

@jeromedalbert
Copy link

jeromedalbert commented Aug 2, 2024

Thanks! Debugging works nice with your latest commit, but input for non-interactive processes is not closed, I think it's missing something like this: jeromedalbert/foreman@interactive...input. When I try that diff, inputs are closed correctly.

Tailwind watch is a particular case though, it expects a continuous stdin, otherwise it exits, through no fault of Foreman. If I use css: bin/rails tailwindcss:watch < /dev/zero in my Procfile.dev, it works and I can debug without swallowed inputs, but the continuous stream coming from /dev/zero is causing high CPU usage on my machine. Maybe this is more of an issue to file with Tailwind.

@dhh
Copy link

dhh commented Aug 2, 2024

Hmm, even with the latest change, I can't get irb to act right. It's resetting the output, not revealing input.

@dhh
Copy link

dhh commented Aug 2, 2024

Here's what I see when running with the latest from here:
Screencast from 2024-08-02 13-53-29.webm

There's no visible input until I hit return. And the IRB session seems to clear itself after showing results.

@jeromedalbert
Copy link

jeromedalbert commented Aug 2, 2024

@dhh Maybe you haven't rebuilt and reinstalled the gem with the latest changes, or didn't run your last command with --interactive web?

Here are the full steps I used:

  • git clone [email protected]:ddollar/foreman.git, cd foreman, git checkout interactive, git pull, bundle
  • gem uninstall foreman -x; gem build; gem install foreman-0.88.1.gem
  • rails new tailwind-esbuild --main -j esbuild -c tailwind
  • cd tailwind-esbuild
  • rails generate scaffold post title:string; rails db:migrate
  • add binding.irb to new action in app/controllers/posts_controller.rb
  • optional: in Procfile.dev, comment out css: yarn build:css --watch if you want to avoid swallowed characters
  • foreman start -f Procfile.dev -p 3000 --interactive web
  • visit http://127.0.0.1:3000/posts/new

Result:

Screenshot 2024-08-02 at 2 19 37 PM

@dhh
Copy link

dhh commented Aug 2, 2024

Egg on my face! I had reset the application I was testing in, and yes, indeed it had removed the --interactive flag. This seems to work very well!

What I also noticed was that while in the debugger session, I made a change to a JavaScript file, and the esbuild watch process didn't interrupt the debugging session? It just ran after I was done. That's certainly ideal!

So this is awesome. Is there anything you have outstanding that you think needs to be validated before we can proceed with this path and make foreman a default for Rails 8?

@dhh
Copy link

dhh commented Aug 2, 2024

I did notice this when quitting after running an irb debug session:

image

@jeromedalbert
Copy link

jeromedalbert commented Aug 2, 2024

Is there anything you have outstanding that you think needs to be validated before we can proceed with this path and make foreman a default for Rails 8?

Are you talking to me or ddollar? 😅 To me it looks pretty good, again besides that tailwind thing I mentioned in #803 (comment). I don't know if it's just me or if you're having the issue as well, and probably an issue to file with tailwind, but for any other processes like solid_queue which was the impetus for all of this, I think there should be no problem.

@dhh
Copy link

dhh commented Aug 2, 2024

Hmm, once I added tailwind, I indeed started getting swallowed inputs. Was fine with esbuild, but not tailwind. Here's a video demonstrating it. In the video, I have several swallowed inputs. And then after continuing from the debugger and starting it again, it would exit and proceed on return. Need to test if this is a tailwind specific issue, in which case I'm sure we can get them to fix it with something like --watch=forever like esbuild has, or if it's also an issue with dart or the like.

Screencast.from.2024-08-02.14-42-11.webm

@jeromedalbert
Copy link

jeromedalbert commented Aug 2, 2024

I think it's tailwind, if you apply my patch to foreman, so non-interactive processes are truly closed for stdin, you'll see that tailwind straight up exits because it has no more stdin. If you try my < /dev/zero trick mentioned in #803 (comment) you shouldn't have swallowed inputs but that'll spike your CPU so not a good option.

or if it's also an issue with dart or the like.

dart worked fine for me, see rails/rails#52459 (comment), I tried dartsass, bulma, bun, esbuild, rollup, webpack and they all worked on my machine. Only tailwind and postcss (edit: and bootstrap) had issues.

@jeromedalbert
Copy link

jeromedalbert commented Aug 2, 2024

I tested rails new myapp --main -j esbuild -c postcss.

If non-interactive processes are open to stdin (tested with the current state of the interactive branch at the time of writing):

  • the esbuild watch process works fine as is.
  • postcss is causing swallowed inputs. Changing my Procfile.dev from css: yarn build:css --watch to css: yarn build:css --watch < /dev/null seems to work fine with no CPU spikes.

If non-interactive processes are closed for stdin (tested with my branch):

  • the esbuild watch process will exit. For the process to keep running, I need to use the watch=forever option that you mentioned, i.e. change my Procfile.dev from js: yarn build --watch to js: yarn build --watch=forever.
  • postcss works well as is.

So either way it works.

@dhh
Copy link

dhh commented Aug 2, 2024

Maybe @adamwathan has someone on his team who can help with this. Whatever the esbuild team did with --watch=forever, and not swallowing stdin, seems like what we need there too.

@jeromedalbert
Copy link

jeromedalbert commented Aug 3, 2024

To get a better overview, I re-tested different combinations with foreman start --interactive web and debugging. Here are my findings (edit: updated tailwind findings with Jordan's tip mentioned below).

If non-interactive processes are open to stdin

rails new myapp --main --javascript options:

  • bun: ok
  • webpack: ok
  • esbuild: ok
  • rollup: ok

rails new myapp --main --css options:

  • tailwind: not ok, a workaround is to use css: bin/rails tailwindcss:watch[always] < /dev/null in Procfile.dev
  • bootstrap (watches with nodemon): not ok, a workaround is to use css: yarn watch:css < /dev/null in Procfile.dev
  • bulma (watches with sass): ok
  • postcss: not ok, a workaround is to use css: yarn build:css --watch < /dev/null in Procfile.dev
  • sass (watches with dartsass): ok

If non-interactive processes are closed for stdin

  • tailwind: ok with the --watch=always option (aka bin/rails tailwindcss:watch[always])
  • bootstrap: ok
  • postcss: ok
  • esbuild: ok with the --watch=forever option
  • Everything else: ok

@thecrypticace
Copy link

Hi! Jordan from the Tailwind team here. You can pass --watch=always to our CLI to keep it running even when stdin closes.

@dhh
Copy link

dhh commented Aug 3, 2024

Oh that's great, @thecrypticace. I just tried that, and it seemed to make the swallowing of input go away in some quick testing.

But IRB itself still seems like it has some issues running like this. That seems like an easier problem for us to short, though!

@dhh
Copy link

dhh commented Aug 3, 2024

Spoke too soon. Still seeing stdin eating. But that's probably just because stdin IS open for the process. @ddollar, seems like maybe it's worth trying just closing stdin for all processes except for the interactive one. Maybe that will make the rest of the IRB weirdness go away too.

@jeromedalbert
Copy link

jeromedalbert commented Aug 3, 2024

Yeah, seems like --watch=always still reads from stdin.

The < /dev/null workaround works with it though, if I use this in my Procfile.dev: css: bin/rails tailwindcss:watch[always] < /dev/null. So either this or closing stdin for all non-interactive processes will work. Closing stdin is probably best to avoid any < /dev/null hacks.

@thecrypticace
Copy link

thecrypticace commented Aug 3, 2024

Yeah, seems like --watch=always still reads from stdin.

Yeah --watch=always isn't intended to disable reading from stdin — passing a filename to --input does that. If you still see it reading from stdin in that case that might be a bug though (would need to double check). The flag is to just stop the CLI from exiting when stdin closes.

@jeromedalbert
Copy link

jeromedalbert commented Aug 3, 2024

Unrelated: when I am in a debugger session and I type help, the output is scrambled (screenshot).

@dhh
Copy link

dhh commented Aug 3, 2024

@thecrypticace I think we should be able to close stdin on this end. @ddollar is there a reason, generally speaking, that stdin needs to be left open for the other processes?

@ddollar
Copy link
Owner Author

ddollar commented Aug 3, 2024

Pulled in @jeromedalbert 's fix and added a few more tweaks to level out prefix injection. Things look pretty good on my end but definitely noted that help goes completely haywire. I'll look into that a bit more.

@dhh
Copy link

dhh commented Aug 15, 2024

Sorry, got away from this for a little while. What are the remaining blockers? Does it seem feasible that we'll be able to make IRB/debugger work as one would expect using this foreground process approach?

@dhh
Copy link

dhh commented Aug 21, 2024

Tested the latest. The IRB help command now displays correctly! Next stumbling block is that after you continued from one debugger break, and you hit another, "enter" will cause the debugger to continue. Instead of just doing a newline. Not sure what that is about.

@jeromedalbert
Copy link

jeromedalbert commented Aug 21, 2024

I can also reproduce the issue mentioned above by DHH.

help is better with the latest commit, but I am getting a couple issues related to paging:

  • On my MBP 13" screen, help goes through a pager, but is only displayed on half of the screen (screenshot).
  • If I scroll down with j or G, then quit the pager with q, all is good. But if after scrolling down, I scroll up with k or g, the output is scrambled, and stays scrambled for any future output even after quitting the pager (screenshot).

I don't use help much so these are not big issues for me.

@duduribeiro
Copy link

duduribeiro commented Aug 30, 2024

This is a very nice improvement for debugging applications. Very nice idea.

I did some tests locally with a brand new test app:

web: bin/rails server
js: yarn build --watch=forever
css: yarn build:css --watch=always

and worked smoothly with ruby debug

Gravacao.de.Tela.2024-08-30.as.17.46.41.mov

I will test to replace one of my apps with this workflow and check if everything works fine with my workflow

Without --watch=forever on the js and --watch=always on css I got a SIGTERM when starting foreman

system | sending SIGTERM to all processes

@duduribeiro
Copy link

Next stumbling block is that after you continued from one debugger break, and you hit another, "enter" will cause the debugger to continue. Instead of just doing a newline. Not sure what that is about.

this happened on my test as well.

@mattbrictson
Copy link

Next stumbling block is that after you continued from one debugger break, and you hit another, "enter" will cause the debugger to continue. Instead of just doing a newline. Not sure what that is about.

I believe this is a bug/unexpected behavior of irb itself and doesn't have anything to do with foreman. I logged an issue: ruby/irb#1002

@st0012
Copy link

st0012 commented Sep 6, 2024

Next stumbling block is that after you continued from one debugger break, and you hit another, "enter" will cause the debugger to continue. Instead of just doing a newline. Not sure what that is about.

this happened on my test as well.

👋 This is an intended behaviour of IRB's debugger integration. Quoting from my comment in ruby/irb#1002:

It's related to a requested feature: ruby/irb#856

For context, this is what ruby/debug does: inserting empty input after stepping commands like step, next, and continue, will repeat the previous command.

And IRB's continue command, along with other debugging commands, are simply shortcuts to start an irb:rdbg session, which is essentially a ruby/debug running IRB as its REPL. So in those sessions we decided to mimic the above behaviour.

@dhh
Copy link

dhh commented Sep 7, 2024

Oh, yeah it seems like there's actually a general problem with the debug gem. Must have been recently introduced. I can now reproduce the "first debugger session, you can use return just fine, second-onward, it doesn't work" problem without foreman or overmind in the loop! This is without even entering IRB. Just staying in the rdbg session that's provided by debugger.

Copy link

@skidipapo skidipapo left a comment

Choose a reason for hiding this comment

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

Not received

Choose a reason for hiding this comment

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

Optimism

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.

8 participants