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

Make local ruby pry work #5718

Merged
merged 5 commits into from Feb 8, 2019

Conversation

Projects
4 participants
@jkburges
Copy link
Contributor

jkburges commented Jan 18, 2019

Make local ruby debugging work

This follows a similar change for local python invocation at

if sys.platform != 'win32':
try:
subprocess.check_call('tty', stdout=subprocess.PIPE, stderr=subprocess.PIPE)
except (OSError, subprocess.CalledProcessError):
pass
else:
sys.stdin = open('/dev/tty')

A handler such as the following will now pause at the breakpoint:

require 'json'
require 'pry'

def hello(event:, context:)
  binding.pry # breakpoint
  { statusCode: 200, body: JSON.generate('Go Serverless v1.0! Your function executed successfully!') }
end

FWIW: I put this PR up for feedback rather than to expecting to get it merged, especially w.r.t. e30e242

Make local ruby debugging work
This follows a similar change for local python invocation at https://github.com/serverless/serverless/blob/ad5decb080a7aa92cac1bc0f8632e3e57b99fe46/lib/plugins/aws/invokeLocal/invoke.py#L76-L82

A handler such as the following will now pause at the breakpoint:

```ruby
require 'json'
require 'pry'

def hello(event:, context:)
  binding.pry # breakpoint
  { statusCode: 200, body: JSON.generate('Go Serverless v1.0! Your function executed successfully!') }
end
```

@jkburges jkburges force-pushed the Biteable:make-local-ruby-pry-work branch from aa1e9f2 to bc01159 Jan 18, 2019

Don't add newlines to echoed input
Prior to this, debug output would look something like the following:

```
[1] pry(Kernel)>
f
o
o

=

"
f
o
o
"

=> "foo"

[2] pry(Kernel)>
```

i.e. there's a newline being added after each character is entered, making things somewhat difficult to read.

@jkburges jkburges force-pushed the Biteable:make-local-ruby-pry-work branch from bc01159 to e30e242 Jan 18, 2019

@pmuens
Copy link
Member

pmuens left a comment

Looking good. Thanks for taking the time to dive deeper into this @jkburges 👍

While I see the need to write to stdout and stderr it would be nifty if we could kepp the usage of consoleLog somehow since this is the central place where we control the overall output formatting.

