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

JRuby 1.7 #41

Closed
mnzaki opened this issue Jul 17, 2012 · 22 comments
Closed

JRuby 1.7 #41

mnzaki opened this issue Jul 17, 2012 · 22 comments

Comments

@mnzaki
Copy link
Contributor

mnzaki commented Jul 17, 2012

I'm trying to use highline on JRuby 1.7 (jruby-head with rvm) and it fails with:
NameError: cannot load Java class jline.ConsoleReader

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 17, 2012

JRuby moved from jline1 to jline2.7 on April 17 (jruby/jruby@bb7dc31)
I suppose highline is expecting jline1

@JEG2
Copy link
Owner

JEG2 commented Jul 17, 2012

I would love to see this fixed. Anyone who know's jline and offers up a patch is my hero.

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 17, 2012

I was just beginning work on a patch. Quick clarification: what's up with all the disableEcho and enableEcho for JRuby and not for other platforms? A lot of the calls (gets, readline, getbyte, etc) on $stdin/$stdout work for JRuby just as well, and I don't see why all the extra logic (and echo disable/re-enable) is needed.

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 18, 2012

Here's an initial patch, mnzaki/highline@721bc1e6194d7a79b19
I'm going to try to refactor a few parts then issue a pull request.

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 18, 2012

I have pushed more changes to the jruby-1.7 branch, which clean up the terminal code a bit. This needs more testing of course. I've tested ncurses, stty and jline.

stty and jline seem fine.

ncurses breaks the console if the process gets a SIGSTOP (ctrl-z on the terminal) and SIGCONT ("fg").
Also, after ending the process the terminal does not handle scroll events properly anymore. Usually when in secondary screen mode, the scroll events simply produce up and down keypresses, and when secondary screen mode is terminated, the scroll events are captured again. It seems like we are not closing down ncurses properly or something else is wrong.

I'd recommend giving 'jline' higher priority than 'ncurses' mode.

If you wish, you can just pull the first commit which makes minimal changes (specific to JRuby) and does not touch anything else.

@JEG2
Copy link
Owner

JEG2 commented Jul 18, 2012

Wow, thanks for working on this.

I agree with you assessment. Let's bump line over ncurses.

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 18, 2012

I need the JRuby functionality, so it's only natural that I work on it :-D

The ncurses terminal breakage is actually a JRuby bug, and an important one IMO. JRuby doesn't seem to run the 'ensure' blocks if the process gets a SIGINT, so NCurses::endwin is never called if we die suddenly. I'll be reporting this. MRI handles this correctly, and ncurses works well there (sort of; I still don't like the occasional screen blank).

I've rearranged the choices to give jline (which is much more reliable) a higher priority, and I removed the comment about needing ncurses for jruby.

BTW, all these changes will probably break highline horribly for users of jruby before 1.5 (and possible 1.6)

@JEG2
Copy link
Owner

JEG2 commented Jul 19, 2012

Hmm, we probably need an option for older JRuby versions. Thoughts?

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 19, 2012

Not sure. You could keep the changes on a different branch for a couple months then merge into master later. The JLine change in JRuby has been since mid April, that's about 2 months ago. I'm guessing there'll be considerably more people using the new JRuby in a month or two.

@tomdz
Copy link

tomdz commented Jul 19, 2012

You could distinguish the jruby versions using JRUBY_VERSION, and keep the jline1 code around for < 1.7.

@JEG2
Copy link
Owner

JEG2 commented Jul 19, 2012

I think I prefer tomdz's approach. HighLine is a general purpose library and we're going to need to support it for a while yet.

@mnzaki
Copy link
Contributor Author

mnzaki commented Jul 19, 2012

That sounds better. What do you think of the rest of the changes in the branch though?
I'm off on vacation for a week, I might work on this when I'm back if you don't.

@JEG2
Copy link
Owner

JEG2 commented Jul 20, 2012

It's looking good to me. I think you're on the right track.

@nirvdrum
Copy link

nirvdrum commented Aug 8, 2012

If it helps any, I've tried @mnzaki's branch on JRuby 1.7.0-preview2 in the context of Capistrano and everything has been working great.

@JEG2
Copy link
Owner

JEG2 commented Aug 8, 2012

Great. So we just need to sort out the backwards compatibility issues and these changes are a go.

@nirvdrum
Copy link

nirvdrum commented Aug 8, 2012

@tomdz's solution makes sense. Otherwise, you could try to require one, rescue the exception, and then try to require the other.

@JEG2
Copy link
Owner

JEG2 commented Aug 8, 2012

Yeah, we just need someone to get the patch ready.

@markmandel
Copy link

@mnzaki - Just tested your branch as well. Worked a charm (was having exactly the same issue). Thanks for putting in the effort.

@mnzaki
Copy link
Contributor Author

mnzaki commented Aug 22, 2012

Apologies for the delay, but my organization has moved away from JRuby for now.
I have update the branch and added back support for 1.6
I have done some basic testing under JRuby 1.6 and 1.7, and MRI 1.9. More testing is probably needed to make sure I didn't break anything.

I will create a pull request now.

@JEG2
Copy link
Owner

JEG2 commented Aug 22, 2012

I've merged the changes. Thank you.

Anyone else willing to double-check this stuff before I release a new gem?

@ryanb
Copy link

ryanb commented Aug 25, 2012

@JEG2 I had the error mentioned in this issue and using the master branch fixed it. I will be mentioning this fix in a RailsCasts episode today but it would be nice to get a release soon. Thanks!

@JEG2
Copy link
Owner

JEG2 commented Aug 25, 2012

I have pushed 1.6.14 with this fix. Thanks all!

@JEG2 JEG2 closed this as completed Aug 25, 2012
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

No branches or pull requests

6 participants