@@ -276,8 +276,8 @@ class AwsInvokeLocal {
const ruby = spawn(runtime,
[path.join(__dirname, 'invoke.rb'), handlerPath, handlerName],
{ env: process.env }, { shell: true });
ruby.stdout.on('data', (buf) => this.serverless.cli.consoleLog(buf.toString()));

This comment has been minimized.

@pmuens

pmuens Jan 18, 2019

Member

Initially we piped everything through cli.consoleLog so that we have full control of the output and its formatting. Is there a different way how we could still leverage consoleLog?

This comment has been minimized.

@jkburges

jkburges Jan 19, 2019

Author Contributor

It looks as though cli.consoleLog delegates straight to console.log - which apparently always adds a newline.

See: https://github.com/serverless/serverless/blob/master/lib/utils/log/consoleLog.js#L4 and https://stackoverflow.com/questions/6157497/node-js-printing-to-console-without-a-trailing-newline

Would this be a misuse of consoleLog though in a way? i.e. it's not really a log that's happening here - just echoing/piping output. I could see how with a log, you might want to print a timestamp, log level etc (which would be a reason for a logging function like consoleLog, but that doesn't apply here perhaps.

But happy to take guidance since I don't know this framework very well :-)

This comment has been minimized.

@pmuens

pmuens Jan 22, 2019

Member

I see. Yes, that makes sense. Thanks for taking the time to look into this 👍

@pmuens pmuens self-assigned this Jan 18, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jan 18, 2019

Serverless automation moved this from In progress to Needs review Jan 22, 2019

@pmuens
Copy link
Member

pmuens left a comment

Hey @jkburges thanks for working on this and following up on the PR review comments.

I think it's GTM. However there are some tests failing. Could you update those? After that we should be in a good position to merge this soon 👍

@@ -60,6 +60,8 @@ def log(message)
handler_method, handler_class = handler_name.split(".").reverse
handler_class ||= "Kernel"

$stdin.reopen "/dev/tty", "a+" unless Gem.win_platform? || $stdin.tty?

This comment has been minimized.

@dschep

dschep Jan 22, 2019

Member

Could this fail if there is no tty? In python invoke local it spawns the tty executable to make sure opening the tty will work before doing so.

This comment has been minimized.

@jkburges

jkburges Jan 22, 2019

Author Contributor

You're right - addressed in 3bfc724.

jkburges added some commits Jan 22, 2019

Revert "Don't add newlines to echoed input"
This reverts commit e30e242.

This breaks a couple of tests which are depending on:

    serverless.cli.consoleLog.lastCall.args[0]

being set.

Also, it introduces an inconsistency c.f. invokeLocalPython et al.

I think here, consistency and passing tests trumps the slight annoyance of each input charact appearing on a new line. At least, for the moment and until a better way is found.
@jkburges

This comment has been minimized.

Copy link
Contributor Author

jkburges commented Jan 22, 2019

@dschep - I had to revert my changes regarding not adding new lines. Do you see this same behaviour in the python debugger (with which I'm not familiar) - i.e. each character typed appears on a new line?

@jkburges

This comment has been minimized.

Copy link
Contributor Author

jkburges commented Jan 22, 2019

However there are some tests failing. Could you update those?

See comments in f490cb5.

@dschep

This comment has been minimized.

Copy link
Member

dschep commented Jan 23, 2019

Hmm. There is a tiny bit of weirdness with new lines IIRC in python, but not on every character.

@dschep

This comment has been minimized.

Copy link
Member

dschep commented Jan 23, 2019

just checked, in python the user in put ends up on a line after (pdb) where as usually its on the same line. Also, I'm less sure about the invoke tty check.. I'd never had a mac to test that on before now.. and it seems to not pass that check :(

@@ -42,6 +42,11 @@ def log(message)
end
end


def attach_tty
$stdin.reopen "/dev/tty", "a+" unless Gem.win_platform? || $stdin.tty? || !File.exist?("/dev/tty")

This comment has been minimized.

@dschep

dschep Jan 28, 2019

Member

Could you add a call to tty if platform is linux? Equivalent python code:

if sys.platform != 'win32':
try:
if sys.platform != 'darwin':
subprocess.check_call('tty', stdout=subprocess.PIPE, stderr=subprocess.PIPE)

This comment has been minimized.

@jkburges

jkburges Jan 30, 2019

Author Contributor

Yes. What is the purpose of doing this though - is it checking that there is a tty terminal?

As an aside, should be ok to do it on os x too by the looks of it:

$ tty
/dev/ttys001
$

This comment has been minimized.

@dschep

dschep Jan 31, 2019

Member

I'm not 100% sure, but I think it ensures the TTY is usable. Despite it working like that for you on a mac, the check didn't actually work :(

This comment has been minimized.

@jkburges

jkburges Feb 7, 2019

Author Contributor

@dschep I tested this PR on linux just now, and it works as expected for me. Nevertheless, I added a rescue block to catch any errors just in case - does that help you?

I tried writing an equivalent check to the python code by adding system("tty") but I couldn't get that to work - it would return exit code of 1. I didn't really pursue this though as it seems redundant (since it works for me as is in linux).

Not sure how I should proceed with this.

This comment has been minimized.

@dschep

dschep Feb 8, 2019

Member

hmm. system('tty') returns a 1 on linux? what if you run it at the CLI?

This comment has been minimized.

@dschep

dschep Feb 8, 2019

Member

Nevermind. Just looked a bit more at various tty stuff in ruby & python. I'm OK with the code as is now. I'm actually gonna change the python check to be more like this one (using language native checks)

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 4, 2019

Hey @jkburges @dschep thanks for the insightful discussion here 👍

I'd love to merge this soon. What needs to be done to get this into a mergeable state? Looks like we're almost there?!

Let me know if you need any help here!

@jkburges

This comment has been minimized.

Copy link
Contributor Author

jkburges commented Feb 5, 2019

What needs to be done to get this into a mergeable state? Looks like we're almost there?!

  1. I need to get this working on linux in relation to #5718 (comment)
  2. the "coverage/coveralls check is failing - so I guess I need to add some test coverage for the added code!

I'm keen to get to this too, will try to get to it in the next day or so.

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 5, 2019

Thanks for the detailed update and for wrapping this up @jkburges 👍

Let us know if you need any help here / face any issues!

@pmuens
Copy link
Member

pmuens left a comment

Great stuff! Thanks for the update @jkburges 👍

I just gave it a sping on my local machine (MacOS) and in a Docker container (Ubuntu) and both work fine!

The only thing which caught my attention is that when I e.g. enter something like event to inspect a variable a newline is inserted after every letter I type (see below for an example):

[5] pry(Kernel)> 
e
v
e
n
t


=> {}

Not sure if this is by design, but if we could fix this I'd be more than happy to merge this right away!

Great job @jkburges 👌

@pmuens pmuens added pr/in-review and removed pr/in-progress labels Feb 8, 2019

@dschep

This comment has been minimized.

Copy link
Member

dschep commented Feb 8, 2019

Huh, @pmuens, worked fine for me:

$ sls invoke local -f hello

From: /Users/dschep/code/tmp/aws-ruby/handler.rb @ line 3 Object#hello:

    2: def hello(event:, context:)
 => 3:   binding.pry
    4:   { statusCode: 200, body: 'Go Serverless v1.0! Your ruby function executed successfully!' }
    5: end



event
=> {}
@dschep

dschep approved these changes Feb 8, 2019

tests pass now

Serverless automation moved this from Needs review to Reviewer approved Feb 8, 2019

@dschep dschep merged commit b32b200 into serverless:master Feb 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Feb 8, 2019

@jkburges

This comment has been minimized.

Copy link
Contributor Author

jkburges commented Feb 8, 2019

Thanks for merging 🎉

The only thing which caught my attention is that when I e.g. enter something like event to inspect a variable a newline is inserted after every letter I type

Yes this is the issue which I had fixed in e30e242 but later rolled back.

Another potential solution is took change consoleLog to take an optional parameter, defaulting to true, for whether to add new lines - but not sure whether that's a good idea or not.

@jkburges jkburges deleted the Biteable:make-local-ruby-pry-work branch Feb 8, 2019

@eahefnawy eahefnawy added this to the 1.38.0 milestone Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.